1. Patchew
  2. linux
  3. [PATCH v15 0/6] Refactor capability search into common macros
  4. View patch

[PATCH v15 1/6] PCI: Clean up __pci_find_next_cap_ttl() readability

Hans Zhang posted 6 patches 4 months, 1 week ago
[PATCH v15 1/6] PCI: Clean up __pci_find_next_cap_ttl() readability
Posted by Hans Zhang 4 months, 1 week ago 
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).

No functional changes intended.

Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/pci.c             | 9 +++++----
 include/uapi/linux/pci_regs.h | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0f4d98036cd..40a5c87d9a6b 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 f5b17745de60..1bba99b46227 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -207,6 +207,9 @@
 
 /* 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 */
 #define  PCI_CAP_ID_PM		0x01	/* Power Management */
 #define  PCI_CAP_ID_AGP		0x02	/* Accelerated Graphics Port */
-- 
2.25.1
Re: [PATCH v15 1/6] PCI: Clean up __pci_find_next_cap_ttl() readability
Posted by Gerd Bayer 4 months ago 
On Wed, 2025-08-13 at 22:45 +0800, 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).
> 
> No functional changes intended.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/pci.c             | 9 +++++----
>  include/uapi/linux/pci_regs.h | 3 +++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0f4d98036cd..40a5c87d9a6b 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 f5b17745de60..1bba99b46227 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -207,6 +207,9 @@
>  
>  /* 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 */
>  #define  PCI_CAP_ID_PM		0x01	/* Power Management */
>  #define  PCI_CAP_ID_AGP		0x02	/* Accelerated Graphics Port */

Hi Hans,

I like your approach to replace the magic numbers here. If you went
further to replace the single pci_bus_read_config_word() with two
single-byte reads at the appropriate places - for CAP_ID and
CAP_LIST_NEXT - you could even go with the already existing offset
defines PCI_CAP_LIST_ID and PCI_CAP_LIST_NEXT from pci_regs.h.

But that might be a more intricate change and involves more HW accesses
than what it's worth.

So feel free to add my
Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>

Thanks,
Gerd
Re: [PATCH v15 1/6] PCI: Clean up __pci_find_next_cap_ttl() readability
Posted by Ilpo Järvinen 4 months ago 
On Wed, 20 Aug 2025, Gerd Bayer wrote:

> On Wed, 2025-08-13 at 22:45 +0800, 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).
> > 
> > No functional changes intended.
> > 
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/pci.c             | 9 +++++----
> >  include/uapi/linux/pci_regs.h | 3 +++
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b0f4d98036cd..40a5c87d9a6b 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 f5b17745de60..1bba99b46227 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -207,6 +207,9 @@
> >  
> >  /* 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 */
> >  #define  PCI_CAP_ID_PM		0x01	/* Power Management */
> >  #define  PCI_CAP_ID_AGP		0x02	/* Accelerated Graphics Port */
> 
> Hi Hans,
> 
> I like your approach to replace the magic numbers here. If you went
> further to replace the single pci_bus_read_config_word() with two
> single-byte reads at the appropriate places - for CAP_ID and
> CAP_LIST_NEXT - you could even go with the already existing offset
> defines PCI_CAP_LIST_ID and PCI_CAP_LIST_NEXT from pci_regs.h.
>
> But that might be a more intricate change and involves more HW accesses
> than what it's worth.

Hi,

As you noted, it'll be less efficient so it's undesirable to split the 
read.

It's somewhat problematic that some of the defines in 
include/uapi/linux/pci_regs.h cannot be easily used efficiently with 
multi-byte reads leading to use of literals in code.

IMO, adding the multi-byte masks like this Hans' change is IMO the correct 
way to address it.

> So feel free to add my
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>
> 
> Thanks,
> Gerd
> 

-- 
 i.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.