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

[PATCH v7 0/6] 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].

After a bit of discussion on v3 about whether or not module parameters
is a good idea, it seems that module parameters in Rust has a place
in the kernel for now. This series is a dependency for `rnull`, the Rust
null block driver [2].

---
Module subsystem people: how do you want to handle merging of the patches and
subsequent maintenance of code?

I think we discussed you guys taking this under the current module maintainer
entry? If that is correct, will you add the new files to your entry yourself, or
should I include an update to MAINTAINERS in this series?

If prefer another solution, let me know and we can figure that out.

To: Miguel Ojeda <ojeda@kernel.org>
To: Alex Gaynor <alex.gaynor@gmail.com>
To: Boqun Feng <boqun.feng@gmail.com>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <benno.lossin@proton.me>
To: Alice Ryhl <aliceryhl@google.com>
To: Masahiro Yamada <masahiroy@kernel.org>
To: Nathan Chancellor <nathan@kernel.org>
To: Nicolas Schier <nicolas@fjasle.eu>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Adam Bratschi-Kaye <ark.email@gmail.com>
Cc: rust-for-linux@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: Petr Pavlu <petr.pavlu@suse.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Daniel Gomez <da.gomez@samsung.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: linux-modules@vger.kernel.org
Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/log/?h=rnull [2]
Signed-off-by: Andreas Hindborg <a.hindborg@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 (6):
      rust: str: implement `PartialEq` for `BStr`
      rust: str: implement `Index` for `BStr`
      rust: str: implement `AsRef<BStr>` for `[u8]` and `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           |   1 +
 rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs           | 163 +++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 +++++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 7 files changed, 629 insertions(+), 18 deletions(-)
---
base-commit: 379487e17ca406b47392e7ab6cf35d1c3bacb371
change-id: 20241211-module-params-v3-ae7e5c8d8b5a

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

[PATCH v7 1/6] rust: str: implement `PartialEq` for `BStr`

[Andreas Hindborg]

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

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
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 28e2201604d67..002dcddf7c768 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -108,6 +108,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

Re: [PATCH v7 1/6] rust: str: implement `PartialEq` for `BStr`

[Daniel Almeida]

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Implement `PartialEq` for `BStr` by comparing underlying byte slices.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> 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 28e2201604d67..002dcddf7c768 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -108,6 +108,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
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v7 2/6] rust: str: implement `Index` for `BStr`

[Andreas Hindborg]

The `Index` implementation on `BStr` was lost when we switched `BStr` from
a type alias of `[u8]` to a newtype. This patch adds back `Index` by
implementing `Index` for `BStr` when `Index` would be implemented for
`[u8]`.

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

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 002dcddf7c768..ba6b1a5c4f99d 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -114,6 +114,17 @@ fn eq(&self, other: &Self) -> bool {
     }
 }
 
+impl<Idx> Index<Idx> for BStr
+where
+    [u8]: Index<Idx, Output = [u8]>,
+{
+    type Output = Self;
+
+    fn index(&self, index: Idx) -> &Self::Output {
+        BStr::from_bytes(&self.0[index])
+    }
+}
+
 /// Creates a new [`BStr`] from a string literal.
 ///
 /// `b_str!` converts the supplied string literal to byte string, so non-ASCII

-- 
2.47.0

Re: [PATCH v7 2/6] rust: str: implement `Index` for `BStr`

[Daniel Almeida]

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> The `Index` implementation on `BStr` was lost when we switched `BStr` from
> a type alias of `[u8]` to a newtype. This patch adds back `Index` by
> implementing `Index` for `BStr` when `Index` would be implemented for
> `[u8]`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 002dcddf7c768..ba6b1a5c4f99d 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -114,6 +114,17 @@ fn eq(&self, other: &Self) -> bool {
>     }
> }
> 
> +impl<Idx> Index<Idx> for BStr
> +where
> +    [u8]: Index<Idx, Output = [u8]>,
> +{
> +    type Output = Self;
> +
> +    fn index(&self, index: Idx) -> &Self::Output {
> +        BStr::from_bytes(&self.0[index])
> +    }
> +}
> +
> /// Creates a new [`BStr`] from a string literal.
> ///
> /// `b_str!` converts the supplied string literal to byte string, so non-ASCII
> 
> -- 
> 2.47.0
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v7 2/6] rust: str: implement `Index` for `BStr`

[Fiona Behrens]

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

> The `Index` implementation on `BStr` was lost when we switched `BStr` from
> a type alias of `[u8]` to a newtype. This patch adds back `Index` by
> implementing `Index` for `BStr` when `Index` would be implemented for
> `[u8]`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Fiona Behrens <me@kloenk.dev>

> ---
>  rust/kernel/str.rs | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 002dcddf7c768..ba6b1a5c4f99d 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -114,6 +114,17 @@ fn eq(&self, other: &Self) -> bool {
>      }
>  }
>  
> +impl<Idx> Index<Idx> for BStr
> +where
> +    [u8]: Index<Idx, Output = [u8]>,
> +{
> +    type Output = Self;
> +
> +    fn index(&self, index: Idx) -> &Self::Output {
> +        BStr::from_bytes(&self.0[index])
> +    }
> +}
> +
>  /// Creates a new [`BStr`] from a string literal.
>  ///
>  /// `b_str!` converts the supplied string literal to byte string, so non-ASCII

[PATCH v7 3/6] rust: str: implement `AsRef<BStr>` for `[u8]` and `BStr`

[Andreas Hindborg]

Implement `AsRef<BStr>` for `[u8]` and `BStr` so these can be used
interchangeably for operations on `BStr`.

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

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index ba6b1a5c4f99d..c6bd2c69543dc 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -125,6 +125,18 @@ fn index(&self, index: Idx) -> &Self::Output {
     }
 }
 
+impl AsRef<BStr> for [u8] {
+    fn as_ref(&self) -> &BStr {
+        BStr::from_bytes(self)
+    }
+}
+
+impl AsRef<BStr> for BStr {
+    fn as_ref(&self) -> &BStr {
+        self
+    }
+}
+
 /// Creates a new [`BStr`] from a string literal.
 ///
 /// `b_str!` converts the supplied string literal to byte string, so non-ASCII

-- 
2.47.0

Re: [PATCH v7 3/6] rust: str: implement `AsRef<BStr>` for `[u8]` and `BStr`

[Daniel Almeida]

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Implement `AsRef<BStr>` for `[u8]` and `BStr` so these can be used
> interchangeably for operations on `BStr`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index ba6b1a5c4f99d..c6bd2c69543dc 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -125,6 +125,18 @@ fn index(&self, index: Idx) -> &Self::Output {
>     }
> }
> 
> +impl AsRef<BStr> for [u8] {
> +    fn as_ref(&self) -> &BStr {
> +        BStr::from_bytes(self)
> +    }
> +}
> +
> +impl AsRef<BStr> for BStr {
> +    fn as_ref(&self) -> &BStr {
> +        self
> +    }
> +}

Why do you need this last one?

> +
> /// Creates a new [`BStr`] from a string literal.
> ///
> /// `b_str!` converts the supplied string literal to byte string, so non-ASCII
> 
> -- 
> 2.47.0
> 
> 
> 

Re: [PATCH v7 3/6] rust: str: implement `AsRef<BStr>` for `[u8]` and `BStr`

[Daniel Almeida]

> On 21 Feb 2025, at 13:01, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> 
> 
>> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> 
>> Implement `AsRef<BStr>` for `[u8]` and `BStr` so these can be used
>> interchangeably for operations on `BStr`.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/str.rs | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index ba6b1a5c4f99d..c6bd2c69543dc 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -125,6 +125,18 @@ fn index(&self, index: Idx) -> &Self::Output {
>>    }
>> }
>> 
>> +impl AsRef<BStr> for [u8] {
>> +    fn as_ref(&self) -> &BStr {
>> +        BStr::from_bytes(self)
>> +    }
>> +}
>> +
>> +impl AsRef<BStr> for BStr {
>> +    fn as_ref(&self) -> &BStr {
>> +        self
>> +    }
>> +}
> 
> Why do you need this last one?

I see that this is used by the following patch.

> 
>> +
>> /// Creates a new [`BStr`] from a string literal.
>> ///
>> /// `b_str!` converts the supplied string literal to byte string, so non-ASCII
>> 
>> -- 
>> 2.47.0

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v7 3/6] rust: str: implement `AsRef<BStr>` for `[u8]` and `BStr`

[Gary Guo]

On Tue, 18 Feb 2025 14:00:45 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> Implement `AsRef<BStr>` for `[u8]` and `BStr` so these can be used
> interchangeably for operations on `BStr`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/str.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index ba6b1a5c4f99d..c6bd2c69543dc 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -125,6 +125,18 @@ fn index(&self, index: Idx) -> &Self::Output {
>      }
>  }
>  
> +impl AsRef<BStr> for [u8] {
> +    fn as_ref(&self) -> &BStr {
> +        BStr::from_bytes(self)
> +    }
> +}
> +
> +impl AsRef<BStr> for BStr {
> +    fn as_ref(&self) -> &BStr {
> +        self
> +    }
> +}
> +
>  /// Creates a new [`BStr`] from a string literal.
>  ///
>  /// `b_str!` converts the supplied string literal to byte string, so non-ASCII
> 

[PATCH v7 4/6] rust: str: implement `strip_prefix` for `BStr`

[Andreas Hindborg]

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

Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
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 c6bd2c69543dc..db272d2198fcc 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: impl AsRef<Self>) -> Option<&BStr> {
+        self.deref()
+            .strip_prefix(pattern.as_ref().deref())
+            .map(Self::from_bytes)
+    }
 }
 
 impl fmt::Display for BStr {

-- 
2.47.0

Re: [PATCH v7 4/6] rust: str: implement `strip_prefix` for `BStr`

[Daniel Almeida]

Hi Andreas,

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Implement `strip_prefix` for `BStr` by deferring to `slice::strip_prefix`
> on the underlying `&[u8]`.
> 
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 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 c6bd2c69543dc..db272d2198fcc 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")));
> +    /// ```

This is passing.

> +    pub fn strip_prefix(&self, pattern: impl AsRef<Self>) -> Option<&BStr> {
> +        self.deref()
> +            .strip_prefix(pattern.as_ref().deref())
> +            .map(Self::from_bytes)
> +    }
> }
> 
> impl fmt::Display for BStr {
> 
> -- 
> 2.47.0
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v7 5/6] 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index db272d2198fcc..8b0d814b47f52 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -945,3 +945,121 @@ 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::prelude::*;
+    use crate::str::BStr;
+    use core::ops::Deref;
+
+    /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
+    ///
+    /// [`&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 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) {
+        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://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(0), 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(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 + TryFrom<i128> {
+        /// 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 i128 as sufficiently large.
+                    let val = i128::from_str_radix(
+                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
+                        radix,
+                    )
+                    .map_err(|_| EINVAL)?;
+                    let val = -val;
+                    val.try_into().map_err(|_| EINVAL)
+                }
+                _ => {
+                    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

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Daniel Almeida]

Hi Andreas,

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 118 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index db272d2198fcc..8b0d814b47f52 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -945,3 +945,121 @@ 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::prelude::*;
> +    use crate::str::BStr;
> +    use core::ops::Deref;
> +
> +    /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
> +    ///
> +    /// [`&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 trait FromStrRadix: Sized {

Is this supposed to be implemented by somebody else? Otherwise we should seal it,
perhaps?


> +        /// 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) {
> +        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://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(0), 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(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());
> +    /// ```

^ These are passing

> +    pub trait ParseInt: FromStrRadix + TryFrom<i128> {

Should this be sealed too?

> +        /// 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 i128 as sufficiently large.
> +                    let val = i128::from_str_radix(
> +                        core::str::from_utf8(digits).map_err(|_| EINVAL)?,
> +                        radix,
> +                    )
> +                    .map_err(|_| EINVAL)?;
> +                    let val = -val;
> +                    val.try_into().map_err(|_| EINVAL)
> +                }
> +                _ => {
> +                    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
> 
> 
> 

This overall LGTM. See my comment on whether we should seal the traits.

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 118 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index db272d2198fcc..8b0d814b47f52 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -945,3 +945,121 @@ 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::prelude::*;
>> +    use crate::str::BStr;
>> +    use core::ops::Deref;
>> +
>> +    /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
>> +    ///
>> +    /// [`&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 trait FromStrRadix: Sized {
>
> Is this supposed to be implemented by somebody else? Otherwise we should seal it,
> perhaps?

That is a good point. I did not intend for the user to implement this,
same for `ParseInt`. I will look into sealing them.


Best regards,
Andreas Hindborg

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Janne Grunau]

On Tue, Feb 18, 2025 at 02:00:47PM +0100, Andreas Hindborg wrote:
> 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index db272d2198fcc..8b0d814b47f52 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -945,3 +945,121 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

[...]

> +    pub trait ParseInt: FromStrRadix + TryFrom<i128> {
> +        /// 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 i128 as sufficiently large.
> +                    let val = i128::from_str_radix(

The usage of i128 causes here following link errors on arm64 with
"rustc 1.84.1 (e71f9a9a9 2025-01-27) (Fedora 1.84.1-1.fc41)"

| ld: rust/kernel.o: in function `<i128>::from_str_radix':
| /usr/lib/rustlib/src/rust/library/core/src/num/mod.rs:1563:(.text+0x3bc): undefined reference to `__muloti4'
| ld: /usr/lib/rustlib/src/rust/library/core/src/num/mod.rs:1563:(.text+0x440): undefined reference to `__muloti4'
| ld: rust/kernel.o: in function `<i128>::overflowing_mul':
| /usr/lib/rustlib/src/rust/library/core/src/num/int_macros.rs:2517:(.text+0x4b4): undefined reference to `__muloti4'
| ld: /usr/lib/rustlib/src/rust/library/core/src/num/int_macros.rs:2517:(.text+0x534): undefined reference to `__muloti4'

The errors go away after exchanging i128 with i64 (while breaking the
parsing for large values).

ciao Janne

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Miguel Ojeda]

On Mon, Feb 24, 2025 at 11:30 PM Janne Grunau <j@jannau.net> wrote:
>
> The errors go away after exchanging i128 with i64 (while breaking the
> parsing for large values).

I don't think we can use 128-bit integers unconditionally for all
architectures (we could eventually get it to work for some though). So
we should do one of the other approaches discussed in the previous
versions, like call into the C one.

Cheers,
Miguel

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

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

> On Mon, Feb 24, 2025 at 11:30 PM Janne Grunau <j@jannau.net> wrote:
>>
>> The errors go away after exchanging i128 with i64 (while breaking the
>> parsing for large values).
>
> I don't think we can use 128-bit integers unconditionally for all
> architectures (we could eventually get it to work for some though). So
> we should do one of the other approaches discussed in the previous
> versions, like call into the C one.

I don't want to call into the C functions for this task if I can stay in
safe Rust.

I think I can solve the issue by parsing into a unsigned version of the
integer and then test the sign bit, or compare against the max value for
the signed version.


Best regards,
Andreas Hindborg

Re: [PATCH v7 5/6] rust: str: add radix prefixed integer parsing functions

[Andreas Hindborg]

"Janne Grunau" <j@jannau.net> writes:

> On Tue, Feb 18, 2025 at 02:00:47PM +0100, Andreas Hindborg wrote:
>> 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 | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index db272d2198fcc..8b0d814b47f52 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -945,3 +945,121 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>
> [...]
>
>> +    pub trait ParseInt: FromStrRadix + TryFrom<i128> {
>> +        /// 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 i128 as sufficiently large.
>> +                    let val = i128::from_str_radix(
>
> The usage of i128 causes here following link errors on arm64 with
> "rustc 1.84.1 (e71f9a9a9 2025-01-27) (Fedora 1.84.1-1.fc41)"
>
> | ld: rust/kernel.o: in function `<i128>::from_str_radix':
> | /usr/lib/rustlib/src/rust/library/core/src/num/mod.rs:1563:(.text+0x3bc): undefined reference to `__muloti4'
> | ld: /usr/lib/rustlib/src/rust/library/core/src/num/mod.rs:1563:(.text+0x440): undefined reference to `__muloti4'
> | ld: rust/kernel.o: in function `<i128>::overflowing_mul':
> | /usr/lib/rustlib/src/rust/library/core/src/num/int_macros.rs:2517:(.text+0x4b4): undefined reference to `__muloti4'
> | ld: /usr/lib/rustlib/src/rust/library/core/src/num/int_macros.rs:2517:(.text+0x534): undefined reference to `__muloti4'
>
> The errors go away after exchanging i128 with i64 (while breaking the
> parsing for large values).

Thanks for reporting! I will have to find a better way to fix this issue
then.


Best regards,
Andreas Hindborg

[PATCH v7 6/6] 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>
Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
---
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 +++++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 6 files changed, 466 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911a..aec04df2bac9f 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -57,6 +57,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 0000000000000..0047126c917f4
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Types 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
+// 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`]: srctree/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 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 crate::error::code::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).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, and
+        // so it is properly initialized.
+        unsafe { core::ptr::replace(old_value, new_value) };
+        Ok(0)
+    })
+}
+
+/// Drop the parameter.
+///
+/// Called when unloading a module.
+///
+/// # Safety
+///
+/// The `arg` field of `param` must be an initialized instance of `T`.
+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::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: 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 563dcd2b7ace5..ffc9f0cccddc8 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())
@@ -107,6 +118,20 @@ 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 d61bc6a56425e..2778292f8cee1 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 cdf94f4982dfc..e6af3ae5fe80e 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,
@@ -98,6 +202,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 +260,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 +289,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 +333,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!(
         "
@@ -362,14 +514,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 4aaf117bf8e3c..d999a77c6eb9a 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)?;

-- 
2.47.0

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

[Daniel Almeida]

Hi Andreas, thanks for working on this, I can see that this patch took a lot
of effort.

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> 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.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
> ---
> rust/kernel/lib.rs           |   1 +
> rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
> rust/macros/helpers.rs       |  25 +++++
> rust/macros/lib.rs           |  31 ++++++
> rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
> samples/rust/rust_minimal.rs |  10 ++
> 6 files changed, 466 insertions(+), 18 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911a..aec04df2bac9f 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -57,6 +57,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 0000000000000..0047126c917f4
> --- /dev/null
> +++ b/rust/kernel/module_param.rs
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Types for module parameters.

nit: maybe “Support for module parameters”?

Or anything else other than “types”, really :)

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

nit: perhaps: “we never access *it* from *a* Rust module” ?

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

nit: perhaps `implementation writes more than`? Although it’d be great if a
native speaker could chime in on this one.

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

I don’t understand what’s being said here. e.g.: what does “Self needs to track
ownership without exposing it” mean? Can you expand on this?

Also this is pub. It should perhaps also be sealed?


> +    // This is required to support string parameters in the future.
> +    type Value: ?Sized;

Why? Can you also expand on this a tad further?

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

Perhaps the above should also be an invariant?

> +///
> +/// 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 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 crate::error::code::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).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, and
> +        // so it is properly initialized.
> +        unsafe { core::ptr::replace(old_value, new_value) };
> +        Ok(0)
> +    })
> +}
> +
> +/// Drop the parameter.
> +///
> +/// Called when unloading a module.
> +///
> +/// # Safety
> +///
> +/// The `arg` field of `param` must be an initialized instance of `T`.
> +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::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 {

I assume that this is pub so that the macro can find it? If so, can you leave a note
outlining this?

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

What lock guard, guarding what exactly?

> +    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: 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 563dcd2b7ace5..ffc9f0cccddc8 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())
> @@ -107,6 +118,20 @@ 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 d61bc6a56425e..2778292f8cee1 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 cdf94f4982dfc..e6af3ae5fe80e 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;
> +        };

Shouldn’t this panic? A call to emit_params() where there’s nothing to emit doesn’t
look right at a first glance.

> +
> +        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,
> @@ -98,6 +202,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 +260,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 +289,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 +333,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);
>         }
>     }

These seem a bit unrelated?

> 
>     // 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!(
>         "
> @@ -362,14 +514,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 4aaf117bf8e3c..d999a77c6eb9a 100644

I wonder if the changes to rust_minimal.rs should be a separate patch.

> --- 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)?;
> 
> -- 
> 2.47.0
> 
> 
> 

— Daniel

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

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas, thanks for working on this, I can see that this patch took a lot
> of effort.

Thanks! It's not all me though, it's based on old code from the
pre-merge days.

[...]

>> index 0000000000000..0047126c917f4
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,226 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Types for module parameters.
>
> nit: maybe “Support for module parameters”?
>
> Or anything else other than “types”, really :)

I agree. I think originally the naming came from this being types
backing the macro implementation.

>
>> +//!
>> +//! 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
>
> nit: perhaps: “we never access *it* from *a* Rust module” ?

Right 👍

>
>> +// 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
>
> nit: perhaps `implementation writes more than`? Although it’d be great if a
> native speaker could chime in on this one.

Actually, we removed the support for displaying in sysfs from the
series, so I will just yank that note.

>
>> +/// [`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.
>
> I don’t understand what’s being said here. e.g.: what does “Self needs to track
> ownership without exposing it” mean? Can you expand on this?

For some parameter types, such as string values, the parameter may
assume a reference value or an owned value. The reference value would be
used as default (as a reference to a static string), while an owned
value would be passed in when the value is set. For that, `Value` can be
an enum.

We yanked support for anything but integer parameters from this series,
but I would like to keep this around to make adding string parameters
less churn in the near future.

If you follow link [1] in the cover letter, you can see the original
code from the pre-merge branch that this patch is based on.

>
> Also this is pub. It should perhaps also be sealed?

We might in the future allow users to implement their own parsers.

>
>
>> +    // This is required to support string parameters in the future.
>> +    type Value: ?Sized;
>
> Why? Can you also expand on this a tad further?

As explained above.

>
>> +
>> +    /// 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 [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.
>
> Perhaps the above should also be an invariant?

Actually, I think it should be part of the safety requirements.

[...]

>> +
>> +impl<T> ModuleParamAccess<T> {
>> +    #[doc(hidden)]
>> +    pub const fn new(value: T) -> Self {
>
> I assume that this is pub so that the macro can find it? If so, can you leave a note
> outlining this?

Yes, it must be accessible from other crates (modules). Will add a note.

>
>> +        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.
>
> What lock guard, guarding what exactly?

That is yet to be determined. When we enable sysfs, we will have async
access to the parameter value when user space interacts with sysfs.
Thus, we need to apply synchronization on parameter value access. I
envision a lock being taken before this method is called and a lock
guard passed in to access the data.

The code has deviated quite a bit from the original, but you can see a
possible implementation here [1].

[1] https://github.com/Rust-for-Linux/linux/blob/bc22545f38d74473cfef3e9fd65432733435b79f/rust/macros/module.rs#L410


[...]

>> +    fn emit_params(&mut self, info: &ModuleInfo) {
>> +        let Some(params) = &info.params else {
>> +            return;
>> +        };
>
> Shouldn’t this panic? A call to emit_params() where there’s nothing to emit doesn’t
> look right at a first glance.

No, having no parameters is a valid configuration. If the "params" key
is left out in the module macro call, the option will be `None`. The
call to this function when generating code is unconditional.

[...]

>> -    if let Some(firmware) = info.firmware {
>> +    if let Some(firmware) = &info.firmware {
>>         for fw in firmware {
>> -            modinfo.emit("firmware", &fw);
>> +            modinfo.emit("firmware", fw);
>>         }
>>     }
>
> These seem a bit unrelated?

They could be split in a precursor patch, but they are required for this
change set. If you insist I will split them out, but I am also happy to
keep them as one big change.

>
>>
>>     // 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!(
>>         "
>> @@ -362,14 +514,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 4aaf117bf8e3c..d999a77c6eb9a 100644
>
> I wonder if the changes to rust_minimal.rs should be a separate patch.

Either way works for me.


Thanks for the comments!


Best regards,
Andreas Hindborg

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

[Gary Guo]

On Tue, 18 Feb 2025 14:00:48 +0100
Andreas Hindborg <a.hindborg@kernel.org> 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.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
> ---
>  rust/kernel/lib.rs           |   1 +
>  rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
>  rust/macros/helpers.rs       |  25 +++++
>  rust/macros/lib.rs           |  31 ++++++
>  rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
>  samples/rust/rust_minimal.rs |  10 ++
>  6 files changed, 466 insertions(+), 18 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911a..aec04df2bac9f 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -57,6 +57,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 0000000000000..0047126c917f4
> --- /dev/null
> +++ b/rust/kernel/module_param.rs
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Types 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
> +// from Rust module.
> +unsafe impl Sync for RacyKernelParam {}

I wonder if we should have a custom impl of `SyncUnsafeCell` for this
kind of usage (so that when it is stabilized by Rust, we can just switc
over).

> +
> +/// 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`]: srctree/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 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 crate::error::code::EINVAL.to_errno();

This is already in prelude, so you can just use `EINVAL` directly.

> +    }
> +
> +    // 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, and
> +        // so it is properly initialized.
> +        unsafe { core::ptr::replace(old_value, new_value) };
> +        Ok(0)
> +    })
> +}
> +
> +/// Drop the parameter.
> +///
> +/// Called when unloading a module.
> +///
> +/// # Safety
> +///
> +/// The `arg` field of `param` must be an initialized instance of `T`.
> +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);

Why isn't `arg` BStr in the first place?

> +                <$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::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()
> +    }
> +}
> +

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

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Tue, 18 Feb 2025 14:00:48 +0100
> Andreas Hindborg <a.hindborg@kernel.org> 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.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
>> ---
>>  rust/kernel/lib.rs           |   1 +
>>  rust/kernel/module_param.rs  | 226 +++++++++++++++++++++++++++++++++++++++++++
>>  rust/macros/helpers.rs       |  25 +++++
>>  rust/macros/lib.rs           |  31 ++++++
>>  rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
>>  samples/rust/rust_minimal.rs |  10 ++
>>  6 files changed, 466 insertions(+), 18 deletions(-)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 496ed32b0911a..aec04df2bac9f 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -57,6 +57,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 0000000000000..0047126c917f4
>> --- /dev/null
>> +++ b/rust/kernel/module_param.rs
>> @@ -0,0 +1,226 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Types 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
>> +// from Rust module.
>> +unsafe impl Sync for RacyKernelParam {}
>
> I wonder if we should have a custom impl of `SyncUnsafeCell` for this
> kind of usage (so that when it is stabilized by Rust, we can just switc
> over).

We discussed this before, I don't recall what we decided. At any rate,
it's orthogonal. We can patch this if we add `SyncUnsafeCell`.

>
>> +
>> +/// 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`]: srctree/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 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 crate::error::code::EINVAL.to_errno();
>
> This is already in prelude, so you can just use `EINVAL` directly.

Thanks.

>
>> +    }
>> +
>> +    // 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, and
>> +        // so it is properly initialized.
>> +        unsafe { core::ptr::replace(old_value, new_value) };
>> +        Ok(0)
>> +    })
>> +}
>> +
>> +/// Drop the parameter.
>> +///
>> +/// Called when unloading a module.
>> +///
>> +/// # Safety
>> +///
>> +/// The `arg` field of `param` must be an initialized instance of `T`.
>> +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);
>
> Why isn't `arg` BStr in the first place?

I'll change it.


Best regards,
Andreas Hindborg

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

[Daniel Almeida]

Hi Andreas,

> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> 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].
> 
> After a bit of discussion on v3 about whether or not module parameters
> is a good idea, it seems that module parameters in Rust has a place
> in the kernel for now. This series is a dependency for `rnull`, the Rust
> null block driver [2].
> 

```
$ sudo modprobe rust_minimal test_parameter=2
[  251.384125] rust_minimal: Rust minimal sample (init)
[  251.384600] rust_minimal: Am I built-in? false
[  251.385010] rust_minimal: My parameter: 2
```

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>

IMHO, this is slightly confusing, since the parameter is named
“test_parameter”, but you’re printing “My parameter”.

This is of course very minor. Overall, congrats on getting this to work :)

— Daniel 

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

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 18 Feb 2025, at 10:00, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> 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].
>>
>> After a bit of discussion on v3 about whether or not module parameters
>> is a good idea, it seems that module parameters in Rust has a place
>> in the kernel for now. This series is a dependency for `rnull`, the Rust
>> null block driver [2].
>>
>
> ```
> $ sudo modprobe rust_minimal test_parameter=2
> [  251.384125] rust_minimal: Rust minimal sample (init)
> [  251.384600] rust_minimal: Am I built-in? false
> [  251.385010] rust_minimal: My parameter: 2
> ```
>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>
> IMHO, this is slightly confusing, since the parameter is named
> “test_parameter”, but you’re printing “My parameter”.

You are right, let's change that to "test_parameter: ".


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

Hi Petr,

"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].
>
> After a bit of discussion on v3 about whether or not module parameters
> is a good idea, it seems that module parameters in Rust has a place
> in the kernel for now. This series is a dependency for `rnull`, the Rust
> null block driver [2].


Luis told me you are the one wearing the modules hat for the moment. How
do you want to handle merging of patch 6 and subsequent maintenance of
the code?

I think we discussed you guys taking this under the current module
maintainer entry? If that is correct, will you add the new files to your
entry yourself, or should I include an update to MAINTAINERS in the next
version of this series?

If prefer another solution, let me know and we can figure that out.


Best regards,
Andreas Hindborg

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

[Petr Pavlu]

On 2/24/25 12:27, Andreas Hindborg wrote:
> Hi Petr,
> 
> "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].
>>
>> After a bit of discussion on v3 about whether or not module parameters
>> is a good idea, it seems that module parameters in Rust has a place
>> in the kernel for now. This series is a dependency for `rnull`, the Rust
>> null block driver [2].
> 
> 
> Luis told me you are the one wearing the modules hat for the moment. How
> do you want to handle merging of patch 6 and subsequent maintenance of
> the code?

I'd say the easiest is for the entire series to go through the Rust
tree. I'd also propose that any updates go primarily through that tree
as well.

> 
> I think we discussed you guys taking this under the current module
> maintainer entry? If that is correct, will you add the new files to your
> entry yourself, or should I include an update to MAINTAINERS in the next
> version of this series?

Makes sense, I think it is useful for all changes to this code to be
looked at by both Rust and modules people. To that effect, we could add
the following under the MODULES SUPPORT entry:
F:	rust/kernel/module_param.rs
F:	rust/macros/module.rs

My slight worry is that this might suggest the entirety of maintenance
is on the modules folks. Fortunately, adding the above and running
get_maintainer.pl on rust/kernel/module_param.rs gives me a list of
people for both Rust and modules, so this could be ok?

-- 
Thanks,
Petr

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

[Miguel Ojeda]

On Tue, Feb 25, 2025 at 11:22 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> I'd say the easiest is for the entire series to go through the Rust
> tree. I'd also propose that any updates go primarily through that tree
> as well.
>
> Makes sense, I think it is useful for all changes to this code to be
> looked at by both Rust and modules people. To that effect, we could add
> the following under the MODULES SUPPORT entry:
> F:      rust/kernel/module_param.rs
> F:      rust/macros/module.rs
>
> My slight worry is that this might suggest the entirety of maintenance
> is on the modules folks. Fortunately, adding the above and running
> get_maintainer.pl on rust/kernel/module_param.rs gives me a list of
> people for both Rust and modules, so this could be ok?

Up to you, of course -- a couple days ago (in the context of the
hrtimer thread) I wrote a summary of the usual discussion we have
around this (copy-pasting here for convenience):

    So far, what we have been doing is ask maintainers, first, if they
    would be willing take the patches themselves -- they are the experts
    of the subsystem, know what changes are incoming, etc. Some subsystems
    have done this (e.g. KUnit). That is ideal, because the goal is to
    scale and allows maintainers to be in full control.

    Of course, sometimes maintainers are not fully comfortable doing that,
    since they may not have the bandwidth, or the setup, or the Rust
    knowledge. In those cases, we typically ask if they would be willing
    to have a co-maintainer (i.e. in their entry, e.g. like locking did),
    or a sub-maintainer (i.e. in a new entry, e.g. like block did), that
    would take care of the bulk of the work from them.

    I think that is a nice middle-ground -- the advantage of doing it like
    that is that you get the benefits of knowing best what is going on
    without too much work (hopefully), and it may allow you to get more
    and more involved over time and confident on what is going on with the
    Rust callers, typical issues that appear, etc. Plus the sub-maintainer
    gets to learn more about the subsystem, its timelines, procedures,
    etc., which you may welcome (if you are looking for new people to get
    involved).

    I think that would be a nice middle-ground. As far as I understand,
    Andreas would be happy to commit to maintain the Rust side as a
    sub-maintainer (for instance). He would also need to make sure the
    tree builds properly with Rust enabled and so on. He already does
    something similar for Jens. Would that work for you?

    You could take the patches directly with his RoBs or Acked-bys, for
    instance. Or perhaps it makes more sense to take PRs from him (on the
    Rust code only, of course), to save you more work. Andreas does not
    send PRs to anyone yet, but I think it would be a good time for him to
    start learning how to apply patches himself etc.

    If not, then the last fallback would be putting it in the Rust tree as
    a sub-entry or similar.

    I hope that clarifies (and thanks whatever you decide!).

    https://lore.kernel.org/rust-for-linux/CANiq72mpYoig2Ro76K0E-sUtP31fW+0403zYWd6MumCgFKfTDQ@mail.gmail.com/

Would any of those other options work for you?

Thanks!

Cheers,
Miguel

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

[Petr Pavlu]

On 2/25/25 12:54, Miguel Ojeda wrote:
> On Tue, Feb 25, 2025 at 11:22 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>>
>> I'd say the easiest is for the entire series to go through the Rust
>> tree. I'd also propose that any updates go primarily through that tree
>> as well.
>>
>> Makes sense, I think it is useful for all changes to this code to be
>> looked at by both Rust and modules people. To that effect, we could add
>> the following under the MODULES SUPPORT entry:
>> F:      rust/kernel/module_param.rs
>> F:      rust/macros/module.rs
>>
>> My slight worry is that this might suggest the entirety of maintenance
>> is on the modules folks. Fortunately, adding the above and running
>> get_maintainer.pl on rust/kernel/module_param.rs gives me a list of
>> people for both Rust and modules, so this could be ok?
> 
> Up to you, of course -- a couple days ago (in the context of the
> hrtimer thread) I wrote a summary of the usual discussion we have
> around this (copy-pasting here for convenience):
> 
>     So far, what we have been doing is ask maintainers, first, if they
>     would be willing take the patches themselves -- they are the experts
>     of the subsystem, know what changes are incoming, etc. Some subsystems
>     have done this (e.g. KUnit). That is ideal, because the goal is to
>     scale and allows maintainers to be in full control.
> 
>     Of course, sometimes maintainers are not fully comfortable doing that,
>     since they may not have the bandwidth, or the setup, or the Rust
>     knowledge. In those cases, we typically ask if they would be willing
>     to have a co-maintainer (i.e. in their entry, e.g. like locking did),
>     or a sub-maintainer (i.e. in a new entry, e.g. like block did), that
>     would take care of the bulk of the work from them.
> 
>     I think that is a nice middle-ground -- the advantage of doing it like
>     that is that you get the benefits of knowing best what is going on
>     without too much work (hopefully), and it may allow you to get more
>     and more involved over time and confident on what is going on with the
>     Rust callers, typical issues that appear, etc. Plus the sub-maintainer
>     gets to learn more about the subsystem, its timelines, procedures,
>     etc., which you may welcome (if you are looking for new people to get
>     involved).
> 
>     I think that would be a nice middle-ground. As far as I understand,
>     Andreas would be happy to commit to maintain the Rust side as a
>     sub-maintainer (for instance). He would also need to make sure the
>     tree builds properly with Rust enabled and so on. He already does
>     something similar for Jens. Would that work for you?
> 
>     You could take the patches directly with his RoBs or Acked-bys, for
>     instance. Or perhaps it makes more sense to take PRs from him (on the
>     Rust code only, of course), to save you more work. Andreas does not
>     send PRs to anyone yet, but I think it would be a good time for him to
>     start learning how to apply patches himself etc.
> 
>     If not, then the last fallback would be putting it in the Rust tree as
>     a sub-entry or similar.
> 
>     I hope that clarifies (and thanks whatever you decide!).
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72mpYoig2Ro76K0E-sUtP31fW+0403zYWd6MumCgFKfTDQ@mail.gmail.com/
> 
> Would any of those other options work for you?

From what I can see in this series, the bindings required adding
a number of generic functions to the Rust support code and also most
discussion revolved around that. I'm worried this might be the case also
for foreseeable future updates, until more building blocks are in place.
It then makes me think most changes to this code will need to go through
the Rust tree for now.

On the other hand, if the changes are reasonably limited to mostly
rust/kernel/module_param.rs and rust/macros/module.rs, then yes, I'd say
it should be ok to take the patches through the modules tree.

@Luis, Sami, Daniel, please shout if you see any problem with this.

-- 
Thanks,
Petr

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

[Miguel Ojeda]

On Thu, Feb 27, 2025 at 3:55 PM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> From what I can see in this series, the bindings required adding
> a number of generic functions to the Rust support code and also most
> discussion revolved around that. I'm worried this might be the case also
> for foreseeable future updates, until more building blocks are in place.
> It then makes me think most changes to this code will need to go through
> the Rust tree for now.

Those would normally go through the Rust tree, yeah. Since we don't
have many users yet, and we avoid adding dead code in general, things
are fairly barebones.

If you prefer, we can take the non-module related dependencies through
the Rust tree this cycle, and then you can pick the modules parts in
the next one.

> On the other hand, if the changes are reasonably limited to mostly
> rust/kernel/module_param.rs and rust/macros/module.rs, then yes, I'd say
> it should be ok to take the patches through the modules tree.

Yeah, that should be the case. Worst case, we can do the delay-a-cycle
thing, or if urgent create a small branch, etc.

Cheers,
Miguel