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

[Ilpo Järvinen]

On Mon, 24 Mar 2025, Hans Zhang wrote:
> 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?

Just make it only Kconfig select'able and not user selectable at all.

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

__pci_find_next_cap_ttl() does not take pci_dev so I'm unsure if the 
problem is real or not?!?

> 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.

At minimum, the loop in __pci_find_next_cap_ttl() could be turned into a 
macro similar to eg. read_poll_timeout() that takes the read function as 
an argument (read_poll_timeout() looks messy because it doesn't align 
backslashed to far right). That would avoid duplicating the parsing logic
on C code level.

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

-- 
 i.