Message-ID: <87plfazi13.fsf@kernel.org>
Date: Wed, 11 Jun 2025 12:31:20 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno
 Lossin" <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,
  "Masahiro Yamada" <masahiroy@...nel.org>,  "Nathan Chancellor"
 <nathan@...nel.org>,  "Luis Chamberlain" <mcgrof@...nel.org>,  "Danilo
 Krummrich" <dakr@...nel.org>,  "Nicolas Schier"
 <nicolas.schier@...ux.dev>,  "Trevor Gross" <tmgross@...ch.edu>,  "Adam
 Bratschi-Kaye" <ark.email@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  <linux-kbuild@...r.kernel.org>,  "Petr
 Pavlu" <petr.pavlu@...e.com>,  "Sami Tolvanen" <samitolvanen@...gle.com>,
  "Daniel Gomez" <da.gomez@...sung.com>,  "Simona Vetter"
 <simona.vetter@...ll.ch>,  "Greg KH" <gregkh@...uxfoundation.org>,  "Fiona
 Behrens" <me@...enk.dev>,  "Daniel Almeida"
 <daniel.almeida@...labora.com>,  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

"Benno Lossin" <lossin@...nel.org> writes:

> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>> Add support for module parameters to the `module!` macro. Implement read
>> only support for integer types without `sysfs` support.
>>
>> Acked-by: Petr Pavlu <petr.pavlu@...e.com> # from modules perspective
>> Tested-by: Daniel Gomez <da.gomez@...sung.com>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>> ---
>>  rust/kernel/lib.rs           |   1 +
>>  rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
>>  rust/macros/helpers.rs       |  25 ++++++
>>  rust/macros/lib.rs           |  31 +++++++
>>  rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
>>  samples/rust/rust_minimal.rs |  10 +++
>
> I know this is already the 12th version, but I think this patch should
> be split into the module_param module introduction, proc-macro
> modifications and the sample changes.
>

OK.

> [...]
>
>> +/// Set the module parameter from a string.
>> +///
>> +/// Used to set the parameter value at kernel initialization, when loading
>> +/// the module or when set through `sysfs`.
>> +///
>> +/// See `struct kernel_param_ops.set`.
>> +///
>> +/// # Safety
>> +///
>> +/// - If `val` is non-null then it must point to a valid null-terminated string.
>> +///   The `arg` field of `param` must be an instance of `T`.
>
> This sentence is conveying the same (or at least similar) requirement as
> the bullet point below.

Yes, you are right. At any rate the wording is wrong, a pointer cannot
"be an instance", it can point to one.

>
>> +/// - `param.arg` must be a pointer to valid `*mut T` as set up by the
>> +///   [`module!`] macro.
>> +///
>> +/// # Invariants
>> +///
>> +/// Currently, we only support read-only parameters that are not readable
>> +/// from `sysfs`. Thus, this function is only called at kernel
>> +/// initialization time, or at module load time, and we have exclusive
>> +/// access to the parameter for the duration of the function.
>
> Why is this an Invariants section?

Looks like a mistake, I'll change it to "# Note".

>
>> +///
>> +/// [`module!`]: macros::module
>> +unsafe extern "C" fn set_param<T>(
>> +    val: *const kernel::ffi::c_char,
>> +    param: *const crate::bindings::kernel_param,
>> +) -> core::ffi::c_int
>> +where
>> +    T: ModuleParam,
>> +{
>> +    // NOTE: If we start supporting arguments without values, val _is_ allowed
>> +    // to be null here.
>> +    if val.is_null() {
>> +        // TODO: Use pr_warn_once available.
>> +        crate::pr_warn!("Null pointer passed to `module_param::set_param`");
>> +        return EINVAL.to_errno();
>> +    }
>> +
>> +    // SAFETY: By function safety requirement, val is non-null and
>> +    // null-terminated. By C API contract, `val` is live and valid for reads
>> +    // for the duration of this function.
>
> We shouldn't mention "C API contract" here and move the liveness
> requirement to the safety requirements of the function. Or change the
> safety requirements for this function to only be called from some
> specific C API.

OK.

>
>> +    let arg = unsafe { CStr::from_char_ptr(val) };
>> +
>> +    crate::error::from_result(|| {
>> +        let new_value = T::try_from_param_arg(arg)?;
>> +
>> +        // SAFETY: `param` is guaranteed to be valid by C API contract
>> +        // and `arg` is guaranteed to point to an instance of `T`.
>
> Ditto.

OK.

>
>> +        let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>> +
>> +        // SAFETY: `old_value` is valid for writes, as we have exclusive
>> +        // access. `old_value` is pointing to an initialized static, and
>> +        // so it is properly initialized.
>
> Why do we have exclusive access? Should be in the safety requirements.

Will add this.

>
>> +        unsafe { *old_value = new_value };
>> +        Ok(0)
>> +    })
>> +}
>
> [...]
>
>> +#[doc(hidden)]
>> +#[macro_export]
>> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +/// make_param_ops!(
>> +///     /// Documentation for new param ops.
>> +///     PARAM_OPS_MYTYPE, // Name for the static.
>> +///     MyType // A type which implements [`ModuleParam`].
>> +/// );
>> +/// ```
>> +macro_rules! make_param_ops {
>> +    ($ops:ident, $ty:ty) => {
>> +        ///
>
> Spurious newline?

Will remove.

>
>> +        /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
>> +        /// struct generated by `make_param_ops`
>
> Is it useful for this fact to be in the docs? I'd remove it.

OK.

>
>> +        #[doc = concat!("for [`", stringify!($ty), "`].")]
>> +        pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
>> +            flags: 0,
>> +            set: Some(set_param::<$ty>),
>> +            get: None,
>> +            free: None,
>> +        };
>> +    };
>> +}
>> +
>> +make_param_ops!(PARAM_OPS_I8, i8);
>> +make_param_ops!(PARAM_OPS_U8, u8);
>> +make_param_ops!(PARAM_OPS_I16, i16);
>> +make_param_ops!(PARAM_OPS_U16, u16);
>> +make_param_ops!(PARAM_OPS_I32, i32);
>> +make_param_ops!(PARAM_OPS_U32, u32);
>> +make_param_ops!(PARAM_OPS_I64, i64);
>> +make_param_ops!(PARAM_OPS_U64, u64);
>> +make_param_ops!(PARAM_OPS_ISIZE, isize);
>> +make_param_ops!(PARAM_OPS_USIZE, usize);
>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
>> index a3ee27e29a6f..16d300ad3d3b 100644
>> --- a/rust/macros/helpers.rs
>> +++ b/rust/macros/helpers.rs
>> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>>      }
>>  }
>>  
>> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
>> +    let peek = it.clone().next();
>> +    match peek {
>> +        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
>
> Should we also allow a leading `+`?

I would argue no, because rust literals cannot start with `+`.

>
>> +            let _ = it.next();
>> +            Some(punct.as_char())
>> +        }
>> +        _ => None,
>> +    }
>> +}
>> +
>>  pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
>>      if let Some(TokenTree::Literal(literal)) = it.next() {
>>          Some(literal.to_string())
>> @@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
>>      }
>>      None
>>  }
>> +
>> +/// Parse a token stream of the form `expected_name: "value",` and return the
>> +/// string in the position of "value".
>> +///
>> +/// # Panics
>> +///
>> +/// - On parse error.
>> +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
>> +    assert_eq!(expect_ident(it), expected_name);
>> +    assert_eq!(expect_punct(it), ':');
>> +    let string = expect_string(it);
>> +    assert_eq!(expect_punct(it), ',');
>
> This won't allow omitting the trailing comma.

This is in line with the rest of the module macro.

>
>> +    string
>> +}
>
> [...]
>
>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>      let info = ModuleInfo::parse(&mut it);
>>  
>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>> -    if let Some(author) = info.author {
>> -        modinfo.emit("author", &author);
>> +    if let Some(author) = &info.author {
>> +        modinfo.emit("author", author);
>>      }
>> -    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);
>
> I don't like that you have to change all of these.

Why not? If I was to write this code in the first place, I would have
used a reference rather than pass by value.

> Can't you just take a
> `&[Parameter]` argument in `emit_params` instead of the whole
> `ModuleInfo` struct?

I don't think that is a nice solution. I would have to pass the name
field as well, increasing the number of parameters to the function.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists