Re: [v5 1/4] PCI: Introduce generic capability search functions

[Hans Zhang <18255117159@163.com>]

On 2025/3/22 01:06, Lukas Wunner wrote:
> On Sat, Mar 22, 2025 at 12:38:00AM +0800, Hans Zhang wrote:
>> Existing controller drivers (e.g., DWC, custom out-of-tree drivers)
>> duplicate logic for scanning PCI capability lists. This creates
>> maintenance burdens and risks inconsistencies.
>>
>> To resolve this:
>>
>> Add pci_host_bridge_find_*capability() in drivers/pci/pci.c, accepting
>> controller-specific read functions and device data as parameters.
> [...]
>>   drivers/pci/pci.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++
> 
> Please put this in a .c file which is only compiled and linked if
> one of the controller drivers using those new helpers is enabled
> in .config.
> 
> 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).
> 

Hi Lukas,

Thanks your for reply. Increasing the size of the .text section was not 
my intention. I see what you mean.


> You could put them in drivers/pci/controller/pci-host-common.c
> and then select PCI_HOST_COMMON for each driver using them.
> Or put them in a separate completely new file.
> 


I add a drivers/pci/controller/pci-host-helpers.c file, how do you like 
it? Below, I have rearranged the patch, please kindly review it, thank 
you very much.

> 
>>   include/linux/pci.h | 16 ++++++++-
> 
> Helpers that are only used internally in the PCI core should be
> declared in drivers/pci/pci.h.  I'd assume this also applies to
> helpers used by controller drivers.
> 

Will change.

> Thanks,
> 
> Lukas

Next version patch:

  drivers/pci/controller/Kconfig            | 16 ++++
  drivers/pci/controller/Makefile           |  1 +
  drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
  drivers/pci/pci.h                         |  7 ++
  4 files changed, 122 insertions(+)
  create mode 100644 drivers/pci/controller/pci-host-helpers.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 9800b7681054..662c775999a1 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -132,6 +132,22 @@ config PCI_HOST_GENERIC
  	  Say Y here if you want to support a simple generic PCI host
  	  controller, such as the one emulated by kvmtool.

+config PCI_HOST_HELPERS
+ 	bool "PCI Host Controller Helper Functions"
+ 	help
+	  This provides common infrastructure for PCI host controller drivers to
+	  handle PCI capability scanning and other shared operations. The helper
+	  functions eliminate code duplication across controller drivers.
+
+	  These functions are used by PCI controller drivers that need to scan
+	  PCI capabilities using controller-specific access methods (e.g. when
+	  the controller is behind a non-standard configuration space).
+
+	  If you are using any PCI host controller drivers that require these
+	  helpers (such as DesignWare, Cadence, etc), this will be
+	  automatically selected. Say N unless you are developing a custom PCI
+	  host controller driver.
+
  config PCIE_HISI_ERR
  	depends on ACPI_APEI_GHES && (ARM64 || COMPILE_TEST)
  	bool "HiSilicon HIP PCIe controller error handling driver"
diff --git a/drivers/pci/controller/Makefile 
b/drivers/pci/controller/Makefile
index 038ccbd9e3ba..e80091eb7597 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o 
pcie-rcar-host.o
  obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
  obj-$(CONFIG_PCI_HOST_COMMON) += pci-host-common.o
  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
+obj-$(CONFIG_PCI_HOST_HELPERS) += pci-host-helpers.o
  obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
diff --git a/drivers/pci/controller/pci-host-helpers.c 
b/drivers/pci/controller/pci-host-helpers.c
new file mode 100644
index 000000000000..cd261a281c60
--- /dev/null
+++ b/drivers/pci/controller/pci-host-helpers.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Host Controller Helper Functions
+ *
+ * Copyright (C) 2025 Hans Zhang
+ *
+ * Author: Hans Zhang <18255117159@163.com>
+ */
+
+#include <linux/pci.h>
+
+#include "../pci.h"
+
+/*
+ * 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 __pci_host_bridge_find_next_cap(void *priv,
+					  pci_host_bridge_read_cfg read_cfg,
+					  u8 cap_ptr, u8 cap)
+{
+	u8 cap_id, next_cap_ptr;
+	u16 reg;
+
+	if (!cap_ptr)
+		return 0;
+
+	reg = read_cfg(priv, cap_ptr, 2);
+	cap_id = (reg & 0x00ff);
+
+	if (cap_id > PCI_CAP_ID_MAX)
+		return 0;
+
+	if (cap_id == cap)
+		return cap_ptr;
+
+	next_cap_ptr = (reg & 0xff00) >> 8;
+	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
+					       cap);
+}
+
+u8 pci_host_bridge_find_capability(void *priv,
+				   pci_host_bridge_read_cfg read_cfg, u8 cap)
+{
+	u8 next_cap_ptr;
+	u16 reg;
+
+	reg = read_cfg(priv, PCI_CAPABILITY_LIST, 2);
+	next_cap_ptr = (reg & 0x00ff);
+
+	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
+					       cap);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_find_capability);
+
+static u16 pci_host_bridge_find_next_ext_capability(
+	void *priv, pci_host_bridge_read_cfg read_cfg, 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 = read_cfg(priv, pos, 4);
+	/*
+	 * 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 = read_cfg(priv, pos, 4);
+	}
+
+	return 0;
+}
+
+u16 pci_host_bridge_find_ext_capability(void *priv,
+					pci_host_bridge_read_cfg read_cfg,
+					u8 cap)
+{
+	return pci_host_bridge_find_next_ext_capability(priv, read_cfg, 0, cap);
+}
+EXPORT_SYMBOL_GPL(pci_host_bridge_find_ext_capability);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 01e51db8d285..8d1c919cbfef 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -1034,4 +1034,11 @@ void pcim_release_region(struct pci_dev *pdev, 
int bar);
  	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
  	 PCI_CONF1_EXT_REG(reg))

+typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size);
+u8 pci_host_bridge_find_capability(void *priv,
+				   pci_host_bridge_read_cfg read_cfg, u8 cap);
+u16 pci_host_bridge_find_ext_capability(void *priv,
+					pci_host_bridge_read_cfg read_cfg,
+					u8 cap);
+
  #endif /* DRIVERS_PCI_H */


Best regards,
Hans