[PATCH v14 0/7] Refactor capability search into common macros

[PATCH v14 0/7] Refactor capability search into common macros

[Hans Zhang]

Dear Maintainers,

This patch series addresses long-standing code duplication in PCI
capability discovery logic across the PCI core and controller drivers.
The existing implementation ties capability search to fully initialized
PCI device structures, limiting its usability during early controller
initialization phases where device/bus structures may not yet be
available.

The primary goal is to decouple capability discovery from PCI device
dependencies by introducing a unified framework using config space
accessor-based macros. This enables:

1. Early Capability Discovery: Host controllers (e.g., Cadence, DWC)
can now perform capability searches during pre-initialization stages
using their native config accessors.

2. Code Consolidation: Common logic for standard and extended capability
searches is refactored into shared macros (`PCI_FIND_NEXT_CAP` and
`PCI_FIND_NEXT_EXT_CAP`), eliminating redundant implementations.

3. Safety and Maintainability: TTL checks are centralized within the
macros to prevent infinite loops, while hardcoded offsets in drivers
are replaced with dynamic discovery, reducing fragility.

Key improvements include:  
- Driver Conversions: DesignWare and Cadence drivers are migrated to
  use the new macros, removing device-specific assumptions and ensuring
  consistent error handling.

- Enhanced Readability: Magic numbers are replaced with symbolic
  constants, and config space accessors are standardized for clarity.

- Backward Compatibility: Existing PCI core behavior remains unchanged.

---
Changes since v13:
- Split patch 3/6 into two patches for searching standard and extended capability. (Bjorn)
- Optimize the code based on the review comments from Bjorn.
- Patch 5/7 and 6/7 use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().
- The other patches have not been modified.

Changes since v12:
- Modify some commit messages, code format issues, and optimize the function return values.

Changes since v11:
- Resolved some compilation warning.
- Add some include.
- Add the *** BLURB HERE *** description(Corrected by Mani and Krzysztof).

Changes since v10:
- The patch [v10 2/6] remove #include <uapi/linux/pci_regs.h> and add macro definition comments.
- The patch [v10 3/6] remove #include <uapi/linux/pci_regs.h> and commit message were modified.
- The other patches have not been modified.

Changes since v9:
- Resolved [v9 4/6] compilation error.
  The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses 
  dw_pcie_find_next_ext_capability.
- The other patches have not been modified.

Changes since v8:
- Split patch.
- The patch commit message were modified.
- Other patches(4/6, 5/6, 6/6) are unchanged.

Changes since v7:
- Patch 2/5 and 3/5 compilation error resolved.
- Other patches are unchanged.

Changes since v6:
- Refactor capability search into common macros.
- Delete pci-host-helpers.c and MAINTAINERS.

Changes since v5:
- If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
  the kernel's .text section even if it's known already at compile time
  that they're never going to be used (e.g. on x86).
- Move the API for find capabilitys to a new file called
  pci-host-helpers.c.
- Add new patch for MAINTAINERS.

Changes since v4:
- Resolved [v4 1/4] compilation warning.
- The patch subject and commit message were modified.

Changes since v3:
- Resolved [v3 1/4] compilation error.
- Other patches are not modified.

Changes since v2:
- Add and split into a series of patches.
---

Hans Zhang (7):
  PCI: Introduce generic bus config read helper function
  PCI: Clean up __pci_find_next_cap_ttl() readability
  PCI: Refactor standard capability search into common macro
  PCI: Refactor extended capability search into common macro
  PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
  PCI: cadence: Use common PCI host bridge APIs for finding the
    capabilities
  PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode

 drivers/pci/access.c                          | 15 ++++
 .../pci/controller/cadence/pcie-cadence-ep.c  | 38 ++++----
 drivers/pci/controller/cadence/pcie-cadence.c | 30 +++++++
 drivers/pci/controller/cadence/pcie-cadence.h | 18 ++--
 drivers/pci/controller/dwc/pcie-designware.c  | 83 ++++--------------
 drivers/pci/pci.c                             | 76 +++-------------
 drivers/pci/pci.h                             | 87 +++++++++++++++++++
 include/uapi/linux/pci_regs.h                 |  3 +
 8 files changed, 196 insertions(+), 154 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.25.1

[PATCH v14 1/7] PCI: Introduce generic bus config read helper function

[Hans Zhang]

The primary PCI config space accessors are tied to the size of the read
(byte/word/dword). Upcoming refactoring of PCI capability discovery logic
requires passing a config accessor function that must be able to perform
read with different sizes.

Add any size config space read accessor pci_bus_read_config() to allow
giving it as the config space accessor to the upcoming PCI capability
discovery macro.

Reconstructs the PCI function discovery logic to prepare for unified
configuration of access modes. No function changes are intended.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
Changes since v13:
- None

Changes since v12:
- Optimize the function return values.

Changes since v9 ~ v11:
- None

Changes since v8:
- The new split is patch 1/6.
- The patch commit message were modified.
---
 drivers/pci/access.c | 15 +++++++++++++++
 drivers/pci/pci.h    |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b63b..ba66f55d2524 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,6 +85,21 @@ 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;
+
+	if (size == 1)
+		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
+	else if (size == 2)
+		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
+	else if (size == 4)
+		return pci_bus_read_config_dword(bus, devfn, where, val);
+	else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+}
+
 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 12215ee72afb..e7d31ed56731 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -90,6 +90,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 */
 
-- 
2.25.1

[PATCH v14 2/7] PCI: Clean up __pci_find_next_cap_ttl() readability

[Hans Zhang]

Refactor the __pci_find_next_cap_ttl() to improve code clarity:
- Replace magic number 0x40 with PCI_STD_HEADER_SIZEOF.
- Use ALIGN_DOWN() for position alignment instead of manual bitmask.
- Extract PCI capability fields via FIELD_GET() with standardized masks.
- Add necessary headers (linux/align.h).

No functional changes intended.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
Changes since v13:
- None

Changes since v12:
- Modify the commit message and the code format issue.

Changes since v11:
- None

Changes since v10:
- Remove #include <uapi/linux/pci_regs.h> and add macro definition comments.

Changes since v9:
- None

Changes since v8:
- Split into patch 1/6, patch 2/6.
- The
---
 drivers/pci/pci.c             | 9 +++++----
 include/uapi/linux/pci_regs.h | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e9448d55113b..1d1d147d007a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/align.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/dmi.h>
@@ -432,17 +433,17 @@ static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
 	pci_bus_read_config_byte(bus, devfn, pos, &pos);
 
 	while ((*ttl)--) {
-		if (pos < 0x40)
+		if (pos < PCI_STD_HEADER_SIZEOF)
 			break;
-		pos &= ~3;
+		pos = ALIGN_DOWN(pos, 4);
 		pci_bus_read_config_word(bus, devfn, pos, &ent);
 
-		id = ent & 0xff;
+		id = FIELD_GET(PCI_CAP_ID_MASK, ent);
 		if (id == 0xff)
 			break;
 		if (id == cap)
 			return pos;
-		pos = (ent >> 8);
+		pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
 	}
 	return 0;
 }
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index a3a3e942dedf..5f9e7633e6e0 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -207,6 +207,9 @@
 
 /* Capability lists */
 
+#define PCI_CAP_ID_MASK		0x00ff	/* Capability ID mask */
+#define PCI_CAP_LIST_NEXT_MASK	0xff00	/* Next Capability Pointer mask */
+
 #define PCI_CAP_LIST_ID		0	/* Capability ID */
 #define  PCI_CAP_ID_PM		0x01	/* Power Management */
 #define  PCI_CAP_ID_AGP		0x02	/* Accelerated Graphics Port */
-- 
2.25.1

[PATCH v14 3/7] PCI: Refactor standard capability search into common macro

[Hans Zhang]

The PCI Capability search functionality is duplicated across the PCI core
and several controller drivers. The core's current implementation requires
fully initialized PCI device and bus structures, which prevents controller
drivers from using it during early initialization phases before these
structures are available.

Move the Capability search logic into a header-based macro that accepts a
config space accessor function as an argument. This enables controller
drivers to perform Capability discovery using their early access
mechanisms prior to full device initialization while sharing the
Capability search code.

Convert the existing PCI core Capability search implementation to use this
new macro. Controller drivers can later use the same macros with their
early access mechanisms while maintaining the existing protection against
infinite loops through preserved TTL checks.

The ttl parameter was originally an additional safeguard to prevent
infinite loops in corrupted config space. However, the
PCI_FIND_NEXT_CAP() macro already enforces a TTL limit internally.
Removing redundant ttl handling simplifies the interface while maintaining
the safety guarantee. This aligns with the macro's design intent of
encapsulating TTL management.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v13:
- Split patch for searching standard capability.

Changes since v12:
- Delete __pci_find_next_cap, use PCI_FIND_NEXT_CAP_TTL() directly.
- Modify the doc description of the function.

Changes since v11:
- Add #include <linux/bitfield.h>, solve the compilation warnings caused by the subsequent patch calls.

Changes since v10:
- Remove #include <uapi/linux/pci_regs.h>.
- The patch commit message were modified.

Changes since v9:
- None

Changes since v8:
- The patch commit message were modified.
---
 drivers/pci/pci.c | 42 ++++++++----------------------------------
 drivers/pci/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1d1d147d007a..c590f697c011 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,7 +9,6 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/align.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/dmi.h>
@@ -424,36 +423,10 @@ 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)
-{
-	u8 id;
-	u16 ent;
-
-	pci_bus_read_config_byte(bus, devfn, pos, &pos);
-
-	while ((*ttl)--) {
-		if (pos < PCI_STD_HEADER_SIZEOF)
-			break;
-		pos = ALIGN_DOWN(pos, 4);
-		pci_bus_read_config_word(bus, devfn, pos, &ent);
-
-		id = FIELD_GET(PCI_CAP_ID_MASK, ent);
-		if (id == 0xff)
-			break;
-		if (id == cap)
-			return pos;
-		pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
-	}
-	return 0;
-}
-
 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(pci_bus_read_config, pos, cap, bus, devfn);
 }
 
 u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
@@ -649,7 +622,7 @@ 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;
+	int rc;
 	u8 cap, mask;
 
 	if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
@@ -657,8 +630,8 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
 	else
 		mask = HT_5BIT_CAP_MASK;
 
-	pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
-				      PCI_CAP_ID_HT, &ttl);
+	pos = PCI_FIND_NEXT_CAP(pci_bus_read_config, pos,
+				PCI_CAP_ID_HT, dev->bus, dev->devfn);
 	while (pos) {
 		rc = pci_read_config_byte(dev, pos + 3, &cap);
 		if (rc != PCIBIOS_SUCCESSFUL)
@@ -667,9 +640,10 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
 		if ((cap & mask) == ht_cap)
 			return pos;
 
-		pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
-					      pos + PCI_CAP_LIST_NEXT,
-					      PCI_CAP_ID_HT, &ttl);
+		pos = PCI_FIND_NEXT_CAP(pci_bus_read_config,
+					pos + PCI_CAP_LIST_NEXT,
+					PCI_CAP_ID_HT, dev->bus,
+					dev->devfn);
 	}
 
 	return 0;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e7d31ed56731..7465501a21e3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -2,6 +2,8 @@
 #ifndef DRIVERS_PCI_H
 #define DRIVERS_PCI_H
 
+#include <linux/align.h>
+#include <linux/bitfield.h>
 #include <linux/pci.h>
 
 struct pcie_tlp_log;
@@ -93,6 +95,49 @@ 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);
 
+/* Standard Capability finder */
+/**
+ * PCI_FIND_NEXT_CAP - Find a PCI standard capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search
+ * @cap: Capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Iterates through the capability list in PCI config space to find
+ * @cap. Implements TTL (time-to-live) protection against infinite loops.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#define PCI_FIND_NEXT_CAP(read_cfg, start, cap, args...)		\
+({									\
+	int __ttl = PCI_FIND_CAP_TTL;					\
+	u8 __id, __found_pos = 0;					\
+	u8 __pos = (start);						\
+	u16 __ent;							\
+									\
+	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
+									\
+	while (__ttl--) {						\
+		if (__pos < PCI_STD_HEADER_SIZEOF)			\
+			break;						\
+									\
+		__pos = ALIGN_DOWN(__pos, 4);				\
+		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
+									\
+		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
+		if (__id == 0xff)					\
+			break;						\
+									\
+		if (__id == (cap)) {					\
+			__found_pos = __pos;				\
+			break;						\
+		}							\
+									\
+		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
+	}								\
+	__found_pos;							\
+})
+
 /* Functions internal to the PCI core code */
 
 #ifdef CONFIG_DMI
-- 
2.25.1

[PATCH v14 4/7] PCI: Refactor extended capability search into common macro

[Hans Zhang]

Move the extended Capability search logic into a header-based macro
that accepts a config space accessor function as an argument. This enables
controller drivers to perform Capability discovery using their early access
mechanisms prior to full device initialization while sharing the Capability
search code.

Convert the existing PCI core extended Capability search implementation to
use this new macro.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v13:
- Split patch for searching extended capability.

Changes since v12:
- Delete __pci_find_next_cap, use PCI_FIND_NEXT_CAP_TTL() directly.
- Modify the doc description of the function.

Changes since v11:
- Add #include <linux/bitfield.h>, solve the compilation warnings caused by the subsequent patch calls.

Changes since v10:
- Remove #include <uapi/linux/pci_regs.h>.
- The patch commit message were modified.

Changes since v9:
- None

Changes since v8:
- The patch commit message were modified.
---
 drivers/pci/pci.c | 35 ++---------------------------------
 drivers/pci/pci.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c590f697c011..b72fd950ff92 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -527,42 +527,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_CAP(pci_bus_read_config, start, cap,
+				     dev->bus, dev->devfn);
 }
 EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 7465501a21e3..17b7db127dad 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -138,6 +138,46 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
 	__found_pos;							\
 })
 
+/* Extended Capability finder */
+/**
+ * PCI_FIND_NEXT_EXT_CAP - Find a PCI extended capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search (0 for initial search)
+ * @cap: Extended capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Searches the extended capability space in PCI config registers
+ * for @cap. Implements TTL protection against infinite loops using
+ * a calculated maximum search count.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#define PCI_FIND_NEXT_EXT_CAP(read_cfg, start, cap, args...)		\
+({									\
+	u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE;			\
+	u16 __found_pos = 0;						\
+	int __ttl, __ret;						\
+	u32 __header;							\
+									\
+	__ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;	\
+	while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {		\
+		__ret = read_cfg(args, __pos, 4, &__header);		\
+		if (__ret != PCIBIOS_SUCCESSFUL)			\
+			break;						\
+									\
+		if (__header == 0)					\
+			break;						\
+									\
+		if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) {\
+			__found_pos = __pos;				\
+			break;						\
+		}							\
+									\
+		__pos = PCI_EXT_CAP_NEXT(__header);			\
+	}								\
+	__found_pos;							\
+})
+
 /* Functions internal to the PCI core code */
 
 #ifdef CONFIG_DMI
-- 
2.25.1

[PATCH v14 5/7] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

Use the PCI core is now exposing generic macros for the host bridges to
search for the PCIe capabilities, make use of them in the DWC driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
Changes since v13:
- Use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().

Changes since v12:
- Modify the function's return value and format issues.

Changes since v11:
- Resolve compilation errors. s/dw_pcie_read_dbi/dw_pcie_read*_dbi

Changes since v10:
- None

Changes since v9:
- Resolved [v9 4/6] compilation error.
  The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses 
  dw_pcie_find_next_ext_capability.

Changes since v8:
- None

Changes since v7:
- Resolve compilation errors.

Changes since v6:
https://lore.kernel.org/linux-pci/20250323164852.430546-3-18255117159@163.com/

- The patch commit message were modified.

Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-3-18255117159@163.com/

- Kconfig add "select PCI_HOST_HELPERS"
---
 drivers/pci/controller/dwc/pcie-designware.c | 83 ++++----------------
 1 file changed, 16 insertions(+), 67 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 4d794964fa0f..b503cb4bcb28 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -213,83 +213,32 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
 		pci->type = ver;
 }
 
-/*
- * These interfaces resemble the pci_find_*capability() interfaces, but these
- * are for configuring host controllers, which are bridges *to* PCI devices but
- * are not PCI devices themselves.
- */
-static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
-				  u8 cap)
+static int dw_pcie_read_cfg(void *priv, int where, int size, u32 *val)
 {
-	u8 cap_id, next_cap_ptr;
-	u16 reg;
-
-	if (!cap_ptr)
-		return 0;
+	struct dw_pcie *pci = priv;
 
-	reg = dw_pcie_readw_dbi(pci, cap_ptr);
-	cap_id = (reg & 0x00ff);
-
-	if (cap_id > PCI_CAP_ID_MAX)
-		return 0;
-
-	if (cap_id == cap)
-		return cap_ptr;
+	if (size == 4)
+		*val = dw_pcie_readl_dbi(pci, where);
+	else if (size == 2)
+		*val = dw_pcie_readw_dbi(pci, where);
+	else if (size == 1)
+		*val = dw_pcie_readb_dbi(pci, where);
+	else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
 
-	next_cap_ptr = (reg & 0xff00) >> 8;
-	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+	return PCIBIOS_SUCCESSFUL;
 }
 
 u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
 {
-	u8 next_cap_ptr;
-	u16 reg;
-
-	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
-	next_cap_ptr = (reg & 0x00ff);
-
-	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+	return PCI_FIND_NEXT_CAP(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
+				 pci);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
 
-static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
-					    u8 cap)
-{
-	u32 header;
-	int ttl;
-	int pos = PCI_CFG_SPACE_SIZE;
-
-	/* minimum 8 bytes per capability */
-	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
-	if (start)
-		pos = start;
-
-	header = dw_pcie_readl_dbi(pci, pos);
-	/*
-	 * 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;
-
-		header = dw_pcie_readl_dbi(pci, pos);
-	}
-
-	return 0;
-}
-
 u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
 {
-	return dw_pcie_find_next_ext_capability(pci, 0, cap);
+	return PCI_FIND_NEXT_EXT_CAP(dw_pcie_read_cfg, 0, cap, pci);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
 
@@ -302,8 +251,8 @@ static u16 __dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id,
 	if (vendor_id != dw_pcie_readw_dbi(pci, PCI_VENDOR_ID))
 		return 0;
 
-	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
-						       PCI_EXT_CAP_ID_VNDR))) {
+	while ((vsec = PCI_FIND_NEXT_EXT_CAP(dw_pcie_read_cfg, vsec,
+					     PCI_EXT_CAP_ID_VNDR, pci))) {
 		header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
 		if (PCI_VNDR_HEADER_ID(header) == vsec_id)
 			return vsec;
-- 
2.25.1

[PATCH v14 6/7] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang]

Use the PCI core is now exposing generic macros for the host bridges to
search for the PCIe capabilities, make use of them in the CDNS driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
Changes since v13:
- Use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().

Changes since v12:
- Modify the function's return value.

Changes since v11:
- Modify the return value of cdns_pcie_readw from u32 to u16. 
- Modify the return value of cdns_pcie_readb from u32 to u8. 

Changes since v8 ~ v10:
- None

Changes since v7:
- Resolve compilation errors.

Changes since v6:
https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@163.com/

- The patch commit message were modified.

Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com

- Kconfig add "select PCI_HOST_HELPERS"
---
 drivers/pci/controller/cadence/pcie-cadence.c | 30 +++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h | 13 ++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 70a19573440e..7b2955e4fafb 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -8,6 +8,36 @@
 #include <linux/of.h>
 
 #include "pcie-cadence.h"
+#include "../../pci.h"
+
+static int cdns_pcie_read_cfg(void *priv, int where, int size, u32 *val)
+{
+	struct cdns_pcie *pcie = priv;
+
+	if (size == 4)
+		*val = cdns_pcie_readl(pcie, where);
+	else if (size == 2)
+		*val = cdns_pcie_readw(pcie, where);
+	else if (size == 1)
+		*val = cdns_pcie_readb(pcie, where);
+	else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
+{
+	return PCI_FIND_NEXT_CAP(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
+				 cap, pcie);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_find_capability);
+
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
+{
+	return PCI_FIND_NEXT_EXT_CAP(cdns_pcie_read_cfg, 0, cap, pcie);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_find_ext_capability);
 
 void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
 {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index a149845d341a..38f7a8cdf7f1 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -387,6 +387,16 @@ static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
 	return readl(pcie->reg_base + reg);
 }
 
+static inline u16 cdns_pcie_readw(struct cdns_pcie *pcie, u32 reg)
+{
+	return readw(pcie->reg_base + reg);
+}
+
+static inline u8 cdns_pcie_readb(struct cdns_pcie *pcie, u32 reg)
+{
+	return readb(pcie->reg_base + reg);
+}
+
 static inline u32 cdns_pcie_read_sz(void __iomem *addr, int size)
 {
 	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
@@ -556,6 +566,9 @@ static inline void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
 }
 #endif
 
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
+
 void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
 
 void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
-- 
2.25.1

[PATCH v14 7/7] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode

[Hans Zhang]

The PCIe capability/extended capability offsets are not guaranteed to be
the same across all SoCs integrating the Cadence PCIe IP. Hence, use the
cdns_pcie_find_{ext}_capability() APIs for finding them.

This avoids hardcoding the offsets in the driver.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
Changes since v8 ~ v13:
- None

Changes since v7:
- Resolve compilation errors.

Changes since v6:
https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@163.com/

- The patch commit message were modified.

Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com

- Kconfig add "select PCI_HOST_HELPERS"
---
 .../pci/controller/cadence/pcie-cadence-ep.c  | 38 +++++++++++--------
 drivers/pci/controller/cadence/pcie-cadence.h |  5 ---
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 8ab6cf70c18e..4f0d2ee5e513 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -21,12 +21,13 @@
 
 static u8 cdns_pcie_get_fn_from_vfn(struct cdns_pcie *pcie, u8 fn, u8 vfn)
 {
-	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
 	u32 first_vf_offset, stride;
+	u16 cap;
 
 	if (vfn == 0)
 		return fn;
 
+	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
 	first_vf_offset = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_OFFSET);
 	stride = cdns_pcie_ep_fn_readw(pcie, fn, cap +  PCI_SRIOV_VF_STRIDE);
 	fn = fn + first_vf_offset + ((vfn - 1) * stride);
@@ -38,10 +39,11 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
 				     struct pci_epf_header *hdr)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
-	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
 	struct cdns_pcie *pcie = &ep->pcie;
 	u32 reg;
+	u16 cap;
 
+	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
 	if (vfn > 1) {
 		dev_err(&epc->dev, "Only Virtual Function #1 has deviceID\n");
 		return -EINVAL;
@@ -227,9 +229,10 @@ static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 nr_irqs)
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
 	u8 mmc = order_base_2(nr_irqs);
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/*
@@ -249,9 +252,10 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags, mme;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Validate that the MSI feature is actually enabled. */
@@ -272,9 +276,10 @@ static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 val, reg;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	func_no = cdns_pcie_get_fn_from_vfn(pcie, func_no, vfunc_no);
 
 	reg = cap + PCI_MSIX_FLAGS;
@@ -292,9 +297,10 @@ static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u8 vfn,
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 val, reg;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	reg = cap + PCI_MSIX_FLAGS;
@@ -380,11 +386,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 				     u8 interrupt_num)
 {
 	struct cdns_pcie *pcie = &ep->pcie;
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	u16 flags, mme, data, data_mask;
-	u8 msi_count;
 	u64 pci_addr, pci_addr_mask = 0xff;
+	u8 msi_count, cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -432,14 +438,14 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
 				    u32 *msi_addr_offset)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
-	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
 	struct cdns_pcie *pcie = &ep->pcie;
 	u64 pci_addr, pci_addr_mask = 0xff;
 	u16 flags, mme, data, data_mask;
-	u8 msi_count;
+	u8 msi_count, cap;
 	int ret;
 	int i;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
 	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
 
 	/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -482,16 +488,16 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
 static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 				      u16 interrupt_num)
 {
-	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
 	u32 tbl_offset, msg_data, reg;
 	struct cdns_pcie *pcie = &ep->pcie;
 	struct pci_epf_msix_tbl *msix_tbl;
 	struct cdns_pcie_epf *epf;
 	u64 pci_addr_mask = 0xff;
 	u64 msg_addr;
+	u8 bir, cap;
 	u16 flags;
-	u8 bir;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
 	epf = &ep->epf[fn];
 	if (vfn > 0)
 		epf = &epf->epf[vfn - 1];
@@ -565,7 +571,9 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 	int max_epfs = sizeof(epc->function_num_map) * 8;
 	int ret, epf, last_fn;
 	u32 reg, value;
+	u8 cap;
 
+	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
 	/*
 	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
 	 * and can't be disabled anyway.
@@ -589,12 +597,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 				continue;
 
 			value = cdns_pcie_ep_fn_readl(pcie, epf,
-					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
-					PCI_EXP_DEVCAP);
+						      cap + PCI_EXP_DEVCAP);
 			value &= ~PCI_EXP_DEVCAP_FLR;
 			cdns_pcie_ep_fn_writel(pcie, epf,
-					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
-					PCI_EXP_DEVCAP, value);
+					       cap + PCI_EXP_DEVCAP, value);
 		}
 	}
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 38f7a8cdf7f1..f0fdeb3863f1 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -125,11 +125,6 @@
  */
 #define CDNS_PCIE_EP_FUNC_BASE(fn)	(((fn) << 12) & GENMASK(19, 12))
 
-#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET	0x90
-#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET	0xb0
-#define CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET	0xc0
-#define CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET	0x200
-
 /*
  * Endpoint PF Registers
  */
-- 
2.25.1

Re: [PATCH v14 0/7] Refactor capability search into common macros

[Bjorn Helgaas]

On Thu, Jul 17, 2025 at 12:11:56AM +0800, Hans Zhang wrote:
> Dear Maintainers,
> 
> This patch series addresses long-standing code duplication in PCI
> capability discovery logic across the PCI core and controller drivers.
> The existing implementation ties capability search to fully initialized
> PCI device structures, limiting its usability during early controller
> initialization phases where device/bus structures may not yet be
> available.
> 
> The primary goal is to decouple capability discovery from PCI device
> dependencies by introducing a unified framework using config space
> accessor-based macros. This enables:
> 
> 1. Early Capability Discovery: Host controllers (e.g., Cadence, DWC)
> can now perform capability searches during pre-initialization stages
> using their native config accessors.
> 
> 2. Code Consolidation: Common logic for standard and extended capability
> searches is refactored into shared macros (`PCI_FIND_NEXT_CAP` and
> `PCI_FIND_NEXT_EXT_CAP`), eliminating redundant implementations.
> 
> 3. Safety and Maintainability: TTL checks are centralized within the
> macros to prevent infinite loops, while hardcoded offsets in drivers
> are replaced with dynamic discovery, reducing fragility.
> 
> Key improvements include:  
> - Driver Conversions: DesignWare and Cadence drivers are migrated to
>   use the new macros, removing device-specific assumptions and ensuring
>   consistent error handling.
> 
> - Enhanced Readability: Magic numbers are replaced with symbolic
>   constants, and config space accessors are standardized for clarity.
> 
> - Backward Compatibility: Existing PCI core behavior remains unchanged.
> 
> ---
> Changes since v13:
> - Split patch 3/6 into two patches for searching standard and extended capability. (Bjorn)
> - Optimize the code based on the review comments from Bjorn.
> - Patch 5/7 and 6/7 use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().
> - The other patches have not been modified.
> 
> Changes since v12:
> - Modify some commit messages, code format issues, and optimize the function return values.
> 
> Changes since v11:
> - Resolved some compilation warning.
> - Add some include.
> - Add the *** BLURB HERE *** description(Corrected by Mani and Krzysztof).
> 
> Changes since v10:
> - The patch [v10 2/6] remove #include <uapi/linux/pci_regs.h> and add macro definition comments.
> - The patch [v10 3/6] remove #include <uapi/linux/pci_regs.h> and commit message were modified.
> - The other patches have not been modified.
> 
> Changes since v9:
> - Resolved [v9 4/6] compilation error.
>   The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses 
>   dw_pcie_find_next_ext_capability.
> - The other patches have not been modified.
> 
> Changes since v8:
> - Split patch.
> - The patch commit message were modified.
> - Other patches(4/6, 5/6, 6/6) are unchanged.
> 
> Changes since v7:
> - Patch 2/5 and 3/5 compilation error resolved.
> - Other patches are unchanged.
> 
> Changes since v6:
> - Refactor capability search into common macros.
> - Delete pci-host-helpers.c and MAINTAINERS.
> 
> Changes since v5:
> - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
>   the kernel's .text section even if it's known already at compile time
>   that they're never going to be used (e.g. on x86).
> - Move the API for find capabilitys to a new file called
>   pci-host-helpers.c.
> - Add new patch for MAINTAINERS.
> 
> Changes since v4:
> - Resolved [v4 1/4] compilation warning.
> - The patch subject and commit message were modified.
> 
> Changes since v3:
> - Resolved [v3 1/4] compilation error.
> - Other patches are not modified.
> 
> Changes since v2:
> - Add and split into a series of patches.
> ---
> 
> Hans Zhang (7):
>   PCI: Introduce generic bus config read helper function
>   PCI: Clean up __pci_find_next_cap_ttl() readability
>   PCI: Refactor standard capability search into common macro
>   PCI: Refactor extended capability search into common macro
>   PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
>   PCI: cadence: Use common PCI host bridge APIs for finding the
>     capabilities
>   PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode
> 
>  drivers/pci/access.c                          | 15 ++++
>  .../pci/controller/cadence/pcie-cadence-ep.c  | 38 ++++----
>  drivers/pci/controller/cadence/pcie-cadence.c | 30 +++++++
>  drivers/pci/controller/cadence/pcie-cadence.h | 18 ++--
>  drivers/pci/controller/dwc/pcie-designware.c  | 83 ++++--------------
>  drivers/pci/pci.c                             | 76 +++-------------
>  drivers/pci/pci.h                             | 87 +++++++++++++++++++
>  include/uapi/linux/pci_regs.h                 |  3 +
>  8 files changed, 196 insertions(+), 154 deletions(-)
> 
> 
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494

Applied to pci/capability-search for v6.17, thanks for all this work!

[REGRESSION] next/master: suspect endianness issue in common PCI capability search macro

[Gerd Bayer]

On Wed, 2025-07-16 at 18:11 -0500, Bjorn Helgaas wrote:
> On Thu, Jul 17, 2025 at 12:11:56AM +0800, Hans Zhang wrote:

<--- snip --->

> > 
> > 
> > Hans Zhang (7):
> >   PCI: Introduce generic bus config read helper function
> >   PCI: Clean up __pci_find_next_cap_ttl() readability
> >   PCI: Refactor standard capability search into common macro
> >   PCI: Refactor extended capability search into common macro
> >   PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
> >   PCI: cadence: Use common PCI host bridge APIs for finding the
> >     capabilities
> >   PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode
> > 
> >  drivers/pci/access.c                          | 15 ++++
> >  .../pci/controller/cadence/pcie-cadence-ep.c  | 38 ++++----
> >  drivers/pci/controller/cadence/pcie-cadence.c | 30 +++++++
> >  drivers/pci/controller/cadence/pcie-cadence.h | 18 ++--
> >  drivers/pci/controller/dwc/pcie-designware.c  | 83 ++++--------------
> >  drivers/pci/pci.c                             | 76 +++-------------
> >  drivers/pci/pci.h                             | 87 +++++++++++++++++++
> >  include/uapi/linux/pci_regs.h                 |  3 +
> >  8 files changed, 196 insertions(+), 154 deletions(-)
> > 
> > 
> > base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> 
> Applied to pci/capability-search for v6.17, thanks for all this work!

Dear all,

with this series commit 2502a619108b ("PCI: Refactor extended
capability search into PCI_FIND_NEXT_EXT_CAP()")
has landed in linux-next.

This breaks PCI capability search on our s390 test systems - that
showed through mlx5_core's error while binding:

[   27.158991] mlx5_core a000:00:00.0: mlx5_load:1355:(pid 998): Failed
to alloc IRQs

apparently, due to struct pci_dev not showing that it is MSI-X capable.
With this commit reverted, mlx5_core binds successfully again.

I'm sending this as a heads-up while I'll continue to debug this
further - presumably an endianness issue in the macro
PCI_FIND_NEXT_EXT_CAP.

Thanks,
Gerd

[PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Gerd Bayer]

Simple pointer-casts to map byte and word reads from PCI config space
into dwords (i.e. u32) produce unintended results on big-endian systems.
Add the necessary adjustments under compile-time switch
CONFIG_CPU_BIG_ENDIAN.

pci_bus_read_config() was just introduced with
https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---

Hi Hans, hi Bjorn,

Sorry to spill this endianness aware code into drivers/pci, feel free to
suggest a cleaner approach. This has fixed the issues seen on s390 systems
Otherwise it is just compile-tested for x86 and arm64.

Since this is still sitting in the a pull-request for upstream, I'm not sure if this
warrants a Fixes: tag.

Thanks,
Gerd
---
 drivers/pci/access.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba66f55d2524..77a73b772a28 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -89,15 +89,24 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
 			u32 *val)
 {
 	struct pci_bus *bus = priv;
+	int rc;
 
-	if (size == 1)
-		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
-	else if (size == 2)
-		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
-	else if (size == 4)
-		return pci_bus_read_config_dword(bus, devfn, where, val);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
+	if (size == 1) {
+		rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
+#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		*val = ((*val >> 24) & 0xff);
+#endif
+	} else if (size == 2) {
+		rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
+#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		*val = ((*val >> 16) & 0xffff);
+#endif
+	} else if (size == 4) {
+		rc = pci_bus_read_config_dword(bus, devfn, where, val);
+	} else {
+		rc =  PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+	return rc;
 }
 
 int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
-- 
2.48.1

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Bjorn Helgaas]

[+cc Arnd]

On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
> 
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
> 
> Hi Hans, hi Bjorn,
> 
> Sorry to spill this endianness aware code into drivers/pci, feel free to
> suggest a cleaner approach. This has fixed the issues seen on s390 systems
> Otherwise it is just compile-tested for x86 and arm64.
> 
> Since this is still sitting in the a pull-request for upstream, I'm not sure if this
> warrants a Fixes: tag.
> 
> Thanks,
> Gerd
> ---
>  drivers/pci/access.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba66f55d2524..77a73b772a28 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,24 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
>  			u32 *val)
>  {
>  	struct pci_bus *bus = priv;
> +	int rc;
>  
> -	if (size == 1)
> -		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> -	else if (size == 2)
> -		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> -	else if (size == 4)
> -		return pci_bus_read_config_dword(bus, devfn, where, val);
> -	else
> -		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	if (size == 1) {
> +		rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +		*val = ((*val >> 24) & 0xff);
> +#endif

Yeah, this is all pretty ugly.  Obviously the previous code in
__pci_find_next_cap_ttl() didn't need this.  My guess is that was
because the destination for the read data was always the correct type
(u8/u16/u32), but here we always use a u32 and cast it to the
appropriate type.  Maybe we can use the correct types here instead of
the casts?

> +	} else if (size == 2) {
> +		rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +		*val = ((*val >> 16) & 0xffff);
> +#endif
> +	} else if (size == 4) {
> +		rc = pci_bus_read_config_dword(bus, devfn, where, val);
> +	} else {
> +		rc =  PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return rc;
>  }
>  
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> -- 
> 2.48.1
> 

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Lukas Wunner]

On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
> 
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
> Sorry to spill this endianness aware code into drivers/pci, feel free to
> suggest a cleaner approach. This has fixed the issues seen on s390 systems

PCI is little-endian.  On big-endian systems, the endianness conversion
of Config Space accesses happens transparently in the struct pci_ops
->read() and ->write() callbacks.  E.g. on s390, zpci_cfg_load() and
zpci_cfg_store() call le64_to_cpu() and cpu_to_le64(), respectively.

We do not want to mess with endianness in the PCI core, so this isn't
a proper fix IMO.

A viable approach might be to turn pci_bus_read_config() into a macro
in include/linux/pci.h which calls the byte/word/dword variant based
on sizeof(*val) or something like that.

But at this point, with the merge window already open, it's probably
better to drop the pci/capability-search topic branch from the pull
request and retry in the next cycle.

> Since this is still sitting in the a pull-request for upstream,
> I'm not sure if this warrants a Fixes: tag.

In cases like this, do include a Fixes tag but no stable designation.

Thanks,

Lukas

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Arnd Bergmann]

On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
> On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
>>  
>> -	if (size == 1)
>> -		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>> -	else if (size == 2)
>> -		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
>> -	else if (size == 4)
>> -		return pci_bus_read_config_dword(bus, devfn, where, val);
>> -	else
>> -		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	if (size == 1) {
>> +		rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> +		*val = ((*val >> 24) & 0xff);
>> +#endif
>
> Yeah, this is all pretty ugly.  Obviously the previous code in
> __pci_find_next_cap_ttl() didn't need this.  My guess is that was
> because the destination for the read data was always the correct type
> (u8/u16/u32), but here we always use a u32 and cast it to the
> appropriate type.  Maybe we can use the correct types here instead of
> the casts?

Agreed, the casts here just add more potential for bugs.

The pci_bus_read_config() interface itself may have been a
mistake, can't the callers just use the underlying helpers
directly?

      Arnd

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Geert Uytterhoeven]

Hi Gerd,

On Thu, 31 Jul 2025 at 20:57, Gerd Bayer <gbayer@linux.ibm.com> wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
>
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>

Thanks for your patch!

> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,24 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
>                         u32 *val)
>  {
>         struct pci_bus *bus = priv;
> +       int rc;
>
> -       if (size == 1)
> -               return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> -       else if (size == 2)
> -               return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> -       else if (size == 4)
> -               return pci_bus_read_config_dword(bus, devfn, where, val);
> -       else
> -               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       if (size == 1) {
> +               rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +               *val = ((*val >> 24) & 0xff);
> +#endif

IMHO this looks ugly and error-prone.  In addition, it still relies
on the caller initializing the upper bits to zero on little-endian.

What about:

    u8 byte;

    rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
    *val = byte;

> +       } else if (size == 2) {
> +               rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +               *val = ((*val >> 16) & 0xffff);
> +#endif

and:

    u16 word;

    rc = pci_bus_read_config_word(bus, devfn, where, &word);
    *val = word;

> +       } else if (size == 4) {
> +               rc = pci_bus_read_config_dword(bus, devfn, where, val);
> +       } else {
> +               rc =  PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +       return rc;
>  }
>
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Manivannan Sadhasivam]

On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
> On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
> > On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> >>  
> >> -	if (size == 1)
> >> -		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> >> -	else if (size == 2)
> >> -		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> >> -	else if (size == 4)
> >> -		return pci_bus_read_config_dword(bus, devfn, where, val);
> >> -	else
> >> -		return PCIBIOS_BAD_REGISTER_NUMBER;
> >> +	if (size == 1) {
> >> +		rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> >> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> >> +		*val = ((*val >> 24) & 0xff);
> >> +#endif
> >
> > Yeah, this is all pretty ugly.  Obviously the previous code in
> > __pci_find_next_cap_ttl() didn't need this.  My guess is that was
> > because the destination for the read data was always the correct type
> > (u8/u16/u32), but here we always use a u32 and cast it to the
> > appropriate type.  Maybe we can use the correct types here instead of
> > the casts?
> 
> Agreed, the casts here just add more potential for bugs.
> 

Ack. Missed the obvious issue during review.

> The pci_bus_read_config() interface itself may have been a
> mistake, can't the callers just use the underlying helpers
> directly?
> 

They can! Since the callers of this API is mostly the macros, we can easily
implement the logic to call relevant accessors based on the requested size.

Hans, could you please respin the series based the feedback since the series is
dropped for 6.17.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
> EXTERNAL EMAIL
> 
> On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
>> On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
>>> On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
>>>>
>>>> -  if (size == 1)
>>>> -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>> -  else if (size == 2)
>>>> -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
>>>> -  else if (size == 4)
>>>> -          return pci_bus_read_config_dword(bus, devfn, where, val);
>>>> -  else
>>>> -          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>> +  if (size == 1) {
>>>> +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>>>> +          *val = ((*val >> 24) & 0xff);
>>>> +#endif
>>>
>>> Yeah, this is all pretty ugly.  Obviously the previous code in
>>> __pci_find_next_cap_ttl() didn't need this.  My guess is that was
>>> because the destination for the read data was always the correct type
>>> (u8/u16/u32), but here we always use a u32 and cast it to the
>>> appropriate type.  Maybe we can use the correct types here instead of
>>> the casts?
>>
>> Agreed, the casts here just add more potential for bugs.
>>
> 
> Ack. Missed the obvious issue during review.
> 
>> The pci_bus_read_config() interface itself may have been a
>> mistake, can't the callers just use the underlying helpers
>> directly?
>>
> 
> They can! Since the callers of this API is mostly the macros, we can easily
> implement the logic to call relevant accessors based on the requested size.
> 
> Hans, could you please respin the series based the feedback since the series is
> dropped for 6.17.
> 

Dear all,

I am once again deeply sorry for the problems that occurred in this 
series. I only test pulling the ARM platform.

Thank you very much, Gerd, for reporting the problem.

Thank you all for your discussions and suggestions for revision.

Hi Mani,

Geert provided a solution. My patch based on this is as follows. Please 
check if there are any problems.
https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/

Also, please ask Gerd to help test whether it works properly. Thank you 
very much.


If there are no issues, am I sending the new version? Can this series of 
pacth 0001 be directly replaced?




The patch is as follows:
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba66f55d2524..2bfd8fc1c0f5 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -89,15 +89,25 @@ int pci_bus_read_config(void *priv, unsigned int 
devfn, int where, u32 size,
                         u32 *val)
  {
         struct pci_bus *bus = priv;
+       int rc;
+
+       if (size == 1) {
+               u8 byte;
+
+               rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
+               *val = byte;
+       } else if (size == 2) {
+               u16 word;
+
+               rc = pci_bus_read_config_word(bus, devfn, where, &word);
+               *val = word;
+       } else if (size == 4) {
+               rc = pci_bus_read_config_dword(bus, devfn, where, val);
+       } else {
+               rc = PCIBIOS_BAD_REGISTER_NUMBER;
+       }

-       if (size == 1)
-               return pci_bus_read_config_byte(bus, devfn, where, (u8 
*)val);
-       else if (size == 2)
-               return pci_bus_read_config_word(bus, devfn, where, (u16 
*)val);
-       else if (size == 4)
-               return pci_bus_read_config_dword(bus, devfn, where, val);
-       else
-               return PCIBIOS_BAD_REGISTER_NUMBER;
+       return rc;
  }

  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,



Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Manivannan Sadhasivam]

On Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
> 
> 
> On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
> > EXTERNAL EMAIL
> > 
> > On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
> > > On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
> > > > On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> > > > > 
> > > > > -  if (size == 1)
> > > > > -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > -  else if (size == 2)
> > > > > -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> > > > > -  else if (size == 4)
> > > > > -          return pci_bus_read_config_dword(bus, devfn, where, val);
> > > > > -  else
> > > > > -          return PCIBIOS_BAD_REGISTER_NUMBER;
> > > > > +  if (size == 1) {
> > > > > +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> > > > > +          *val = ((*val >> 24) & 0xff);
> > > > > +#endif
> > > > 
> > > > Yeah, this is all pretty ugly.  Obviously the previous code in
> > > > __pci_find_next_cap_ttl() didn't need this.  My guess is that was
> > > > because the destination for the read data was always the correct type
> > > > (u8/u16/u32), but here we always use a u32 and cast it to the
> > > > appropriate type.  Maybe we can use the correct types here instead of
> > > > the casts?
> > > 
> > > Agreed, the casts here just add more potential for bugs.
> > > 
> > 
> > Ack. Missed the obvious issue during review.
> > 
> > > The pci_bus_read_config() interface itself may have been a
> > > mistake, can't the callers just use the underlying helpers
> > > directly?
> > > 
> > 
> > They can! Since the callers of this API is mostly the macros, we can easily
> > implement the logic to call relevant accessors based on the requested size.
> > 
> > Hans, could you please respin the series based the feedback since the series is
> > dropped for 6.17.
> > 
> 
> Dear all,
> 
> I am once again deeply sorry for the problems that occurred in this series.
> I only test pulling the ARM platform.
> 
> Thank you very much, Gerd, for reporting the problem.
> 
> Thank you all for your discussions and suggestions for revision.
> 
> Hi Mani,
> 
> Geert provided a solution. My patch based on this is as follows. Please
> check if there are any problems.
> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
> 
> Also, please ask Gerd to help test whether it works properly. Thank you very
> much.
> 
> 
> If there are no issues, am I sending the new version? Can this series of
> pacth 0001 be directly replaced?
> 

What benefit does this helper provide if it simply invokes the accessors based
on the requested size? IMO, the API should not return 'int' sized value if the
caller has explicitly requested to read variable size from config space.

- Mani

> 
> 
> 
> The patch is as follows:
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba66f55d2524..2bfd8fc1c0f5 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,25 @@ int pci_bus_read_config(void *priv, unsigned int devfn,
> int where, u32 size,
>                         u32 *val)
>  {
>         struct pci_bus *bus = priv;
> +       int rc;
> +
> +       if (size == 1) {
> +               u8 byte;
> +
> +               rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
> +               *val = byte;
> +       } else if (size == 2) {
> +               u16 word;
> +
> +               rc = pci_bus_read_config_word(bus, devfn, where, &word);
> +               *val = word;
> +       } else if (size == 4) {
> +               rc = pci_bus_read_config_dword(bus, devfn, where, val);
> +       } else {
> +               rc = PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> 
> -       if (size == 1)
> -               return pci_bus_read_config_byte(bus, devfn, where, (u8
> *)val);
> -       else if (size == 2)
> -               return pci_bus_read_config_word(bus, devfn, where, (u16
> *)val);
> -       else if (size == 4)
> -               return pci_bus_read_config_dword(bus, devfn, where, val);
> -       else
> -               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       return rc;
>  }
> 
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> 
> 
> 
> Best regards,
> Hans
> 
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/1 17:47, Manivannan Sadhasivam wrote:
> EXTERNAL EMAIL
> 
> On Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
>>
>>
>> On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
>>> EXTERNAL EMAIL
>>>
>>> On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
>>>> On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
>>>>> On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
>>>>>>
>>>>>> -  if (size == 1)
>>>>>> -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>> -  else if (size == 2)
>>>>>> -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
>>>>>> -  else if (size == 4)
>>>>>> -          return pci_bus_read_config_dword(bus, devfn, where, val);
>>>>>> -  else
>>>>>> -          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  if (size == 1) {
>>>>>> +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>>>>>> +          *val = ((*val >> 24) & 0xff);
>>>>>> +#endif
>>>>>
>>>>> Yeah, this is all pretty ugly.  Obviously the previous code in
>>>>> __pci_find_next_cap_ttl() didn't need this.  My guess is that was
>>>>> because the destination for the read data was always the correct type
>>>>> (u8/u16/u32), but here we always use a u32 and cast it to the
>>>>> appropriate type.  Maybe we can use the correct types here instead of
>>>>> the casts?
>>>>
>>>> Agreed, the casts here just add more potential for bugs.
>>>>
>>>
>>> Ack. Missed the obvious issue during review.
>>>
>>>> The pci_bus_read_config() interface itself may have been a
>>>> mistake, can't the callers just use the underlying helpers
>>>> directly?
>>>>
>>>
>>> They can! Since the callers of this API is mostly the macros, we can easily
>>> implement the logic to call relevant accessors based on the requested size.
>>>
>>> Hans, could you please respin the series based the feedback since the series is
>>> dropped for 6.17.
>>>
>>
>> Dear all,
>>
>> I am once again deeply sorry for the problems that occurred in this series.
>> I only test pulling the ARM platform.
>>
>> Thank you very much, Gerd, for reporting the problem.
>>
>> Thank you all for your discussions and suggestions for revision.
>>
>> Hi Mani,
>>
>> Geert provided a solution. My patch based on this is as follows. Please
>> check if there are any problems.
>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>
>> Also, please ask Gerd to help test whether it works properly. Thank you very
>> much.
>>
>>
>> If there are no issues, am I sending the new version? Can this series of
>> pacth 0001 be directly replaced?
>>
> 
> What benefit does this helper provide if it simply invokes the accessors based
> on the requested size? IMO, the API should not return 'int' sized value if the
> caller has explicitly requested to read variable size from config space.
> 

Dear Mani,

This newly added macro definition PCI_FIND_NEXT_CAP is derived from 
__pci_find_next_cap_ttl. Another newly added macro definition, 
PCI_FIND_NEXT_EXT_CAP, is derived from pci_find_next_ext_capability. The 
first one has no return value judgment, while the second one has a 
judgment return value. So, pci_bus_read_config is defined as having an 
int return value.

Best regards,
Hans

> 
>>
>>
>>
>> The patch is as follows:
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index ba66f55d2524..2bfd8fc1c0f5 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -89,15 +89,25 @@ int pci_bus_read_config(void *priv, unsigned int devfn,
>> int where, u32 size,
>>                          u32 *val)
>>   {
>>          struct pci_bus *bus = priv;
>> +       int rc;
>> +
>> +       if (size == 1) {
>> +               u8 byte;
>> +
>> +               rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
>> +               *val = byte;
>> +       } else if (size == 2) {
>> +               u16 word;
>> +
>> +               rc = pci_bus_read_config_word(bus, devfn, where, &word);
>> +               *val = word;
>> +       } else if (size == 4) {
>> +               rc = pci_bus_read_config_dword(bus, devfn, where, val);
>> +       } else {
>> +               rc = PCIBIOS_BAD_REGISTER_NUMBER;
>> +       }
>>
>> -       if (size == 1)
>> -               return pci_bus_read_config_byte(bus, devfn, where, (u8
>> *)val);
>> -       else if (size == 2)
>> -               return pci_bus_read_config_word(bus, devfn, where, (u16
>> *)val);
>> -       else if (size == 4)
>> -               return pci_bus_read_config_dword(bus, devfn, where, val);
>> -       else
>> -               return PCIBIOS_BAD_REGISTER_NUMBER;
>> +       return rc;
>>   }
>>
>>   int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>>
>>
>>
>> Best regards,
>> Hans
>>
>>
>>
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Manivannan Sadhasivam]

On Fri, Aug 01, 2025 at 06:06:16PM GMT, Hans Zhang wrote:
> 
> 
> On 2025/8/1 17:47, Manivannan Sadhasivam wrote:
> > EXTERNAL EMAIL
> > 
> > On Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
> > > 
> > > 
> > > On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
> > > > EXTERNAL EMAIL
> > > > 
> > > > On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
> > > > > On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
> > > > > > On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> > > > > > > 
> > > > > > > -  if (size == 1)
> > > > > > > -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > > > -  else if (size == 2)
> > > > > > > -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> > > > > > > -  else if (size == 4)
> > > > > > > -          return pci_bus_read_config_dword(bus, devfn, where, val);
> > > > > > > -  else
> > > > > > > -          return PCIBIOS_BAD_REGISTER_NUMBER;
> > > > > > > +  if (size == 1) {
> > > > > > > +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > > > +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> > > > > > > +          *val = ((*val >> 24) & 0xff);
> > > > > > > +#endif
> > > > > > 
> > > > > > Yeah, this is all pretty ugly.  Obviously the previous code in
> > > > > > __pci_find_next_cap_ttl() didn't need this.  My guess is that was
> > > > > > because the destination for the read data was always the correct type
> > > > > > (u8/u16/u32), but here we always use a u32 and cast it to the
> > > > > > appropriate type.  Maybe we can use the correct types here instead of
> > > > > > the casts?
> > > > > 
> > > > > Agreed, the casts here just add more potential for bugs.
> > > > > 
> > > > 
> > > > Ack. Missed the obvious issue during review.
> > > > 
> > > > > The pci_bus_read_config() interface itself may have been a
> > > > > mistake, can't the callers just use the underlying helpers
> > > > > directly?
> > > > > 
> > > > 
> > > > They can! Since the callers of this API is mostly the macros, we can easily
> > > > implement the logic to call relevant accessors based on the requested size.
> > > > 
> > > > Hans, could you please respin the series based the feedback since the series is
> > > > dropped for 6.17.
> > > > 
> > > 
> > > Dear all,
> > > 
> > > I am once again deeply sorry for the problems that occurred in this series.
> > > I only test pulling the ARM platform.
> > > 
> > > Thank you very much, Gerd, for reporting the problem.
> > > 
> > > Thank you all for your discussions and suggestions for revision.
> > > 
> > > Hi Mani,
> > > 
> > > Geert provided a solution. My patch based on this is as follows. Please
> > > check if there are any problems.
> > > https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
> > > 
> > > Also, please ask Gerd to help test whether it works properly. Thank you very
> > > much.
> > > 
> > > 
> > > If there are no issues, am I sending the new version? Can this series of
> > > pacth 0001 be directly replaced?
> > > 
> > 
> > What benefit does this helper provide if it simply invokes the accessors based
> > on the requested size? IMO, the API should not return 'int' sized value if the
> > caller has explicitly requested to read variable size from config space.
> > 
> 
> Dear Mani,
> 
> This newly added macro definition PCI_FIND_NEXT_CAP is derived from
> __pci_find_next_cap_ttl. Another newly added macro definition,
> PCI_FIND_NEXT_EXT_CAP, is derived from pci_find_next_ext_capability. The
> first one has no return value judgment, while the second one has a judgment
> return value. So, pci_bus_read_config is defined as having an int return
> value.
> 

Sorry, my previous reply was not clear. I was opposed to returning 'u32 *val'
for a variable 'size' value. The API should only return 'val' of 'size' ie. if
size is 1, it should return 'u8 *val' and so on. It finally breaks down to
calling the underlying accessors. So I don't see a value in having this API.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Gerd Bayer]

On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:

<--- snip --->

> > > > > > The pci_bus_read_config() interface itself may have been a
> > > > > > mistake, can't the callers just use the underlying helpers
> > > > > > directly?
> > > > > > 
> > > > > 
> > > > > They can! Since the callers of this API is mostly the macros, we can easily
> > > > > implement the logic to call relevant accessors based on the requested size.
> > > > > 
> > > > > Hans, could you please respin the series based the feedback since the series is
> > > > > dropped for 6.17.
> > > > > 
> > > > 
> > > > Dear all,
> > > > 
> > > > I am once again deeply sorry for the problems that occurred in this series.
> > > > I only test pulling the ARM platform.
> > > > 
> > > > Thank you very much, Gerd, for reporting the problem.

no worries!

> > > > Thank you all for your discussions and suggestions for revision.
> > > > 
> > > > Hi Mani,
> > > > 
> > > > Geert provided a solution. My patch based on this is as follows. Please
> > > > check if there are any problems.
> > > > https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
> > > > 
> > > > Also, please ask Gerd to help test whether it works properly. Thank you very
> > > > much.
> > > > 

I found Geert's proposal intriguing for a quick resolution of the
issue. Yet, I have not tried that proposal, though.

Instead I spent some more cycles on Lukas' and Mani's question about
the value of the pci_bus_read_config() helper. So I changed
PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
of read_cfg accessor functions like this:

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ac954584d991..9e2f75ede95f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
devfn, int where, u32 size,
 ({                                                                   
\
        int __ttl = PCI_FIND_CAP_TTL;                                 
\
        u8 __id, __found_pos = 0;                                     
\
-       u32 __pos = (start);                                          
\
-       u32 __ent;                                                    
\
+       u8 __pos = (start);                                           
\
+       u16 __ent;                                                    
\
                                                                      
\
-       read_cfg(args, __pos, 1, &__pos);                             
\
+       read_cfg##_byte(args, __pos, &__pos);                         
\
                                                                      
\
        while (__ttl--) {                                             
\
                if (__pos < PCI_STD_HEADER_SIZEOF)                    
\
                        break;                                        
\
                                                                      
\
                __pos = ALIGN_DOWN(__pos, 4);                         
\
-               read_cfg(args, __pos, 2, &__ent);                     
\
+               read_cfg##_word(args, __pos, &__ent);                 
\
                                                                      
\
                __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);             
\
                if (__id == 0xff)                                     
\
@@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
devfn, int where, u32 size,
                                                                      
\
        __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;    
\
        while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {          
\
-               __ret = read_cfg(args, __pos, 4, &__header);          
\
+               __ret = read_cfg##_dword(args, __pos, &__header);     
\
                if (__ret != PCIBIOS_SUCCESSFUL)                      
\
                        break;                                        
\
                                                                      
\


This fixes the issue for s390's use-cases. With that
pci_bus_read_config() becomes unused - and could be removed in further
refinements.
                                                                      
However, this probably breaks your dwc and cdns use-cases. I think,
with the accessor functions for dwc and cadence changed to follow the
{_byte|_word|_dword} naming pattern they could be used straight out of
PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
cdns_pcie_read_cfg become obsolete as well.

Thoughts?

Re: [PATCH v14 0/7] Refactor capability search into common macros

[Bjorn Helgaas]

On Wed, Jul 16, 2025 at 06:11:21PM -0500, Bjorn Helgaas wrote:
> On Thu, Jul 17, 2025 at 12:11:56AM +0800, Hans Zhang wrote:
> > Dear Maintainers,
> > 
> > This patch series addresses long-standing code duplication in PCI
> > capability discovery logic across the PCI core and controller drivers.
> > The existing implementation ties capability search to fully initialized
> > PCI device structures, limiting its usability during early controller
> > initialization phases where device/bus structures may not yet be
> > available.
> > 
> > The primary goal is to decouple capability discovery from PCI device
> > dependencies by introducing a unified framework using config space
> > accessor-based macros. This enables:
> > 
> > 1. Early Capability Discovery: Host controllers (e.g., Cadence, DWC)
> > can now perform capability searches during pre-initialization stages
> > using their native config accessors.
> > 
> > 2. Code Consolidation: Common logic for standard and extended capability
> > searches is refactored into shared macros (`PCI_FIND_NEXT_CAP` and
> > `PCI_FIND_NEXT_EXT_CAP`), eliminating redundant implementations.
> > 
> > 3. Safety and Maintainability: TTL checks are centralized within the
> > macros to prevent infinite loops, while hardcoded offsets in drivers
> > are replaced with dynamic discovery, reducing fragility.
> > 
> > Key improvements include:  
> > - Driver Conversions: DesignWare and Cadence drivers are migrated to
> >   use the new macros, removing device-specific assumptions and ensuring
> >   consistent error handling.
> > 
> > - Enhanced Readability: Magic numbers are replaced with symbolic
> >   constants, and config space accessors are standardized for clarity.
> > 
> > - Backward Compatibility: Existing PCI core behavior remains unchanged.
> > 
> > ---
> > Changes since v13:
> > - Split patch 3/6 into two patches for searching standard and extended capability. (Bjorn)
> > - Optimize the code based on the review comments from Bjorn.
> > - Patch 5/7 and 6/7 use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().
> > - The other patches have not been modified.
> > 
> > Changes since v12:
> > - Modify some commit messages, code format issues, and optimize the function return values.
> > 
> > Changes since v11:
> > - Resolved some compilation warning.
> > - Add some include.
> > - Add the *** BLURB HERE *** description(Corrected by Mani and Krzysztof).
> > 
> > Changes since v10:
> > - The patch [v10 2/6] remove #include <uapi/linux/pci_regs.h> and add macro definition comments.
> > - The patch [v10 3/6] remove #include <uapi/linux/pci_regs.h> and commit message were modified.
> > - The other patches have not been modified.
> > 
> > Changes since v9:
> > - Resolved [v9 4/6] compilation error.
> >   The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses 
> >   dw_pcie_find_next_ext_capability.
> > - The other patches have not been modified.
> > 
> > Changes since v8:
> > - Split patch.
> > - The patch commit message were modified.
> > - Other patches(4/6, 5/6, 6/6) are unchanged.
> > 
> > Changes since v7:
> > - Patch 2/5 and 3/5 compilation error resolved.
> > - Other patches are unchanged.
> > 
> > Changes since v6:
> > - Refactor capability search into common macros.
> > - Delete pci-host-helpers.c and MAINTAINERS.
> > 
> > Changes since v5:
> > - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
> >   the kernel's .text section even if it's known already at compile time
> >   that they're never going to be used (e.g. on x86).
> > - Move the API for find capabilitys to a new file called
> >   pci-host-helpers.c.
> > - Add new patch for MAINTAINERS.
> > 
> > Changes since v4:
> > - Resolved [v4 1/4] compilation warning.
> > - The patch subject and commit message were modified.
> > 
> > Changes since v3:
> > - Resolved [v3 1/4] compilation error.
> > - Other patches are not modified.
> > 
> > Changes since v2:
> > - Add and split into a series of patches.
> > ---
> > 
> > Hans Zhang (7):
> >   PCI: Introduce generic bus config read helper function
> >   PCI: Clean up __pci_find_next_cap_ttl() readability
> >   PCI: Refactor standard capability search into common macro
> >   PCI: Refactor extended capability search into common macro
> >   PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
> >   PCI: cadence: Use common PCI host bridge APIs for finding the
> >     capabilities
> >   PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode
> > 
> >  drivers/pci/access.c                          | 15 ++++
> >  .../pci/controller/cadence/pcie-cadence-ep.c  | 38 ++++----
> >  drivers/pci/controller/cadence/pcie-cadence.c | 30 +++++++
> >  drivers/pci/controller/cadence/pcie-cadence.h | 18 ++--
> >  drivers/pci/controller/dwc/pcie-designware.c  | 83 ++++--------------
> >  drivers/pci/pci.c                             | 76 +++-------------
> >  drivers/pci/pci.h                             | 87 +++++++++++++++++++
> >  include/uapi/linux/pci_regs.h                 |  3 +
> >  8 files changed, 196 insertions(+), 154 deletions(-)
> > 
> > 
> > base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> 
> Applied to pci/capability-search for v6.17, thanks for all this work!

Deferred until the next cycle for the big-endian issue.

Bjorn

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/1 18:54, Manivannan Sadhasivam wrote:
> On Fri, Aug 01, 2025 at 06:06:16PM GMT, Hans Zhang wrote:
>>
>>
>> On 2025/8/1 17:47, Manivannan Sadhasivam wrote:
>>> EXTERNAL EMAIL
>>>
>>> On Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
>>>>
>>>>
>>>> On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
>>>>> EXTERNAL EMAIL
>>>>>
>>>>> On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
>>>>>> On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
>>>>>>> On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
>>>>>>>>
>>>>>>>> -  if (size == 1)
>>>>>>>> -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>>>> -  else if (size == 2)
>>>>>>>> -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
>>>>>>>> -  else if (size == 4)
>>>>>>>> -          return pci_bus_read_config_dword(bus, devfn, where, val);
>>>>>>>> -  else
>>>>>>>> -          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>>>> +  if (size == 1) {
>>>>>>>> +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>>>> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>>>>>>>> +          *val = ((*val >> 24) & 0xff);
>>>>>>>> +#endif
>>>>>>>
>>>>>>> Yeah, this is all pretty ugly.  Obviously the previous code in
>>>>>>> __pci_find_next_cap_ttl() didn't need this.  My guess is that was
>>>>>>> because the destination for the read data was always the correct type
>>>>>>> (u8/u16/u32), but here we always use a u32 and cast it to the
>>>>>>> appropriate type.  Maybe we can use the correct types here instead of
>>>>>>> the casts?
>>>>>>
>>>>>> Agreed, the casts here just add more potential for bugs.
>>>>>>
>>>>>
>>>>> Ack. Missed the obvious issue during review.
>>>>>
>>>>>> The pci_bus_read_config() interface itself may have been a
>>>>>> mistake, can't the callers just use the underlying helpers
>>>>>> directly?
>>>>>>
>>>>>
>>>>> They can! Since the callers of this API is mostly the macros, we can easily
>>>>> implement the logic to call relevant accessors based on the requested size.
>>>>>
>>>>> Hans, could you please respin the series based the feedback since the series is
>>>>> dropped for 6.17.
>>>>>
>>>>
>>>> Dear all,
>>>>
>>>> I am once again deeply sorry for the problems that occurred in this series.
>>>> I only test pulling the ARM platform.
>>>>
>>>> Thank you very much, Gerd, for reporting the problem.
>>>>
>>>> Thank you all for your discussions and suggestions for revision.
>>>>
>>>> Hi Mani,
>>>>
>>>> Geert provided a solution. My patch based on this is as follows. Please
>>>> check if there are any problems.
>>>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>>>
>>>> Also, please ask Gerd to help test whether it works properly. Thank you very
>>>> much.
>>>>
>>>>
>>>> If there are no issues, am I sending the new version? Can this series of
>>>> pacth 0001 be directly replaced?
>>>>
>>>
>>> What benefit does this helper provide if it simply invokes the accessors based
>>> on the requested size? IMO, the API should not return 'int' sized value if the
>>> caller has explicitly requested to read variable size from config space.
>>>
>>
>> Dear Mani,
>>
>> This newly added macro definition PCI_FIND_NEXT_CAP is derived from
>> __pci_find_next_cap_ttl. Another newly added macro definition,
>> PCI_FIND_NEXT_EXT_CAP, is derived from pci_find_next_ext_capability. The
>> first one has no return value judgment, while the second one has a judgment
>> return value. So, pci_bus_read_config is defined as having an int return
>> value.
>>
> 
> Sorry, my previous reply was not clear. I was opposed to returning 'u32 *val'
> for a variable 'size' value. The API should only return 'val' of 'size' ie. if
> size is 1, it should return 'u8 *val' and so on. It finally breaks down to
> calling the underlying accessors. So I don't see a value in having this API.

Dear Mani,

In this series, I had similar confusion before.
https://lore.kernel.org/linux-pci/4d77e199-8df8-4510-ad49-9a452a29c923@163.com/


I think there are a few pieces of code that stand out, such as:

Forced type conversion is also used here. (*value = (type)data;)


drivers/pci/access.c
#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
{									\
	unsigned long flags;						\
	u32 data = 0;							\
	int res;							\
									\
	if (PCI_##size##_BAD)						\
		return PCIBIOS_BAD_REGISTER_NUMBER;			\
									\
	pci_lock_config(flags);						\
	res = bus->ops->read(bus, devfn, pos, len, &data);		\
	if (res)							\
		PCI_SET_ERROR_RESPONSE(value);				\
	else								\
		*value = (type)data;					\
	pci_unlock_config(flags);					\
									\
	return res;							\
}


This function also uses u32 *val as its return value.

int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
			    int where, int size, u32 *val)
{
	void __iomem *addr;

	addr = bus->ops->map_bus(bus, devfn, where);
	if (!addr)
		return PCIBIOS_DEVICE_NOT_FOUND;

	if (size == 1)
		*val = readb(addr);
	else if (size == 2)
		*val = readw(addr);
	else
		*val = readl(addr);

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(pci_generic_config_read);


And it's the same here.
drivers/pci/controller/dwc/pcie-designware.c
int dw_pcie_read(void __iomem *addr, int size, u32 *val)
{
	if (!IS_ALIGNED((uintptr_t)addr, size)) {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	if (size == 4) {
		*val = readl(addr);
	} else if (size == 2) {
		*val = readw(addr);
	} else if (size == 1) {
		*val = readb(addr);
	} else {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(dw_pcie_read);


Mani, I'm not here to refute you. I just want to ask if there are bugs 
everywhere here?

I think it's a good idea as mentioned in Gerd's latest reply email. For 
dw_pcie_read_cfg() and cdns_pcie_read_cfg, I can delete it and provide 
the macro definition function of {_byte/_word/_dword}.

Similar to this macro definition:
PCI_OP_READ(byte, u8, 1)
PCI_OP_READ(word, u16, 2)
PCI_OP_READ(dword, u32, 4)
https://lore.kernel.org/linux-pci/06f16b1a55eede3dc3e0bf31ff14eca89ab6f009.camel@linux.ibm.com/


Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/1 19:30, Gerd Bayer wrote:
> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> 
> <--- snip --->
> 
>>>>>>> The pci_bus_read_config() interface itself may have been a
>>>>>>> mistake, can't the callers just use the underlying helpers
>>>>>>> directly?
>>>>>>>
>>>>>>
>>>>>> They can! Since the callers of this API is mostly the macros, we can easily
>>>>>> implement the logic to call relevant accessors based on the requested size.
>>>>>>
>>>>>> Hans, could you please respin the series based the feedback since the series is
>>>>>> dropped for 6.17.
>>>>>>
>>>>>
>>>>> Dear all,
>>>>>
>>>>> I am once again deeply sorry for the problems that occurred in this series.
>>>>> I only test pulling the ARM platform.
>>>>>
>>>>> Thank you very much, Gerd, for reporting the problem.
> 
> no worries!
> 
>>>>> Thank you all for your discussions and suggestions for revision.
>>>>>
>>>>> Hi Mani,
>>>>>
>>>>> Geert provided a solution. My patch based on this is as follows. Please
>>>>> check if there are any problems.
>>>>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>>>>
>>>>> Also, please ask Gerd to help test whether it works properly. Thank you very
>>>>> much.
>>>>>
> 
> I found Geert's proposal intriguing for a quick resolution of the
> issue. Yet, I have not tried that proposal, though.
> 

Hi Gerd,

As I mentioned in my reply to Mani's email, the data ultimately read 
here is also a forced type conversion.

#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
{									\
	unsigned long flags;						\
	u32 data = 0;							\
	int res;							\
									\
	if (PCI_##size##_BAD)						\
		return PCIBIOS_BAD_REGISTER_NUMBER;			\
									\
	pci_lock_config(flags);						\
	res = bus->ops->read(bus, devfn, pos, len, &data);		\
	if (res)							\
		PCI_SET_ERROR_RESPONSE(value);				\
	else								\
		*value = (type)data;					\
	pci_unlock_config(flags);					\
									\
	return res;							\
}

And this function. Could it be that I misunderstood something?

int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
			    int where, int size, u32 *val)
{
	void __iomem *addr;

	addr = bus->ops->map_bus(bus, devfn, where);
	if (!addr)
		return PCIBIOS_DEVICE_NOT_FOUND;

	if (size == 1)
		*val = readb(addr);
	else if (size == 2)
		*val = readw(addr);
	else
		*val = readl(addr);

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(pci_generic_config_read);

> Instead I spent some more cycles on Lukas' and Mani's question about
> the value of the pci_bus_read_config() helper. So I changed
> PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
> of read_cfg accessor functions like this:
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ac954584d991..9e2f75ede95f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>   ({
> \
>          int __ttl = PCI_FIND_CAP_TTL;
> \
>          u8 __id, __found_pos = 0;
> \
> -       u32 __pos = (start);
> \
> -       u32 __ent;
> \
> +       u8 __pos = (start);
> \
> +       u16 __ent;
> \
>                                                                        
> \
> -       read_cfg(args, __pos, 1, &__pos);
> \
> +       read_cfg##_byte(args, __pos, &__pos);
> \
>                                                                        
> \
>          while (__ttl--) {
> \
>                  if (__pos < PCI_STD_HEADER_SIZEOF)
> \
>                          break;
> \
>                                                                        
> \
>                  __pos = ALIGN_DOWN(__pos, 4);
> \
> -               read_cfg(args, __pos, 2, &__ent);
> \
> +               read_cfg##_word(args, __pos, &__ent);
> \
>                                                                        
> \
>                  __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);
> \
>                  if (__id == 0xff)
> \
> @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>                                                                        
> \
>          __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> \
>          while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {
> \
> -               __ret = read_cfg(args, __pos, 4, &__header);
> \
> +               __ret = read_cfg##_dword(args, __pos, &__header);
> \
>                  if (__ret != PCIBIOS_SUCCESSFUL)
> \
>                          break;
> \
>                                                                        
> \
> 
> 
> This fixes the issue for s390's use-cases. With that
> pci_bus_read_config() becomes unused - and could be removed in further
> refinements.
>                                                                        
> However, this probably breaks your dwc and cdns use-cases. I think,
> with the accessor functions for dwc and cadence changed to follow the
> {_byte|_word|_dword} naming pattern they could be used straight out of
> PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
> cdns_pcie_read_cfg become obsolete as well.
> 
> Thoughts?

In my opinion, it's no problem. I can provide the corresponding function 
of {_byte / _word / _dword}. But it is necessary to know Bjorn, Mani, 
Arnd, Lukas... Their viewpoints or suggestions.

Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Keith Busch]

On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
> As I mentioned in my reply to Mani's email, the data ultimately read here is
> also a forced type conversion.
> 
> #define PCI_OP_READ(size, type, len) \
> int noinline pci_bus_read_config_##size \
> 	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
> {									\
> 	unsigned long flags;						\
> 	u32 data = 0;							\
> 	int res;							\
> 									\
> 	if (PCI_##size##_BAD)						\
> 		return PCIBIOS_BAD_REGISTER_NUMBER;			\
> 									\
> 	pci_lock_config(flags);						\
> 	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> 	if (res)							\
> 		PCI_SET_ERROR_RESPONSE(value);				\
> 	else								\
> 		*value = (type)data;					\
> 	pci_unlock_config(flags);					\
> 									\
> 	return res;							\
> }
> 
> And this function. Could it be that I misunderstood something?

The above macro retains the caller's type for "value". If the caller
passes a "u8 *", the value is deferenced as a u8.

The function below promotes everything to a u32 pointer and deferences
it as such regardless of what type the user passed in.
 
> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> 			    int where, int size, u32 *val)
> {
> 	void __iomem *addr;
> 
> 	addr = bus->ops->map_bus(bus, devfn, where);
> 	if (!addr)
> 		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> 	if (size == 1)
> 		*val = readb(addr);
> 	else if (size == 2)
> 		*val = readw(addr);
> 	else
> 		*val = readl(addr);
> 
> 	return PCIBIOS_SUCCESSFUL;
> }

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/2 02:08, Keith Busch wrote:
> On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
>> As I mentioned in my reply to Mani's email, the data ultimately read here is
>> also a forced type conversion.
>>
>> #define PCI_OP_READ(size, type, len) \
>> int noinline pci_bus_read_config_##size \
>> 	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
>> {									\
>> 	unsigned long flags;						\
>> 	u32 data = 0;							\
>> 	int res;							\
>> 									\
>> 	if (PCI_##size##_BAD)						\
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;			\
>> 									\
>> 	pci_lock_config(flags);						\
>> 	res = bus->ops->read(bus, devfn, pos, len, &data);		\
>> 	if (res)							\
>> 		PCI_SET_ERROR_RESPONSE(value);				\
>> 	else								\
>> 		*value = (type)data;					\
>> 	pci_unlock_config(flags);					\
>> 									\
>> 	return res;							\
>> }
>>
>> And this function. Could it be that I misunderstood something?
> 
> The above macro retains the caller's type for "value". If the caller
> passes a "u8 *", the value is deferenced as a u8.

Dear Keith,

In this macro definition, bus->ops->read needs to ensure the byte order 
of the read, as Lukas mentioned; otherwise, there is also a big-endian 
issue at this location.

> 
> The function below promotes everything to a u32 pointer and deferences
> it as such regardless of what type the user passed in.

I searched and learned that readb/readw/readl automatically handle byte 
order, so there is no big-endian order issue.

>   
>> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>> 			    int where, int size, u32 *val)
>> {
>> 	void __iomem *addr;
>>
>> 	addr = bus->ops->map_bus(bus, devfn, where);
>> 	if (!addr)
>> 		return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> 	if (size == 1)
>> 		*val = readb(addr);
>> 	else if (size == 2)
>> 		*val = readw(addr);
>> 	else
>> 		*val = readl(addr);
>>
>> 	return PCIBIOS_SUCCESSFUL;
>> }


Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Arnd Bergmann]

On Sat, Aug 2, 2025, at 17:23, Hans Zhang wrote:
> On 2025/8/2 02:08, Keith Busch wrote:
>> On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
>>>
>>> 		*value = (type)data;					\
>>>
>>> And this function. Could it be that I misunderstood something?
>> 
>> The above macro retains the caller's type for "value". If the caller
>> passes a "u8 *", the value is deferenced as a u8.
>
> In this macro definition, bus->ops->read needs to ensure the byte order 
> of the read, as Lukas mentioned; otherwise, there is also a big-endian 
> issue at this location.

No, there is no endianness problem here, the problem with casting
the pointer type like

      u32 *value;
      *(type *)value = data;

or any variation of that is is that it only writes to the first
few bytes of *value, and that introduces both the observed endianess
problem and possibly worse uninitialized data usage or out-of-bounds
stack access.

      Arnd

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/1 19:30, Gerd Bayer wrote:
> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> 
> <--- snip --->
> 
>>>>>>> The pci_bus_read_config() interface itself may have been a
>>>>>>> mistake, can't the callers just use the underlying helpers
>>>>>>> directly?
>>>>>>>
>>>>>>
>>>>>> They can! Since the callers of this API is mostly the macros, we can easily
>>>>>> implement the logic to call relevant accessors based on the requested size.
>>>>>>
>>>>>> Hans, could you please respin the series based the feedback since the series is
>>>>>> dropped for 6.17.
>>>>>>
>>>>>
>>>>> Dear all,
>>>>>
>>>>> I am once again deeply sorry for the problems that occurred in this series.
>>>>> I only test pulling the ARM platform.
>>>>>
>>>>> Thank you very much, Gerd, for reporting the problem.
> 
> no worries!
> 
>>>>> Thank you all for your discussions and suggestions for revision.
>>>>>
>>>>> Hi Mani,
>>>>>
>>>>> Geert provided a solution. My patch based on this is as follows. Please
>>>>> check if there are any problems.
>>>>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>>>>
>>>>> Also, please ask Gerd to help test whether it works properly. Thank you very
>>>>> much.
>>>>>
> 
> I found Geert's proposal intriguing for a quick resolution of the
> issue. Yet, I have not tried that proposal, though.
> 
> Instead I spent some more cycles on Lukas' and Mani's question about
> the value of the pci_bus_read_config() helper. So I changed
> PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
> of read_cfg accessor functions like this:
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ac954584d991..9e2f75ede95f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>   ({
> \
>          int __ttl = PCI_FIND_CAP_TTL;
> \
>          u8 __id, __found_pos = 0;
> \
> -       u32 __pos = (start);
> \
> -       u32 __ent;
> \
> +       u8 __pos = (start);
> \
> +       u16 __ent;
> \
>                                                                        
> \
> -       read_cfg(args, __pos, 1, &__pos);
> \
> +       read_cfg##_byte(args, __pos, &__pos);
> \
>                                                                        
> \
>          while (__ttl--) {
> \
>                  if (__pos < PCI_STD_HEADER_SIZEOF)
> \
>                          break;
> \
>                                                                        
> \
>                  __pos = ALIGN_DOWN(__pos, 4);
> \
> -               read_cfg(args, __pos, 2, &__ent);
> \
> +               read_cfg##_word(args, __pos, &__ent);
> \
>                                                                        
> \
>                  __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);
> \
>                  if (__id == 0xff)
> \
> @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>                                                                        
> \
>          __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> \
>          while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {
> \
> -               __ret = read_cfg(args, __pos, 4, &__header);
> \
> +               __ret = read_cfg##_dword(args, __pos, &__header);
> \
>                  if (__ret != PCIBIOS_SUCCESSFUL)
> \
>                          break;
> \
>                                                                        
> \
> 
> 
> This fixes the issue for s390's use-cases. With that
> pci_bus_read_config() becomes unused - and could be removed in further
> refinements.
>                                                                        
> However, this probably breaks your dwc and cdns use-cases. I think,
> with the accessor functions for dwc and cadence changed to follow the
> {_byte|_word|_dword} naming pattern they could be used straight out of
> PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
> cdns_pcie_read_cfg become obsolete as well.
> 
> Thoughts?

Dear all,

According to the issue mentioned by Lukas and Mani. Gerd has already 
been tested on the s390. I have tested it on the RK3588 and it works 
fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our 
company's is based on Cadence's PCIe 4.0 IP, and the test function is 
normal. All the platforms I tested were based on ARM.

The following is the patch based on the capability-search branch. May I 
ask everyone, do you have any more questions?

Gerd, if there's no problem, I'll add your Tested-by label.

Branch: 
ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search


Patch:
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba66f55d2524..b123da16b63b 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,21 +85,6 @@ 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;
-
-	if (size == 1)
-		return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
-	else if (size == 2)
-		return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
-	else if (size == 4)
-		return pci_bus_read_config_dword(bus, devfn, where, val);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-}
-
  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
  			    int where, int size, u32 *val)
  {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c 
b/drivers/pci/controller/cadence/pcie-cadence.c
index 7b2955e4fafb..c45585ae1746 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -10,22 +10,6 @@
  #include "pcie-cadence.h"
  #include "../../pci.h"

-static int cdns_pcie_read_cfg(void *priv, int where, int size, u32 *val)
-{
-	struct cdns_pcie *pcie = priv;
-
-	if (size == 4)
-		*val = cdns_pcie_readl(pcie, where);
-	else if (size == 2)
-		*val = cdns_pcie_readw(pcie, where);
-	else if (size == 1)
-		*val = cdns_pcie_readb(pcie, where);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
  u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
  {
  	return PCI_FIND_NEXT_CAP(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h 
b/drivers/pci/controller/cadence/pcie-cadence.h
index f0fdeb3863f1..4ad874e68783 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -392,6 +392,26 @@ static inline u8 cdns_pcie_readb(struct cdns_pcie 
*pcie, u32 reg)
  	return readb(pcie->reg_base + reg);
  }

+#define CDNS_PCI_OP_READ(size, type, len)		\
+static inline int cdns_pcie_read_cfg_##size		\
+	(struct cdns_pcie *pcie, int where, type *val)	\
+{							\
+	if (len == 4)					\
+		*val = cdns_pcie_readl(pcie, where);	\
+	else if (len == 2)				\
+		*val = cdns_pcie_readw(pcie, where);	\
+	else if (len == 1)				\
+		*val = cdns_pcie_readb(pcie, where);	\
+	else						\
+		return PCIBIOS_BAD_REGISTER_NUMBER;	\
+							\
+	return PCIBIOS_SUCCESSFUL;			\
+}
+
+CDNS_PCI_OP_READ(byte, u8, 1)
+CDNS_PCI_OP_READ(word, u16, 2)
+CDNS_PCI_OP_READ(dword, u32, 4)
+
  static inline u32 cdns_pcie_read_sz(void __iomem *addr, int size)
  {
  	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c 
b/drivers/pci/controller/dwc/pcie-designware.c
index b503cb4bcb28..befb9df3123f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -213,22 +213,6 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
  		pci->type = ver;
  }

-static int dw_pcie_read_cfg(void *priv, int where, int size, u32 *val)
-{
-	struct dw_pcie *pci = priv;
-
-	if (size == 4)
-		*val = dw_pcie_readl_dbi(pci, where);
-	else if (size == 2)
-		*val = dw_pcie_readw_dbi(pci, where);
-	else if (size == 1)
-		*val = dw_pcie_readb_dbi(pci, where);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
  {
  	return PCI_FIND_NEXT_CAP(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h 
b/drivers/pci/controller/dwc/pcie-designware.h
index ce9e18554e42..3b429a8ade70 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -614,6 +614,26 @@ static inline void dw_pcie_writel_dbi2(struct 
dw_pcie *pci, u32 reg, u32 val)
  	dw_pcie_write_dbi2(pci, reg, 0x4, val);
  }

+#define DW_PCI_OP_READ(size, type, len)			\
+static inline int dw_pcie_read_cfg_##size		\
+	(struct dw_pcie *pci, int where, type *val)	\
+{							\
+	if (len == 4)					\
+		*val = dw_pcie_readl_dbi(pci, where);	\
+	else if (len == 2)				\
+		*val = dw_pcie_readw_dbi(pci, where);	\
+	else if (len == 1)				\
+		*val = dw_pcie_readb_dbi(pci, where);	\
+	else						\
+		return PCIBIOS_BAD_REGISTER_NUMBER;	\
+							\
+	return PCIBIOS_SUCCESSFUL;			\
+}
+
+DW_PCI_OP_READ(byte, u8, 1)
+DW_PCI_OP_READ(word, u16, 2)
+DW_PCI_OP_READ(dword, u32, 4)
+
  static inline unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep 
*ep,
  						     u8 func_no)
  {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e53706d1d806..9c410e47e19a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,8 +92,6 @@ 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);

  /* Standard Capability finder */
  /**
@@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int 
devfn, int where, u32 size,
  ({									\
  	int __ttl = PCI_FIND_CAP_TTL;					\
  	u8 __id, __found_pos = 0;					\
-	u32 __pos = (start);						\
-	u32 __ent;							\
+	u8 __pos = (start);						\
+	u16 __ent;							\
  									\
-	read_cfg(args, __pos, 1, &__pos);				\
+	read_cfg##_byte(args, __pos, &__pos);				\
  									\
  	while (__ttl--) {						\
  		if (__pos < PCI_STD_HEADER_SIZEOF)			\
  			break;						\
  									\
  		__pos = ALIGN_DOWN(__pos, 4);				\
-		read_cfg(args, __pos, 2, &__ent);			\
+		read_cfg##_word(args, __pos, &__ent);			\
  									\
  		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
  		if (__id == 0xff)					\
@@ -161,7 +159,7 @@ int pci_bus_read_config(void *priv, unsigned int 
devfn, int where, u32 size,
  									\
  	__ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;	\
  	while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {		\
-		__ret = read_cfg(args, __pos, 4, &__header);		\
+		__ret = read_cfg##_dword(args, __pos, &__header);	\
  		if (__ret != PCIBIOS_SUCCESSFUL)			\
  			break;						\
  									\



Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Arnd Bergmann]

On Mon, Aug 4, 2025, at 05:06, Hans Zhang wrote:
> On 2025/8/1 19:30, Gerd Bayer wrote:
>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>   }
>
> +#define CDNS_PCI_OP_READ(size, type, len)		\
> +static inline int cdns_pcie_read_cfg_##size		\
> +	(struct cdns_pcie *pcie, int where, type *val)	\
> +{							\
> +	if (len == 4)					\
> +		*val = cdns_pcie_readl(pcie, where);	\
> +	else if (len == 2)				\
> +		*val = cdns_pcie_readw(pcie, where);	\
> +	else if (len == 1)				\
> +		*val = cdns_pcie_readb(pcie, where);	\
> +	else						\
> +		return PCIBIOS_BAD_REGISTER_NUMBER;	\
> +							\
> +	return PCIBIOS_SUCCESSFUL;			\
> +}
> +
> +CDNS_PCI_OP_READ(byte, u8, 1)
> +CDNS_PCI_OP_READ(word, u16, 2)
> +CDNS_PCI_OP_READ(dword, u32, 4)
> +

This is fine for the calling conventions, but the use of a macro
doesn't really help readability, so I'd suggest just having
separate inline functions if they are even needed.

> @@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int 
> devfn, int where, u32 size,
>   ({									\
>   	int __ttl = PCI_FIND_CAP_TTL;					\
>   	u8 __id, __found_pos = 0;					\
> -	u32 __pos = (start);						\
> -	u32 __ent;							\
> +	u8 __pos = (start);						\
> +	u16 __ent;							\
>   									\
> -	read_cfg(args, __pos, 1, &__pos);				\
> +	read_cfg##_byte(args, __pos, &__pos);				\
>   									\
>   	while (__ttl--) {						\
>   		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>   			break;						\
>   									\
>   		__pos = ALIGN_DOWN(__pos, 4);				\
> -		read_cfg(args, __pos, 2, &__ent);			\
> +		read_cfg##_word(args, __pos, &__ent);			\
>   									\
>   		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>   		if (__id == 0xff)					\

I still don't feel great about this macro either, and suspect
we should have a better abstraction with fixed types and a
global function to do it, but I don't see anything obviously
wrong here either.

     Arnd

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/4 16:03, Arnd Bergmann wrote:
> On Mon, Aug 4, 2025, at 05:06, Hans Zhang wrote:
>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>    }
>>
>> +#define CDNS_PCI_OP_READ(size, type, len)		\
>> +static inline int cdns_pcie_read_cfg_##size		\
>> +	(struct cdns_pcie *pcie, int where, type *val)	\
>> +{							\
>> +	if (len == 4)					\
>> +		*val = cdns_pcie_readl(pcie, where);	\
>> +	else if (len == 2)				\
>> +		*val = cdns_pcie_readw(pcie, where);	\
>> +	else if (len == 1)				\
>> +		*val = cdns_pcie_readb(pcie, where);	\
>> +	else						\
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;	\
>> +							\
>> +	return PCIBIOS_SUCCESSFUL;			\
>> +}
>> +
>> +CDNS_PCI_OP_READ(byte, u8, 1)
>> +CDNS_PCI_OP_READ(word, u16, 2)
>> +CDNS_PCI_OP_READ(dword, u32, 4)
>> +
> 
> This is fine for the calling conventions, but the use of a macro
> doesn't really help readability, so I'd suggest just having
> separate inline functions if they are even needed.
> 

Dear Arnd,

Thank you very much for your reply.

Will change.

>> @@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int
>> devfn, int where, u32 size,
>>    ({									\
>>    	int __ttl = PCI_FIND_CAP_TTL;					\
>>    	u8 __id, __found_pos = 0;					\
>> -	u32 __pos = (start);						\
>> -	u32 __ent;							\
>> +	u8 __pos = (start);						\
>> +	u16 __ent;							\
>>    									\
>> -	read_cfg(args, __pos, 1, &__pos);				\
>> +	read_cfg##_byte(args, __pos, &__pos);				\
>>    									\
>>    	while (__ttl--) {						\
>>    		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>>    			break;						\
>>    									\
>>    		__pos = ALIGN_DOWN(__pos, 4);				\
>> -		read_cfg(args, __pos, 2, &__ent);			\
>> +		read_cfg##_word(args, __pos, &__ent);			\
>>    									\
>>    		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>>    		if (__id == 0xff)					\
> 
> I still don't feel great about this macro either, and suspect
> we should have a better abstraction with fixed types and a
> global function to do it, but I don't see anything obviously
> wrong here either.


Here is the history of communication with Bjorn and Ilpo. Because 
variable parameters need to be used, otherwise it will be very difficult 
to unify. I'll also think about your proposal again.


Bjorn:
https://lore.kernel.org/linux-pci/20250715224705.GA2504490@bhelgaas/
 > > I would like this a lot better if it could be implemented as a
 > > function, but I assume it has to be a macro for some varargs reason.
 > >
 >
Hans:
https://lore.kernel.org/linux-pci/903ea9c4-87d6-45a8-9825-4a0d45ec608f@163.com/
 > Yes. The macro definitions used in combination with the previous review
 > opinions of Ilpo.

Ilpo:
https://lore.kernel.org/linux-pci/d59fde6e-3e4a-248a-ae56-c35b4c6ec44c@linux.intel.com/
The other option would be to standardize the accessor interface signature
and pass the function as a pointer. One might immediately think of generic
PCI core accessors making it sound simpler than it is but here the
complication is the controller drivers want to pass some internal
structure due to lack of pci_dev so it would need to be void
*read_cfg_data. Then we'd need to deal with those voids also in some
generic PCI accessors which is a bit ugly.


Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Gerd Bayer]

On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
> 
> On 2025/8/1 19:30, Gerd Bayer wrote:
> > On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> > 
> > <--- snip --->
> > 
> > > > > 
> 
> Dear all,
> 
> According to the issue mentioned by Lukas and Mani. Gerd has already 
> been tested on the s390. I have tested it on the RK3588 and it works 
> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our 
> company's is based on Cadence's PCIe 4.0 IP, and the test function is 
> normal. All the platforms I tested were based on ARM.
> 
> The following is the patch based on the capability-search branch. May I 
> ask everyone, do you have any more questions?
> 
> Gerd, if there's no problem, I'll add your Tested-by label.

Before you add that I'd like to re-test with the "final" patch.

> Branch: 
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> 
> Patch:

<--- snip --->

Please bear with me while I'm working on that.

Thanks, Gerd

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Bjorn Helgaas]

On Mon, Aug 04, 2025 at 11:06:36AM +0800, Hans Zhang wrote:
> ...

> According to the issue mentioned by Lukas and Mani. Gerd has already been
> tested on the s390. I have tested it on the RK3588 and it works fine. RK3588
> uses Synopsys' PCIe IP, that is, the DWC driver. Our company's is based on
> Cadence's PCIe 4.0 IP, and the test function is normal. All the platforms I
> tested were based on ARM.
> 
> The following is the patch based on the capability-search branch. May I ask
> everyone, do you have any more questions?
> 
> Gerd, if there's no problem, I'll add your Tested-by label.
> 
> Branch: ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search

Since this series will now target v6.18, I'll watch for a complete v15
series based on v6.17-rc1, with this fix and any typo or other fixes
from pci/capability-search fully integrated.

Then that series can be tested and completely replace the current
pci/capability-search branch.

Bjorn

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/4 22:33, Bjorn Helgaas wrote:
> On Mon, Aug 04, 2025 at 11:06:36AM +0800, Hans Zhang wrote:
>> ...
> 
>> According to the issue mentioned by Lukas and Mani. Gerd has already been
>> tested on the s390. I have tested it on the RK3588 and it works fine. RK3588
>> uses Synopsys' PCIe IP, that is, the DWC driver. Our company's is based on
>> Cadence's PCIe 4.0 IP, and the test function is normal. All the platforms I
>> tested were based on ARM.
>>
>> The following is the patch based on the capability-search branch. May I ask
>> everyone, do you have any more questions?
>>
>> Gerd, if there's no problem, I'll add your Tested-by label.
>>
>> Branch: ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> 
> Since this series will now target v6.18, I'll watch for a complete v15
> series based on v6.17-rc1, with this fix and any typo or other fixes
> from pci/capability-search fully integrated.
> 
> Then that series can be tested and completely replace the current
> pci/capability-search branch.
> 

Dear Bjorn,

Here is the patch based on pci/capability-search branch. It's to discuss 
clearly how the final v15 should be modified. The final plan has been 
confirmed. I will submit the v15 version of this series based on v6.17-rc1.

Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/4 18:09, Gerd Bayer wrote:
> On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
>>
>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>>
>>> <--- snip --->
>>>
>>>>>>
>>
>> Dear all,
>>
>> According to the issue mentioned by Lukas and Mani. Gerd has already
>> been tested on the s390. I have tested it on the RK3588 and it works
>> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
>> company's is based on Cadence's PCIe 4.0 IP, and the test function is
>> normal. All the platforms I tested were based on ARM.
>>
>> The following is the patch based on the capability-search branch. May I
>> ask everyone, do you have any more questions?
>>
>> Gerd, if there's no problem, I'll add your Tested-by label.
> 
> Before you add that I'd like to re-test with the "final" patch.
> 
>> Branch:
>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
>>
>> Patch:
> 
> <--- snip --->
> 
> Please bear with me while I'm working on that.


Dear Gerd,

May I ask if there is any update?



I plan to submit the v15 version of my series based on v6.17-rc1.
The modification method is like the previous comment:
https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/

Best regards,
Hans

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Niklas Schnelle]

On Tue, 2025-08-12 at 22:44 +0800, Hans Zhang wrote:
> 
> On 2025/8/4 18:09, Gerd Bayer wrote:
> > On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
> > > 
> > > On 2025/8/1 19:30, Gerd Bayer wrote:
> > > > On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> > > > 
> > > > <--- snip --->
> > > > 
> > > > > > > 
> > > 
> > > Dear all,
> > > 
> > > According to the issue mentioned by Lukas and Mani. Gerd has already
> > > been tested on the s390. I have tested it on the RK3588 and it works
> > > fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
> > > company's is based on Cadence's PCIe 4.0 IP, and the test function is
> > > normal. All the platforms I tested were based on ARM.
> > > 
> > > The following is the patch based on the capability-search branch. May I
> > > ask everyone, do you have any more questions?
> > > 
> > > Gerd, if there's no problem, I'll add your Tested-by label.
> > 
> > Before you add that I'd like to re-test with the "final" patch.
> > 
> > > Branch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> > > 
> > > Patch:
> > 
> > <--- snip --->
> > 
> > Please bear with me while I'm working on that.
> 
> 
> Dear Gerd,
> 
> May I ask if there is any update?
> 
> 
> 
> I plan to submit the v15 version of my series based on v6.17-rc1.
> The modification method is like the previous comment:
> https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/
> 
> Best regards,
> Hans
> 

Hi Hans,

Gerd is currently out so I just gave the patch you provided against
capability-search-v14 a try on s390. Didn't see any issues with the
previously broken device probing. As I understand it Bjorn asked you to
send a complete v15 and then for people to test that. I like that
approach and would prefer to provide a Tested-by for v15 rather than
via a patch on top. Gerd should be back next week too. Does that work
for you?

Thanks,
Niklas Schnelle

Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang]

On 2025/8/13 15:47, Niklas Schnelle wrote:
> On Tue, 2025-08-12 at 22:44 +0800, Hans Zhang wrote:
>>
>> On 2025/8/4 18:09, Gerd Bayer wrote:
>>> On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
>>>>
>>>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>>>>
>>>>> <--- snip --->
>>>>>
>>>>>>>>
>>>>
>>>> Dear all,
>>>>
>>>> According to the issue mentioned by Lukas and Mani. Gerd has already
>>>> been tested on the s390. I have tested it on the RK3588 and it works
>>>> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
>>>> company's is based on Cadence's PCIe 4.0 IP, and the test function is
>>>> normal. All the platforms I tested were based on ARM.
>>>>
>>>> The following is the patch based on the capability-search branch. May I
>>>> ask everyone, do you have any more questions?
>>>>
>>>> Gerd, if there's no problem, I'll add your Tested-by label.
>>>
>>> Before you add that I'd like to re-test with the "final" patch.
>>>
>>>> Branch:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
>>>>
>>>> Patch:
>>>
>>> <--- snip --->
>>>
>>> Please bear with me while I'm working on that.
>>
>>
>> Dear Gerd,
>>
>> May I ask if there is any update?
>>
>>
>>
>> I plan to submit the v15 version of my series based on v6.17-rc1.
>> The modification method is like the previous comment:
>> https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/
>>
>> Best regards,
>> Hans
>>
> 
> Hi Hans,
> 
> Gerd is currently out so I just gave the patch you provided against
> capability-search-v14 a try on s390. Didn't see any issues with the
> previously broken device probing. As I understand it Bjorn asked you to
> send a complete v15 and then for people to test that. I like that
> approach and would prefer to provide a Tested-by for v15 rather than
> via a patch on top. Gerd should be back next week too. Does that work
> for you?
> 

Hi Niklas,

Ok, no problem. I'm starting to prepare the patch for V15 now.

Best regards,
Hans