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
| ||
|
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/
Powered by blists - more mailing lists