Message-ID: <D88RCQTNVD7B.3RIN253F8LODY@proton.me>
Date: Thu, 06 Mar 2025 00:31:14 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>, airlied@...il.com, simona@...ll.ch, corbet@....net, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, ajanulgu@...hat.com, lyude@...hat.com, pstanner@...hat.com, zhiw@...dia.com, cjia@...dia.com, jhubbard@...dia.com, bskeggs@...dia.com, acurrid@...dia.com
Cc: ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu, gregkh@...uxfoundation.org, mcgrof@...nel.org, russ.weight@...ux.dev, dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> Analogous to the `module!` macro `module_firmware!` adds additional
> firmware path strings to the .modinfo section.
>
> In contrast to `module!`, where path strings need to be string literals,
> path strings can be composed with the `firmware::ModInfoBuilder`.
>
> Some drivers require a lot of firmware files (such as nova-core) and
> hence benefit from more flexibility composing firmware path strings.
>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  rust/kernel/firmware.rs | 79 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6e6972d94597..5d1ac8287171 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -116,6 +116,85 @@ unsafe impl Send for Firmware {}
>  // be used from any thread.
>  unsafe impl Sync for Firmware {}
>
> +/// Create firmware .modinfo entries.
> +///
> +/// This macro is the counterpart of the C macro `MODULE_FIRMWARE()`, but instead of taking a
> +/// simple string literals, which is already covered by the `firmware` field of
> +/// [`crate::prelude::module!`], it allows the caller to pass a builder type (e.g.
> +/// [`ModInfoBuilder`]) which can create the firmware modinfo strings in a more flexible way.
> +///
> +/// Drivers should extend the [`ModInfoBuilder`] with their own driver specific builder type.
> +///
> +/// The `builder` argument must be a type which implements the following function.
> +///
> +/// `const fn create(module_name: &'static CStr) -> ModInfoBuilder`
> +///
> +/// `create` should pass the `module_name` to the [`ModInfoBuilder`] and, with the help of
> +/// it construct the corresponding firmware modinfo.
> +///
> +/// Typically, such contracts would be enforced by a trait, however traits do not (yet) support
> +/// const functions.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # mod module_firmware_test {
> +/// # use kernel::firmware;
> +/// # use kernel::prelude::*;
> +/// #
> +/// # struct MyModule;
> +/// #
> +/// # impl kernel::Module for MyModule {
> +/// #     fn init(_module: &'static ThisModule) -> Result<Self> {
> +/// #         Ok(Self)
> +/// #     }
> +/// # }
> +/// #
> +/// #
> +/// struct Builder<const N: usize>;
> +///
> +/// impl<const N: usize> Builder<N> {
> +///     const fn create(module_name: &'static kernel::str::CStr) -> firmware::ModInfoBuilder<N> {
> +///         firmware::ModInfoBuilder::new(module_name)
> +///             .prepare()
> +///             .push("vendor/foo.bin")
> +///             .prepare()
> +///             .push("vendor/bar.bin")
> +///     }
> +/// }
> +///
> +/// module! {
> +///    type: MyModule,
> +///    name: "module_firmware_test",
> +///    author: "Rust for Linux",
> +///    description: "module_firmware! test module",
> +///    license: "GPL",
> +/// }
> +///
> +/// kernel::module_firmware!(Builder);
> +/// # }
> +/// ```

Would be nice to see a more complex example here like the one from nova
you sent in the other thread. So with "dynamic" string interpolation and
multiple pushes.

> +#[macro_export]
> +macro_rules! module_firmware {
> +    ($($builder:tt)*) => {

This should probably be `$builder:expr` instead.

> +
> +        #[cfg(not(MODULE))]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            <LocalModule as kernel::ModuleMetadata>::NAME

Please either use `::kernel::` or `$crate::` instead of `kernel::`.

Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
the current module for some reason, no idea if there is a rational
argument behind that, but it just doesn't sit right with me.

Essentially you're doing this for convenience, right? So you don't want
to have to repeat the name of the module type every time?

> +        }
> +
> +        #[cfg(MODULE)]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            kernel::c_str!("")

Ditto.

> +        }

Are these two functions used outside of the `static` below? If no, then
you can just move them into the static? You can also probably use a
`const` instead of a function, that way you only have 4 lines instead
of 8.

---
Cheers,
Benno

> +
> +        #[link_section = ".modinfo"]
> +        #[used]
> +        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__module_name()).build_length()] =
> +            $($builder)*::create(__module_name()).build();
> +    };
> +}
> +
>  /// Builder for firmware module info.
>  ///
>  /// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> --
> 2.48.1

Powered by blists - more mailing lists