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: <fffcd237-9084-4be3-8967-f012d981b3f5@163.com>
Date: Fri, 4 Apr 2025 00:31:26 +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 20:22, Hans Zhang wrote:
>>>>> +#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.
>>>>
>>>
>>> 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.
>>
> 
> Thank you so much for your patience in explaining it to me.

Hi Ilpo,

The [v9 2/6]patch I plan to submit is as follows, please review it.

 From 300fe1f428930d0bf8a361ea1d1a3272a6153107 Mon Sep 17 00:00:00 2001
From: Hans Zhang <18255117159@....com>
Date: Fri, 4 Apr 2025 00:20:03 +0800
Subject: [v9 2/6] PCI: Clean up __pci_find_next_cap_ttl() readability

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, uapi/linux/pci_regs.h).

The changes are purely non-functional cleanups, ensuring behavior remains
identical to the original implementation.

Signed-off-by: Hans Zhang <18255117159@....com>
---
  drivers/pci/pci.c             | 10 ++++++----
  include/uapi/linux/pci_regs.h |  2 ++
  2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..e4d3719b653d 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>
@@ -30,6 +31,7 @@
  #include <asm/dma.h>
  #include <linux/aer.h>
  #include <linux/bitfield.h>
+#include <uapi/linux/pci_regs.h>
  #include "pci.h"

  DEFINE_MUTEX(pci_slot_mutex);
@@ -432,17 +434,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 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 */




Best regards,
Hans