| Message ID | 20250321163803.391056-2-18255117159@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Introduce generic capability search functions | expand |
On Sat, Mar 22, 2025 at 12:38:00AM +0800, 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 drivers/pci/pci.c, accepting > controller-specific read functions and device data as parameters. [...] > drivers/pci/pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ Please put this in a .c file which is only compiled and linked if one of the controller drivers using those new helpers is enabled in .config. 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). You could put them in drivers/pci/controller/pci-host-common.c and then select PCI_HOST_COMMON for each driver using them. Or put them in a separate completely new file. > include/linux/pci.h | 16 ++++++++- Helpers that are only used internally in the PCI core should be declared in drivers/pci/pci.h. I'd assume this also applies to helpers used by controller drivers. Thanks, Lukas
Hi Hans,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a1cffe8cc8aef85f1b07c4464f0998b9785b795a]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250322-004312
base: a1cffe8cc8aef85f1b07c4464f0998b9785b795a
patch link: https://lore.kernel.org/r/20250321163803.391056-2-18255117159%40163.com
patch subject: [v5 1/4] PCI: Introduce generic capability search functions
config: arm-randconfig-001-20250322 (https://download.01.org/0day-ci/archive/20250322/202503220356.59PxEhDx-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220356.59PxEhDx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503220356.59PxEhDx-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from arch/arm/mm/iomap.c:9:0:
include/linux/pci.h:2025:1: error: expected identifier or '(' before '{' token
{ return 0; }
^
include/linux/pci.h:2029:1: error: expected identifier or '(' before '{' token
{ return 0; }
^
In file included from arch/arm/mm/iomap.c:9:0:
>> include/linux/pci.h:2023:1: warning: 'pci_host_bridge_find_capability' declared 'static' but never defined [-Wunused-function]
pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pci.h:2027:1: warning: 'pci_host_bridge_find_ext_capability' declared 'static' but never defined [-Wunused-function]
pci_host_bridge_find_ext_capability(void *priv,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +2023 include/linux/pci.h
2000
2001 static inline void pci_set_master(struct pci_dev *dev) { }
2002 static inline void pci_clear_master(struct pci_dev *dev) { }
2003 static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
2004 static inline void pci_disable_device(struct pci_dev *dev) { }
2005 static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
2006 static inline int pci_assign_resource(struct pci_dev *dev, int i)
2007 { return -EBUSY; }
2008 static inline int __must_check __pci_register_driver(struct pci_driver *drv,
2009 struct module *owner,
2010 const char *mod_name)
2011 { return 0; }
2012 static inline int pci_register_driver(struct pci_driver *drv)
2013 { return 0; }
2014 static inline void pci_unregister_driver(struct pci_driver *drv) { }
2015 static inline u8 pci_find_capability(struct pci_dev *dev, int cap)
2016 { return 0; }
2017 static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post, int cap)
2018 { return 0; }
2019 static inline u16 pci_find_ext_capability(struct pci_dev *dev, int cap)
2020 { return 0; }
2021 typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size);
2022 static inline u8
> 2023 pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg,
2024 u8 cap);
> 2025 { return 0; }
2026 static inline u16
> 2027 pci_host_bridge_find_ext_capability(void *priv,
2028 pci_host_bridge_read_cfg read_cfg, u8 cap);
> 2029 { return 0; }
2030 static inline u64 pci_get_dsn(struct pci_dev *dev)
2031 { return 0; }
2032
Hi Hans, kernel test robot noticed the following build errors: [auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a] url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250322-004312 base: a1cffe8cc8aef85f1b07c4464f0998b9785b795a patch link: https://lore.kernel.org/r/20250321163803.391056-2-18255117159%40163.com patch subject: [v5 1/4] PCI: Introduce generic capability search functions config: arm64-randconfig-001-20250322 (https://download.01.org/0day-ci/archive/20250322/202503220409.NDrvLkQF-lkp@intel.com/config) compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220409.NDrvLkQF-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503220409.NDrvLkQF-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/hv/vmbus_drv.c:38: >> include/linux/pci.h:2025:1: error: expected identifier or '(' 2025 | { return 0; } | ^ include/linux/pci.h:2029:1: error: expected identifier or '(' 2029 | { return 0; } | ^ drivers/hv/vmbus_drv.c:1928:42: warning: shift count >= width of type [-Wshift-count-overflow] 1928 | dma_set_mask(&child_device_obj->device, DMA_BIT_MASK(64)); | ^~~~~~~~~~~~~~~~ include/linux/dma-mapping.h:73:54: note: expanded from macro 'DMA_BIT_MASK' 73 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1)) | ^ ~~~ 1 warning and 2 errors generated. vim +2025 include/linux/pci.h 2000 2001 static inline void pci_set_master(struct pci_dev *dev) { } 2002 static inline void pci_clear_master(struct pci_dev *dev) { } 2003 static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; } 2004 static inline void pci_disable_device(struct pci_dev *dev) { } 2005 static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; } 2006 static inline int pci_assign_resource(struct pci_dev *dev, int i) 2007 { return -EBUSY; } 2008 static inline int __must_check __pci_register_driver(struct pci_driver *drv, 2009 struct module *owner, 2010 const char *mod_name) 2011 { return 0; } 2012 static inline int pci_register_driver(struct pci_driver *drv) 2013 { return 0; } 2014 static inline void pci_unregister_driver(struct pci_driver *drv) { } 2015 static inline u8 pci_find_capability(struct pci_dev *dev, int cap) 2016 { return 0; } 2017 static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post, int cap) 2018 { return 0; } 2019 static inline u16 pci_find_ext_capability(struct pci_dev *dev, int cap) 2020 { return 0; } 2021 typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size); 2022 static inline u8 2023 pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg, 2024 u8 cap); > 2025 { return 0; } 2026 static inline u16 2027 pci_host_bridge_find_ext_capability(void *priv, 2028 pci_host_bridge_read_cfg read_cfg, u8 cap); 2029 { return 0; } 2030 static inline u64 pci_get_dsn(struct pci_dev *dev) 2031 { return 0; } 2032
Hi Hans, kernel test robot noticed the following build errors: [auto build test ERROR on a1cffe8cc8aef85f1b07c4464f0998b9785b795a] url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Introduce-generic-capability-search-functions/20250322-004312 base: a1cffe8cc8aef85f1b07c4464f0998b9785b795a patch link: https://lore.kernel.org/r/20250321163803.391056-2-18255117159%40163.com patch subject: [v5 1/4] PCI: Introduce generic capability search functions config: arm-randconfig-001-20250322 (https://download.01.org/0day-ci/archive/20250322/202503220416.dfoSTxfs-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220416.dfoSTxfs-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503220416.dfoSTxfs-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from arch/arm/mm/iomap.c:9:0: >> include/linux/pci.h:2025:1: error: expected identifier or '(' before '{' token { return 0; } ^ include/linux/pci.h:2029:1: error: expected identifier or '(' before '{' token { return 0; } ^ In file included from arch/arm/mm/iomap.c:9:0: include/linux/pci.h:2023:1: warning: 'pci_host_bridge_find_capability' declared 'static' but never defined [-Wunused-function] pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/pci.h:2027:1: warning: 'pci_host_bridge_find_ext_capability' declared 'static' but never defined [-Wunused-function] pci_host_bridge_find_ext_capability(void *priv, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +2025 include/linux/pci.h 2000 2001 static inline void pci_set_master(struct pci_dev *dev) { } 2002 static inline void pci_clear_master(struct pci_dev *dev) { } 2003 static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; } 2004 static inline void pci_disable_device(struct pci_dev *dev) { } 2005 static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; } 2006 static inline int pci_assign_resource(struct pci_dev *dev, int i) 2007 { return -EBUSY; } 2008 static inline int __must_check __pci_register_driver(struct pci_driver *drv, 2009 struct module *owner, 2010 const char *mod_name) 2011 { return 0; } 2012 static inline int pci_register_driver(struct pci_driver *drv) 2013 { return 0; } 2014 static inline void pci_unregister_driver(struct pci_driver *drv) { } 2015 static inline u8 pci_find_capability(struct pci_dev *dev, int cap) 2016 { return 0; } 2017 static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post, int cap) 2018 { return 0; } 2019 static inline u16 pci_find_ext_capability(struct pci_dev *dev, int cap) 2020 { return 0; } 2021 typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size); 2022 static inline u8 2023 pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg, 2024 u8 cap); > 2025 { return 0; } 2026 static inline u16 2027 pci_host_bridge_find_ext_capability(void *priv, 2028 pci_host_bridge_read_cfg read_cfg, u8 cap); 2029 { return 0; } 2030 static inline u64 pci_get_dsn(struct pci_dev *dev) 2031 { return 0; } 2032
On 2025/3/22 01:06, Lukas Wunner wrote: > On Sat, Mar 22, 2025 at 12:38:00AM +0800, 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 drivers/pci/pci.c, accepting >> controller-specific read functions and device data as parameters. > [...] >> drivers/pci/pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ > > Please put this in a .c file which is only compiled and linked if > one of the controller drivers using those new helpers is enabled > in .config. > > 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). > Hi Lukas, Thanks your for reply. Increasing the size of the .text section was not my intention. I see what you mean. > You could put them in drivers/pci/controller/pci-host-common.c > and then select PCI_HOST_COMMON for each driver using them. > Or put them in a separate completely new file. > I add a drivers/pci/controller/pci-host-helpers.c file, how do you like it? Below, I have rearranged the patch, please kindly review it, thank you very much. > >> include/linux/pci.h | 16 ++++++++- > > Helpers that are only used internally in the PCI core should be > declared in drivers/pci/pci.h. I'd assume this also applies to > helpers used by controller drivers. > Will change. > Thanks, > > Lukas Next version patch: drivers/pci/controller/Kconfig | 16 ++++ drivers/pci/controller/Makefile | 1 + drivers/pci/controller/pci-host-helpers.c | 98 +++++++++++++++++++++++ drivers/pci/pci.h | 7 ++ 4 files changed, 122 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..662c775999a1 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -132,6 +132,22 @@ 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 "PCI Host Controller Helper Functions" + 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. + 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@163.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); +} + +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 */ Best regards, Hans
On Sat, Mar 22, 2025 at 11:47:18PM +0800, Hans Zhang wrote: > On 2025/3/22 01:06, Lukas Wunner wrote: > > On Sat, Mar 22, 2025 at 12:38:00AM +0800, Hans Zhang wrote: > > > Add pci_host_bridge_find_*capability() in drivers/pci/pci.c, accepting > > > controller-specific read functions and device data as parameters. > > > > Please put this in a .c file which is only compiled and linked if > > one of the controller drivers using those new helpers is enabled > > in .config. > > > > 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). > > > > You could put them in drivers/pci/controller/pci-host-common.c > > and then select PCI_HOST_COMMON for each driver using them. > > Or put them in a separate completely new file. > > I add a drivers/pci/controller/pci-host-helpers.c file, how do you like it? > Below, I have rearranged the patch, please kindly review it, thank you very > much. Looks fine to me, but I'm not one of the maintainers for the controller drivers, they may have a different opinion. I'd recommending submitting this and see if any of them doesn't like it. Just one nit that caught my eye: > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -132,6 +132,22 @@ 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 "PCI Host Controller Helper Functions" > + 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. I like the comprehensive help text, but I'd recommend removing the input prompt "PCI Host Controller Helper Functions" after the "bool". I think generally only menu entries should be displayed that are relevant for end users. The prompt is only needed for developers and they can easily modify Kconfig to select the item when they add their driver. If you absolutely positively want to have a prompt, I'd recommend hiding the menu entry unless CONFIG_EXPERT is also enabled, i.e.: bool prompt "PCI Host Controller Helper Functions" if EXPERT Or maybe there's something better than CONFIG_EXPERT for cases like this, dunno. Thanks, Lukas
On 2025/3/23 00:11, Lukas Wunner wrote: > On Sat, Mar 22, 2025 at 11:47:18PM +0800, Hans Zhang wrote: >> On 2025/3/22 01:06, Lukas Wunner wrote: >>> On Sat, Mar 22, 2025 at 12:38:00AM +0800, Hans Zhang wrote: >>>> Add pci_host_bridge_find_*capability() in drivers/pci/pci.c, accepting >>>> controller-specific read functions and device data as parameters. >>> >>> Please put this in a .c file which is only compiled and linked if >>> one of the controller drivers using those new helpers is enabled >>> in .config. >>> >>> 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). >>> >>> You could put them in drivers/pci/controller/pci-host-common.c >>> and then select PCI_HOST_COMMON for each driver using them. >>> Or put them in a separate completely new file. >> >> I add a drivers/pci/controller/pci-host-helpers.c file, how do you like it? >> Below, I have rearranged the patch, please kindly review it, thank you very >> much. > > Looks fine to me, but I'm not one of the maintainers for the controller > drivers, they may have a different opinion. I'd recommending submitting > this and see if any of them doesn't like it. > Hi Lukas, Thanks your for reply. I will submit it in the next version. > Just one nit that caught my eye: > >> --- a/drivers/pci/controller/Kconfig >> +++ b/drivers/pci/controller/Kconfig >> @@ -132,6 +132,22 @@ 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 "PCI Host Controller Helper Functions" >> + 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. > > I like the comprehensive help text, but I'd recommend removing the > input prompt "PCI Host Controller Helper Functions" after the "bool". > > I think generally only menu entries should be displayed that are relevant > for end users. The prompt is only needed for developers and they can > easily modify Kconfig to select the item when they add their driver. > > If you absolutely positively want to have a prompt, I'd recommend > hiding the menu entry unless CONFIG_EXPERT is also enabled, i.e.: > > bool > prompt "PCI Host Controller Helper Functions" if EXPERT > > Or maybe there's something better than CONFIG_EXPERT for cases like this, > dunno. > Well, thank you for your advice. Best regards, Hans
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 869d204a70a3..5ed31d723a45 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -612,6 +612,92 @@ u16 pci_find_ext_capability(struct pci_dev *dev, int cap) } EXPORT_SYMBOL_GPL(pci_find_ext_capability); +/* + * 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); +} + +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); + /** * pci_get_dsn - Read and return the 8-byte Device Serial Number * @dev: PCI device to query diff --git a/include/linux/pci.h b/include/linux/pci.h index 47b31ad724fa..e4e8d437a864 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1205,6 +1205,12 @@ u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap); u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap); u16 pci_find_ext_capability(struct pci_dev *dev, int cap); u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 pos, int cap); +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); struct pci_bus *pci_find_next_bus(const struct pci_bus *from); u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap); u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec); @@ -2012,7 +2018,15 @@ static inline u8 pci_find_next_capability(struct pci_dev *dev, u8 post, int cap) { return 0; } static inline u16 pci_find_ext_capability(struct pci_dev *dev, int cap) { return 0; } - +typedef u32 (*pci_host_bridge_read_cfg)(void *priv, int where, int size); +static inline u8 +pci_host_bridge_find_capability(void *priv, pci_host_bridge_read_cfg read_cfg, + u8 cap); +{ return 0; } +static inline u16 +pci_host_bridge_find_ext_capability(void *priv, + pci_host_bridge_read_cfg read_cfg, u8 cap); +{ return 0; } static inline u64 pci_get_dsn(struct pci_dev *dev) { return 0; }
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 drivers/pci/pci.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@163.com> --- 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/pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 16 ++++++++- 2 files changed, 101 insertions(+), 1 deletion(-)