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

[Ram Pai <linuxram@us.ibm.com>]

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);

RP