Re: [v7 1/5] PCI: Refactor capability search into common macros
From: Hans Zhang
Date: Thu Apr 03 2025 - 12:32:24 EST
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@xxxxxxx>
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@xxxxxxx>
---
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