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

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

[Andreas Hindborg]

Extend the `module!` macro with support module parameters. Also add some string
to integer parsing functions and updates `BStr` with a method to strip a string
prefix.

Based on 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 in v12:
- Assign through pointer rather than using `core::ptr::replace`.
- Prevent a potential use-after-free during module teardown.
- Link to v11: https://lore.kernel.org/r/20250502-module-params-v3-v11-0-6096875a2b78@kernel.org

Changes in v11:
- Apply a few nits from Miguel.
- Link to v10: https://lore.kernel.org/r/20250501-module-params-v3-v10-0-4da485d343d5@kernel.org

Changes in v10:
- Apply fixups from Miguel:
  - Add integer type suffixes to `assert!` in tests.
  - Fix links to docs.kernel.org.
  - Applyy markdown and intra-doc links where possible.
  - Change to `///` for `mod` docs.
  - Slightly reword a comment.
  - Pluralize "Examples" section name.
  - Hide `use`s in example.
  - Removed `#[expect]` for the `rusttest` target.
- Link to v9: https://lore.kernel.org/r/20250321-module-params-v3-v9-0-28b905f2e345@kernel.org

Changes in v9:
- Remove UB when parsing the minimum integer values.
- Make `FromStr` trait unsafe, since wrong implementations can cause UB.
- Drop patches that were applied to rust-next.
- Link to v8: https://lore.kernel.org/r/20250227-module-params-v3-v8-0-ceeee85d9347@kernel.org

Changes in v8:
- Change print statement in sample to better communicate parameter name.
- Use imperative mode in commit messages.
- Remove prefix path from `EINVAL`.
- Change `try_from_param_arg` to accept `&BStr` rather than `&[u8]`.
- Parse integers without 128 bit integer types.
- Seal trait `FromStrRadix`.
- Strengthen safety requirement of `set_param`.
- Remove comment about Display and `PAGE_SIZE`.
- Add note describing why `ModuleParamAccess` is pub.
- Typo and grammar fixes for documentation.
- Update MAINTAINERS with rust module files.
- Link to v7: https://lore.kernel.org/r/20250218-module-params-v3-v7-0-5e1afabcac1b@kernel.org

Changes in v7:
- Remove dependency on `pr_warn_once` patches, replace with TODO.
- Rework `ParseInt::from_str` to avoid allocating.
- Add a comment explaining how we parse "0".
- Change trait bound on `Index` impl for `BStr` to match std library approach.
- Link to v6: https://lore.kernel.org/r/20250211-module-params-v3-v6-0-24b297ddc43d@kernel.org

Changes in v6:
- Fix a bug that prevented parsing of negative default values for
  parameters in the `module!` macro.
- Fix a bug that prevented parsing zero in `strip_radix`. Also add a
  test case for this.
- Add `AsRef<BStr>` for `[u8]` and `BStr`.
- Use `impl AsRef<BStr>` as type of prefix in `BStr::strip_prefix`.
- Link to v5: https://lore.kernel.org/r/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org

Changes in v5:
- Fix a typo in a safety comment in `set_param`.
- Use a match statement in `parse_int::strip_radix`.
- Add an implementation of `Index` for `BStr`.
- Fix a logic inversion bug where parameters would not be parsed.
- Use `kernel::ffi::c_char` in `set_param` rather than the one in `core`.
- Use `kernel::c_str!` rather than `c"..."` literal in module macro.
- Rebase on v6.14-rc1.
- Link to v4: https://lore.kernel.org/r/20250109-module-params-v3-v4-0-c208bcfbe11f@kernel.org

Changes in v4:
- Add module maintainers to Cc list (sorry)
- Add a few missing [`doc_links`]
- Add panic section to `expect_string_field`
- Fix a typo in safety requirement of `module_params::free`
- Change `assert!` to `pr_warn_once!` in `module_params::set_param`
- Remove `module_params::get_param` and install null pointer instead
- Remove use of the unstable feature `sync_unsafe_cell`
- Link to v3: https://lore.kernel.org/r/20241213-module-params-v3-v3-0-485a015ac2cf@kernel.org

Changes in v3:
- 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`
- Link to v2: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/

Changes in v2:
- 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 to v1: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/

---
Andreas Hindborg (3):
      rust: str: add radix prefixed integer parsing functions
      rust: add parameter support to the `module!` macro
      modules: add rust modules files to MAINTAINERS

 MAINTAINERS                  |   2 +
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs           | 172 +++++++++++++++++++++++++++++++++++-
 rust/macros/helpers.rs       |  25 ++++++
 rust/macros/lib.rs           |  31 +++++++
 rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
 samples/rust/rust_minimal.rs |  10 +++
 8 files changed, 619 insertions(+), 21 deletions(-)
---
base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
change-id: 20241211-module-params-v3-ae7e5c8d8b5a

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

[PATCH v12 1/3] 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.

Tested-by: Daniel Gomez <da.gomez@samsung.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 878111cb77bc..174e70397305 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -573,7 +573,6 @@ macro_rules! c_str {
 }
 
 #[cfg(test)]
-#[expect(clippy::items_after_test_module)]
 mod tests {
     use super::*;
 
@@ -946,3 +945,174 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 macro_rules! fmt {
     ($($f:tt)*) => ( core::format_args!($($f)*) )
 }
+
+/// Integer parsing functions for parsing signed and unsigned integers
+/// potentially prefixed with `0x`, `0o`, or `0b`.
+pub mod parse_int {
+    use crate::prelude::*;
+    use crate::str::BStr;
+    use core::ops::Deref;
+
+    // Make `FromStrRadix` a public type with a private name. This seals
+    // `ParseInt`, that is, prevents downstream users from implementing the
+    // trait.
+    mod private {
+        use crate::str::BStr;
+
+        /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
+        ///
+        /// # Safety
+        ///
+        /// The member functions of this trait must be implemented according to
+        /// their documentation.
+        ///
+        /// [`&BStr`]: kernel::str::BStr
+        // This is required because the `from_str_radix` function on the primitive
+        // integer types is not part of any trait.
+        pub unsafe trait FromStrRadix: Sized {
+            /// The minimum value this integer type can assume.
+            const MIN: Self;
+
+            /// Parse `src` to [`Self`] using radix `radix`.
+            fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
+
+            /// Return the absolute value of [`Self::MIN`].
+            fn abs_min() -> u64;
+
+            /// Perform bitwise 2's complement on `self`.
+            ///
+            /// Note: This function does not make sense for unsigned integers.
+            fn complement(self) -> Self;
+        }
+    }
+
+    /// 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) {
+        match src.deref() {
+            [b'0', b'x' | b'X', rest @ ..] => (16, rest.as_ref()),
+            [b'0', b'o' | b'O', rest @ ..] => (8, rest.as_ref()),
+            [b'0', b'b' | b'B', rest @ ..] => (2, rest.as_ref()),
+            // NOTE: We are including the leading zero to be able to parse
+            // literal `0` here. If we removed it as a radix prefix, we would
+            // not be able to parse `0`.
+            [b'0', ..] => (8, src),
+            _ => (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://docs.kernel.org/core-api/kernel-api.html#c.kstrtol
+    /// [`kstrtoul()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtoul
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::str::parse_int::ParseInt;
+    /// # use kernel::b_str;
+    ///
+    /// assert_eq!(Ok(0u8), u8::from_str(b_str!("0")));
+    ///
+    /// 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(127i8), i8::from_str(b_str!("127")));
+    /// assert!(i8::from_str(b_str!("128")).is_err());
+    /// assert_eq!(Ok(-128i8), i8::from_str(b_str!("-128")));
+    /// assert!(i8::from_str(b_str!("-129")).is_err());
+    /// assert_eq!(Ok(255u8), u8::from_str(b_str!("255")));
+    /// assert!(u8::from_str(b_str!("256")).is_err());
+    /// ```
+    pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
+        /// Parse a string according to the description in [`Self`].
+        fn from_str(src: &BStr) -> Result<Self> {
+            match src.deref() {
+                [b'-', rest @ ..] => {
+                    let (radix, digits) = strip_radix(rest.as_ref());
+                    // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
+                    // So if we want to parse negative numbers as positive and
+                    // later multiply by -1, we have to parse into a larger
+                    // integer. We choose `u64` as sufficiently large.
+                    //
+                    // NOTE: 128 bit integers are not available on all
+                    // platforms, hence the choice of 64 bits.
+                    let val = u64::from_str_radix(
+                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
+                        radix,
+                    )
+                    .map_err(|_| EINVAL)?;
+
+                    if val > Self::abs_min() {
+                        return Err(EINVAL);
+                    }
+
+                    if val == Self::abs_min() {
+                        return Ok(Self::MIN);
+                    }
+
+                    // SAFETY: We checked that `val` will fit in `Self` above.
+                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
+
+                    Ok(val.complement())
+                }
+                _ => {
+                    let (radix, digits) = strip_radix(src);
+                    Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+                }
+            }
+        }
+    }
+
+    macro_rules! impl_parse_int {
+        ($ty:ty) => {
+            // SAFETY: We implement the trait according to the documentation.
+            unsafe impl private::FromStrRadix for $ty {
+                const MIN: Self = <$ty>::MIN;
+
+                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)
+                }
+
+                fn abs_min() -> u64 {
+                    #[allow(unused_comparisons)]
+                    if Self::MIN < 0 {
+                        1u64 << (Self::BITS - 1)
+                    } else {
+                        0
+                    }
+                }
+
+                fn complement(self) -> Self {
+                    (!self).wrapping_add((1 as $ty))
+                }
+            }
+
+            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.2

[PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

Add support for module parameters to the `module!` macro. Implement read
only support for integer types without `sysfs` support.

Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
Tested-by: Daniel Gomez <da.gomez@samsung.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 ++++++
 rust/macros/lib.rs           |  31 +++++++
 rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
 samples/rust/rust_minimal.rs |  10 +++
 6 files changed, 446 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index de07aadd1ff5..9afb7eae9183 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -61,6 +61,7 @@
 pub mod kunit;
 pub mod list;
 pub mod miscdevice;
+pub mod module_param;
 #[cfg(CONFIG_NET)]
 pub mod net;
 pub mod of;
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
new file mode 100644
index 000000000000..ba80c9c87317
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for module parameters.
+//!
+//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
+
+use crate::prelude::*;
+use crate::str::BStr;
+
+/// 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 it
+// from Rust module.
+unsafe impl Sync for RacyKernelParam {}
+
+/// Types that can be used for module parameters.
+pub trait ModuleParam: Sized + Copy {
+    /// The [`ModuleParam`] will be used by the kernel module through this type.
+    ///
+    /// This may differ from `Self` if, for example, `Self` needs to track
+    /// ownership without exposing it or allocate extra space for other possible
+    /// parameter values.
+    // This is required to support string parameters in the future.
+    type Value: ?Sized;
+
+    /// 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`]: srctree/include/linux/slab.h
+    fn try_from_param_arg(arg: &'static BStr) -> 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`.
+///
+/// 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`.
+/// - `param.arg` must be a pointer to valid `*mut T` as set up by the
+///   [`module!`] macro.
+///
+/// # Invariants
+///
+/// Currently, we only support read-only parameters that are not readable
+/// from `sysfs`. Thus, this function is only called at kernel
+/// initialization time, or at module load time, and we have exclusive
+/// access to the parameter for the duration of the function.
+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(
+    val: *const kernel::ffi::c_char,
+    param: *const crate::bindings::kernel_param,
+) -> core::ffi::c_int
+where
+    T: ModuleParam,
+{
+    // NOTE: If we start supporting arguments without values, val _is_ allowed
+    // to be null here.
+    if val.is_null() {
+        // TODO: Use pr_warn_once available.
+        crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+        return EINVAL.to_errno();
+    }
+
+    // SAFETY: By function safety requirement, val is non-null and
+    // null-terminated. By C API contract, `val` is live and valid for reads
+    // for the duration of this function.
+    let arg = unsafe { CStr::from_char_ptr(val) };
+
+    crate::error::from_result(|| {
+        let new_value = T::try_from_param_arg(arg)?;
+
+        // SAFETY: `param` is guaranteed to be valid by C API contract
+        // and `arg` is guaranteed to point to an instance of `T`.
+        let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
+
+        // SAFETY: `old_value` is valid for writes, as we have exclusive
+        // access. `old_value` is pointing to an initialized static, and
+        // so it is properly initialized.
+        unsafe { *old_value = new_value };
+        Ok(0)
+    })
+}
+
+macro_rules! impl_int_module_param {
+    ($ty:ident) => {
+        impl ModuleParam for $ty {
+            type Value = $ty;
+
+            fn try_from_param_arg(arg: &'static BStr) -> Result<Self> {
+                <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
+            }
+        }
+    };
+}
+
+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.
+///
+/// Note: This type is `pub` because it is used by module crates to access
+/// parameter values.
+#[repr(transparent)]
+pub struct ModuleParamAccess<T> {
+    data: core::cell::UnsafeCell<T>,
+}
+
+// SAFETY: We only create shared references to the contents of this container,
+// so if `T` is `Sync`, so is `ModuleParamAccess`.
+unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
+
+impl<T> ModuleParamAccess<T> {
+    #[doc(hidden)]
+    pub const fn new(value: T) -> Self {
+        Self {
+            data: core::cell::UnsafeCell::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`](srctree/include/linux/moduleparam.h) struct.
+///
+/// # Examples
+///
+/// ```ignore
+/// make_param_ops!(
+///     /// Documentation for new param ops.
+///     PARAM_OPS_MYTYPE, // Name for the static.
+///     MyType // A type which implements [`ModuleParam`].
+/// );
+/// ```
+macro_rules! make_param_ops {
+    ($ops:ident, $ty:ty) => {
+        ///
+        /// Static [`kernel_param_ops`](srctree/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: None,
+            free: None,
+        };
+    };
+}
+
+make_param_ops!(PARAM_OPS_I8, i8);
+make_param_ops!(PARAM_OPS_U8, u8);
+make_param_ops!(PARAM_OPS_I16, i16);
+make_param_ops!(PARAM_OPS_U16, u16);
+make_param_ops!(PARAM_OPS_I32, i32);
+make_param_ops!(PARAM_OPS_U32, u32);
+make_param_ops!(PARAM_OPS_I64, i64);
+make_param_ops!(PARAM_OPS_U64, u64);
+make_param_ops!(PARAM_OPS_ISIZE, isize);
+make_param_ops!(PARAM_OPS_USIZE, usize);
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index a3ee27e29a6f..16d300ad3d3b 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
     }
 }
 
+pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
+    let peek = it.clone().next();
+    match peek {
+        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
+            let _ = it.next();
+            Some(punct.as_char())
+        }
+        _ => None,
+    }
+}
+
 pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
     if let Some(TokenTree::Literal(literal)) = it.next() {
         Some(literal.to_string())
@@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
     }
     None
 }
+
+/// Parse a token stream of the form `expected_name: "value",` and return the
+/// string in the position of "value".
+///
+/// # Panics
+///
+/// - On parse error.
+pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
+    assert_eq!(expect_ident(it), expected_name);
+    assert_eq!(expect_punct(it), ':');
+    let string = expect_string(it);
+    assert_eq!(expect_punct(it), ',');
+    string
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 9acaa68c974e..7a3a54fcf88e 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -23,6 +23,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
@@ -39,6 +63,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);
@@ -47,6 +77,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 a9418fbc9b44..342c9e91fb62 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,116 @@ 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) {{
+                                ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
+                            }} else {{
+                                ::kernel::c_str!(\"{module_name}.{param_name}\").as_bytes_with_nul()
+                            }}.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 sign = try_sign(param_it);
+    let default = try_literal(param_it).expect("Expected default param value");
+    assert_eq!(expect_punct(param_it), ',');
+    let mut value = sign.map(String::from).unwrap_or_default();
+    value.push_str(&default);
+    value
+}
+
 #[derive(Debug, Default)]
 struct ModuleInfo {
     type_: String,
@@ -99,6 +203,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 {
@@ -114,6 +262,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();
@@ -143,6 +292,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
@@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     let info = ModuleInfo::parse(&mut it);
 
     let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
-    if let Some(author) = info.author {
-        modinfo.emit("author", &author);
+    if let Some(author) = &info.author {
+        modinfo.emit("author", author);
     }
-    if let Some(authors) = info.authors {
+    if let Some(authors) = &info.authors {
         for author in authors {
-            modinfo.emit("author", &author);
+            modinfo.emit("author", author);
         }
     }
-    if let Some(description) = info.description {
-        modinfo.emit("description", &description);
+    if let Some(description) = &info.description {
+        modinfo.emit("description", description);
     }
     modinfo.emit("license", &info.license);
-    if let Some(aliases) = info.alias {
+    if let Some(aliases) = &info.alias {
         for alias in aliases {
-            modinfo.emit("alias", &alias);
+            modinfo.emit("alias", alias);
         }
     }
-    if let Some(firmware) = info.firmware {
+    if let Some(firmware) = &info.firmware {
         for fw in firmware {
-            modinfo.emit("firmware", &fw);
+            modinfo.emit("firmware", fw);
         }
     }
 
     // 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!(
         "
@@ -374,14 +526,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 1fc7a1be6b6d..c04cc07b3249 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -10,6 +10,12 @@
     authors: ["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!(
+            "test_parameter: {}\n",
+            *module_parameters::test_parameter.get()
+        );
 
         let mut numbers = KVec::new();
         numbers.push(72, GFP_KERNEL)?;

-- 
2.47.2

[PATCH v12 3/3] modules: add rust modules files to MAINTAINERS

[Andreas Hindborg]

The module subsystem people agreed to maintain rust support for modules
[1]. Thus, add entries for relevant files to modules entry in MAINTAINERS.

Link: https://lore.kernel.org/all/0d9e596a-5316-4e00-862b-fd77552ae4b5@suse.com/ [1]

Acked-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3cbf9ac0d83f..d283874843a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16395,6 +16395,8 @@ F:	include/linux/module*.h
 F:	kernel/module/
 F:	lib/test_kmod.c
 F:	lib/tests/module/
+F:	rust/kernel/module_param.rs
+F:	rust/macros/module.rs
 F:	scripts/module*
 F:	tools/testing/selftests/kmod/
 F:	tools/testing/selftests/module/

-- 
2.47.2

Re: [PATCH v12 1/3] rust: str: add radix prefixed integer parsing functions

[Benno Lossin]

On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 878111cb77bc..174e70397305 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -573,7 +573,6 @@ macro_rules! c_str {
>  }
>  
>  #[cfg(test)]
> -#[expect(clippy::items_after_test_module)]
>  mod tests {
>      use super::*;
>  
> @@ -946,3 +945,174 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>  macro_rules! fmt {
>      ($($f:tt)*) => ( core::format_args!($($f)*) )
>  }
> +
> +/// Integer parsing functions for parsing signed and unsigned integers
> +/// potentially prefixed with `0x`, `0o`, or `0b`.
> +pub mod parse_int {

Why not make this its own file? It's 172 lines long already.

> +    pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
> +        /// Parse a string according to the description in [`Self`].
> +        fn from_str(src: &BStr) -> Result<Self> {
> +            match src.deref() {
> +                [b'-', rest @ ..] => {
> +                    let (radix, digits) = strip_radix(rest.as_ref());
> +                    // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
> +                    // So if we want to parse negative numbers as positive and
> +                    // later multiply by -1, we have to parse into a larger
> +                    // integer. We choose `u64` as sufficiently large.
> +                    //
> +                    // NOTE: 128 bit integers are not available on all
> +                    // platforms, hence the choice of 64 bits.
> +                    let val = u64::from_str_radix(
> +                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
> +                        radix,
> +                    )
> +                    .map_err(|_| EINVAL)?;
> +
> +                    if val > Self::abs_min() {
> +                        return Err(EINVAL);
> +                    }
> +
> +                    if val == Self::abs_min() {
> +                        return Ok(Self::MIN);
> +                    }
> +
> +                    // SAFETY: We checked that `val` will fit in `Self` above.
> +                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
> +
> +                    Ok(val.complement())

You're allowing to parse `u32` with a leading `-`? I'd expect an error
in that case. Maybe `complement` should be named `negate` and return a
`Result`?

---
Cheers,
Benno

> +                }
> +                _ => {
> +                    let (radix, digits) = strip_radix(src);
> +                    Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
> +                }
> +            }
> +        }
> +    }

Re: [PATCH v12 1/3] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

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

> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index 878111cb77bc..174e70397305 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -573,7 +573,6 @@ macro_rules! c_str {
>>  }
>>
>>  #[cfg(test)]
>> -#[expect(clippy::items_after_test_module)]
>>  mod tests {
>>      use super::*;
>>
>> @@ -946,3 +945,174 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>  macro_rules! fmt {
>>      ($($f:tt)*) => ( core::format_args!($($f)*) )
>>  }
>> +
>> +/// Integer parsing functions for parsing signed and unsigned integers
>> +/// potentially prefixed with `0x`, `0o`, or `0b`.
>> +pub mod parse_int {
>
> Why not make this its own file? It's 172 lines long already.

Sure. I'm really hoping to land this series for this cycle though, so if
it's OK I would move the code next cycle.

>
>> +    pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
>> +        /// Parse a string according to the description in [`Self`].
>> +        fn from_str(src: &BStr) -> Result<Self> {
>> +            match src.deref() {
>> +                [b'-', rest @ ..] => {
>> +                    let (radix, digits) = strip_radix(rest.as_ref());
>> +                    // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>> +                    // So if we want to parse negative numbers as positive and
>> +                    // later multiply by -1, we have to parse into a larger
>> +                    // integer. We choose `u64` as sufficiently large.
>> +                    //
>> +                    // NOTE: 128 bit integers are not available on all
>> +                    // platforms, hence the choice of 64 bits.
>> +                    let val = u64::from_str_radix(
>> +                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
>> +                        radix,
>> +                    )
>> +                    .map_err(|_| EINVAL)?;
>> +
>> +                    if val > Self::abs_min() {
>> +                        return Err(EINVAL);
>> +                    }
>> +
>> +                    if val == Self::abs_min() {
>> +                        return Ok(Self::MIN);
>> +                    }
>> +
>> +                    // SAFETY: We checked that `val` will fit in `Self` above.
>> +                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
>> +
>> +                    Ok(val.complement())
>
> You're allowing to parse `u32` with a leading `-`? I'd expect an error
> in that case. Maybe `complement` should be named `negate` and return a
> `Result`?

You would get `Err(EINVAL)` in that case, hitting this:

  if val > Self::abs_min() {
      return Err(EINVAL);
  }


Best regards,
Andreas Hindborg

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Benno Lossin]

On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
> Add support for module parameters to the `module!` macro. Implement read
> only support for integer types without `sysfs` support.
>
> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/lib.rs           |   1 +
>  rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
>  rust/macros/helpers.rs       |  25 ++++++
>  rust/macros/lib.rs           |  31 +++++++
>  rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
>  samples/rust/rust_minimal.rs |  10 +++

I know this is already the 12th version, but I think this patch should
be split into the module_param module introduction, proc-macro
modifications and the sample changes.

[...]

> +/// Set the module parameter from a string.
> +///
> +/// Used to set the parameter value at kernel initialization, when loading
> +/// the module or when set through `sysfs`.
> +///
> +/// See `struct kernel_param_ops.set`.
> +///
> +/// # Safety
> +///
> +/// - If `val` is non-null then it must point to a valid null-terminated string.
> +///   The `arg` field of `param` must be an instance of `T`.

This sentence is conveying the same (or at least similar) requirement as
the bullet point below.

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

Why is this an Invariants section?

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

We shouldn't mention "C API contract" here and move the liveness
requirement to the safety requirements of the function. Or change the
safety requirements for this function to only be called from some
specific C API. 

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

Ditto.

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

Why do we have exclusive access? Should be in the safety requirements.

> +        unsafe { *old_value = new_value };
> +        Ok(0)
> +    })
> +}

[...]

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

Spurious newline?

> +        /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
> +        /// struct generated by `make_param_ops`

Is it useful for this fact to be in the docs? I'd remove it.

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

Should we also allow a leading `+`?

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

This won't allow omitting the trailing comma.

> +    string
> +}

[...]

> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>      let info = ModuleInfo::parse(&mut it);
>  
>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
> -    if let Some(author) = info.author {
> -        modinfo.emit("author", &author);
> +    if let Some(author) = &info.author {
> +        modinfo.emit("author", author);
>      }
> -    if let Some(authors) = info.authors {
> +    if let Some(authors) = &info.authors {
>          for author in authors {
> -            modinfo.emit("author", &author);
> +            modinfo.emit("author", author);
>          }
>      }
> -    if let Some(description) = info.description {
> -        modinfo.emit("description", &description);
> +    if let Some(description) = &info.description {
> +        modinfo.emit("description", description);
>      }
>      modinfo.emit("license", &info.license);
> -    if let Some(aliases) = info.alias {
> +    if let Some(aliases) = &info.alias {
>          for alias in aliases {
> -            modinfo.emit("alias", &alias);
> +            modinfo.emit("alias", alias);
>          }
>      }
> -    if let Some(firmware) = info.firmware {
> +    if let Some(firmware) = &info.firmware {
>          for fw in firmware {
> -            modinfo.emit("firmware", &fw);
> +            modinfo.emit("firmware", fw);

I don't like that you have to change all of these. Can't you just take a
`&[Parameter]` argument in `emit_params` instead of the whole
`ModuleInfo` struct?

---
Cheers,
Benno

>          }
>      }

Re: [PATCH v12 1/3] rust: str: add radix prefixed integer parsing functions

[Benno Lossin]

On Wed May 7, 2025 at 11:15 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>> +    pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
>>> +        /// Parse a string according to the description in [`Self`].
>>> +        fn from_str(src: &BStr) -> Result<Self> {
>>> +            match src.deref() {
>>> +                [b'-', rest @ ..] => {
>>> +                    let (radix, digits) = strip_radix(rest.as_ref());
>>> +                    // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>>> +                    // So if we want to parse negative numbers as positive and
>>> +                    // later multiply by -1, we have to parse into a larger
>>> +                    // integer. We choose `u64` as sufficiently large.
>>> +                    //
>>> +                    // NOTE: 128 bit integers are not available on all
>>> +                    // platforms, hence the choice of 64 bits.
>>> +                    let val = u64::from_str_radix(
>>> +                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
>>> +                        radix,
>>> +                    )
>>> +                    .map_err(|_| EINVAL)?;
>>> +
>>> +                    if val > Self::abs_min() {
>>> +                        return Err(EINVAL);
>>> +                    }
>>> +
>>> +                    if val == Self::abs_min() {
>>> +                        return Ok(Self::MIN);
>>> +                    }
>>> +
>>> +                    // SAFETY: We checked that `val` will fit in `Self` above.
>>> +                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
>>> +
>>> +                    Ok(val.complement())
>>
>> You're allowing to parse `u32` with a leading `-`? I'd expect an error
>> in that case. Maybe `complement` should be named `negate` and return a
>> `Result`?
>
> You would get `Err(EINVAL)` in that case, hitting this:
>
>   if val > Self::abs_min() {
>       return Err(EINVAL);
>   }

Ah, I think I asked this in a previous version already... Thanks.

---
Cheers,
Benno

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

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

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

OK.

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

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

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

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

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

OK.

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

OK.

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

Will add this.

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

Will remove.

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

OK.

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

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

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

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

>
>> +    string
>> +}
>
> [...]
>
>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>      let info = ModuleInfo::parse(&mut it);
>>  
>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>> -    if let Some(author) = info.author {
>> -        modinfo.emit("author", &author);
>> +    if let Some(author) = &info.author {
>> +        modinfo.emit("author", author);
>>      }
>> -    if let Some(authors) = info.authors {
>> +    if let Some(authors) = &info.authors {
>>          for author in authors {
>> -            modinfo.emit("author", &author);
>> +            modinfo.emit("author", author);
>>          }
>>      }
>> -    if let Some(description) = info.description {
>> -        modinfo.emit("description", &description);
>> +    if let Some(description) = &info.description {
>> +        modinfo.emit("description", description);
>>      }
>>      modinfo.emit("license", &info.license);
>> -    if let Some(aliases) = info.alias {
>> +    if let Some(aliases) = &info.alias {
>>          for alias in aliases {
>> -            modinfo.emit("alias", &alias);
>> +            modinfo.emit("alias", alias);
>>          }
>>      }
>> -    if let Some(firmware) = info.firmware {
>> +    if let Some(firmware) = &info.firmware {
>>          for fw in firmware {
>> -            modinfo.emit("firmware", &fw);
>> +            modinfo.emit("firmware", fw);
>
> I don't like that you have to change all of these.

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

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

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


Best regards,
Andreas Hindborg

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

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

> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>> Add support for module parameters to the `module!` macro. Implement read
>>> only support for integer types without `sysfs` support.
>>>
>>> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
>>> Tested-by: Daniel Gomez <da.gomez@samsung.com>
>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>>  rust/kernel/lib.rs           |   1 +
>>>  rust/kernel/module_param.rs  | 204 +++++++++++++++++++++++++++++++++++++++++++
>>>  rust/macros/helpers.rs       |  25 ++++++
>>>  rust/macros/lib.rs           |  31 +++++++
>>>  rust/macros/module.rs        | 195 ++++++++++++++++++++++++++++++++++++-----
>>>  samples/rust/rust_minimal.rs |  10 +++
>>
>> I know this is already the 12th version, but I think this patch should
>> be split into the module_param module introduction, proc-macro
>> modifications and the sample changes.
>>
>
> OK.
>
>> [...]
>>
>>> +/// Set the module parameter from a string.
>>> +///
>>> +/// Used to set the parameter value at kernel initialization, when loading
>>> +/// the module or when set through `sysfs`.
>>> +///
>>> +/// See `struct kernel_param_ops.set`.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// - If `val` is non-null then it must point to a valid null-terminated string.
>>> +///   The `arg` field of `param` must be an instance of `T`.
>>
>> This sentence is conveying the same (or at least similar) requirement as
>> the bullet point below.
>
> Yes, you are right. At any rate the wording is wrong, a pointer cannot
> "be an instance", it can point to one.
>
>>
>>> +/// - `param.arg` must be a pointer to valid `*mut T` as set up by the
>>> +///   [`module!`] macro.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// Currently, we only support read-only parameters that are not readable
>>> +/// from `sysfs`. Thus, this function is only called at kernel
>>> +/// initialization time, or at module load time, and we have exclusive
>>> +/// access to the parameter for the duration of the function.
>>
>> Why is this an Invariants section?
>
> Looks like a mistake, I'll change it to "# Note".
>
>>
>>> +///
>>> +/// [`module!`]: macros::module
>>> +unsafe extern "C" fn set_param<T>(
>>> +    val: *const kernel::ffi::c_char,
>>> +    param: *const crate::bindings::kernel_param,
>>> +) -> core::ffi::c_int
>>> +where
>>> +    T: ModuleParam,
>>> +{
>>> +    // NOTE: If we start supporting arguments without values, val _is_ allowed
>>> +    // to be null here.
>>> +    if val.is_null() {
>>> +        // TODO: Use pr_warn_once available.
>>> +        crate::pr_warn!("Null pointer passed to `module_param::set_param`");
>>> +        return EINVAL.to_errno();
>>> +    }
>>> +
>>> +    // SAFETY: By function safety requirement, val is non-null and
>>> +    // null-terminated. By C API contract, `val` is live and valid for reads
>>> +    // for the duration of this function.
>>
>> We shouldn't mention "C API contract" here and move the liveness
>> requirement to the safety requirements of the function. Or change the
>> safety requirements for this function to only be called from some
>> specific C API.
>
> OK.
>
>>
>>> +    let arg = unsafe { CStr::from_char_ptr(val) };
>>> +
>>> +    crate::error::from_result(|| {
>>> +        let new_value = T::try_from_param_arg(arg)?;
>>> +
>>> +        // SAFETY: `param` is guaranteed to be valid by C API contract
>>> +        // and `arg` is guaranteed to point to an instance of `T`.
>>
>> Ditto.
>
> OK.
>
>>
>>> +        let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>> +
>>> +        // SAFETY: `old_value` is valid for writes, as we have exclusive
>>> +        // access. `old_value` is pointing to an initialized static, and
>>> +        // so it is properly initialized.
>>
>> Why do we have exclusive access? Should be in the safety requirements.
>
> Will add this.
>
>>
>>> +        unsafe { *old_value = new_value };
>>> +        Ok(0)
>>> +    })
>>> +}
>>
>> [...]
>>
>>> +#[doc(hidden)]
>>> +#[macro_export]
>>> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```ignore
>>> +/// make_param_ops!(
>>> +///     /// Documentation for new param ops.
>>> +///     PARAM_OPS_MYTYPE, // Name for the static.
>>> +///     MyType // A type which implements [`ModuleParam`].
>>> +/// );
>>> +/// ```
>>> +macro_rules! make_param_ops {
>>> +    ($ops:ident, $ty:ty) => {
>>> +        ///
>>
>> Spurious newline?
>
> Will remove.
>
>>
>>> +        /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
>>> +        /// struct generated by `make_param_ops`
>>
>> Is it useful for this fact to be in the docs? I'd remove it.
>
> OK.
>

Clippy thinks it is useful:

    error: missing documentation for a static
      --> /home/aeh/src/linux-rust/module-params/rust/kernel/module_param.rs:182:9
        |
    182 |         pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
        |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ...
    191 | make_param_ops!(PARAM_OPS_I8, i8);
        | --------------------------------- in this macro invocation
        |

So either we need to `#[allow(missing_docs)]` or just add the line back. What do you prefer?


Best regards,
Andreas Hindborg

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Miguel Ojeda]

On Wed, Jun 11, 2025 at 2:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> So either we need to `#[allow(missing_docs)]` or just add the line back. What do you prefer?

Do you mean if you remove the `concat!` too, right?

It would need to be `expect`, but I think even an
`expect(missing_docs)` is not really useful vs. having some docs.

But another question is: if the docs are not useful, should an item be
hidden to begin with?

(By the way, that one is `rustc`, not Clippy)

Cheers,
Miguel

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Miguel Ojeda]

On Wed, May 7, 2025 at 1:23 PM Benno Lossin <lossin@kernel.org> wrote:
>
> > +unsafe extern "C" fn set_param<T>(
> > +    val: *const kernel::ffi::c_char,
> > +    param: *const crate::bindings::kernel_param,
> > +) -> core::ffi::c_int

New instance of `core::ffi::` (and both these `c_*` should be unqualified).

Cheers,
Miguel

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

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

> On Wed, Jun 11, 2025 at 2:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> So either we need to `#[allow(missing_docs)]` or just add the line back. What do you prefer?
>
> Do you mean if you remove the `concat!` too, right?

Yes, all of it.

>
> It would need to be `expect`, but I think even an
> `expect(missing_docs)` is not really useful vs. having some docs.
>
> But another question is: if the docs are not useful, should an item be
> hidden to begin with?

That is probably the best solution, yes.

> (By the way, that one is `rustc`, not Clippy)

Right.


Best regards,
Andreas Hindborg

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Benno Lossin]

On Wed Jun 11, 2025 at 12:31 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
>>> index a3ee27e29a6f..16d300ad3d3b 100644
>>> --- a/rust/macros/helpers.rs
>>> +++ b/rust/macros/helpers.rs
>>> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>>>      }
>>>  }
>>>  
>>> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
>>> +    let peek = it.clone().next();
>>> +    match peek {
>>> +        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
>>
>> Should we also allow a leading `+`?
>
> I would argue no, because rust literals cannot start with `+`.

Makes sense.

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

Then we should change that:

    https://github.com/Rust-for-Linux/linux/issues/1172

>>> +    string
>>> +}
>>
>> [...]
>>
>>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>>      let info = ModuleInfo::parse(&mut it);
>>>  
>>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>>> -    if let Some(author) = info.author {
>>> -        modinfo.emit("author", &author);
>>> +    if let Some(author) = &info.author {
>>> +        modinfo.emit("author", author);
>>>      }
>>> -    if let Some(authors) = info.authors {
>>> +    if let Some(authors) = &info.authors {
>>>          for author in authors {
>>> -            modinfo.emit("author", &author);
>>> +            modinfo.emit("author", author);
>>>          }
>>>      }
>>> -    if let Some(description) = info.description {
>>> -        modinfo.emit("description", &description);
>>> +    if let Some(description) = &info.description {
>>> +        modinfo.emit("description", description);
>>>      }
>>>      modinfo.emit("license", &info.license);
>>> -    if let Some(aliases) = info.alias {
>>> +    if let Some(aliases) = &info.alias {
>>>          for alias in aliases {
>>> -            modinfo.emit("alias", &alias);
>>> +            modinfo.emit("alias", alias);
>>>          }
>>>      }
>>> -    if let Some(firmware) = info.firmware {
>>> +    if let Some(firmware) = &info.firmware {
>>>          for fw in firmware {
>>> -            modinfo.emit("firmware", &fw);
>>> +            modinfo.emit("firmware", fw);
>>
>> I don't like that you have to change all of these.
>
> Why not? If I was to write this code in the first place, I would have
> used a reference rather than pass by value.

That's fine, but do it in a separate commit then.

>> Can't you just take a
>> `&[Parameter]` argument in `emit_params` instead of the whole
>> `ModuleInfo` struct?
>
> I don't think that is a nice solution. I would have to pass the name
> field as well, increasing the number of parameters to the function.

Ah right makes sense.

---
Cheers,
Benno

Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg]

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

> On Wed Jun 11, 2025 at 12:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
>>>> index a3ee27e29a6f..16d300ad3d3b 100644
>>>> --- a/rust/macros/helpers.rs
>>>> +++ b/rust/macros/helpers.rs
>>>> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      }
>>>>  }
>>>>
>>>> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
>>>> +    let peek = it.clone().next();
>>>> +    match peek {
>>>> +        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
>>>
>>> Should we also allow a leading `+`?
>>
>> I would argue no, because rust literals cannot start with `+`.
>
> Makes sense.
>
>>>> +            let _ = it.next();
>>>> +            Some(punct.as_char())
>>>> +        }
>>>> +        _ => None,
>>>> +    }
>>>> +}
>>>> +
>>>>  pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      if let Some(TokenTree::Literal(literal)) = it.next() {
>>>>          Some(literal.to_string())
>>>> @@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
>>>>      }
>>>>      None
>>>>  }
>>>> +
>>>> +/// Parse a token stream of the form `expected_name: "value",` and return the
>>>> +/// string in the position of "value".
>>>> +///
>>>> +/// # Panics
>>>> +///
>>>> +/// - On parse error.
>>>> +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
>>>> +    assert_eq!(expect_ident(it), expected_name);
>>>> +    assert_eq!(expect_punct(it), ':');
>>>> +    let string = expect_string(it);
>>>> +    assert_eq!(expect_punct(it), ',');
>>>
>>> This won't allow omitting the trailing comma.
>>
>> This is in line with the rest of the module macro.
>
> Then we should change that:
>
>     https://github.com/Rust-for-Linux/linux/issues/1172
>
>>>> +    string
>>>> +}
>>>
>>> [...]
>>>
>>>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>>>      let info = ModuleInfo::parse(&mut it);
>>>>
>>>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>>>> -    if let Some(author) = info.author {
>>>> -        modinfo.emit("author", &author);
>>>> +    if let Some(author) = &info.author {
>>>> +        modinfo.emit("author", author);
>>>>      }
>>>> -    if let Some(authors) = info.authors {
>>>> +    if let Some(authors) = &info.authors {
>>>>          for author in authors {
>>>> -            modinfo.emit("author", &author);
>>>> +            modinfo.emit("author", author);
>>>>          }
>>>>      }
>>>> -    if let Some(description) = info.description {
>>>> -        modinfo.emit("description", &description);
>>>> +    if let Some(description) = &info.description {
>>>> +        modinfo.emit("description", description);
>>>>      }
>>>>      modinfo.emit("license", &info.license);
>>>> -    if let Some(aliases) = info.alias {
>>>> +    if let Some(aliases) = &info.alias {
>>>>          for alias in aliases {
>>>> -            modinfo.emit("alias", &alias);
>>>> +            modinfo.emit("alias", alias);
>>>>          }
>>>>      }
>>>> -    if let Some(firmware) = info.firmware {
>>>> +    if let Some(firmware) = &info.firmware {
>>>>          for fw in firmware {
>>>> -            modinfo.emit("firmware", &fw);
>>>> +            modinfo.emit("firmware", fw);
>>>
>>> I don't like that you have to change all of these.
>>
>> Why not? If I was to write this code in the first place, I would have
>> used a reference rather than pass by value.
>
> That's fine, but do it in a separate commit then.

OK, I can do that 👍


Best regards,
Andreas Hindborg