linux-kernel - Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

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: <Z3ZeG5NJRJEVGCYi@cassiopeiae>
Date: Thu, 2 Jan 2025 10:36:27 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Gary Guo <gary@...yguo.net>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, tmgross@...ch.edu,
	a.hindborg@...sung.com, aliceryhl@...gle.com, airlied@...il.com,
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org,
	daniel.almeida@...labora.com, saravanak@...gle.com,
	dirk.behme@...bosch.com, j@...nau.net, fabien.parent@...aro.org,
	chrisi.schrefl@...il.com, paulmck@...nel.org,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	rcu@...r.kernel.org, Wedson Almeida Filho <wedsonaf@...il.com>
Subject: Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and
 `RawDeviceId`

On Tue, Dec 24, 2024 at 08:10:02PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:05 +0100
> Danilo Krummrich <dakr@...nel.org> wrote:
> 
> > Most subsystems use some kind of ID to match devices and drivers. Hence,
> > we have to provide Rust drivers an abstraction to register an ID table
> > for the driver to match.
> > 
> > Generally, those IDs are subsystem specific and hence need to be
> > implemented by the corresponding subsystem. However, the `IdArray`,
> > `IdTable` and `RawDeviceId` types provide a generalized implementation
> > that makes the life of subsystems easier to do so.
> > 
> > Co-developed-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Co-developed-by: Gary Guo <gary@...yguo.net>
> > Signed-off-by: Gary Guo <gary@...yguo.net>
> > Co-developed-by: Fabien Parent <fabien.parent@...aro.org>
> > Signed-off-by: Fabien Parent <fabien.parent@...aro.org>
> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> 
> Thank you for converting my prototype to a working patch. There's a nit below.
> 
> > ---
> >  MAINTAINERS              |   1 +
> >  rust/kernel/device_id.rs | 165 +++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs       |   6 ++
> >  3 files changed, 172 insertions(+)
> >  create mode 100644 rust/kernel/device_id.rs
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2ad58ed40079..3cfb68650347 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
> >  F:	include/linux/property.h
> >  F:	lib/kobj*
> >  F:	rust/kernel/device.rs
> > +F:	rust/kernel/device_id.rs
> >  F:	rust/kernel/driver.rs
> >  
> >  DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
> > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> > new file mode 100644
> > index 000000000000..e5859217a579
> > --- /dev/null
> > +++ b/rust/kernel/device_id.rs
> > 
> > <snip>
> >
> > +
> > +impl<T: RawDeviceId, const N: usize> RawIdArray<T, N> {
> > +    #[doc(hidden)]
> > +    pub const fn size(&self) -> usize {
> > +        core::mem::size_of::<Self>()
> > +    }
> > +}
> > +
> 
> This is not necessary, see below.
> 
> > <snip>
> >
> > +
> > +/// Create device table alias for modpost.
> > +#[macro_export]
> > +macro_rules! module_device_table {
> > +    ($table_type: literal, $module_table_name:ident, $table_name:ident) => {
> > +        #[rustfmt::skip]
> > +        #[export_name =
> > +            concat!("__mod_device_table__", $table_type,
> > +                    "__", module_path!(),
> > +                    "_", line!(),
> > +                    "_", stringify!($table_name))
> > +        ]
> > +        static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] =
> > +            unsafe { core::mem::transmute_copy($table_name.raw_ids()) };
> 
> const_size_of_val will be stable in Rust 1.85, so we can start using the
> feature and 
> 
> static $module_table_name: [core::mem::MaybeUninit<u8>; core::mem::size_of_val($table_name.raw_ids())] =
>     unsafe { core::mem::transmute_copy($table_name.raw_ids()) };
> 
> should work.

Thanks for the note, this indeed saves a few lines.

Since the series has been applied already, feel free to send a patch.

> 
> > +    };
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 7818407f9aac..66149ac5c0c9 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -18,6 +18,11 @@
> >  #![feature(inline_const)]
> >  #![feature(lint_reasons)]
> >  #![feature(unsize)]
> > +// Stable in Rust 1.83
> > +#![feature(const_maybe_uninit_as_mut_ptr)]
> > +#![feature(const_mut_refs)]
> > +#![feature(const_ptr_write)]
> > +#![feature(const_refs_to_cell)]
> >  
> >  // Ensure conditional compilation based on the kernel configuration works;
> >  // otherwise we may silently break things like initcall handling.
> > @@ -35,6 +40,7 @@
> >  mod build_assert;
> >  pub mod cred;
> >  pub mod device;
> > +pub mod device_id;
> >  pub mod driver;
> >  pub mod error;
> >  #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
>