| 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: <c6706073-86b0-445a-b39f-993ac9b054fa@163.com>
Date: Wed, 2 Apr 2025 23:37:34 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.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 2025/4/2 20:38, Ilpo Järvinen wrote:
> 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.
>
Hi Ilpo,
Thank you very much for your guidance. Will change.
>> {
>> - 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.
>
Ok. I'll change it to the following. The rest I'll combine into a patch.
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b63b..bb2e26c2eb81 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
EXPORT_SYMBOL(pci_bus_write_config_word);
EXPORT_SYMBOL(pci_bus_write_config_dword);
+
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val)
+{
+ struct pci_bus *bus = priv;
+ int ret;
+
+ 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);
+
+ return ret;
+}
+
int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..6a7c88b9cd35 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -88,6 +88,8 @@ extern bool pci_early_dump;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val);
/* Functions internal to the PCI core code */
>> }
>> 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.
>
__pci_find_next_cap_ttl
// This macro definition already has ttl loop restrictions inside it.
PCI_FIND_NEXT_CAP_TTL
Do I understand that you agree to remove ttl initialization and
parameter passing?
Best regards,
Hans
>> }
>>
>> return 0;
>>
>
Powered by blists - more mailing lists