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

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

[FUJITA Tomonori]

Hi,

This patch is necessary for a new QT2025 PHY driver in Rust:

https://lwn.net/Articles/969888/

Are there places to be fixed or imporved before sending this in the v2
of the PHY driver patchset?


-
This adds 'firmware' tag support to module! macro, corresponds to
MODULE_FIRMWARE macro.

A module can have multiple 'firmware' tags like 'alias'.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/macros/module.rs | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27979e582e4b..5fdaac17a733 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -97,14 +97,22 @@ struct ModuleInfo {
     author: Option<String>,
     description: Option<String>,
     alias: Option<Vec<String>>,
+    firmware: Option<Vec<String>>,
 }
 
 impl ModuleInfo {
     fn parse(it: &mut token_stream::IntoIter) -> Self {
         let mut info = ModuleInfo::default();
 
-        const EXPECTED_KEYS: &[&str] =
-            &["type", "name", "author", "description", "license", "alias"];
+        const EXPECTED_KEYS: &[&str] = &[
+            "type",
+            "name",
+            "author",
+            "description",
+            "license",
+            "alias",
+            "firmware",
+        ];
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
 
@@ -131,6 +139,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
                 "description" => info.description = Some(expect_string(it)),
                 "license" => info.license = expect_string_ascii(it),
                 "alias" => info.alias = Some(expect_string_array(it)),
+                "firmware" => info.firmware = Some(expect_string_array(it)),
                 _ => panic!(
                     "Unknown key \"{}\". Valid keys are: {:?}.",
                     key, EXPECTED_KEYS
@@ -186,6 +195,11 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             modinfo.emit("alias", &alias);
         }
     }
+    if let Some(firmware) = info.firmware {
+        for fw in firmware {
+            modinfo.emit("firmware", &fw);
+        }
+    }
 
     // Built-in modules also export the `file` modinfo string.
     let file =
-- 
2.34.1

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

[Greg KH]

On Fri, Apr 19, 2024 at 10:53:53PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> This patch is necessary for a new QT2025 PHY driver in Rust:
> 
> https://lwn.net/Articles/969888/

Please use lore.kernel.org links for mailing list stuff.

thanks,

greg k-h

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

[Benno Lossin]

On 19.04.24 15:53, FUJITA Tomonori wrote:
> Hi,
> 
> This patch is necessary for a new QT2025 PHY driver in Rust:
> 
> https://lwn.net/Articles/969888/
> 
> Are there places to be fixed or imporved before sending this in the v2
> of the PHY driver patchset?

Please also update the documentation.

Also what exactly is expected as the parameter to the firmware field?
What do the strings in the array mean? If there is documentation on the
C side, please link to it or explain it again.

-- 
Cheers,
Benno

> -
> This adds 'firmware' tag support to module! macro, corresponds to
> MODULE_FIRMWARE macro.
> 
> A module can have multiple 'firmware' tags like 'alias'.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/macros/module.rs | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)

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

[FUJITA Tomonori]

Hi,

On Fri, 19 Apr 2024 16:41:36 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 19.04.24 15:53, FUJITA Tomonori wrote:
>> Hi,
>> 
>> This patch is necessary for a new QT2025 PHY driver in Rust:
>> 
>> https://lwn.net/Articles/969888/
>> 
>> Are there places to be fixed or imporved before sending this in the v2
>> of the PHY driver patchset?
> 
> Please also update the documentation.
> 
> Also what exactly is expected as the parameter to the firmware field?

Oops, I overlooked the documentation in rust/macros/lib.rs


> 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?


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"],
 ///     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.
 #[proc_macro]
 pub fn module(ts: TokenStream) -> TokenStream {
     module::module(ts)

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

[Benno Lossin]

On 20.04.24 03:14, FUJITA Tomonori wrote:
> Hi,
> 
> On Fri, 19 Apr 2024 16:41:36 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 19.04.24 15:53, FUJITA Tomonori wrote:
>>> Hi,
>>>
>>> This patch is necessary for a new QT2025 PHY driver in Rust:
>>>
>>> https://lwn.net/Articles/969888/
>>>
>>> Are there places to be fixed or imporved before sending this in the v2
>>> of the PHY driver patchset?
>>
>> Please also update the documentation.
>>
>> Also what exactly is expected as the parameter to the firmware field?
> 
> Oops, I overlooked the documentation in rust/macros/lib.rs
> 
> 
>> 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?)?
- Are those firmware files in the kernel tree?
- How are they "used" when building the initrd?

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...

It might also lead people to believe that they need the firmware field,
so I think it should be its own example.

>  ///     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.

-- 
Cheers,
Benno

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

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

[FUJITA Tomonori]

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.


> 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?

Can you tell me your proposal?


> 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)
///     }
/// }
/// ```


>>  ///     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.


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

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

[Benno Lossin]

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

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

[Miguel Ojeda]

On Fri, Apr 26, 2024 at 3:11 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> - `firmware`: array of firmware names required by the kernel

I would say "file names", not just "names".

Cheers,
Miguel

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

[FUJITA Tomonori]

On Fri, 26 Apr 2024 09:21:03 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Fri, Apr 26, 2024 at 3:11 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> - `firmware`: array of firmware names required by the kernel
> 
> I would say "file names", not just "names".

makes sense. I'll send a non-RFC patch for rust-next shortly.