"Benno Lossin" <lossin@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> I have some nits below, but overall
>
> Reviewed-by: Benno Lossin <lossin@xxxxxxxxxx>
>
>> ---
>> 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
