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.
