"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(¶m.ptype);
>> +
>> + // Note: The spelling of these fields is dictated by the user space
>> + // tool `modinfo`.
>> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
>> + self.emit_param("parm", ¶m.name, ¶m.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
