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

[Don Dutile <ddutile@redhat.com>]

On 12/07/2011 03:35 PM, Nishanth Aravamudan wrote:
> 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.
>
+1  for those not living it, yes, good for historical (& noobie) understanding

> -Nish
>