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