linux-kernel - Re: [PATCH v12 10/25] CXL/AER: Update PCI class code check to use FIELD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <639cb124-4c92-4d10-a4ce-78d6d862060f@amd.com>
Date: Thu, 30 Oct 2025 12:16:06 -0500
From: "Bowman, Terry" <terry.bowman@....com>
To: Lukas Wunner <lukas@...ner.de>,
 Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: 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 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@....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