Message-ID: <87jz4orxh9.fsf@kernel.org>
Date: Fri, 04 Jul 2025 13:45:06 +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>,  "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 v14 3/7] rust: introduce module_param module

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

> On Wed Jul 2, 2025 at 3:18 PM CEST, Andreas Hindborg wrote:
>> Add types and traits for interfacing the C moduleparam API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> I have some nits below, but overall
>
> Reviewed-by: Benno Lossin <lossin@...nel.org>
>
>> ---
>>  rust/kernel/lib.rs          |   1 +
>>  rust/kernel/module_param.rs | 191 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 192 insertions(+)
>
> I really like how the `OnceLock` usage turned out here! Thanks for the
> quick impl!
>
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c3..2b439ea06185 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -87,6 +87,7 @@
>>  pub mod list;
>>  pub mod miscdevice;
>>  pub mod mm;
>> +pub mod module_param;
>>  #[cfg(CONFIG_NET)]
>>  pub mod net;
>>  pub mod of;
>> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
>> new file mode 100644
>> index 000000000000..ca4be7e45ff7
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Support for module parameters.
>> +//!
>> +//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
>> +
>> +use crate::prelude::*;
>> +use crate::str::BStr;
>> +use bindings;
>> +use kernel::sync::once_lock::OnceLock;
>> +
>> +/// Newtype to make `bindings::kernel_param` [`Sync`].
>> +#[repr(transparent)]
>> +#[doc(hidden)]
>> +pub struct RacyKernelParam(bindings::kernel_param);
>
> Can you remind me why this is called `Racy`? Maybe add the explainer in
> a comment? (and if it's named racy, why is it okay?)
>
> If it doesn't have a real reason, maybe it should be called
> `KernelParam`?

It is an inherited name from way back. The type exists to allow a static
`bindings::kernel_param`, as this C type is not `Sync`.

I agree, it should just be `KernelParam`.

>
>> +
>> +impl RacyKernelParam {
>> +    #[doc(hidden)]
>> +    pub const fn new(val: bindings::kernel_param) -> Self {
>> +        Self(val)
>> +    }
>> +}
>> +
>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>> +
>> +/// Types that can be used for module parameters.
>> +// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
>> +pub trait ModuleParam: Sized + Copy {
>> +    /// The [`ModuleParam`] will be used by the kernel module through this type.
>> +    ///
>> +    /// This may differ from `Self` if, for example, `Self` needs to track
>> +    /// ownership without exposing it or allocate extra space for other possible
>> +    /// parameter values.
>> +    // This is required to support string parameters in the future.
>> +    type Value: ?Sized;
>
> This isn't used anywhere in the patchset and AFAIK the kernel is moving
> away from module params, so I'm not so sure if we're going to have
> strings as params.

The kernel dropping module parameters depends on who you ask. But
regardless, we should probably remove it for now.

>
> Or do you already have those patches ready/plan to use strings? If not,
> then I think we should just remove this type and when we actually need
> them add it.

They are in the old rust branch and they need some work. I do not have a
user for them, which is why I am not including them in this series.

>
>> +
>> +    /// Parse a parameter argument into the parameter value.
>> +    fn try_from_param_arg(arg: &BStr) -> Result<Self>;
>> +}
>> +
>
>> +impl<T> ModuleParamAccess<T> {
>> +    #[doc(hidden)]
>> +    pub const fn new(default: T) -> Self {
>> +        Self {
>> +            value: OnceLock::new(),
>> +            default,
>> +        }
>> +    }
>> +
>> +    /// Get a shared reference to the parameter value.
>> +    // Note: When sysfs access to parameters are enabled, we have to pass in a
>> +    // held lock guard here.
>> +    pub fn get(&self) -> &T {
>> +        self.value.as_ref().unwrap_or(&self.default)
>> +    }
>> +
>> +    /// Get a mutable pointer to `self`.
>> +    ///
>> +    /// NOTE: In most cases it is not safe deref the returned pointer.
>> +    pub const fn as_void_ptr(&self) -> *mut c_void {
>> +        (self as *const Self).cast_mut().cast()
>
> There is `core::ptr::from_ref` that we should use instead of the `as`
> cast.

Cool 👍


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists