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

[Don Dutile <ddutile@redhat.com>]

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

> 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