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: <DC5JTQ7VHA65.2SNDMXYIKB9GO@nvidia.com>
Date: Mon, 18 Aug 2025 21:23:01 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "John Hubbard" <jhubbard@...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 Mon Aug 18, 2025 at 10:33 AM JST, John Hubbard wrote:
> Allow callers to write Class::STORAGE_SCSI instead of
> bindings::PCI_CLASS_STORAGE_SCSI, for example.
>
> New APIs:
>     Class::STORAGE_SCSI, Class::NETWORK_ETHERNET, etc.
>     Class::from_u32(), as_u32()
>     Class::MASK_FULL, MASK_CLASS_SUBCLASS
>     DeviceId::from_class_and_vendor()
>     Device::class_code_raw(), class_enum()
>
> Cc: Danilo Krummrich <dakr@...nel.org>
> Signed-off-by: John Hubbard <jhubbard@...dia.com>
> ---
>  rust/kernel/pci.rs | 202 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 202 insertions(+)
>
> 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. :)