Re: [PATCH v12 10/25] CXL/AER: Update PCI class code check to use FIELD_GET()

[Lukas Wunner <lukas@wunner.de>]

On Wed, Oct 01, 2025 at 05:12:16PM +0100, Jonathan Cameron wrote:
> On Thu, 25 Sep 2025 17:34:25 -0500 Terry Bowman <terry.bowman@amd.com> wrote:
> The way that class code definitions work in pci_ids.h is somewhat odd
> in my opinion, so I'd like input from Bjorn, Lukas etc on whether a
> generic mask definition is a good idea or more likely to cause problems.
> 
> See for example. 
> #define PCI_BASE_CLASS_STORAGE		0x01
> ...
> 
> #define PCI_CLASS_STORAGE_SATA		0x0106
> #define PCI_CLASS_STORAGE_SATA_AHCI	0x010601
> 
> This variability in what is called CLASS_* leads to fun
> situations like in drivers/ata/ahci.c where we have some
> PCI_CLASS_* shifted and some not...

Macros working with PCI class codes generally accept a "shift" parameter
to know by how many bits the class code needs to be shifted, see e.g.
DECLARE_PCI_FIXUP_CLASS_EARLY() and friends.

A macro which shifts exactly by 8 is hence only of limited use.


> > +++ b/drivers/pci/pcie/aer_cxl_rch.c
> > @@ -17,10 +17,10 @@ static bool is_cxl_mem_dev(struct pci_dev *dev)
> >  		return false;
> >  
> >  	/*
> > -	 * CXL Memory Devices must have the 502h class code set (CXL
> > -	 * 3.0, 8.1.12.1).
> > +	 * CXL Memory Devices must have the 502h class code set
> > +	 * (CXL 3.2, 8.1.12.1).
> >  	 */
> > -	if ((dev->class >> 8) != PCI_CLASS_MEMORY_CXL)
> > +	if (FIELD_GET(PCI_CLASS_CODE_MASK, dev->class) != PCI_CLASS_MEMORY_CXL)
> >  		return false;

Hm, this doesn't look more readable TBH.

Refactoring changes like this one should be submitted separately from
this patch set.  If any of them are controversial, they delay upstreaming
of the actual change, i.e. the CXL AER plumbing.  They also increase the
size of the patch set, making it more difficult to review.


> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -73,6 +73,8 @@
> >  #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
> >  #define PCI_CLASS_DEVICE	0x0a	/* Device class */
> >  
> > +#define PCI_CLASS_CODE_MASK     __GENMASK(23, 8)
> > +
> >  #define PCI_CACHE_LINE_SIZE	0x0c	/* 8 bits */
> >  #define PCI_LATENCY_TIMER	0x0d	/* 8 bits */
> >  #define PCI_HEADER_TYPE		0x0e	/* 8 bits */

Putting this in a uapi header means we'll have to support it
indefinitely.  Usually such macros are added to kernel-internal
headers first and moved to uapi headers if/when the need arises.

Thanks,

Lukas