Message-Id: <DB1O6I32IYI4.OFHKKMD9JV40@kernel.org>
Date: Wed, 02 Jul 2025 17:21:08 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Andreas Hindborg" <a.hindborg@...nel.org>, "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>
Cc: "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

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

> +
> +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.

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.

> +
> +    /// 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.

---
Cheers,
Benno

> +    }
> +}
> +
> +#[doc(hidden)]
> +/// 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) => {
> +        #[doc(hidden)]
> +        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);

Powered by blists - more mailing lists