Re: [PATCH v1] rust: add 'firmware' field support to module! macro

[Benno Lossin <benno.lossin@proton.me>]

On 26.04.24 11:56, FUJITA Tomonori wrote:
> This adds 'firmware' field support to module! macro, corresponds to
> MODULE_FIRMWARE macro. You can specify the file names of binary
> firmware that the kernel module requires. The information is embedded
> in the modinfo section of the kernel module. For example, a tool to
> build an initramfs uses this information to put the firmware files
> into the initramfs image.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/macros/lib.rs    | 31 +++++++++++++++++++++++++++++++
>  rust/macros/module.rs | 18 ++++++++++++++++--
>  2 files changed, 47 insertions(+), 2 deletions(-)

I left one comment below, otherwise LGTM:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index f489f3157383..73fa768fce64 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -67,6 +67,36 @@
>  /// }
>  /// ```
>  ///
> +/// ## Firmware
> +///
> +/// The following example shows how to declare a kernel module that needs
> +/// to load binary firmware files. You need to specify the file names of
> +/// the firmware in the `firmware` field. The information is embedded
> +/// in the `modinfo` section of the kernel module. For example, a tool to
> +/// build an initramfs uses this information to put the firmware files into
> +/// the initramfs image.
> +///
> +/// ```ignore
> +/// use kernel::prelude::*;
> +///
> +/// module!{
> +///     type: MyDeviceDriverModule,
> +///     name: "my_device_driver_module",
> +///     author: "Rust for Linux Contributors",
> +///     description: "My device driver requires firmware",
> +///     license: "GPL",
> +///     firmware: ["my_device_firmware1.bin", "my_device_firmware2.bin"],
> +/// }
> +///
> +/// struct MyDeviceDriverModule;
> +///
> +/// impl kernel::Module for MyDeviceDriverModule {
> +///     fn init() -> Result<Self> {
> +///         Ok(Self)
> +///     }
> +/// }
> +/// ```
> +///
>  /// # Supported argument types
>  ///   - `type`: type which implements the [`Module`] trait (required).
>  ///   - `name`: byte array of the name of the kernel module (required).
> @@ -74,6 +104,7 @@
>  ///   - `description`: byte array of the description of the kernel module.
>  ///   - `license`: byte array of the license of the kernel module (required).
>  ///   - `alias`: byte array of alias name of the kernel module.
> +///   - `firmware`: array of the file names of firmware required by the kernel module.

There is a patch [1] that updates this list. It will have a v2, with
this list specifying the types that can be put there due to this [2]
comment.

We should have the same kind of item for the `firmware` field here. I
would suggest:

     ///   - `firmware`: array of ASCII string literals of the firmware files of the kernel module.

[1]: https://lore.kernel.org/rust-for-linux/20240419215015.157258-3-aswinunni01@gmail.com/
[2]: https://lore.kernel.org/rust-for-linux/CANiq72=g4f60n8x=-fCxqQ4QzD2eOrEb=vEXgoNidY-N4omN1A@mail.gmail.com/

-- 
Cheers,
Benno

>  #[proc_macro]
>  pub fn module(ts: TokenStream) -> TokenStream {
>      module::module(ts)