Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Nishanth Aravamudan <nacc@us.ibm.com>]

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@virtuousgeek.org>  wrote:
> >>>On Sun, 6 Nov 2011 10:33:10 +0800
> >>>Ram Pai<linuxram@us.ibm.com>  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@us.ibm.com>
> >>| 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@kernel.org>
> >>
> >>---
> >>  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@us.ibm.com>
IBM Linux Technology Center