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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
