linux-kernel - Re: [v7 1/5] PCI: Refactor capability search into common macros

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <909653ac-7ba2-9da7-f519-3d849146f433@linux.intel.com>
Date: Wed, 2 Apr 2025 15:42:14 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, bhelgaas@...gle.com, kw@...ux.com, 
    manivannan.sadhasivam@...aro.org, robh@...nel.org, jingoohan1@...il.com, 
    thomas.richard@...tlin.com, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v7 1/5] PCI: Refactor capability search into common macros

On Wed, 2 Apr 2025, Hans Zhang wrote:

> Introduce PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY macros
> to consolidate duplicate PCI capability search logic found throughout the
> driver tree. This refactoring:
> 
>   1. Eliminates code duplication in capability scanning routines
>   2. Provides a standardized, maintainable implementation
>   3. Reduces error-prone copy-paste implementations
>   4. Maintains identical functionality to existing code
> 
> The macros abstract the low-level capability register scanning while
> preserving the existing PCI configuration space access patterns. They will
> enable future conversions of multiple capability search implementations
> across various drivers (e.g., PCI core, controller drivers) to use
> this centralized logic.
> 
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
>  drivers/pci/pci.h             | 81 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h |  2 +
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e9cf26a9ee9..f705b8bd3084 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>  bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
>  bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>  
> +/* Standard Capability finder */
> +/**
> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search
> + * @cap: Capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Iterates through the capability list in PCI config space to find
> + * the specified capability. Implements TTL (time-to-live) protection
> + * against infinite loops.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
> +({									\
> +	u8 __pos = (start);						\
> +	int __ttl = PCI_FIND_CAP_TTL;					\
> +	u16 __ent;							\
> +	u8 __found_pos = 0;						\
> +	u8 __id;							\
> +									\
> +	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
> +									\
> +	while (__ttl--) {						\
> +		if (__pos < PCI_STD_HEADER_SIZEOF)			\
> +			break;						\
> +		__pos = ALIGN_DOWN(__pos, 4);				\
> +		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
> +		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
> +		if (__id == 0xff)					\
> +			break;						\
> +		if (__id == (cap)) {					\
> +			__found_pos = __pos;				\
> +			break;						\
> +		}							\
> +		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\

Could you please separate the coding style cleanups into own patch that 
is before the actual move patch. IMO, all those cleanups can be in the 
same patch.

You also need to add #includes for the defines you now started to use.

> +	}								\
> +	__found_pos;							\
> +})
> +
> +/* Extended Capability finder */
> +/**
> + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search (0 for initial search)
> + * @cap: Extended capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Searches the extended capability space in PCI config registers
> + * for the specified capability. Implements TTL protection against
> + * infinite loops using a calculated maximum search count.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...)		\
> +({										\
> +	u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE;				\
> +	u16 __found_pos = 0;							\
> +	int __ttl, __ret;							\
> +	u32 __header;								\
> +										\
> +	__ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;		\
> +	while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {			\
> +		__ret = read_cfg(args, __pos, 4, &__header);			\
> +		if (__ret != PCIBIOS_SUCCESSFUL)				\
> +			break;							\
> +										\
> +		if (__header == 0)						\
> +			break;							\
> +										\
> +		if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) {	\
> +			__found_pos = __pos;					\
> +			break;							\
> +		}								\
> +										\
> +		__pos = PCI_EXT_CAP_NEXT(__header);				\
> +	}									\
> +	__found_pos;								\
> +})
> +
>  /* Functions internal to the PCI core code */
>  
>  #ifdef CONFIG_DMI
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..a11ebbab99fc 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -206,6 +206,8 @@
>  /* 0x48-0x7f reserved */
>  
>  /* Capability lists */
> +#define PCI_CAP_ID_MASK		0x00ff
> +#define PCI_CAP_LIST_NEXT_MASK	0xff00
>  
>  #define PCI_CAP_LIST_ID		0	/* Capability ID */
>  #define  PCI_CAP_ID_PM		0x01	/* Power Management */
> 

-- 
 i.