| 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
| ||
|
Message-ID: <8b693bfc-73e0-2956-2ba3-1bfd639660b6@linux.intel.com>
Date: Wed, 2 Apr 2025 15:38:37 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, bhelgaas@...gle.com, kw@...ux.com,
manivannan.sadhasivam@...aro.org, robh@...nel.org, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v7 2/5] PCI: Refactor capability search functions to eliminate
code duplication
On Wed, 2 Apr 2025, Hans Zhang wrote:
> Refactor the PCI capability and extended capability search functions
> by consolidating common code patterns into reusable macros
> (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
> changes include:
>
> 1. Introducing a unified config space read helper (__pci_bus_read_config).
> 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
> pci_find_next_ext_capability.
> 3. Implementing consistent capability discovery using the new macros.
> 4. Simplifying HyperTransport capability lookup by leveraging the
> refactored code.
>
> The refactoring maintains existing functionality while reducing code
> duplication and improving maintainability. By centralizing the search
> logic, we achieve better code consistency and make future updates easier.
>
> This change has been verified to maintain backward compatibility with
> existing capability discovery patterns through thorough testing of PCI
> device enumeration and capability probing.
>
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
> drivers/pci/pci.c | 79 +++++++++++++----------------------------------
> 1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..521096c73686 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -423,36 +423,33 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
> return 1;
> }
>
> -static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap, int *ttl)
> +static int __pci_bus_read_config(void *priv, unsigned int devfn, int where,
> + u32 size, u32 *val)
This probably should be where the other accessors are so in access.c. I'd
put its prototype into drivers/pci/pci.h only for now.
> {
> - u8 id;
> - u16 ent;
> + struct pci_bus *bus = priv;
> + int ret;
>
> - pci_bus_read_config_byte(bus, devfn, pos, &pos);
> + if (size == 1)
> + ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> + else if (size == 2)
> + ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> + else
> + ret = pci_bus_read_config_dword(bus, devfn, where, val);
>
> - while ((*ttl)--) {
> - if (pos < 0x40)
> - break;
> - pos &= ~3;
> - pci_bus_read_config_word(bus, devfn, pos, &ent);
> + return ret;
> +}
>
> - id = ent & 0xff;
> - if (id == 0xff)
> - break;
> - if (id == cap)
> - return pos;
> - pos = (ent >> 8);
> - }
> - return 0;
> +static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> + u8 pos, int cap)
> +{
> + return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
> + devfn);
> }
>
> static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> u8 pos, int cap)
> {
> - int ttl = PCI_FIND_CAP_TTL;
> -
> - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
> }
>
> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -553,42 +550,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
> */
> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
> {
> - u32 header;
> - int ttl;
> - u16 pos = PCI_CFG_SPACE_SIZE;
> -
> - /* minimum 8 bytes per capability */
> - ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> return 0;
>
> - if (start)
> - pos = start;
> -
> - if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> - return 0;
> -
> - /*
> - * If we have no capabilities, this is indicated by cap ID,
> - * cap version and next pointer all being 0.
> - */
> - if (header == 0)
> - return 0;
> -
> - while (ttl-- > 0) {
> - if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> - return pos;
> -
> - pos = PCI_EXT_CAP_NEXT(header);
> - if (pos < PCI_CFG_SPACE_SIZE)
> - break;
> -
> - if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> - break;
> - }
> -
> - return 0;
> + return PCI_FIND_NEXT_EXT_CAPABILITY(__pci_bus_read_config, start, cap,
> + dev->bus, dev->devfn);
I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
together. However, (IMO) you can introduce the new all-size config space
accessor in a separate patch before the combined patch.
> }
> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>
> @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>
> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> - int rc, ttl = PCI_FIND_CAP_TTL;
> u8 cap, mask;
>
> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> mask = HT_5BIT_CAP_MASK;
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> while (pos) {
> rc = pci_read_config_byte(dev, pos + 3, &cap);
> if (rc != PCIBIOS_SUCCESSFUL)
> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> pos + PCI_CAP_LIST_NEXT,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
This function kind of had the idea to share the ttl but I suppose that was
just a final safeguard to make sure the loop will always terminate in case
the config space is corrupted so the unsharing is not a big issue.
> }
>
> return 0;
>
--
i.
Powered by blists - more mailing lists