From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:10090 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757158Ab1LGUXu (ORCPT ); Wed, 7 Dec 2011 15:23:50 -0500 Message-ID: <4EDFCB4A.60106@redhat.com> Date: Wed, 07 Dec 2011 15:23:38 -0500 From: Don Dutile MIME-Version: 1.0 To: Ram Pai CC: Yinghai Lu , Jesse Barnes , linux-pci@vger.kernel.org, Benjamin Herrenschmidt , Bjorn Helgaas , Nishanth Aravamudan , prarit@redhat.com, brking@linux.vnet.ibm.com Subject: Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS References: <20111007232516.GF2980@ram-ThinkPad-T61> <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> In-Reply-To: <20111207092531.GF19129@ram-ThinkPad-T61> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: linux-pci-owner@vger.kernel.org List-ID: 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); } > RP > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html