Re: [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation

[Jiang Liu <jiang.liu@huawei.com>]

On 2012-7-11 2:35, Bjorn Helgaas wrote:
>> @@ -2042,7 +1994,6 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>   */
>>  void pci_enable_ari(struct pci_dev *dev)
>>  {
>> -       int pos;
>>         u32 cap;
>>         u16 ctrl;
>>         struct pci_dev *bridge;
>> @@ -2050,8 +2001,7 @@ void pci_enable_ari(struct pci_dev *dev)
>>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>>                 return;
>>
>> -       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
>> -       if (!pos)
>> +       if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> 
> What's going on here?  This looks wrong, and unrelated to the rest of the patch.
Yeah, it's a bug. My original idea is to get rid of "int pos", and it should be
if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))

> 
>>                 return;
>>
>>         bridge = dev->bus->self;
>> @@ -2059,17 +2009,14 @@ void pci_enable_ari(struct pci_dev *dev)
>>                 return;
>>
>>         /* ARI is a PCIe cap v2 feature */
> 
> If we're doing this right, we should be able to remove this comment
> (and similar ones below).
Will remove them.

> 
>> -       pos = pci_pcie_cap2(bridge);
>> -       if (!pos)
>> +       if (pci_pcie_cap_read_dword(bridge, PCI_EXP_DEVCAP2, &cap) ||
>> +          !(cap & PCI_EXP_DEVCAP2_ARI))
> 
> I don't think there's any point in checking every return from
> pci_pcie_cap_read_dword().  We've already checked pci_is_pcie() above,
> and checking here just clutters the code.  In cases like this, my
> opinion is that clean code leads to fewer bugs, and that benefit
> outweighs whatever technical "every return must be checked" benefit
> there might be.
Good point!

>> @@ -2223,17 +2152,14 @@ EXPORT_SYMBOL(pci_disable_obff);
>>   */
>>  static bool pci_ltr_supported(struct pci_dev *dev)
>>  {
>> -       int pos;
>>         u32 cap;
>>
>>         /* LTR is a PCIe cap v2 feature */
>> -       pos = pci_pcie_cap2(dev);
>> -       if (!pos)
>> +       if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
>> +           !(cap & PCI_EXP_DEVCAP2_LTR))
>>                 return false;
> 
> How about if you restructure this to avoid the double negatives?  E.g.,
> 
>     pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
>     if (cap & PCI_EXP_DEVCAP2_LTR)
>             return true;
> 
>     return false;
> 
Good point.

Thanks!
Gerry