Re: [PATCH v2 01/23] PCI: Add SNIA SDXI accelerator sub-class

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] PCI_CLASS_ACCELERATOR_SDXI is incorrectly defined as a 24-bit value instead of a standard 16-bit sub-class code.
> - [Low] The newly added PCI_CLASS_ACCELERATOR_SDXI class code is missing from the Rust PCI class bindings.
> --
>
> commit 5c21a6a19a3c154b4d13c65d1905d5ed4e273dd8
> Author: Nathan Lynch <nathan.lynch@amd.com>
>
> PCI: Add SNIA SDXI accelerator sub-class
>
> This commit adds the sub-class code for the SNIA Smart Data Accelerator
> Interface (SDXI) to the PCI IDs list.
>
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index 24cb42f66e4b6..83ab3f27eb5a2 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -154,6 +154,7 @@
>>  
>>  #define PCI_BASE_CLASS_ACCELERATOR	0x12
>>  #define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
>> +#define PCI_CLASS_ACCELERATOR_SDXI		0x120100
>
> The commit message mentions adding a sub-class code, but is it expected to
> define this as a 24-bit value?
>
> Other generic sub-class macros in this file are standard 16-bit constants,
> like the adjacent PCI_CLASS_ACCELERATOR_PROCESSING (0x1200). Drivers
> traditionally apply an 8-bit left shift to them when populating ID tables:
>
>     .class = PCI_CLASS_ACCELERATOR_SDXI << 8
>
> If this standard idiom is used with a 24-bit macro, it will result in a
> malformed 32-bit class code (0x12010000) and break device matching. Should
> this be defined as 0x1201 instead?

I had wondered about whether to follow this convention but I didn't see
a strong reason for it. I'll do it if that's what maintainers want.


> Additionally, does this new class code need to be added to the Rust PCI class
> bindings? It appears to be missing from the define_all_pci_classes! macro in
> rust/kernel/pci/id.rs, which explicitly mirrors these C macros to expose
> them to Rust drivers.

Seems like a reasonable suggestion.


>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=1