linux-kernel - Re: [PATCH v2 1/3] rust: pci: provide access to PCI Class, subclass, implementation values

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: <a82172da-a029-4208-b92d-e68320ed31f4@nvidia.com>
Date: Mon, 18 Aug 2025 11:49:43 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>,
 Danilo Krummrich <dakr@...nel.org>
Cc: Joel Fernandes <joelagnelf@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 Alistair Popple <apopple@...dia.com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 nouveau@...ts.freedesktop.org, linux-pci@...r.kernel.org,
 rust-for-linux@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] rust: pci: provide access to PCI Class, subclass,
 implementation values

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