Re: [v6 1/5] PCI: Introduce generic capability search functions
From: Hans Zhang
Date: Mon Mar 24 2025 - 10:40:42 EST
On 2025/3/24 21:28, Ilpo Järvinen wrote:
On Mon, 24 Mar 2025, 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 pci-host-helpers.c, accepting
controller-specific read functions and device data as parameters.
This approach:
- Centralizes critical PCI capability scanning logic
- Allows flexible adaptation to varied hardware access methods
- Reduces future maintenance overhead
- Aligns with kernel code reuse best practices
Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
---
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@xxxxxxx
- 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.
Changes since v4:
https://lore.kernel.org/linux-pci/20250321101710.371480-2-18255117159@xxxxxxx
- Resolved [v4 1/4] compilation warning.
- The patch commit message were modified.
---
drivers/pci/controller/Kconfig | 17 ++++
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++
drivers/pci/pci.h | 7 ++
4 files changed, 123 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..0020a892a55b 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -132,6 +132,23 @@ 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
+ prompt "PCI Host Controller Helper Functions" if EXPERT
+ 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.
Hi,
Does this need to be user selectable at all? What's the benefit? If
somebody is developing a driver, they can just as well add the select
clause in that driver to get it built.
Dear Ilpo,
Thanks your for reply. Only DWC and CDNS drivers are used here, what do
you suggest should be done?
+
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@xxxxxxx>
+ */
+
+#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);
This is doing (tail) recursion?? Why??
What should be done, IMO, is that code in __pci_find_next_cap_ttl()
refactored such that it can be reused instead of duplicating it in a
slightly different form here and the functions below.
The capability list parser should be the same?
The original function is in the following file:
drivers/pci/controller/dwc/pcie-designware.c
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
CDNS has the same need to find the offset of the capability.
We don't have pci_dev before calling pci_host_probe, but we want to get
the offset of the capability and configure some registers to initialize
the root port. Therefore, the __pci_find_next_cap_ttl function cannot be
used. This is also the purpose of dw_pcie_find_*capability.
The CDNS driver does not have a cdns_pcie_find_*capability function.
Therefore, separate the find capability, and then DWC and CDNS can be
used at the same time to reduce duplicate code.
Communication history:
Bjorn HelgaasMarch 14, 2025, 8:31 p.m. UTC | #8
On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote:
> ...
> Even though this patch is mostly for an out of tree controller
> driver which is not going to be upstreamed, the patch itself is
> serving some purpose. I really like to avoid the hardcoded offsets
> wherever possible. So I'm in favor of this patch.
>
> However, these newly introduced functions are a duplicated version
> of DWC functions. So we will end up with duplicated functions in
> multiple places. I'd like them to be moved (both this and DWC) to
> drivers/pci/pci.c if possible. The generic function
> *_find_capability() can accept the controller specific readl/ readw
> APIs and the controller specific private data.
I agree, it would be really nice to share this code.
It looks a little messy to deal with passing around pointers to
controller read ops, and we'll still end up with a lot of duplicated
code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(),
etc.
Maybe someday we'll make a generic way to access non-PCI "config"
space like this host controller space and PCIe RCRBs.
Or if you add interfaces that accept read/write ops, maybe the
existing pci_find_capability() etc could be refactored on top of them
by passing in pci_bus_read_config_word() as the accessor.
+}
+
+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);
Best regards,
Hans