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<jbarnes@xxxxxxxxxxxxxxxx> wrote:
> >>>On Sun, 6 Nov 2011 10:33:10 +0800
> >>>Ram Pai<linuxram@xxxxxxxxxx> 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<linuxram@xxxxxxxxxx>
> >>| 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<yinghai@xxxxxxxxxx>
> >>
> >>---
> >> 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 <nacc@xxxxxxxxxx>
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
