[PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

This series extends the `module!` macro with support module parameters. It
also adds some string to integer parsing functions and updates `BStr` with
a method to strip a string prefix.

This series stated out as code by Adam Bratschi-Kaye lifted from the original
`rust` branch [1].

Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes since v2 [1]:
- use `SyncUnsafeCell` rather than `static mut` and simplify parameter access
- remove `Display` bound from `ModuleParam`
- automatically generate documentation for `PARAM_OPS_.*`
- remove `as *const _ as *mut_` phrasing
- inline parameter name in struct instantiation in  `emit_params`
- move `RacyKernelParam` out of macro template
- use C string literals rather than byte string literals with explicit null
- template out `__{name}_{param_name}` in `emit_param`
- indent template in `emit_params`
- use let-else expression in `emit_params` to get rid of an indentation level
- document `expect_string_field`
- move invication of `impl_int_module_param` to be closer to macro def
- move attributes after docs in `make_param_ops`
- rename `impl_module_param` to impl_int_module_param`
- use `ty` instead of `ident` in `impl_parse_int`
- use `BStr` instead of `&str` for string manipulation
- move string parsing functions to seperate patch and add examples, fix bugs
- degrade comment about future support from doc comment to regular comment
- remove std lib path from `Sized` marker
- update documentation for `trait ModuleParam`

Changes since v1 [2]:
- Remove support for params without values (`NOARG_ALLOWED`).
- Improve documentation for `try_from_param_arg`.
- Use prelude import.
- Refactor `try_from_param_arg` to return `Result`.
- Refactor `ParseInt::from_str` to return `Result`.
- Move C callable functions out of `ModuleParam` trait.
- Rename literal string field parser to `expect_string_field`.
- Move parameter parsing from generation to parsing stage.
- Use absolute type paths in macro code.
- Inline `kparam`and `read_func` values.
- Resolve TODO regarding alignment attributes.
- Remove unnecessary unsafe blocks in macro code.
- Improve error message for unrecognized parameter types.
- Do not use `self` receiver when reading parameter value.
- Add parameter documentation to `module!` macro.
- Use empty `enum` for parameter type.
- Use `addr_of_mut` to get address of parameter value variable.
- Enabled building of docs for for `module_param` module.

Link: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/ [2]
Link: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/ [1]

---

---
Andreas Hindborg (4):
      rust: str: implement `PartialEq` for `BStr`
      rust: str: implement `strip_prefix` for `BStr`
      rust: str: add radix prefixed integer parsing functions
      rust: add parameter support to the `module!` macro

 rust/kernel/lib.rs           |   2 +
 rust/kernel/module_param.rs  | 238 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs           | 138 +++++++++++++++++++++++++
 rust/macros/helpers.rs       |  10 ++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 188 ++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 scripts/Makefile.build       |   2 +-
 8 files changed, 600 insertions(+), 19 deletions(-)
---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241211-module-params-v3-ae7e5c8d8b5a

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v3 1/4] rust: str: implement `PartialEq` for `BStr`

[Andreas Hindborg]

Implement `PartialEq` for `BStr` by comparing underlying byte slices.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index d04c12a1426d1c1edeb88325bcd9c63bf45f9b60..c441acf76ebd1f14919b6d233edffbbbbf944619 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -106,6 +106,12 @@ fn deref(&self) -> &Self::Target {
     }
 }
 
+impl PartialEq for BStr {
+    fn eq(&self, other: &Self) -> bool {
+        self.deref().eq(other.deref())
+    }
+}
+
 /// Creates a new [`BStr`] from a string literal.
 ///
 /// `b_str!` converts the supplied string literal to byte string, so non-ASCII

-- 
2.47.0

[PATCH v3 2/4] rust: str: implement `strip_prefix` for `BStr`

[Andreas Hindborg]

Implement `strip_prefix` for `BStr` by deferring to `slice::strip_prefix`
on the underlying `&[u8]`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

---

It is also possible to get this method by implementing
`core::slice::SlicePattern` for `BStr`. `SlicePattern` is unstable, so this
seems more reasonable.
---
 rust/kernel/str.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index c441acf76ebd1f14919b6d233edffbbbbf944619..9c446ff1ad7adba7ca09a5ae9df00fd369a32899 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -31,6 +31,22 @@ pub const fn from_bytes(bytes: &[u8]) -> &Self {
         // SAFETY: `BStr` is transparent to `[u8]`.
         unsafe { &*(bytes as *const [u8] as *const BStr) }
     }
+
+    /// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`].
+    ///
+    /// # Example
+    /// ```
+    /// use kernel::b_str;
+    /// assert_eq!(Some(b_str!("bar")), b_str!("foobar").strip_prefix(b_str!("foo")));
+    /// assert_eq!(None, b_str!("foobar").strip_prefix(b_str!("bar")));
+    /// assert_eq!(Some(b_str!("foobar")), b_str!("foobar").strip_prefix(b_str!("")));
+    /// assert_eq!(Some(b_str!("")), b_str!("foobar").strip_prefix(b_str!("foobar")));
+    /// ```
+    pub fn strip_prefix(&self, pattern: &Self) -> Option<&BStr> {
+        self.deref()
+            .strip_prefix(pattern.deref())
+            .map(Self::from_bytes)
+    }
 }
 
 impl fmt::Display for BStr {

-- 
2.47.0

[PATCH v3 3/4] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

Add the trait `ParseInt` for parsing string representations of integers
where the string representations are optionally prefixed by a radix
specifier. Implement the trait for the primitive integer types.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 9c446ff1ad7adba7ca09a5ae9df00fd369a32899..4da711f264397c7a1fce34edb69a71253d03911e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -914,3 +914,119 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 macro_rules! fmt {
     ($($f:tt)*) => ( core::format_args!($($f)*) )
 }
+
+pub mod parse_int {
+    //! Integer parsing functions for parsing signed and unsigned integers
+    //! potentially prefixed with `0x`, `0o`, or `0b`.
+
+    use crate::alloc::flags;
+    use crate::prelude::*;
+    use crate::str::BStr;
+
+    /// Trait that allows parsing a `&BStr` to an integer with a radix.
+    // This is required because the `from_str_radix` function on the primitive
+    // integer types is not part of any trait.
+    pub trait FromStrRadix: Sized {
+        /// Parse `src` to `Self` using radix `radix`.
+        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
+    }
+
+    /// Extract the radix from an integer literal optionally prefixed with
+    /// one of `0x`, `0X`, `0o`, `0O`, `0b`, `0B`, `0`.
+    fn strip_radix(src: &BStr) -> (u32, &BStr) {
+        if let Some(n) = src.strip_prefix(b_str!("0x")) {
+            (16, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0X")) {
+            (16, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0o")) {
+            (8, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0O")) {
+            (8, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0b")) {
+            (2, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0B")) {
+            (2, n)
+        } else if let Some(n) = src.strip_prefix(b_str!("0")) {
+            (8, n)
+        } else {
+            (10, src)
+        }
+    }
+
+    /// Trait for parsing string representations of integers.
+    ///
+    /// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or
+    /// binary respectively. Strings beginning with `0` otherwise are parsed as
+    /// octal. Anything else is parsed as decimal. A leading `+` or `-` is also
+    /// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be
+    /// successfully parsed.
+    ///
+    /// [`kstrtol()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtol
+    /// [`kstrtoul()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtoul
+    ///
+    /// # Example
+    /// ```
+    /// use kernel::str::parse_int::ParseInt;
+    /// use kernel::b_str;
+    ///
+    /// assert_eq!(Ok(0xa2u8), u8::from_str(b_str!("0xa2")));
+    /// assert_eq!(Ok(-0xa2i32), i32::from_str(b_str!("-0xa2")));
+    ///
+    /// assert_eq!(Ok(-0o57i8), i8::from_str(b_str!("-0o57")));
+    /// assert_eq!(Ok(0o57i8), i8::from_str(b_str!("057")));
+    ///
+    /// assert_eq!(Ok(0b1001i16), i16::from_str(b_str!("0b1001")));
+    /// assert_eq!(Ok(-0b1001i16), i16::from_str(b_str!("-0b1001")));
+    ///
+    /// assert_eq!(Ok(127), i8::from_str(b_str!("127")));
+    /// assert!(i8::from_str(b_str!("128")).is_err());
+    /// assert_eq!(Ok(-128), i8::from_str(b_str!("-128")));
+    /// assert!(i8::from_str(b_str!("-129")).is_err());
+    /// assert_eq!(Ok(255), u8::from_str(b_str!("255")));
+    /// assert!(u8::from_str(b_str!("256")).is_err());
+    /// ```
+    pub trait ParseInt: FromStrRadix {
+        /// Parse a string according to the description in [`Self`].
+        fn from_str(src: &BStr) -> Result<Self> {
+            match src.iter().next() {
+                None => Err(EINVAL),
+                Some(sign @ b'-') | Some(sign @ b'+') => {
+                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
+                    let mut n_digits: KVec<u8> =
+                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
+                    n_digits.push(*sign, flags::GFP_KERNEL)?;
+                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;
+                    Self::from_str_radix(BStr::from_bytes(&n_digits), radix).map_err(|_| EINVAL)
+                }
+                Some(_) => {
+                    let (radix, digits) = strip_radix(src);
+                    Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+                }
+            }
+        }
+    }
+
+    macro_rules! impl_parse_int {
+        ($ty:ty) => {
+            impl FromStrRadix for $ty {
+                fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
+                    <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+                        .map_err(|_| EINVAL)
+                }
+            }
+
+            impl ParseInt for $ty {}
+        };
+    }
+
+    impl_parse_int!(i8);
+    impl_parse_int!(u8);
+    impl_parse_int!(i16);
+    impl_parse_int!(u16);
+    impl_parse_int!(i32);
+    impl_parse_int!(u32);
+    impl_parse_int!(i64);
+    impl_parse_int!(u64);
+    impl_parse_int!(isize);
+    impl_parse_int!(usize);
+}

-- 
2.47.0

[PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

This patch includes changes required for Rust kernel modules to utilize
module parameters. This code implements read only support for integer
types without `sysfs` support.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/lib.rs           |   2 +
 rust/kernel/module_param.rs  | 238 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  10 ++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 188 ++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 scripts/Makefile.build       |   2 +-
 7 files changed, 462 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a39e68d6379031d80d4be336e652a3..7670c47a967cdd7e4f63e00b8338c6042e391fee 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -18,6 +18,7 @@
 #![feature(inline_const)]
 #![feature(lint_reasons)]
 #![feature(unsize)]
+#![feature(sync_unsafe_cell)]
 
 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.
@@ -46,6 +47,7 @@
 pub mod kunit;
 pub mod list;
 pub mod miscdevice;
+pub mod module_param;
 #[cfg(CONFIG_NET)]
 pub mod net;
 pub mod page;
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
new file mode 100644
index 0000000000000000000000000000000000000000..f63e50a6f02ff3f3fd1117717faf8fd37377c3f1
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Types for module parameters.
+//!
+//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h)
+
+use crate::prelude::*;
+use crate::str::BStr;
+
+/// Newtype to make `bindings::kernel_param` `Sync`.
+#[repr(transparent)]
+#[doc(hidden)]
+pub struct RacyKernelParam(pub ::kernel::bindings::kernel_param);
+
+// SAFETY: C kernel handles serializing access to this type. We never access
+// from Rust module.
+unsafe impl Sync for RacyKernelParam {}
+
+/// Types that can be used for module parameters.
+///
+/// Note that displaying the type in `sysfs` will fail if
+/// [`Display`](core::fmt::Display) implementation would write more than
+/// [`PAGE_SIZE`] - 1 bytes.
+///
+/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
+pub trait ModuleParam: Sized {
+    /// 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;
+
+    /// Parse a parameter argument into the parameter value.
+    ///
+    /// `Err(_)` should be returned when parsing of the argument fails.
+    ///
+    /// Parameters passed at boot time will be set before [`kmalloc`] is
+    /// available (even if the module is loaded at a later time). However, in
+    /// this case, the argument buffer will be valid for the entire lifetime of
+    /// the kernel. So implementations of this method which need to allocate
+    /// should first check that the allocator is available (with
+    /// [`crate::bindings::slab_is_available`]) and when it is not available
+    /// provide an alternative implementation which doesn't allocate. In cases
+    /// where the allocator is not available it is safe to save references to
+    /// `arg` in `Self`, but in other cases a copy should be made.
+    ///
+    /// [`kmalloc`]: ../../../include/linux/slab.h
+    fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>;
+}
+
+/// 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`.
+///
+/// `param.arg` is a pointer to `*mut T` as set up by the [`module!`]
+/// macro.
+///
+/// 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`.
+///
+/// # 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.
+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(
+    val: *const core::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.
+    assert!(!val.is_null());
+
+    // 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.
+    let arg = unsafe { CStr::from_char_ptr(val).as_bytes() };
+
+    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`.
+        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, an
+        // so it is properly initialized.
+        unsafe { core::ptr::replace(old_value, new_value) };
+        Ok(0)
+    })
+}
+
+/// Write a string representation of the current parameter value to `buf`.
+///
+/// # Safety
+///
+/// Must not be called.
+///
+/// # Note
+///
+/// This should not be called as we declare all parameters as read only.
+#[allow(clippy::extra_unused_type_parameters)]
+unsafe extern "C" fn get_param<T>(
+    _buf: *mut core::ffi::c_char,
+    _param: *const crate::bindings::kernel_param,
+) -> core::ffi::c_int
+where
+    T: ModuleParam,
+{
+    unreachable!("Parameters are not readable");
+}
+
+/// Drop the parameter.
+///
+/// Called when unloading a module.
+///
+/// # Safety
+///
+/// The `arg` field of `param` must be an initialized instance of `Self`.
+unsafe extern "C" fn free<T>(arg: *mut core::ffi::c_void)
+where
+    T: ModuleParam,
+{
+    // SAFETY: By function safety requirement, `arg` is an initialized
+    // instance of `T`. By C API contract, `arg` will not be used after
+    // this function returns.
+    unsafe { core::ptr::drop_in_place(arg as *mut T) };
+}
+
+macro_rules! impl_int_module_param {
+    ($ty:ident) => {
+        impl ModuleParam for $ty {
+            type Value = $ty;
+
+            fn try_from_param_arg(arg: &'static [u8]) -> Result<Self> {
+                let bstr = BStr::from_bytes(arg);
+                <$ty as crate::str::parse_int::ParseInt>::from_str(bstr)
+            }
+        }
+    };
+}
+
+impl_int_module_param!(i8);
+impl_int_module_param!(u8);
+impl_int_module_param!(i16);
+impl_int_module_param!(u16);
+impl_int_module_param!(i32);
+impl_int_module_param!(u32);
+impl_int_module_param!(i64);
+impl_int_module_param!(u64);
+impl_int_module_param!(isize);
+impl_int_module_param!(usize);
+
+/// A wrapper for kernel parameters.
+///
+/// This type is instantiated by the [`module!`] macro when module parameters are
+/// defined. You should never need to instantiate this type directly.
+#[repr(transparent)]
+pub struct ModuleParamAccess<T> {
+    data: core::cell::SyncUnsafeCell<T>,
+}
+
+impl<T> ModuleParamAccess<T> {
+    #[doc(hidden)]
+    pub const fn new(value: T) -> Self {
+        Self {
+            data: core::cell::SyncUnsafeCell::new(value),
+        }
+    }
+
+    /// 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 {
+        // SAFETY: As we only support read only parameters with no sysfs
+        // exposure, the kernel will not touch the parameter data after module
+        // initialization.
+        unsafe { &*self.data.get() }
+    }
+
+    /// Get a mutable pointer to the parameter value.
+    pub const fn as_mut_ptr(&self) -> *mut T {
+        self.data.get()
+    }
+}
+
+#[doc(hidden)]
+#[macro_export]
+/// Generate a static [`kernel_param_ops`](../../../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) => {
+        ///
+        /// Static [`kernel_param_ops`](../../../include/linux/moduleparam.h)
+        /// struct generated by `make_param_ops`
+        #[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: Some(get_param::<$ty>),
+            free: Some(free::<$ty>),
+        };
+    };
+}
+
+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 563dcd2b7ace5e8322d0fddb409571cca2dd31ea..e4f2751e0f6bbd6a980ba57d630480f05682f59f 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -107,6 +107,16 @@ pub(crate) struct Generics {
     pub(crate) ty_generics: Vec<TokenTree>,
 }
 
+/// 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), ',');
+    string
+}
+
 /// Parses the given `TokenStream` into `Generics` and the rest.
 ///
 /// The generics are not present in the rest, but a where clause might remain.
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 4ab94e44adfe3206faad159e81417ea41a35815b..a75d354e3eec49fd4066ac3dd96c4afda2606d50 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -24,6 +24,30 @@
 /// The `type` argument should be a type which implements the [`Module`]
 /// trait. Also accepts various forms of kernel metadata.
 ///
+/// The `params` field describe module parameters. Each entry has the form
+///
+/// ```ignore
+/// parameter_name: type {
+///     default: default_value,
+///     description: "Description",
+/// }
+/// ```
+///
+/// `type` may be one of
+///
+/// - `i8`
+/// - `u8`
+/// - `i8`
+/// - `u8`
+/// - `i16`
+/// - `u16`
+/// - `i32`
+/// - `u32`
+/// - `i64`
+/// - `u64`
+/// - `isize`
+/// - `usize`
+///
 /// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
 ///
 /// [`Module`]: ../kernel/trait.Module.html
@@ -40,6 +64,12 @@
 ///     description: "My very own kernel module!",
 ///     license: "GPL",
 ///     alias: ["alternate_module_name"],
+///     params: {
+///         my_parameter: i64 {
+///             default: 1,
+///             description: "This parameter has a default of 1",
+///         },
+///     },
 /// }
 ///
 /// struct MyModule(i32);
@@ -48,6 +78,7 @@
 ///     fn init(_module: &'static ThisModule) -> Result<Self> {
 ///         let foo: i32 = 42;
 ///         pr_info!("I contain:  {}\n", foo);
+///         pr_info!("i32 param is:  {}\n", module_parameters::my_parameter.read());
 ///         Ok(Self(foo))
 ///     }
 /// }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2587f41b0d3929af7ceac5f42b4711f70b4f8749..627279cf318b14b50128045bce980f9d60b9c6d0 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> {
     module: &'a str,
     counter: usize,
     buffer: String,
+    param_buffer: String,
 }
 
 impl<'a> ModInfoBuilder<'a> {
@@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self {
             module,
             counter: 0,
             buffer: String::new(),
+            param_buffer: String::new(),
         }
     }
 
-    fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
+    fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) {
         let string = if builtin {
             // Built-in modules prefix their modinfo strings by `module.`.
             format!(
@@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
             format!("{field}={content}\0", field = field, content = content)
         };
 
+        let buffer = if param {
+            &mut self.param_buffer
+        } else {
+            &mut self.buffer
+        };
+
         write!(
-            &mut self.buffer,
+            buffer,
             "
                 {cfg}
                 #[doc(hidden)]
@@ -75,20 +83,113 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
         self.counter += 1;
     }
 
-    fn emit_only_builtin(&mut self, field: &str, content: &str) {
-        self.emit_base(field, content, true)
+    fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_base(field, content, true, param)
     }
 
-    fn emit_only_loadable(&mut self, field: &str, content: &str) {
-        self.emit_base(field, content, false)
+    fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_base(field, content, false, param)
     }
 
     fn emit(&mut self, field: &str, content: &str) {
-        self.emit_only_builtin(field, content);
-        self.emit_only_loadable(field, content);
+        self.emit_internal(field, content, false);
+    }
+
+    fn emit_internal(&mut self, field: &str, content: &str, param: bool) {
+        self.emit_only_builtin(field, content, param);
+        self.emit_only_loadable(field, content, param);
+    }
+
+    fn emit_param(&mut self, field: &str, param: &str, content: &str) {
+        let content = format!("{param}:{content}", param = param, content = content);
+        self.emit_internal(field, &content, true);
+    }
+
+    fn emit_params(&mut self, info: &ModuleInfo) {
+        let Some(params) = &info.params else {
+            return;
+        };
+
+        for param in params {
+            let ops = param_ops_path(&param.ptype);
+
+            // Note: The spelling of these fields is dictated by the user space
+            // tool `modinfo`.
+            self.emit_param("parmtype", &param.name, &param.ptype);
+            self.emit_param("parm", &param.name, &param.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:
+                        ::kernel::module_param::RacyKernelParam =
+                        ::kernel::module_param::RacyKernelParam(::kernel::bindings::kernel_param {{
+                            name: if cfg!(MODULE) {{
+                                c\"{module_name}.{param_name}\"
+                            }} else {{
+                                c\"{param_name}\"
+                            }}.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()
+                            }},
+                            #[cfg(not(MODULE))]
+                            mod_: ::core::ptr::null_mut(),
+                            ops: &{ops} as *const ::kernel::bindings::kernel_param_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_mut_ptr().cast()
+                                }},
+                        }});
+                ",
+                module_name = info.name,
+                param_type = param.ptype,
+                param_default = param.default,
+                param_name = param.name,
+                ops = ops,
+            )
+            .unwrap();
+        }
+    }
+}
+
+fn param_ops_path(param_type: &str) -> &'static str {
+    match param_type {
+        "i8" => "::kernel::module_param::PARAM_OPS_I8",
+        "u8" => "::kernel::module_param::PARAM_OPS_U8",
+        "i16" => "::kernel::module_param::PARAM_OPS_I16",
+        "u16" => "::kernel::module_param::PARAM_OPS_U16",
+        "i32" => "::kernel::module_param::PARAM_OPS_I32",
+        "u32" => "::kernel::module_param::PARAM_OPS_U32",
+        "i64" => "::kernel::module_param::PARAM_OPS_I64",
+        "u64" => "::kernel::module_param::PARAM_OPS_U64",
+        "isize" => "::kernel::module_param::PARAM_OPS_ISIZE",
+        "usize" => "::kernel::module_param::PARAM_OPS_USIZE",
+        t => panic!("Unsupported parameter type {}", t),
     }
 }
 
+fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
+    assert_eq!(expect_ident(param_it), "default");
+    assert_eq!(expect_punct(param_it), ':');
+    let default = try_literal(param_it).expect("Expected default param value");
+    assert_eq!(expect_punct(param_it), ',');
+    default
+}
+
 #[derive(Debug, Default)]
 struct ModuleInfo {
     type_: String,
@@ -98,6 +199,50 @@ struct ModuleInfo {
     description: Option<String>,
     alias: Option<Vec<String>>,
     firmware: Option<Vec<String>>,
+    params: Option<Vec<Parameter>>,
+}
+
+#[derive(Debug)]
+struct Parameter {
+    name: String,
+    ptype: String,
+    default: String,
+    description: String,
+}
+
+fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
+    let params = expect_group(it);
+    assert_eq!(params.delimiter(), Delimiter::Brace);
+    let mut it = params.stream().into_iter();
+    let mut parsed = Vec::new();
+
+    loop {
+        let param_name = match it.next() {
+            Some(TokenTree::Ident(ident)) => ident.to_string(),
+            Some(_) => panic!("Expected Ident or end"),
+            None => break,
+        };
+
+        assert_eq!(expect_punct(&mut it), ':');
+        let param_type = expect_ident(&mut it);
+        let group = expect_group(&mut it);
+        assert_eq!(group.delimiter(), Delimiter::Brace);
+        assert_eq!(expect_punct(&mut it), ',');
+
+        let mut param_it = group.stream().into_iter();
+        let param_default = expect_param_default(&mut param_it);
+        let param_description = expect_string_field(&mut param_it, "description");
+        expect_end(&mut param_it);
+
+        parsed.push(Parameter {
+            name: param_name,
+            ptype: param_type,
+            default: param_default,
+            description: param_description,
+        })
+    }
+
+    parsed
 }
 
 impl ModuleInfo {
@@ -112,6 +257,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
             "license",
             "alias",
             "firmware",
+            "params",
         ];
         const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
         let mut seen_keys = Vec::new();
@@ -140,6 +286,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
                 "license" => info.license = expect_string_ascii(it),
                 "alias" => info.alias = Some(expect_string_array(it)),
                 "firmware" => info.firmware = Some(expect_string_array(it)),
+                "params" => info.params = Some(expect_params(it)),
                 _ => panic!(
                     "Unknown key \"{}\". Valid keys are: {:?}.",
                     key, EXPECTED_KEYS
@@ -183,28 +330,30 @@ 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(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);
         }
     }
 
     // Built-in modules also export the `file` modinfo string.
     let file =
         std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable");
-    modinfo.emit_only_builtin("file", &file);
+    modinfo.emit_only_builtin("file", &file, false);
+
+    modinfo.emit_params(&info);
 
     format!(
         "
@@ -358,14 +507,17 @@ unsafe fn __exit() {{
                             __MOD.assume_init_drop();
                         }}
                     }}
-
                     {modinfo}
                 }}
             }}
+            mod module_parameters {{
+                {params}
+            }}
         ",
         type_ = info.type_,
         name = info.name,
         modinfo = modinfo.buffer,
+        params = modinfo.param_buffer,
         initcall_section = ".initcall6.init"
     )
     .parse()
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 4aaf117bf8e3c0cc77e188b1ad0708e3650a6654..d999a77c6eb9a0db9acf5fa03d4feca5e6525fc8 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -10,6 +10,12 @@
     author: "Rust for Linux Contributors",
     description: "Rust minimal sample",
     license: "GPL",
+    params: {
+        test_parameter: i64 {
+            default: 1,
+            description: "This parameter has a default of 1",
+        },
+    },
 }
 
 struct RustMinimal {
@@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
     fn init(_module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust minimal sample (init)\n");
         pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+        pr_info!(
+            "My parameter: {}\n",
+            *module_parameters::test_parameter.get()
+        );
 
         let mut numbers = KVec::new();
         numbers.push(72, GFP_KERNEL)?;
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index c16e4cf54d770fd2cf585a65dc4ad9b6a6bd6e42..dabae1007afcb376218e65b022df72c494c8ab51 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -213,7 +213,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons
+rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,sync_unsafe_cell
 
 # `--out-dir` is required to avoid temporaries being created by `rustc` in the
 # current working directory, which may be not accessible in the out-of-tree

-- 
2.47.0

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> This series extends the `module!` macro with support module parameters.

Eeek, why?

Module parameters are from the 1990's, back when we had no idea what we
were doing and thought that a simple "one variable for a driver that
controls multiple devices" was somehow a valid solution :)

Please only really add module parameters if you can prove that you
actually need a module parameter.

thanks,

greg k-h

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Greg KH]

On Fri, Dec 13, 2024 at 12:30:49PM +0100, Andreas Hindborg wrote:
> This patch includes changes required for Rust kernel modules to utilize
> module parameters. This code implements read only support for integer
> types without `sysfs` support.

read-only is VERY limited, and as such, only good for boot options,
which as I mentioned before, is not how any "modern" kernel driver
should be doing things.

And no sysfs interaction?  That's going to confuse the heck out of
people wondering why the option they added doesn't show up in the same
place it normally would if they did it in C, right?  Not that I'm saying
this should be done at all, just that this is going to be confusing
right off the bat which is probably not a good idea.

Friends don't let friends add new module parameters to the kernel :)

thanks,

greg k-h

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
>> This series extends the `module!` macro with support module parameters.
>
> Eeek, why?
>
> Module parameters are from the 1990's, back when we had no idea what we
> were doing and thought that a simple "one variable for a driver that
> controls multiple devices" was somehow a valid solution :)
>
> Please only really add module parameters if you can prove that you
> actually need a module parameter.

I really need module parameters to make rust null block feature
compatible with C null block.

Let's not block interfacing parts of the kernel because we decided that
the way we (well not me, I was not around) did things in the 80's was
less than stellar. I mean, we would get nowhere.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

Hi Luis, Petr, Sami, Dani,

I just noticed that I failed in email and forgot to add you to the
recipient list of this series. Do you want a resend, or is this
sufficient?

Best regards,
Andreas Hindborg


"Andreas Hindborg" <a.hindborg@kernel.org> writes:

> This series extends the `module!` macro with support module parameters. It
> also adds some string to integer parsing functions and updates `BStr` with
> a method to strip a string prefix.
>
> This series stated out as code by Adam Bratschi-Kaye lifted from the original
> `rust` branch [1].
>
> Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1]
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> Changes since v2 [1]:
> - use `SyncUnsafeCell` rather than `static mut` and simplify parameter access
> - remove `Display` bound from `ModuleParam`
> - automatically generate documentation for `PARAM_OPS_.*`
> - remove `as *const _ as *mut_` phrasing
> - inline parameter name in struct instantiation in  `emit_params`
> - move `RacyKernelParam` out of macro template
> - use C string literals rather than byte string literals with explicit null
> - template out `__{name}_{param_name}` in `emit_param`
> - indent template in `emit_params`
> - use let-else expression in `emit_params` to get rid of an indentation level
> - document `expect_string_field`
> - move invication of `impl_int_module_param` to be closer to macro def
> - move attributes after docs in `make_param_ops`
> - rename `impl_module_param` to impl_int_module_param`
> - use `ty` instead of `ident` in `impl_parse_int`
> - use `BStr` instead of `&str` for string manipulation
> - move string parsing functions to seperate patch and add examples, fix bugs
> - degrade comment about future support from doc comment to regular comment
> - remove std lib path from `Sized` marker
> - update documentation for `trait ModuleParam`
>
> Changes since v1 [2]:
> - Remove support for params without values (`NOARG_ALLOWED`).
> - Improve documentation for `try_from_param_arg`.
> - Use prelude import.
> - Refactor `try_from_param_arg` to return `Result`.
> - Refactor `ParseInt::from_str` to return `Result`.
> - Move C callable functions out of `ModuleParam` trait.
> - Rename literal string field parser to `expect_string_field`.
> - Move parameter parsing from generation to parsing stage.
> - Use absolute type paths in macro code.
> - Inline `kparam`and `read_func` values.
> - Resolve TODO regarding alignment attributes.
> - Remove unnecessary unsafe blocks in macro code.
> - Improve error message for unrecognized parameter types.
> - Do not use `self` receiver when reading parameter value.
> - Add parameter documentation to `module!` macro.
> - Use empty `enum` for parameter type.
> - Use `addr_of_mut` to get address of parameter value variable.
> - Enabled building of docs for for `module_param` module.
>
> Link: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/ [2]
> Link: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/ [1]
>
> ---
>
> ---
> Andreas Hindborg (4):
>       rust: str: implement `PartialEq` for `BStr`
>       rust: str: implement `strip_prefix` for `BStr`
>       rust: str: add radix prefixed integer parsing functions
>       rust: add parameter support to the `module!` macro
>
>  rust/kernel/lib.rs           |   2 +
>  rust/kernel/module_param.rs  | 238 +++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/str.rs           | 138 +++++++++++++++++++++++++
>  rust/macros/helpers.rs       |  10 ++
>  rust/macros/lib.rs           |  31 ++++++
>  rust/macros/module.rs        | 188 ++++++++++++++++++++++++++++++----
>  samples/rust/rust_minimal.rs |  10 ++
>  scripts/Makefile.build       |   2 +-
>  8 files changed, 600 insertions(+), 19 deletions(-)
> ---
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> change-id: 20241211-module-params-v3-ae7e5c8d8b5a
>
> Best regards,

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Fri, Dec 13, 2024 at 12:30:49PM +0100, Andreas Hindborg wrote:
>> This patch includes changes required for Rust kernel modules to utilize
>> module parameters. This code implements read only support for integer
>> types without `sysfs` support.
>
> read-only is VERY limited, and as such, only good for boot options,
> which as I mentioned before, is not how any "modern" kernel driver
> should be doing things.

I only added what is required to get rust null block compatibility
going. I did not want to add dead code - I heard that is frowned upon.

> And no sysfs interaction?  That's going to confuse the heck out of
> people wondering why the option they added doesn't show up in the same
> place it normally would if they did it in C, right?  Not that I'm saying
> this should be done at all, just that this is going to be confusing
> right off the bat which is probably not a good idea.

No, these work the same way as their counter parts in C. They reuse the
same C machinery. They just only allow the user to specify a subset of
the permission flags.

The C null_blk parameters are actually configured to appear read-only in
sysfs. I guess I should add that in a follow-up.

> Friends don't let friends add new module parameters to the kernel :)

OK, that makes sense. But I'm trying to build something that plugs
in where we currently have a piece of C code that relies on module
parameters.

Jens, would you be OK with Rust null block only providing configuration
through configfs?


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Alice Ryhl]

On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Greg KH" <gregkh@linuxfoundation.org> writes:
>
> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> >> This series extends the `module!` macro with support module parameters.
> >
> > Eeek, why?
> >
> > Module parameters are from the 1990's, back when we had no idea what we
> > were doing and thought that a simple "one variable for a driver that
> > controls multiple devices" was somehow a valid solution :)
> >
> > Please only really add module parameters if you can prove that you
> > actually need a module parameter.
>
> I really need module parameters to make rust null block feature
> compatible with C null block.

Instead of providing module parameters to Rust code, you could
implement that part of Rust nullblk in C. That way, you discourage
future Rust drivers from using module parameters without making it
impossible to have them in Rust nullblk.

Alice

Re: [PATCH v3 1/4] rust: str: implement `PartialEq` for `BStr`

[Alice Ryhl]

On Fri, Dec 13, 2024 at 12:33 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Implement `PartialEq` for `BStr` by comparing underlying byte slices.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 12:33 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> +#![feature(sync_unsafe_cell)]

Please mention this in the commit message, the status of the feature
and justify the addition.

> +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h)

Please use `srctree`.

> +/// Newtype to make `bindings::kernel_param` `Sync`.

Please add intra-doc links where applicable, e.g. `Sync` here.

> +unsafe extern "C" fn set_param<T>(
> +    val: *const core::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.
> +    assert!(!val.is_null());

Should this return an error instead?

> +/// Write a string representation of the current parameter value to `buf`.
> +///
> +/// # Safety
> +///
> +/// Must not be called.
> +///
> +/// # Note
> +///
> +/// This should not be called as we declare all parameters as read only.
> +#[allow(clippy::extra_unused_type_parameters)]
> +unsafe extern "C" fn get_param<T>(
> +    _buf: *mut core::ffi::c_char,
> +    _param: *const crate::bindings::kernel_param,
> +) -> core::ffi::c_int
> +where
> +    T: ModuleParam,
> +{
> +    unreachable!("Parameters are not readable");
> +}

Do we need this? Can't the `ops` callback be `null`?

> +/// The `arg` field of `param` must be an initialized instance of `Self`.

`Self`?

> +/// Generate a static [`kernel_param_ops`](../../../include/linux/moduleparam.h) struct.

`srctree`.

> +/// Parse a token stream of the form `expected_name: "value",` and return the
> +/// string in the position of "value". Panics on parse error.

`# Panics` section.

> +/// `type` may be one of
> +///
> +/// - `i8`
> +/// - `u8`
> +/// - `i8`
> +/// - `u8`
> +/// - `i16`
> +/// - `u16`
> +/// - `i32`
> +/// - `u32`
> +/// - `i64`
> +/// - `u64`
> +/// - `isize`
> +/// - `usize`

Can these be intra-doc links?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>
>> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
>> >> This series extends the `module!` macro with support module parameters.
>> >
>> > Eeek, why?
>> >
>> > Module parameters are from the 1990's, back when we had no idea what we
>> > were doing and thought that a simple "one variable for a driver that
>> > controls multiple devices" was somehow a valid solution :)
>> >
>> > Please only really add module parameters if you can prove that you
>> > actually need a module parameter.
>>
>> I really need module parameters to make rust null block feature
>> compatible with C null block.
>
> Instead of providing module parameters to Rust code, you could
> implement that part of Rust nullblk in C. That way, you discourage
> future Rust drivers from using module parameters without making it
> impossible to have them in Rust nullblk.

If the opinion of the community is really to phase out module parameters
for all new drivers (is it?), I can maybe move the code in question into
the Rust null_blk driver.

I was kind of looking forward to having zero unsafe code in the driver
though.

On the other hand, rust null block might not be the only "rewrite in rust and keep
compatibility" project to ever see the light of day.


Best regards,
Andreas Hindborg

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

Hi Miguel,

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Dec 13, 2024 at 12:33 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> +#![feature(sync_unsafe_cell)]
>
> Please mention this in the commit message, the status of the feature
> and justify the addition.

I forgot, thanks for pointing that out. Following the discussion of v2
of this series I can understand that `mut static` is discouraged and
scheduled for removal. Interior mutability via `SyncUnsafeCell` provides
the same functionality and it is my understanding that this feature is
on track to be stabilized.

>
>> +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h)
>
> Please use `srctree`.

Ok.

>
>> +/// Newtype to make `bindings::kernel_param` `Sync`.
>
> Please add intra-doc links where applicable, e.g. `Sync` here.

Will do.

>
>> +unsafe extern "C" fn set_param<T>(
>> +    val: *const core::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.
>> +    assert!(!val.is_null());
>
> Should this return an error instead?

Not sure. `val` being null not supposed to happen in the current
configuration. It should be an unreachable state. So BUG is the right thing?

>
>> +/// Write a string representation of the current parameter value to `buf`.
>> +///
>> +/// # Safety
>> +///
>> +/// Must not be called.
>> +///
>> +/// # Note
>> +///
>> +/// This should not be called as we declare all parameters as read only.
>> +#[allow(clippy::extra_unused_type_parameters)]
>> +unsafe extern "C" fn get_param<T>(
>> +    _buf: *mut core::ffi::c_char,
>> +    _param: *const crate::bindings::kernel_param,
>> +) -> core::ffi::c_int
>> +where
>> +    T: ModuleParam,
>> +{
>> +    unreachable!("Parameters are not readable");
>> +}
>
> Do we need this? Can't the `ops` callback be `null`?

Not in the current configuration. The parameters can only be declared
"read only". It should be impossible for anyone to call this function.

>
>> +/// The `arg` field of `param` must be an initialized instance of `Self`.
>
> `Self`?

That whole line is wrong, thanks for spotting. It should read "`arg` must
be an initialized instance of `T`. This function takes ownership of
the `T` pointed to by `arg`.".

>
>> +/// Generate a static [`kernel_param_ops`](../../../include/linux/moduleparam.h) struct.
>
> `srctree`.

👍

>
>> +/// Parse a token stream of the form `expected_name: "value",` and return the
>> +/// string in the position of "value". Panics on parse error.
>
> `# Panics` section.

Ok.

>
>> +/// `type` may be one of
>> +///
>> +/// - `i8`
>> +/// - `u8`
>> +/// - `i8`
>> +/// - `u8`
>> +/// - `i16`
>> +/// - `u16`
>> +/// - `i32`
>> +/// - `u32`
>> +/// - `i64`
>> +/// - `u64`
>> +/// - `isize`
>> +/// - `usize`
>
> Can these be intra-doc links?

Yes!


Thanks for the comments!



Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote:
> "Greg KH" <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> >> This series extends the `module!` macro with support module parameters.
> >
> > Eeek, why?
> >
> > Module parameters are from the 1990's, back when we had no idea what we
> > were doing and thought that a simple "one variable for a driver that
> > controls multiple devices" was somehow a valid solution :)
> >
> > Please only really add module parameters if you can prove that you
> > actually need a module parameter.
> 
> I really need module parameters to make rust null block feature
> compatible with C null block.

Is that a requirement?  That wasn't documented here :(

You should have put the user of these apis in the series as you have
that code already in the tree, right?

> Let's not block interfacing parts of the kernel because we decided that
> the way we (well not me, I was not around) did things in the 80's was
> less than stellar. I mean, we would get nowhere.

On the contrary, if we don't learn from our past mistakes, we will
constantly keep making them and prevent others from "doing the right
thing" by default.

I would strongly prefer that any driver not have any module parameters
at all, as drivers don't work properly that way (again, they need to
handle multiple devices, which does not work for a module parameter.)

That's why we created sysfs, configfs, and lots of other things, to
learn from our past mistakes.

thanks,

greg k-h

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote:
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>
>> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
>> >> This series extends the `module!` macro with support module parameters.
>> >
>> > Eeek, why?
>> >
>> > Module parameters are from the 1990's, back when we had no idea what we
>> > were doing and thought that a simple "one variable for a driver that
>> > controls multiple devices" was somehow a valid solution :)
>> >
>> > Please only really add module parameters if you can prove that you
>> > actually need a module parameter.
>>
>> I really need module parameters to make rust null block feature
>> compatible with C null block.
>
> Is that a requirement?  That wasn't documented here :(
>
> You should have put the user of these apis in the series as you have
> that code already in the tree, right?

Sorry, my mistake. I'm trying to build a rust implementation of C
null_blk, and one the bits I need for that is module parameters.

>
>> Let's not block interfacing parts of the kernel because we decided that
>> the way we (well not me, I was not around) did things in the 80's was
>> less than stellar. I mean, we would get nowhere.
>
> On the contrary, if we don't learn from our past mistakes, we will
> constantly keep making them and prevent others from "doing the right
> thing" by default.
>
> I would strongly prefer that any driver not have any module parameters
> at all, as drivers don't work properly that way (again, they need to
> handle multiple devices, which does not work for a module parameter.)
>
> That's why we created sysfs, configfs, and lots of other things, to
> learn from our past mistakes.

OK. I understand. It makes sense even :) I wish I knew that this was a
thing before I spent spare cycles fixing this up for v3 though.

I'm not getting a clear reading on the following, perhaps you can
clarify:

 - Is the community aligned on dropping module parameters for all new
   drivers?
   - If so, was this decided upon at some point or is this a fluid
     decision that is just manifesting now?
 - Does this ban of module parameters also cover cases where backwards
   compatibility is desirable?
 - Can we merge this so I can move forward at my current projected
   course, or should I plan on dealing with not having this available?

Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote:
> "Greg KH" <gregkh@linuxfoundation.org> writes:
> > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote:
> >> "Greg KH" <gregkh@linuxfoundation.org> writes:
> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> I'm not getting a clear reading on the following, perhaps you can
> clarify:
> 
>  - Is the community aligned on dropping module parameters for all new
>    drivers?
>    - If so, was this decided upon at some point or is this a fluid
>      decision that is just manifesting now?

It's something that I've been saying in review comments of drivers for
many many years now.  Again, it was one of the main reasons we created
configfs and sysfs all those decades ago, because module parameters just
do not work properly for drivers in almost all cases.

>  - Does this ban of module parameters also cover cases where backwards
>    compatibility is desirable?

No, we don't break existing kernel features, but if you are writing a
new driver, don't add them and then there's no compatibility issue.

We don't normally allow "rewrites" of drivers, but if we do, yes, you
would have to implement the old features if needed.

As you just seem to want to write an "example" block driver, no need to
add the module option there, just do it right this time in how to
properly configure things.

>  - Can we merge this so I can move forward at my current projected
>    course, or should I plan on dealing with not having this available?

We generally do not want to merge apis without any real users, as it's
hard to justify them, right?  Also, we don't even know if they work
properly or not.

thanks,

greg k-h

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 2:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> scheduled for removal. Interior mutability via `SyncUnsafeCell` provides
> the same functionality and it is my understanding that this feature is
> on track to be stabilized.

I am not sure about the last bit, but even if it is on track, we do
not want to start using new language features or APIs that could
potentially change.

And even if it is a feature that we are sure will not change, we
should still let upstream Rust know before using it, since we are
actively discussing with them the remaining unstable items that the
kernel needs and they are putting the kernel in their roadmap.

So I suggest we mention it next week in the Rust/Rust for Linux meeting.

> Not sure. `val` being null not supposed to happen in the current
> configuration. It should be an unreachable state. So BUG is the right thing?

Since you can easily return an error in this situation, I would say
ideally a `WARN_ON_ONCE` + returning an error would be the best
option, and covers you in case the rest changes and someone forgets to
update this.

> Not in the current configuration. The parameters can only be declared
> "read only". It should be impossible for anyone to call this function.

What I meant is, can you avoid writing the function to begin with, by
leaving a null function pointer in the `kernel_param_ops` struct, i.e.
`None`?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Luis Chamberlain]

On Fri, Dec 13, 2024 at 01:28:27PM +0100, Andreas Hindborg wrote:
> 
> Hi Luis, Petr, Sami, Dani,
> 
> I just noticed that I failed in email and forgot to add you to the
> recipient list of this series. Do you want a resend, or is this
> sufficient?

If the patches didn't go to linux-modules, then yes, a resend would be
good so that others subscribed to that list can also review. You can
also wait for a v2. Up to you. I won't read it until I get it on my
inbox.

  Luis

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote:
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>> > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote:
>> >> "Greg KH" <gregkh@linuxfoundation.org> writes:
>> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
>> I'm not getting a clear reading on the following, perhaps you can
>> clarify:
>>
>>  - Is the community aligned on dropping module parameters for all new
>>    drivers?
>>    - If so, was this decided upon at some point or is this a fluid
>>      decision that is just manifesting now?
>
> It's something that I've been saying in review comments of drivers for
> many many years now.  Again, it was one of the main reasons we created
> configfs and sysfs all those decades ago, because module parameters just
> do not work properly for drivers in almost all cases.

Thinking a bit about this - are you sure there are no situations where
module parameters is actually the right choice? I could imagine deeply
embedded deployments, potentially with all required modules linked in to
a static kernel image. It is probably desirable to be able to pass
configuration to the modules via the kernel command line in this
situation. If not for configuration in the field, then for a development
situation.

Surely there are also situations in regular PC setups where it is
desirable to pass configuration data to a module, so that it is
available at load time. With configfs, module initialization becomes a
two stage process of first loading and then configuring. That is not
always what you would want.

Since module parameters actually do show up in sysfs as writable
entries, when the right flags are passed, they do feel very similar to
sysfs/configfs for simple use cases. What are the reasons they should
not be usable for these simple use cases?

>
>>  - Does this ban of module parameters also cover cases where backwards
>>    compatibility is desirable?
>
> No, we don't break existing kernel features, but if you are writing a
> new driver, don't add them and then there's no compatibility issue.
>
> We don't normally allow "rewrites" of drivers, but if we do, yes, you
> would have to implement the old features if needed.
>
> As you just seem to want to write an "example" block driver, no need to
> add the module option there, just do it right this time in how to
> properly configure things.
>
>>  - Can we merge this so I can move forward at my current projected
>>    course, or should I plan on dealing with not having this available?
>
> We generally do not want to merge apis without any real users, as it's
> hard to justify them, right?  Also, we don't even know if they work
> properly or not.

While null_blk is just a piece of testing infrastructure that you would
not deploy in a production environment, it is still a "real user". I am
sure we can agree on the importance of testing.

The exercise I am undertaking is to produce a drop in replacement of the
existing C null_blk driver. If all goes well, we are considering to swap
the C implementation for the Rust implementation in X number of years.
Granted - a lot of things have to fall into place for that to happen,
but that is the plan. This plan does not really work well if the two
modules do not have the same interface.

I understand that you would like to phase out module parameters, but I
don't think blocking their use from Rust is the right way to go about
that task. If you really feel that module parameters have no place in
new drivers, I would suggest that to be part of review process for each
individual new driver - not at the stage of enabling module parameters
for Rust in general.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote:
> "Greg KH" <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote:
> >> "Greg KH" <gregkh@linuxfoundation.org> writes:
> >> > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote:
> >> >> "Greg KH" <gregkh@linuxfoundation.org> writes:
> >> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> >> I'm not getting a clear reading on the following, perhaps you can
> >> clarify:
> >>
> >>  - Is the community aligned on dropping module parameters for all new
> >>    drivers?
> >>    - If so, was this decided upon at some point or is this a fluid
> >>      decision that is just manifesting now?
> >
> > It's something that I've been saying in review comments of drivers for
> > many many years now.  Again, it was one of the main reasons we created
> > configfs and sysfs all those decades ago, because module parameters just
> > do not work properly for drivers in almost all cases.
> 
> Thinking a bit about this - are you sure there are no situations where
> module parameters is actually the right choice? I could imagine deeply
> embedded deployments, potentially with all required modules linked in to
> a static kernel image. It is probably desirable to be able to pass
> configuration to the modules via the kernel command line in this
> situation. If not for configuration in the field, then for a development
> situation.

I'm not saying "no situations at all", I'm saying "almost no situations
need a module parameter, and almost NO driver needs one."  That does not
mean "none".

> Surely there are also situations in regular PC setups where it is
> desirable to pass configuration data to a module, so that it is
> available at load time. With configfs, module initialization becomes a
> two stage process of first loading and then configuring. That is not
> always what you would want.

"regular" PC setups do not want module parameters either, they should
all be dynamic busses and everything should "just work".

> Since module parameters actually do show up in sysfs as writable
> entries, when the right flags are passed, they do feel very similar to
> sysfs/configfs for simple use cases. What are the reasons they should
> not be usable for these simple use cases?

Because they are almost never a good idea for a driver because drivers
can control multiple devices and how do you show that in a single value?

For non-drivers, it's a different issue, but that's not what you are
considering here :)

> >>  - Does this ban of module parameters also cover cases where backwards
> >>    compatibility is desirable?
> >
> > No, we don't break existing kernel features, but if you are writing a
> > new driver, don't add them and then there's no compatibility issue.
> >
> > We don't normally allow "rewrites" of drivers, but if we do, yes, you
> > would have to implement the old features if needed.
> >
> > As you just seem to want to write an "example" block driver, no need to
> > add the module option there, just do it right this time in how to
> > properly configure things.
> >
> >>  - Can we merge this so I can move forward at my current projected
> >>    course, or should I plan on dealing with not having this available?
> >
> > We generally do not want to merge apis without any real users, as it's
> > hard to justify them, right?  Also, we don't even know if they work
> > properly or not.
> 
> While null_blk is just a piece of testing infrastructure that you would
> not deploy in a production environment, it is still a "real user". I am
> sure we can agree on the importance of testing.
> 
> The exercise I am undertaking is to produce a drop in replacement of the
> existing C null_blk driver. If all goes well, we are considering to swap
> the C implementation for the Rust implementation in X number of years.
> Granted - a lot of things have to fall into place for that to happen,
> but that is the plan. This plan does not really work well if the two
> modules do not have the same interface.

Why do you have to have the same interface?  Why not do it "properly"
and make it use configfs that way you can have multiple devices and test
them all at the same time?

As this is "just" a testing driver, there should not be any need to keep
the same user/kernel api for setting things up, right?

Again, don't make the mistakes we have in the past, drivers should NOT
be using module parameters.

> I understand that you would like to phase out module parameters, but I
> don't think blocking their use from Rust is the right way to go about
> that task. If you really feel that module parameters have no place in
> new drivers, I would suggest that to be part of review process for each
> individual new driver - not at the stage of enabling module parameters
> for Rust in general.

I'm saying that module parameters do NOT belong in a driver, which is
what you are wanting to do here.  And as for adding new apis, please
only do so when you have a real user, I don't see a real user for module
parameters in rust just yet.  If that changes, I'll reconsider my stance :)

thanks,

greg k-h

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Mon, Dec 16, 2024 at 01:14:16PM +0100, Greg KH wrote:
> On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote:
> > The exercise I am undertaking is to produce a drop in replacement of the
> > existing C null_blk driver. If all goes well, we are considering to swap
> > the C implementation for the Rust implementation in X number of years.
> > Granted - a lot of things have to fall into place for that to happen,
> > but that is the plan. This plan does not really work well if the two
> > modules do not have the same interface.
> 
> Why do you have to have the same interface?  Why not do it "properly"
> and make it use configfs that way you can have multiple devices and test
> them all at the same time?

Wait, null_blk already uses configfs, so just use that!  I'd like to see
the rust bindings for that api as that will be needed by lots of code.

thanks,

greg k-h

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote:
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>
>> > On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote:

[cut]

>> >
>> >>  - Can we merge this so I can move forward at my current projected
>> >>    course, or should I plan on dealing with not having this available?
>> >
>> > We generally do not want to merge apis without any real users, as it's
>> > hard to justify them, right?  Also, we don't even know if they work
>> > properly or not.
>>
>> While null_blk is just a piece of testing infrastructure that you would
>> not deploy in a production environment, it is still a "real user". I am
>> sure we can agree on the importance of testing.
>>
>> The exercise I am undertaking is to produce a drop in replacement of the
>> existing C null_blk driver. If all goes well, we are considering to swap
>> the C implementation for the Rust implementation in X number of years.
>> Granted - a lot of things have to fall into place for that to happen,
>> but that is the plan. This plan does not really work well if the two
>> modules do not have the same interface.
>
> Why do you have to have the same interface?  Why not do it "properly"
> and make it use configfs that way you can have multiple devices and test
> them all at the same time?
>
> As this is "just" a testing driver, there should not be any need to keep
> the same user/kernel api for setting things up, right?

Because that would break all the users that use the old interface.

>
> Again, don't make the mistakes we have in the past, drivers should NOT
> be using module parameters.
>
>> I understand that you would like to phase out module parameters, but I
>> don't think blocking their use from Rust is the right way to go about
>> that task. If you really feel that module parameters have no place in
>> new drivers, I would suggest that to be part of review process for each
>> individual new driver - not at the stage of enabling module parameters
>> for Rust in general.
>
> I'm saying that module parameters do NOT belong in a driver, which is
> what you are wanting to do here.  And as for adding new apis, please
> only do so when you have a real user, I don't see a real user for module
> parameters in rust just yet.  If that changes, I'll reconsider my stance :)

I guess we disagree about what is "real" and what is not.

In my view, null_blk is real, it is used by real people to do real work.
They get real annoyed when the interface for their real tools change -
thus making it more difficult to do this experiment.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Andreas Hindborg]

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Mon, Dec 16, 2024 at 01:14:16PM +0100, Greg KH wrote:
>> On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote:
>> > The exercise I am undertaking is to produce a drop in replacement of the
>> > existing C null_blk driver. If all goes well, we are considering to swap
>> > the C implementation for the Rust implementation in X number of years.
>> > Granted - a lot of things have to fall into place for that to happen,
>> > but that is the plan. This plan does not really work well if the two
>> > modules do not have the same interface.
>>
>> Why do you have to have the same interface?  Why not do it "properly"
>> and make it use configfs that way you can have multiple devices and test
>> them all at the same time?
>
> Wait, null_blk already uses configfs, so just use that!  I'd like to see
> the rust bindings for that api as that will be needed by lots of code.

I intend to support both.


Best regards,
Andreas Hindborg

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Jens Axboe]

On 12/16/24 6:02 AM, Andreas Hindborg wrote:
>>> I understand that you would like to phase out module parameters, but I
>>> don't think blocking their use from Rust is the right way to go about
>>> that task. If you really feel that module parameters have no place in
>>> new drivers, I would suggest that to be part of review process for each
>>> individual new driver - not at the stage of enabling module parameters
>>> for Rust in general.
>>
>> I'm saying that module parameters do NOT belong in a driver, which is
>> what you are wanting to do here.  And as for adding new apis, please
>> only do so when you have a real user, I don't see a real user for module
>> parameters in rust just yet.  If that changes, I'll reconsider my stance :)
> 
> I guess we disagree about what is "real" and what is not.
> 
> In my view, null_blk is real, it is used by real people to do real work.
> They get real annoyed when the interface for their real tools change -
> thus making it more difficult to do this experiment.

I'd have to agree with that - yes, null_blk doesn't host any real
applications, but it is the backbone of a lot of testing that blktests
and others do. Hence it's very real in that sense, and the rust version
of null_blk should provide and mimic how the C version works for ease of
testing.

If this was a new driver where no prior art exists in terms of users and
API, then I'd certainly agree with Greg. But that's not the case here.

-- 
Jens Axboe

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Greg KH]

On Mon, Dec 16, 2024 at 07:43:12AM -0700, Jens Axboe wrote:
> On 12/16/24 6:02 AM, Andreas Hindborg wrote:
> >>> I understand that you would like to phase out module parameters, but I
> >>> don't think blocking their use from Rust is the right way to go about
> >>> that task. If you really feel that module parameters have no place in
> >>> new drivers, I would suggest that to be part of review process for each
> >>> individual new driver - not at the stage of enabling module parameters
> >>> for Rust in general.
> >>
> >> I'm saying that module parameters do NOT belong in a driver, which is
> >> what you are wanting to do here.  And as for adding new apis, please
> >> only do so when you have a real user, I don't see a real user for module
> >> parameters in rust just yet.  If that changes, I'll reconsider my stance :)
> > 
> > I guess we disagree about what is "real" and what is not.
> > 
> > In my view, null_blk is real, it is used by real people to do real work.
> > They get real annoyed when the interface for their real tools change -
> > thus making it more difficult to do this experiment.
> 
> I'd have to agree with that - yes, null_blk doesn't host any real
> applications, but it is the backbone of a lot of testing that blktests
> and others do. Hence it's very real in that sense, and the rust version
> of null_blk should provide and mimic how the C version works for ease of
> testing.
> 
> If this was a new driver where no prior art exists in terms of users and
> API, then I'd certainly agree with Greg. But that's not the case here.

Ok, so are you going to drop the C version and go with the rust version
if it shows up?  Surely you don't want duplicate drivers for the same
thing in the tree, right?

thanks,

greg k-h

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Jens Axboe]

On 12/16/24 8:03 AM, Greg KH wrote:
> On Mon, Dec 16, 2024 at 07:43:12AM -0700, Jens Axboe wrote:
>> On 12/16/24 6:02 AM, Andreas Hindborg wrote:
>>>>> I understand that you would like to phase out module parameters, but I
>>>>> don't think blocking their use from Rust is the right way to go about
>>>>> that task. If you really feel that module parameters have no place in
>>>>> new drivers, I would suggest that to be part of review process for each
>>>>> individual new driver - not at the stage of enabling module parameters
>>>>> for Rust in general.
>>>>
>>>> I'm saying that module parameters do NOT belong in a driver, which is
>>>> what you are wanting to do here.  And as for adding new apis, please
>>>> only do so when you have a real user, I don't see a real user for module
>>>> parameters in rust just yet.  If that changes, I'll reconsider my stance :)
>>>
>>> I guess we disagree about what is "real" and what is not.
>>>
>>> In my view, null_blk is real, it is used by real people to do real work.
>>> They get real annoyed when the interface for their real tools change -
>>> thus making it more difficult to do this experiment.
>>
>> I'd have to agree with that - yes, null_blk doesn't host any real
>> applications, but it is the backbone of a lot of testing that blktests
>> and others do. Hence it's very real in that sense, and the rust version
>> of null_blk should provide and mimic how the C version works for ease of
>> testing.
>>
>> If this was a new driver where no prior art exists in terms of users and
>> API, then I'd certainly agree with Greg. But that's not the case here.
> 
> Ok, so are you going to drop the C version and go with the rust version
> if it shows up?  Surely you don't want duplicate drivers for the same
> thing in the tree, right?

Maybe at some point? The rust version is already there, it's just very
limited compared to the C version so far. The point of the rust null_blk
is to build out the API so that a real driver can get implemented as
well. For now, I think the plan is to just have it be the rust
playground on the block side.

Given that null_blk is the center piece of a lot of testing, it's not
necessarily an issue to have duplicate implementations of the same
driver. In fact it may be pretty useful for people coming from either
side and wanting to compare implementations and how to do various things
in either language. It's like an actually useful skeleton driver in that
sense too.

Whether one or the other will be the only one in the tree in the future
depends on a lot of other things that aren't necessarily driven or
decided by the rust null_blk driver.

-- 
Jens Axboe

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Miguel Ojeda]

On Mon, Dec 16, 2024 at 4:08 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Maybe at some point? The rust version is already there, it's just very
> limited compared to the C version so far. The point of the rust null_blk
> is to build out the API so that a real driver can get implemented as
> well. For now, I think the plan is to just have it be the rust
> playground on the block side.
>
> Given that null_blk is the center piece of a lot of testing, it's not
> necessarily an issue to have duplicate implementations of the same
> driver. In fact it may be pretty useful for people coming from either
> side and wanting to compare implementations and how to do various things
> in either language. It's like an actually useful skeleton driver in that
> sense too.

Agreed. I would suggest to consider marking it as a Rust reference
driver, since it is a prime candidate for it:

    https://rust-for-linux.com/rust-reference-drivers

That way, it is clearer that the duplication is meant to build the
abstractions and temporary in the long-term.

Then we can also easily track which ones are meant to be those, and
Greg can get justifiably angry at you/us if the duplication isn't
resolved when the right time comes... :)

Cheers,
Miguel

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Miguel Ojeda]

On Mon, Dec 16, 2024 at 4:39 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Agreed. I would suggest to consider marking it as a Rust reference
> driver, since it is a prime candidate for it:
>
>     https://rust-for-linux.com/rust-reference-drivers
>
> That way, it is clearer that the duplication is meant to build the
> abstractions and temporary in the long-term.
>
> Then we can also easily track which ones are meant to be those, and
> Greg can get justifiably angry at you/us if the duplication isn't
> resolved when the right time comes... :)

By the way, I half-jokingly suggested this elsewhere, but we could
trivially allow module parameters only for particular modules, i.e.
only allow to use the `params` key here if the name matches `rnull`
(or if they set a special flag or whatever).

Yes, it is a hack, but it would give people pause when trying to use
the feature, i.e. to think twice. And, to me, it makes sense to
encode/acknowledge this kind of thing explicitly anyway.

So if that would unblock this and reduce the chance of repeating
mistakes of the past, then we can easily do that too.

Cheers,
Miguel

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Simona Vetter]

On Fri, Dec 13, 2024 at 01:57:59PM +0100, Andreas Hindborg wrote:
> Alice Ryhl <aliceryhl@google.com> writes:
> 
> > On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> "Greg KH" <gregkh@linuxfoundation.org> writes:
> >>
> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> >> >> This series extends the `module!` macro with support module parameters.
> >> >
> >> > Eeek, why?
> >> >
> >> > Module parameters are from the 1990's, back when we had no idea what we
> >> > were doing and thought that a simple "one variable for a driver that
> >> > controls multiple devices" was somehow a valid solution :)
> >> >
> >> > Please only really add module parameters if you can prove that you
> >> > actually need a module parameter.
> >>
> >> I really need module parameters to make rust null block feature
> >> compatible with C null block.
> >
> > Instead of providing module parameters to Rust code, you could
> > implement that part of Rust nullblk in C. That way, you discourage
> > future Rust drivers from using module parameters without making it
> > impossible to have them in Rust nullblk.
> 
> If the opinion of the community is really to phase out module parameters
> for all new drivers (is it?), I can maybe move the code in question into
> the Rust null_blk driver.
> 
> I was kind of looking forward to having zero unsafe code in the driver
> though.
> 
> On the other hand, rust null block might not be the only "rewrite in rust and keep
> compatibility" project to ever see the light of day.

We still have tons of module parameters with the _unsafe annotations,
because they're really convenient for debugging and testing. Yes they're
hacks, yes you cannot use them for production, but they're useful.

So for drivers I'd say we definitely want to keep modules parameters
around, they're just too convenient compared to debugfs, especially when
it's things you have to set before the driver binds to the device.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/4] rust: extend `module!` macro with integer parameter support

[Josh Triplett]

On Mon, Dec 16, 2024 at 04:48:21PM +0100, Miguel Ojeda wrote:
> On Mon, Dec 16, 2024 at 4:39 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > Agreed. I would suggest to consider marking it as a Rust reference
> > driver, since it is a prime candidate for it:
> >
> >     https://rust-for-linux.com/rust-reference-drivers
> >
> > That way, it is clearer that the duplication is meant to build the
> > abstractions and temporary in the long-term.
> >
> > Then we can also easily track which ones are meant to be those, and
> > Greg can get justifiably angry at you/us if the duplication isn't
> > resolved when the right time comes... :)
> 
> By the way, I half-jokingly suggested this elsewhere, but we could
> trivially allow module parameters only for particular modules, i.e.
> only allow to use the `params` key here if the name matches `rnull`
> (or if they set a special flag or whatever).
> 
> Yes, it is a hack, but it would give people pause when trying to use
> the feature, i.e. to think twice. And, to me, it makes sense to
> encode/acknowledge this kind of thing explicitly anyway.
> 
> So if that would unblock this and reduce the chance of repeating
> mistakes of the past, then we can easily do that too.

This seems like a great idea. An allowlist of drivers that are allowed
to use module parameters would encourage *new* drivers to not use them,
and that allowlist can have a comment atop it saying "Only add your
driver to this list if it needs to maintain an interface compatible with
an existing driver in order to avoid breaking userspace. Otherwise, use
configfs, sysfs, debugfs, or something else other than module
parameters."

I wonder if we can implement such an allowlist for C modules, too. :)

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Dec 13, 2024 at 2:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> scheduled for removal. Interior mutability via `SyncUnsafeCell` provides
>> the same functionality and it is my understanding that this feature is
>> on track to be stabilized.
>
> I am not sure about the last bit, but even if it is on track, we do
> not want to start using new language features or APIs that could
> potentially change.
>
> And even if it is a feature that we are sure will not change, we
> should still let upstream Rust know before using it, since we are
> actively discussing with them the remaining unstable items that the
> kernel needs and they are putting the kernel in their roadmap.
>
> So I suggest we mention it next week in the Rust/Rust for Linux meeting.

I don't think we ever discussed this?

I was going to put this in the commit trailer for v4:

---
Version 3 of this patch enabled the unstable feature `sync_unsafe_cell` [1] to
avoid `static mut` variables as suggested by Trevor Tross and Benno Lossin [2].

As we are moving closer to a new edition, it is now clear that `static mut` is
not being deprecated in the 2024 edition as first anticipated [3].

Still, `SyncUnsafeCell` allows us to use safe code when referring to the
parameter value:

```
{param_name}.as_mut_ptr().cast()
```

rather than unsafe code:

```
unsafe { addr_of_mut!(__{name}_{param_name}_value) }.cast()
```

Thus, this version (4) keeps the feature enabled.

[1] https://github.com/rust-lang/rust/issues/95439
[2] https://lore.kernel.org/all/CALNs47sqt==o+hM5M1b0vTayKH177naybg_KurcirXszYAa22A@mail.gmail.com/
[3] https://github.com/rust-lang/rust/issues/53639#issuecomment-2434023115
---

What do you think?

>
>> Not sure. `val` being null not supposed to happen in the current
>> configuration. It should be an unreachable state. So BUG is the right thing?
>
> Since you can easily return an error in this situation, I would say
> ideally a `WARN_ON_ONCE` + returning an error would be the best
> option, and covers you in case the rest changes and someone forgets to
> update this.

Returning an error and `pr_warn!` is doable. As far as I can tell, we do
not have `WARN_ON_ONCE` yet?

>
>> Not in the current configuration. The parameters can only be declared
>> "read only". It should be impossible for anyone to call this function.
>
> What I meant is, can you avoid writing the function to begin with, by
> leaving a null function pointer in the `kernel_param_ops` struct, i.e.
> `None`?
>

It turns out we can!


Best regards,
Andreas Hindborg

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Alice Ryhl]

On Wed, Jan 8, 2025 at 1:45 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
>
> > On Fri, Dec 13, 2024 at 2:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> scheduled for removal. Interior mutability via `SyncUnsafeCell` provides
> >> the same functionality and it is my understanding that this feature is
> >> on track to be stabilized.
> >
> > I am not sure about the last bit, but even if it is on track, we do
> > not want to start using new language features or APIs that could
> > potentially change.
> >
> > And even if it is a feature that we are sure will not change, we
> > should still let upstream Rust know before using it, since we are
> > actively discussing with them the remaining unstable items that the
> > kernel needs and they are putting the kernel in their roadmap.
> >
> > So I suggest we mention it next week in the Rust/Rust for Linux meeting.
>
> I don't think we ever discussed this?
>
> I was going to put this in the commit trailer for v4:
>
> ---
> Version 3 of this patch enabled the unstable feature `sync_unsafe_cell` [1] to
> avoid `static mut` variables as suggested by Trevor Tross and Benno Lossin [2].
>
> As we are moving closer to a new edition, it is now clear that `static mut` is
> not being deprecated in the 2024 edition as first anticipated [3].
>
> Still, `SyncUnsafeCell` allows us to use safe code when referring to the
> parameter value:
>
> ```
> {param_name}.as_mut_ptr().cast()
> ```
>
> rather than unsafe code:
>
> ```
> unsafe { addr_of_mut!(__{name}_{param_name}_value) }.cast()
> ```
>
> Thus, this version (4) keeps the feature enabled.
>
> [1] https://github.com/rust-lang/rust/issues/95439
> [2] https://lore.kernel.org/all/CALNs47sqt==o+hM5M1b0vTayKH177naybg_KurcirXszYAa22A@mail.gmail.com/
> [3] https://github.com/rust-lang/rust/issues/53639#issuecomment-2434023115
> ---
>
> What do you think?

It's pretty easy to avoid needing this feature, so I don't think we
should enable it.

Alice

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Miguel Ojeda]

On Wed, Jan 8, 2025 at 1:45 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> I don't think we ever discussed this?

I don't think so -- we discussed other things related to 2025H1 the
last meeting. Perhaps you/we can bring it up in the next one?

> Version 3 of this patch enabled the unstable feature `sync_unsafe_cell` [1] to
> avoid `static mut` variables as suggested by Trevor Tross and Benno Lossin [2].
>
> As we are moving closer to a new edition, it is now clear that `static mut` is
> not being deprecated in the 2024 edition as first anticipated [3].
>
> Still, `SyncUnsafeCell` allows us to use safe code when referring to the
> parameter value:

> What do you think?

The justification seems fairly weak... Unless we are fairly confident
the API will be stable (even if not stabilized right now), I am not
sure why we would want to do this right now.

Can we provide our own `SyncUnsafeCell` instead in the meantime, if
you want to keep the advantages?

> Returning an error and `pr_warn!` is doable. As far as I can tell, we do
> not have `WARN_ON_ONCE` yet?

Please see https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/
in case it helps.

> It turns out we can!

That is what I expected :)

Thanks!

Cheers,
Miguel

Re: [PATCH v3 4/4] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Wed, Jan 8, 2025 at 1:45 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> I don't think we ever discussed this?
>
> I don't think so -- we discussed other things related to 2025H1 the
> last meeting. Perhaps you/we can bring it up in the next one?
>
>> Version 3 of this patch enabled the unstable feature `sync_unsafe_cell` [1] to
>> avoid `static mut` variables as suggested by Trevor Tross and Benno Lossin [2].
>>
>> As we are moving closer to a new edition, it is now clear that `static mut` is
>> not being deprecated in the 2024 edition as first anticipated [3].
>>
>> Still, `SyncUnsafeCell` allows us to use safe code when referring to the
>> parameter value:
>
>> What do you think?
>
> The justification seems fairly weak... Unless we are fairly confident
> the API will be stable (even if not stabilized right now), I am not
> sure why we would want to do this right now.
>
> Can we provide our own `SyncUnsafeCell` instead in the meantime, if
> you want to keep the advantages?

I like having the shared `ModuleParamAccess` struct to encapsulate the access
to the parameters rather than emitting an enum for every parameter
instance. For that to work without unsafe code when the user accesses
the parameter value, we need a non-mutable static that provides interior
mutability.

I could implement `Sync` for `ModuleParamAccess` or I could add
`kernel::types::SyncUnsafeCell`. Either way is fine for me.

>
>> Returning an error and `pr_warn!` is doable. As far as I can tell, we do
>> not have `WARN_ON_ONCE` yet?
>
> Please see https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/
> in case it helps.

Cool, I can list that as a prerequisite.


Best regards,
Andreas Hindborg