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

[Benno Lossin]

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.hindb...@kernel.org>

A few nits below, with those fixed

Reviewed-by: Benno Lossin <los...@kernel.org>

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

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

s/cfg/::core::cfg/

:)

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

> +                                
> ::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 :)

> +                            #[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()
+                            }},

---
Cheers,
Benno

> +                          }}
> +                        );
> +                ",
> +                module_name = info.name,
> +                param_type = param.ptype,
> +                param_default = param.default,
> +                param_name = param.name,
> +                ops = ops,
> +            )
> +            .unwrap();
> +        }
> +    }
> +}