Re: [v6 1/5] PCI: Introduce generic capability search functions

[Ilpo Järvinen]

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.

--
 i.

> +
>  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?

> +}
> +
> +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 */
> 

-- 
 i.