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

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

On 26.04.24 03:10, FUJITA Tomonori wrote:
> Hi,
> 
> On Thu, 25 Apr 2024 15:35:44 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>>> What do the strings in the array mean? If there is documentation on the
>>>> C side, please link to it or explain it again.
>>>
>>> I can't find any on the C side. They are used when building
>>> initrd. How about the following?
>>
>> What does that mean? For the other fields, I know what they mean, but
>> when I see firmware I don't know if what I am thinking is correct.
>> A couple of questions:
>> - How do I know that I need to put anything in that field (and also what
>>     do I need to put there?)?
> 
> If you implement a device driver which requires firmware and the
> driver is likely added to an initramfs image, you need to put the
> names of firmware in that field.
> 
>> - Are those firmware files in the kernel tree?
> 
> I don't think so.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/
> 
>> - How are they "used" when building the initrd?
> 
> initramfs is used before the real root file system is mounted. So
> device drivers in initramfs can't use firmware on the real root file
> system. An initramfs image needs include drivers with necessary
> firmware. If you put the names of firmware in 'firmware' field, the
> names are embedded in modinfo section in the module file. A tool to
> build an initramfs image can use that information.

Thanks for the answer, can you also put this into the documentation, so
that it is not lost. Please also use markdown back-tics (`) for stuff
that should be rendered in a monospace font (eg the `firmware` field).

>> This might just be my lack of knowledge. But as you did not find any
>> docs on the C side, I think we should take this as an opportunity to
>> improve the state of things.
>>
>> You don't need to answer all of the questions above perfectly, but
>> provide at least some starting point to learn more.
>>
>>> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>>> index f489f3157383..19175b3e7172 100644
>>> --- a/rust/macros/lib.rs
>>> +++ b/rust/macros/lib.rs
>>> @@ -35,6 +35,7 @@
>>>   ///     author: "Rust for Linux Contributors",
>>>   ///     description: "My very own kernel module!",
>>>   ///     license: "GPL",
>>> +///     firmware: ["file0", "file1"],
>>
>> Changing the example is good, but "file0" is not really meaningful...
> 
> ["device-driver-fw0", "device-driver-fw1"] are better?

They look better to me.

> Can you tell me your proposal?

This is really not my area of expertise. I have no idea what you would
use here and hoped you knew :)

>> It might also lead people to believe that they need the firmware field,
>> so I think it should be its own example.
> 
> You want to add the complete example for 'firmware' field like the
> following?
> 
> /// ```ignore
> /// use kernel::prelude::*;
> ///
> /// module!{
> ///     type: MyDeviceDriverModule,
> ///     name: "my_device_driver_module",
> ///     author: "Rust for Linux Contributors",
> ///     description: "A device driver requres firmware",
> ///     license: "GPL",
> ///     firmware: ["device-driver-fw0", "device-driver-fw1"],
> /// }
> ///
> /// struct MyDeviceDriverModule;
> ///
> /// impl kernel::Module for MyDeviceDriverModule {
> ///     fn init() -> Result<Self> {
> ///         Ok(Self)
> ///     }
> /// }
> /// ```

That looks good, but please add another sentence above like "when
needing to specify firmware, do this...".

>>>   ///     params: {
>>>   ///        my_i32: i32 {
>>>   ///            default: 42,
>>> @@ -74,6 +75,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 firmware names required by the kernel module.
>>
>> I think it is fine to keep the entry in this list, since this list
>> describes the types that are expected for the parameters, but please
>> extend the docs as I detailed above.
> 
> Where you want the `firmware` explanation? Extending like the
> following works for you?
> 
> - `firmware`: array of firmware names required by the kernel
>    module. This field is optional, used as a hint to build an initramfs
>    image with firmware included.

I think the list above should only be a short list explaining the types
that are accepted for the various fields.

> If you want the explanation of the 'firmware' field elsewhere, please
> tell me where.

You can put it below the list, maybe in its own section `## Firmware`
for example.

-- 
Cheers,
Benno