Re: [PATCH v14 5/7] rust: module: update the module macro with module parameter support

[Andreas Hindborg]

"Benno Lossin" <lossin@xxxxxxxxxx> writes:

> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Allow module parameters to be declared in the rust `module!` macro.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>
> A few nits below, with those fixed
>
> Reviewed-by: Benno Lossin <lossin@xxxxxxxxxx>
>
>> ---
>>  rust/macros/helpers.rs |  25 +++++++
>>  rust/macros/lib.rs     |  31 +++++++++
>>  rust/macros/module.rs  | 177 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 223 insertions(+), 10 deletions(-)
>
>> +    fn emit_params(&mut self, info: &ModuleInfo) {
>> +        let Some(params) = &info.params else {
>> +            return;
>> +        };
>> +
>> +        for param in params {
>> +            let ops = param_ops_path(&param.ptype);
>> +
>> +            // Note: The spelling of these fields is dictated by the user space
>> +            // tool `modinfo`.
>> +            self.emit_param("parmtype", &param.name, &param.ptype);
>> +            self.emit_param("parm", &param.name, &param.description);
>> +
>> +            write!(
>> +                self.param_buffer,
>> +                "
>> +                    pub(crate) static {param_name}:
>> +                        ::kernel::module_param::ModuleParamAccess<{param_type}> =
>> +                            ::kernel::module_param::ModuleParamAccess::new({param_default});
>> +
>> +                    #[link_section = \"__param\"]
>> +                    #[used]
>> +                    static __{module_name}_{param_name}_struct:
>
> Does it make sense to move this static to a `const _: () = {};` block?

Yes, that makes sense.

>
>> +                        ::kernel::module_param::RacyKernelParam =
>> +                        ::kernel::module_param::RacyKernelParam::new(
>> +                          ::kernel::bindings::kernel_param {{
>> +                            name: if cfg!(MODULE) {{
>
> s/cfg/::core::cfg/

OK.

>
> :)
>
> Also there seems to only be a 2-space indentation here.

Fixed.

>
>> +                                ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
>> +                            }} else {{
>> +                                ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
>> +                            }}.as_ptr(),
>> +                            // SAFETY: `__this_module` is constructed by the kernel at load time
>> +                            // and will not be freed until the module is unloaded.
>> +                            #[cfg(MODULE)]
>> +                            mod_: unsafe {{
>> +                                (&::kernel::bindings::__this_module
>> +                                    as *const ::kernel::bindings::module)
>> +                                    .cast_mut()
>> +                            }},
>
> It doesn't stop with the improvements...
>
>     https://github.com/Rust-for-Linux/linux/issues/1176
>
> Maybe we should also have one to use it here, but eh we can do that
> later (and it's not as bad to forget about :)

Applying `from_ref`.

>
>> +                            #[cfg(not(MODULE))]
>> +                            mod_: ::core::ptr::null_mut(),
>> +                            ops: &{ops} as *const ::kernel::bindings::kernel_param_ops,
>
>     ::core::ptr::from_ref(&{ops})

👍

>
>> +                            perm: 0, // Will not appear in sysfs
>> +                            level: -1,
>> +                            flags: 0,
>> +                            __bindgen_anon_1:
>> +                                ::kernel::bindings::kernel_param__bindgen_ty_1 {{
>> +                                    arg: {param_name}.as_void_ptr()
>> +                                }},
>
> Formatting?
>
> +                            __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
> +                                arg: {param_name}.as_void_ptr()
> +                            }},


That makes the line more than 100 characters after changing other
formatting things. Perhaps I should just left shift all this?


Best regards,
Andreas Hindborg