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

["Bowman, Terry" <terry.bowman@amd.com>]

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.
>
> Thanks,
>
> Lukas
Apologies for the late response.

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/

Would you like this patch removed altogether? 

Terry