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

Hans Zhang
2025-04-03T12:23:21Z

On 2025/4/3 17:10, Ilpo Järvinen wrote:
>>>> 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.
>

Hi Ilpo,

Thank you so much for your patience in explaining it to me.

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

OK，will change.

Best regards,
Hans