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

[Lukas Wunner <lukas@wunner.de>]

On Thu, Oct 30, 2025 at 12:16:06PM -0500, Bowman, Terry wrote:
> On 10/2/2025 2:40 AM, Lukas Wunner wrote:
> > 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.
> 
> The PCI_CLASS_CODE_MASK definition move to include/uapi/linux/pci_regs.h
> was requested here:
> https://lore.kernel.org/linux-cxl/6881626a784f_134cc7100b4@dwillia2-xfh.jf.intel.com.notmuch/

Hm, I'm not seeing such a request in the linked message.

> Would you like this patch removed altogether?

"I wouldn't object to it being removed" would be a more pleasant way
of phrasing it. ;)

Thanks,

Lukas