From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e7.ny.us.ibm.com ([32.97.182.137]:59316 "EHLO e7.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757719Ab1LGUfk (ORCPT ); Wed, 7 Dec 2011 15:35:40 -0500 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 7 Dec 2011 15:35:39 -0500 Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pB7KZNiH3231882 for ; Wed, 7 Dec 2011 15:35:23 -0500 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pB7KZKi8024778 for ; Wed, 7 Dec 2011 18:35:22 -0200 Date: Wed, 7 Dec 2011 12:35:17 -0800 From: Nishanth Aravamudan To: Don Dutile 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 Message-ID: <20111207203517.GB6431@us.ibm.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <4EDFCB4A.60106@redhat.com> Sender: linux-pci-owner@vger.kernel.org List-ID: 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. -Nish -- Nishanth Aravamudan IBM Linux Technology Center