From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:62108 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757048Ab1LGWef (ORCPT ); Wed, 7 Dec 2011 17:34:35 -0500 Message-ID: <4EDFE9F2.40108@redhat.com> Date: Wed, 07 Dec 2011 17:34:26 -0500 From: Don Dutile MIME-Version: 1.0 To: Nishanth Aravamudan CC: Ram Pai , Yinghai Lu , Jesse Barnes , linux-pci@vger.kernel.org, Benjamin Herrenschmidt , Bjorn Helgaas , prarit@redhat.com, brking@linux.vnet.ibm.com Subject: Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS References: <1318057168.29415.333.camel@pasglop> <20111008075353.GK2980@ram-ThinkPad-T61> <1318060793.29415.347.camel@pasglop> <20111102140325.004b9dad@jbarnes-desktop> <20111103013014.GB393@ram-ThinkPad-T61> <20111106023310.GA2383@ram-ThinkPad-T61> <20111205103202.29faf6e1@jbarnes-desktop> <20111207092531.GF19129@ram-ThinkPad-T61> <4EDFCB4A.60106@redhat.com> <20111207203517.GB6431@us.ibm.com> In-Reply-To: <20111207203517.GB6431@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: linux-pci-owner@vger.kernel.org List-ID: On 12/07/2011 03:35 PM, Nishanth Aravamudan wrote: > On 07.12.2011 [15:23:38 -0500], Don Dutile wrote: >> On 12/07/2011 04:25 AM, Ram Pai wrote: >>> On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote: >>>> On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes wrote: >>>>> On Sun, 6 Nov 2011 10:33:10 +0800 >>>>> Ram Pai wrote: >>>>> >>>>>> >>>>>> NOTE: Note, there is subtle change in the pci_enable_device() API. >>>>>> Any driver that depends on SRIOV BARS to be enabled in pci_enable_device() >>>>>> can fail. >>>>>> >>>>>> --- >>>>> >>>>> Applied to my for-linus branch, thanks. >>>>> >>>> >>>> please don't push to linus now. >>>> >>>> this one causes regression. >>>> >>>> please check attached patch. >>>> >>>> Thanks >>>> >>>> Yinghai >>> >>>> [PATCH] pci: Fix hotplug of Express Module with pci bridges >>>> >>>> Found hotplug of one setup does not work with recent change in pci tree. >>>> >>>> After checking the bridge conf setup, found bridges get assigned, but not get enabled. >>>> >>>> Finally found following commit, simplely ignore bridge resource when enabling pci device. >>>> >>>> | commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b >>>> | Author: Ram Pai >>>> | Date: Sun Nov 6 10:33:10 2011 +0800 >>>> | >>>> | PCI: defer enablement of SRIOV BARS >>>> |... >>>> | NOTE: Note, there is subtle change in the pci_enable_device() API. Any >>>> | driver that depends on SRIOV BARS to be enabled in pci_enable_device() >>>> | can fail. >>>> >>>> Put back bridge resource and ROM resource checking to fix the problem. >>>> >>>> That should fix regression like BIOS does not assign correct resource to bridge. >>>> >>>> Signed-off-by: Yinghai Lu >>>> >>>> --- >>>> drivers/pci/pci.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> Index: linux-2.6/drivers/pci/pci.c >>>> =================================================================== >>>> --- linux-2.6.orig/drivers/pci/pci.c >>>> +++ linux-2.6/drivers/pci/pci.c >>>> @@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str >>>> if (atomic_add_return(1,&dev->enable_cnt)> 1) >>>> return 0; /* already enabled */ >>>> >>>> - for (i = 0; i< PCI_ROM_RESOURCE; i++) >>>> + /* only skip sriov related */ >>>> + for (i = 0; i<= PCI_ROM_RESOURCE; i++) >>>> + if (dev->resource[i].flags& flags) >>>> + bars |= (1<< i); >>>> + for (i = PCI_BRIDGE_RESOURCES; i< DEVICE_COUNT_RESOURCE; i++) >>>> if (dev->resource[i].flags& flags) >>>> bars |= (1<< i); >>>> >>> >>> Oops. My patch inadvertently dropped ROM and BRIDGE resources. >>> >>> This patch is right. However would it help if we did something like this >>> to avoid some code duplication? >>> >>> for (i = 0; i< DEVICE_COUNT_RESOURCE; >>> i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++) >>> if (dev->resource[i].flags& flags) >>> bars |= (1<< i); >>> >> >> why not something more explicit like: >> >> for (i = 0; i< DEVICE_COUNT_RESOURCE; i++) { >> if ((i>= PCI_IOV_RESOURCES)&& (i<= PCI_IOV_RESOURCE_END)) >> continue; /* skip sriov related resources */ >> if (dev->resource[i].flags& flags) >> bars |= (1<< i); >> } > > I agree with the more explicit approach, self-commenting and easier to > read. Although, you may want to add a comment as to *why* we're skipping > IOV BARs here. > +1 for those not living it, yes, good for historical (& noobie) understanding > -Nish >