Re: [PATCH v18 0/7] rust: extend `module!` macro with integer parameter support

[Daniel Gomez <da.gomez@kernel.org>]

On 02/11/2025 10.56, Uwe Kleine-König wrote:
> Hello Daniel,
> 
> [Adding Stephen and linux-next to Cc]
> 
> On Sat, Nov 01, 2025 at 10:39:08PM +0100, Daniel Gomez wrote:
>> On 24/09/2025 14.39, Andreas Hindborg wrote:
>>> Extend the `module!` macro with support module parameters. Also add some
>>> string to integer parsing functions.
>>>
>>> Based on the original module parameter support by Miguel [1],
>>> later extended and generalized by Adam for more types [2][3].
>>> Originally tracked at [4].
>>>
>>> Link: https://github.com/Rust-for-Linux/linux/pull/7 [1]
>>> Link: https://github.com/Rust-for-Linux/linux/pull/82 [2]
>>> Link: https://github.com/Rust-for-Linux/linux/pull/87 [3]
>>> Link: https://github.com/Rust-for-Linux/linux/issues/11 [4]
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>
>> I tested this series with rust_minimal module. They LGTM,
>>
>> Tested-by: Daniel Gomez <da.gomez@samsung.com>
>>
>> The patches did not apply cleanly to v6.18-rc3, at least not when using b4.
>> However, when applying them to the base commit and then rebasing onto v6.18-rc3,
>> I didn't see any conflicts.
> 
> I don't know how you use b4, but
> 
> 	git checkout v6.18-rc3
> 	b4 am -3 49af6d76-bcb7-4343-8903-390040e2c49b@kernel.org
> 	git am -3 ./v18_20250924_a_hindborg_rust_extend_module_macro_with_integer_parameter_support.mbx
> 
> works fine on my end. Using `-3` should have the same effect as applying
> the series on top of the original base and rebase it.

Right, that's what I did but manually. I didn't know about that argument :).

> 
> 	git fetch https://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git rebase/20250924-module-params-v3-v18-0-bf512c35d910@kernel.org
> 	git range-diff FETCH_HEAD...HEAD
> 
> confirms that.
>  
>> I've created a temporary branch with this rebase here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git/log/?h=rebase/20250924-module-params-v3-v18-0-bf512c35d910@kernel.org
>>
>> Can you take a look when you can? I'll merge this shortly after checking with
>> Uwe, as there are some minor conflicts with his tree.
>>
>> + Uwe
>>
>> These are the conflicts I see when merging the patch series from Michal [1]
>> (Introduce import_ns support for Rust). I believe these are trivial things that
>> we will get notified from linux-next merging. But let me know what you think as
>> you have requested in that thread.
>>
>> [1] Link: https://lore.kernel.org/all/20251028-pwm_fixes-v1-0-25a532d31998@samsung.com/
> 
> Yeah, I expect that Stephen will highlight the conflicts, but I prefer
> to not be surprised by that and consider linux-next more a fallback
> security net that I don't want to use. I like it to be the other way
> round and tell Stephen about conflicts to expect :-)

Please Stephen, check the proposed changes below. I plan to merge this series in
modules' tree and it will conflict with Uwe's tree on rust/macros/module.rs file.

> 
>> ...
>> Applying: rust: macros: Add support for 'imports_ns' to module!
>> Patch failed at 0008 rust: macros: Add support for 'imports_ns' to module!
>> error: patch failed: rust/macros/module.rs:98
>> error: rust/macros/module.rs: patch does not apply
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> hint: When you have resolved this problem, run "git am --continue".
>> hint: If you prefer to skip this patch, run "git am --skip" instead.
>> hint: To restore the original branch and stop patching, run "git am --abort".
>> hint: Disable this message with "git config set advice.mergeConflict false"
>>
>> git am --show-current-patch=diff
> 
> That command shows the patch to apply, but not the conflict, let alone
> your resolution.
> 
>> ---
>>  rust/macros/module.rs | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> ---
>>  rust/macros/module.rs | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
>> index 5ee54a00c0b65699596e660b2d4d60e64be2a50c..408cd115487514c8be79724d901c676435696376 100644
>> --- a/rust/macros/module.rs
>> +++ b/rust/macros/module.rs
>> @@ -98,6 +98,7 @@ struct ModuleInfo {
>>      description: Option<String>,
>>      alias: Option<Vec<String>>,
>>      firmware: Option<Vec<String>>,
>> +    imports_ns: Option<Vec<String>>,
>>  }
> 
> So here the addition of `params` is missing.
> 
>> [...]
> 
> When I merge your branch mentioned above with my pwm/for-next and
> resolve the merge conflicts, the resolution looks as follows. The only
> non-trivial thing is that
> 
> 	if let Some(imports) = info.imports_ns {
> 
> now needs a & for `info`.

Correct.

In case it's necessary, I've merged your changes into the modules's -next
branch and attach the diff for you and Stephen. Not sure which order trees are
taken/merged, though.

> 
> Best regards
> Uwe
> 
> diff --cc rust/macros/module.rs
> index d62e9c1e2a89,408cd1154875..000000000000
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@@ -205,50 -98,7 +205,51 @@@ struct ModuleInfo 
>       description: Option<String>,
>       alias: Option<Vec<String>>,
>       firmware: Option<Vec<String>>,
> +     imports_ns: Option<Vec<String>>,
>  +    params: Option<Vec<Parameter>>,
>  +}
>  +
>  +#[derive(Debug)]
>  +struct Parameter {
>  +    name: String,
>  +    ptype: String,
>  +    default: String,
>  +    description: String,
>  +}
>  +
>  +fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
>  +    let params = expect_group(it);
>  +    assert_eq!(params.delimiter(), Delimiter::Brace);
>  +    let mut it = params.stream().into_iter();
>  +    let mut parsed = Vec::new();
>  +
>  +    loop {
>  +        let param_name = match it.next() {
>  +            Some(TokenTree::Ident(ident)) => ident.to_string(),
>  +            Some(_) => panic!("Expected Ident or end"),
>  +            None => break,
>  +        };
>  +
>  +        assert_eq!(expect_punct(&mut it), ':');
>  +        let param_type = expect_ident(&mut it);
>  +        let group = expect_group(&mut it);
>  +        assert_eq!(group.delimiter(), Delimiter::Brace);
>  +        assert_eq!(expect_punct(&mut it), ',');
>  +
>  +        let mut param_it = group.stream().into_iter();
>  +        let param_default = expect_param_default(&mut param_it);
>  +        let param_description = expect_string_field(&mut param_it, "description");
>  +        expect_end(&mut param_it);
>  +
>  +        parsed.push(Parameter {
>  +            name: param_name,
>  +            ptype: param_type,
>  +            default: param_default,
>  +            description: param_description,
>  +        })
>  +    }
>  +
>  +    parsed
>   }
>   
>   impl ModuleInfo {
> @@@ -263,7 -113,7 +264,8 @@@
>               "license",
>               "alias",
>               "firmware",
> +             "imports_ns",
>  +            "params",
>           ];
>           const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
>           let mut seen_keys = Vec::new();
> @@@ -289,7 -139,7 +291,8 @@@
>                   "license" => info.license = expect_string_ascii(it),
>                   "alias" => info.alias = Some(expect_string_array(it)),
>                   "firmware" => info.firmware = Some(expect_string_array(it)),
> +                 "imports_ns" => info.imports_ns = Some(expect_string_array(it)),
>  +                "params" => info.params = Some(expect_params(it)),
>                   _ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
>               }
>   
> @@@ -329,25 -179,30 +332,30 @@@ pub(crate) fn module(ts: TokenStream) -
>       // Rust does not allow hyphens in identifiers, use underscore instead.
>       let ident = info.name.replace('-', "_");
>       let mut modinfo = ModInfoBuilder::new(ident.as_ref());
>  -    if let Some(authors) = info.authors {
>  +    if let Some(authors) = &info.authors {
>           for author in authors {
>  -            modinfo.emit("author", &author);
>  +            modinfo.emit("author", author);
>           }
>       }
>  -    if let Some(description) = info.description {
>  -        modinfo.emit("description", &description);
>  +    if let Some(description) = &info.description {
>  +        modinfo.emit("description", description);
>       }
>       modinfo.emit("license", &info.license);
>  -    if let Some(aliases) = info.alias {
>  +    if let Some(aliases) = &info.alias {
>           for alias in aliases {
>  -            modinfo.emit("alias", &alias);
>  +            modinfo.emit("alias", alias);
>           }
>       }
>  -    if let Some(firmware) = info.firmware {
>  +    if let Some(firmware) = &info.firmware {
>           for fw in firmware {
>  -            modinfo.emit("firmware", &fw);
>  +            modinfo.emit("firmware", fw);
>           }
>       }
>  -    if let Some(imports) = info.imports_ns {
> ++    if let Some(imports) = &info.imports_ns {
> +         for ns in imports {
> +             modinfo.emit("import_ns", &ns);
> +         }
> +     }
>   
>       // Built-in modules also export the `file` modinfo string.
>       let file =

The resolution looks good to me.

As I applied Michal's patch on top of Andreas changes, it looks like this on
my side:

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index d62e9c1e2a89..5bf0a487de50 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -206,6 +206,7 @@ struct ModuleInfo {
     alias: Option<Vec<String>>,
     firmware: Option<Vec<String>>,
     params: Option<Vec<Parameter>>,
+    imports_ns: Option<Vec<String>>,
 }

 #[derive(Debug)]
@@ -264,6 +265,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
             "alias",
             "firmware",
             "params",
+            "imports_ns",
         ];
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
@@ -290,6 +292,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
                 "alias" => info.alias = Some(expect_string_array(it)),
                 "firmware" => info.firmware = Some(expect_string_array(it)),
                 "params" => info.params = Some(expect_params(it)),
+                "imports_ns" => info.imports_ns = Some(expect_string_array(it)),
                 _ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
             }

@@ -348,6 +351,11 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             modinfo.emit("firmware", fw);
         }
     }
+    if let Some(imports) = &info.imports_ns {
+        for ns in imports {
+            modinfo.emit("import_ns", &ns);
+        }
+    }

     // Built-in modules also export the `file` modinfo string.
     let file =