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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4FFCE9C0.3050705@huawei.com>
Date:	Wed, 11 Jul 2012 10:49:36 +0800
From:	Jiang Liu <jiang.liu@...wei.com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
CC:	Jiang Liu <liuj97@...il.com>, Don Dutile <ddutile@...hat.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Taku Izumi <izumi.taku@...fujitsu.com>,
	"Rafael J . Wysocki" <rjw@...k.pl>,
	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
	Yijing Wang <wangyijing@...wei.com>,
	Keping Chen <chenkeping@...wei.com>,
	<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify
 PCI core implementation

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/