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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7227 bytes --]

On Wed, 2 Apr 2025, Hans Zhang wrote:
> On 2025/4/2 20:42, Ilpo Järvinen wrote:
> > 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@163.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.
> > 
> 
> Hi Ilpo,
> 
> Thanks your for reply. I don't understand. Is it like this?

Add a patch before the first patch which does only the cleanups to 
__pci_find_next_cap_ttl(). The patch that creates PCI_FIND_NEXT_CAP_TTL() 
and converts its PCI core users (most of the patches 1&2) is to be based 
on top of that cleanup patch.

> #define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
> ({									\
> 	int __ttl = PCI_FIND_CAP_TTL;					\
> 	u8 __id, __found_pos = 0;					\
> 	u8 __pos = (start);						\
> 	u16 __ent;							\
> 									\
> 	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);	\
> 	}								\
> 	__found_pos;							\
> })
> 
> > You also need to add #includes for the defines you now started to use.
> > 
> 
> Is that what you mean?
> 
> +#include <linux/bitfield.h>
> +#include <linux/align.h>
> +#include <uapi/linux/pci_regs.h>

Almost, including pci_regs.h is not strictly necessary as linux/pci.h will 
always pull that one in (not that it would hurt).

Also, sort the includes alphabetically.

--
 i.

> 
> Best regards,
> Hans
> 
> > > +	}								\
> > > +	__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 */
> > > 
> > 
>