Re: [PATCH v12 2/6] PCI: Clean up __pci_find_next_cap_ttl() readability

[Hans Zhang <18255117159@163.com>]

On 2025/6/3 17:30, Ilpo Järvinen wrote:
> On Thu, 15 May 2025, Hans Zhang wrote:
> 
>> Refactor the __pci_find_next_cap_ttl() to improve code clarity:
>> - Replace magic number 0x40 with PCI_STD_HEADER_SIZEOF.
>> - Use ALIGN_DOWN() for position alignment instead of manual bitmask.
>> - Extract PCI capability fields via FIELD_GET() with standardized masks.
>> - Add necessary headers (linux/align.h).
>>
>> The changes are purely non-functional cleanups, ensuring behavior remains
>> identical to the original implementation.
> 
> If you want a simpler wording for this, this is often used:
> 
> No functional changes intended.

Dear Ilpo,

Will change.

> 
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> Changes since v11:
>> - None
>>
>> Changes since v10:
>> - Remove #include <uapi/linux/pci_regs.h> and add macro definition comments.
>>
>> Changes since v9:
>> - None
>>
>> Changes since v8:
>> - Split into patch 1/6, patch 2/6.
>> - The
>> ---
>>   drivers/pci/pci.c             | 9 +++++----
>>   include/uapi/linux/pci_regs.h | 2 ++
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e77d5b53c0ce..27d2adb18a30 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -9,6 +9,7 @@
>>    */
>>   
>>   #include <linux/acpi.h>
>> +#include <linux/align.h>
>>   #include <linux/kernel.h>
>>   #include <linux/delay.h>
>>   #include <linux/dmi.h>
>> @@ -432,17 +433,17 @@ static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
>>   	pci_bus_read_config_byte(bus, devfn, pos, &pos);
>>   
>>   	while ((*ttl)--) {
>> -		if (pos < 0x40)
>> +		if (pos < PCI_STD_HEADER_SIZEOF)
>>   			break;
>> -		pos &= ~3;
>> +		pos = ALIGN_DOWN(pos, 4);
>>   		pci_bus_read_config_word(bus, devfn, pos, &ent);
>>   
>> -		id = ent & 0xff;
>> +		id = FIELD_GET(PCI_CAP_ID_MASK, ent);
>>   		if (id == 0xff)
>>   			break;
>>   		if (id == cap)
>>   			return pos;
>> -		pos = (ent >> 8);
>> +		pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
>>   	}
>>   	return 0;
>>   }
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index ba326710f9c8..35051f9ac16a 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	/* Capability ID mask */
>> +#define PCI_CAP_LIST_NEXT_MASK	0xff00	/* Next Capability Pointer mask */
>>   
>>   #define PCI_CAP_LIST_ID		0	/* Capability ID */
> 
> I'd add those here with the extra space before name and add empty line in
> between them and the capability id list.
> 

Will change.


Best regards,
Hans

>>   #define  PCI_CAP_ID_PM		0x01	/* Power Management */
>>
>