On 8/18/25 5:23 AM, Alexandre Courbot wrote:
> On Mon Aug 18, 2025 at 10:33 AM JST, John Hubbard wrote:
...
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 887ee611b553..9caa1d342d52 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -23,6 +23,179 @@
>> };
>> use kernel::prelude::*;
>>
>> +macro_rules! define_all_pci_classes {
>> + (
>> + $($variant:ident = $binding:expr,)+
>> + ) => {
>> + /// Converts a PCI class constant to 24-bit format.
>> + ///
>> + /// Many device drivers use only the upper 16 bits (base class and subclass), but some
>> + /// use the full 24 bits. In order to support both cases, store the class code as a 24-bit
>> + /// value, where 16-bit values are shifted up 8 bits.
>> + const fn to_24bit_class(val: u32) -> u32 {
>> + if val > 0xFFFF { val } else { val << 8 }
>> + }
>
> This convenience function doesn't look like it needs to be defined
> inside the macro.
>
>> +
>> + /// PCI device class codes.
>> + ///
>> + /// Each entry contains the full 24-bit PCI class code (base class in bits 23-16, subclass
>> + /// in bits 15-8, programming interface in bits 7-0).
>> + ///
>> + #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> + #[repr(transparent)]
>> + pub struct Class(u32);
>
> Same for this struct.
>
>> +
>> + impl Class {
>> + $(
>> + #[allow(missing_docs)]
>> + pub const $variant: Self = Self(to_24bit_class($binding));
>> + )+
>
> This obviously needs to be part of the macro...
>
>> +
>> + /// Match the full class code.
>> + pub const MASK_FULL: u32 = 0xffffff;
>> +
>> + /// Match the upper 16 bits of the class code (base class and subclass only).
>> + pub const MASK_CLASS_SUBCLASS: u32 = 0xffff00;
>
> But these two definitions (and `as_u32` below) can again be part of
> their own impl block outside of it.
>
>> +
>> + /// Create a `Class` from the raw class code value, or `None` if the value doesn't
>> + /// match any known class.
>> + pub fn from_u32(value: u32) -> Option<Self> {
>> + match value {
>> + $(x if x == Self::$variant.0 => Some(Self::$variant),)+
>> + _ => None,
>> + }
>> + }
>> +
>> + /// Get the raw 24-bit class code value.
>> + pub const fn as_u32(self) -> u32 {
>> + self.0
>> + }
>> + }
>
> So I believe that if you move the declarations that do not depend on the
> macro parameters outside of if, the macro's body could be limited to the
> following:
>
> impl Class {
> $(
> #[allow(missing_docs)]
> pub const $variant: Self = Self(to_24bit_class($binding));
> )+
>
> /// Create a `Class` from the raw class code value, or `None` if the value doesn't
> /// match any known class.
> pub fn from_u32(value: u32) -> Option<Self> {
> match value {
> $(x if x == Self::$variant.0 => Some(Self::$variant),)+
> _ => None,
> }
> }
>
> Shorter macros are generally considered better. :)
All of the above comments make sense, I'll make those changes. Thanks for the
review!
thanks,
--
John Hubbard
