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: <a0483c8d-3cd4-4da2-aca5-586379870e3a@163.com>
Date: Thu, 3 Apr 2025 20:22:07 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.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 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