[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aN4sWxjrzxeCyGBO@wunner.de>
Date: Thu, 2 Oct 2025 09:40:11 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Terry Bowman <terry.bowman@....com>, dave@...olabs.net,
dave.jiang@...el.com, alison.schofield@...el.com,
dan.j.williams@...el.com, bhelgaas@...gle.com,
shiju.jose@...wei.com, ming.li@...omail.com,
Smita.KoralahalliChannabasappa@....com, rrichter@....com,
dan.carpenter@...aro.org, PradeepVineshReddy.Kodamati@....com,
Benjamin.Cheatham@....com,
sathyanarayanan.kuppuswamy@...ux.intel.com,
linux-cxl@...r.kernel.org, alucerop@....com, ira.weiny@...el.com,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v12 10/25] CXL/AER: Update PCI class code check to use
FIELD_GET()
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@....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
Powered by blists - more mailing lists