Re: [PATCH] PCI: Use pci_is_root_bus() to check for root bus

[Wei Yang <weiyang@linux.vnet.ibm.com>]

On Thu, Nov 07, 2013 at 02:59:18PM -0700, Bjorn Helgaas wrote:
>On Thu, Nov 07, 2013 at 11:00:54AM +0800, Wei Yang wrote:
>> Thanks Bjorn, this is really a potential problme. And your patches fix this
>> problem.
>> 
>> While I did a small change on the seconde one like this. Hope you like it :-)
>> 
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index bdd64b1..8d0ce48 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1153,7 +1153,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
>>         if (!dev)
>>                 return;
>>  
>> -       pci_enable_bridge(dev->bus->self);
>> +       pci_enable_bridge(pci_upstream_bridge(dev));
>>  
>>         if (pci_is_enabled(dev)) {
>>                 if (!dev->is_busmaster) {
>> @@ -1190,7 +1190,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>         if (atomic_inc_return(&dev->enable_cnt) > 1)
>>                 return 0;               /* already enabled */
>>  
>> -       pci_enable_bridge(dev->bus->self);
>> +       pci_enable_bridge(pci_upstream_bridge(dev));
>>  
>>         /* only skip sriov related */
>>         for (i = 0; i <= PCI_ROM_RESOURCE; i++)
>
>Thanks for looking at these.  I think the latest version (the ones
>acked by Yinghai) do basically what you're suggesting.

Agree :-)

>
>> BTW, pci_enable_bridge() is only called in pci_enable_device_flags(). After
>> change in these two patches, we pass a 'upstream bridge' to
>> pci_enable_bridge(). I am not sure whether this 'upstream bridge' could be a
>> VF? I took a look at the SPEC again, but not find clear clause.
>> 
>> In case the 'upstream bridge' is always a PF, maybe we could simplize the
>> logic in pci_enable_bridge(). While current logic is reasonable and clear.
>
>I doubt it's possible for a VF to be a bridge, but I don't think
>there's really any reason to build that assumption into the code
>here.

Yep, the latest version is more general.

>
>Bjorn

-- 
Richard Yang
Help you, Help me