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

[Hans Zhang <18255117159@163.com>]

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?

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

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 */
>>
>