| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f89f3d00-4423-f65d-293e-8aec3be14418@linux.intel.com>
Date: Mon, 24 Mar 2025 15:28:55 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 1/5] PCI: Introduce generic capability search functions
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@....com>
> ---
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-2-18255117159@163.com
>
> - 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@163.com
>
> - 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@....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);
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.
Powered by blists - more mailing lists