Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
From: Hans Zhang
Date: Wed Apr 02 2025 - 11:39:35 EST
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@xxxxxxx>
---
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;