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

[PATCH v6 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].

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 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  | 225 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs           | 156 ++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 +++++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 7 files changed, 621 insertions(+), 18 deletions(-)
---
base-commit: 379487e17ca406b47392e7ab6cf35d1c3bacb371
change-id: 20241211-module-params-v3-ae7e5c8d8b5a
prerequisite-change-id: 20241107-pr_once_macros-6438e6f5b923:v4
prerequisite-patch-id: 57743fff5d9c649ff7c1aed7e374d08ae67dda91
prerequisite-patch-id: fe607e3e1f666e7250bf099e581d53c83fea5f7d
prerequisite-patch-id: eb030eccf23466b0e22e7c699f252c40bd5f21bf

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

[PATCH v6 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

[PATCH v6 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..1eb945bed77d6 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
+    Idx: core::slice::SliceIndex<[u8], 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

[PATCH v6 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 1eb945bed77d6..389341455b962 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

[PATCH v6 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 389341455b962..c102adac32757 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

[PATCH v6 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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index c102adac32757..192cd0ff5974f 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -945,3 +945,114 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 macro_rules! fmt {
     ($($f:tt)*) => ( core::format_args!($($f)*) )
 }
+
+pub mod parse_int {
+    //! Integer parsing functions for parsing signed and unsigned integers
+    //! potentially prefixed with `0x`, `0o`, or `0b`.
+
+    use crate::alloc::flags;
+    use crate::prelude::*;
+    use crate::str::BStr;
+    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', ..] => (16, &src[2..]),
+            [b'0', b'o' | b'O', ..] => (8, &src[2..]),
+            [b'0', b'b' | b'B', ..] => (2, &src[2..]),
+            [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 {
+        /// Parse a string according to the description in [`Self`].
+        fn from_str(src: &BStr) -> Result<Self> {
+            match src.iter().next() {
+                None => Err(EINVAL),
+                Some(sign @ b'-') | Some(sign @ b'+') => {
+                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
+                    let mut n_digits: KVec<u8> =
+                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
+                    n_digits.push(*sign, flags::GFP_KERNEL)?;
+                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;
+                    Self::from_str_radix(BStr::from_bytes(&n_digits), radix).map_err(|_| EINVAL)
+                }
+                Some(_) => {
+                    let (radix, digits) = strip_radix(src);
+                    Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
+                }
+            }
+        }
+    }
+
+    macro_rules! impl_parse_int {
+        ($ty:ty) => {
+            impl FromStrRadix for $ty {
+                fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
+                    <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+                        .map_err(|_| EINVAL)
+                }
+            }
+
+            impl ParseInt for $ty {}
+        };
+    }
+
+    impl_parse_int!(i8);
+    impl_parse_int!(u8);
+    impl_parse_int!(i16);
+    impl_parse_int!(u16);
+    impl_parse_int!(i32);
+    impl_parse_int!(u32);
+    impl_parse_int!(i64);
+    impl_parse_int!(u64);
+    impl_parse_int!(isize);
+    impl_parse_int!(usize);
+}

-- 
2.47.0

[PATCH v6 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>
---
 rust/kernel/lib.rs           |   1 +
 rust/kernel/module_param.rs  | 225 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/helpers.rs       |  25 +++++
 rust/macros/lib.rs           |  31 ++++++
 rust/macros/module.rs        | 191 ++++++++++++++++++++++++++++++++----
 samples/rust/rust_minimal.rs |  10 ++
 6 files changed, 465 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ee48cf68c3111..3d3b0d346b248 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..6c0f5691f33c0
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,225 @@
+// 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() {
+        crate::pr_warn_once!("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 v6 2/6] rust: str: implement `Index` for `BStr`

[Gary Guo]

On Tue, 11 Feb 2025 16:57:36 +0100
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..1eb945bed77d6 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
> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,

I think I'd prefer

	[T]: Index<Idx>,

here.

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

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

[Gary Guo]

On Tue, 11 Feb 2025 16:57:39 +0100
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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index c102adac32757..192cd0ff5974f 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -945,3 +945,114 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>  macro_rules! fmt {
>      ($($f:tt)*) => ( core::format_args!($($f)*) )
>  }
> +
> +pub mod parse_int {
> +    //! Integer parsing functions for parsing signed and unsigned integers
> +    //! potentially prefixed with `0x`, `0o`, or `0b`.
> +
> +    use crate::alloc::flags;
> +    use crate::prelude::*;
> +    use crate::str::BStr;
> +    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', ..] => (16, &src[2..]),

This can be written as

	[b'0', b'x' | b'X', rest @ ..] => (16, rest),

to avoid manual indexing. Same for o and b below.

> +            [b'0', b'o' | b'O', ..] => (8, &src[2..]),
> +            [b'0', b'b' | b'B', ..] => (2, &src[2..]),
> +            [b'0', ..] => (8, src),

Perhaps add a comment saying that this isn't using `src[1..]` so `0`
can be parsed.

> +            _ => (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 {
> +        /// Parse a string according to the description in [`Self`].
> +        fn from_str(src: &BStr) -> Result<Self> {
> +            match src.iter().next() {
> +                None => Err(EINVAL),
> +                Some(sign @ b'-') | Some(sign @ b'+') => {
> +                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
> +                    let mut n_digits: KVec<u8> =
> +                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
> +                    n_digits.push(*sign, flags::GFP_KERNEL)?;
> +                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;

I think my comment from a previous series saying that this shouldn't
need allocation is not addressed.

> +                    Self::from_str_radix(BStr::from_bytes(&n_digits), radix).map_err(|_| EINVAL)
> +                }
> +                Some(_) => {
> +                    let (radix, digits) = strip_radix(src);
> +                    Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
> +                }
> +            }
> +        }
> +    }
> +
> +    macro_rules! impl_parse_int {
> +        ($ty:ty) => {
> +            impl FromStrRadix for $ty {
> +                fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
> +                    <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
> +                        .map_err(|_| EINVAL)
> +                }
> +            }
> +
> +            impl ParseInt for $ty {}
> +        };
> +    }
> +
> +    impl_parse_int!(i8);
> +    impl_parse_int!(u8);
> +    impl_parse_int!(i16);
> +    impl_parse_int!(u16);
> +    impl_parse_int!(i32);
> +    impl_parse_int!(u32);
> +    impl_parse_int!(i64);
> +    impl_parse_int!(u64);
> +    impl_parse_int!(isize);
> +    impl_parse_int!(usize);
> +}
> 

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

[Andreas Hindborg]

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

> On Tue, 11 Feb 2025 16:57:39 +0100
> 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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 111 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index c102adac32757..192cd0ff5974f 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -945,3 +945,114 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>  macro_rules! fmt {
>>      ($($f:tt)*) => ( core::format_args!($($f)*) )
>>  }
>> +
>> +pub mod parse_int {
>> +    //! Integer parsing functions for parsing signed and unsigned integers
>> +    //! potentially prefixed with `0x`, `0o`, or `0b`.
>> +
>> +    use crate::alloc::flags;
>> +    use crate::prelude::*;
>> +    use crate::str::BStr;
>> +    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', ..] => (16, &src[2..]),
>
> This can be written as
>
> 	[b'0', b'x' | b'X', rest @ ..] => (16, rest),
>
> to avoid manual indexing. Same for o and b below.

error[E0308]: mismatched types
   --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:972:52
    |
972 |             [b'0', b'x' | b'X', rest @ ..] => (16, rest),
    |                                                    ^^^^ expected `&BStr`, found `&[u8]`
    |
    = note: expected reference `&BStr`
               found reference `&[u8]`

But I guess I could use the new AsRef impl. Or is it more idiomatic to
implement `From<&[u8]> for &BStr` and go with `rest.into()`?

>
>> +            [b'0', b'o' | b'O', ..] => (8, &src[2..]),
>> +            [b'0', b'b' | b'B', ..] => (2, &src[2..]),
>> +            [b'0', ..] => (8, src),
>
> Perhaps add a comment saying that this isn't using `src[1..]` so `0`
> can be parsed.

Good idea.

>
>> +            _ => (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 {
>> +        /// Parse a string according to the description in [`Self`].
>> +        fn from_str(src: &BStr) -> Result<Self> {
>> +            match src.iter().next() {
>> +                None => Err(EINVAL),
>> +                Some(sign @ b'-') | Some(sign @ b'+') => {
>> +                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
>> +                    let mut n_digits: KVec<u8> =
>> +                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
>> +                    n_digits.push(*sign, flags::GFP_KERNEL)?;
>> +                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;
>
> I think my comment from a previous series saying that this shouldn't
> need allocation is not addressed.

Thanks for noticing. This is the discussion from v4:

>> I don't think we should allocate for parsing. This can trivially be a
>> non-allocating. Just check that the next byte is an ASCII digit (reject
>> if so, in case people give multiple signs), and then from_str_radix and
>> return as is or use `checked_neg`.
>
>The issue with that approach is that 2s complement signed integer types
>of width `b` can assume values from -2^(b-1) to (2^(b-1))-1. We would
>reject the value -2^(b-1) when trying to parse as 2^(b-1).
>
>We could parse into an unsigned type, but it gets kind of clunky.
>
>Another option is to stop relying on `from_str_radix` from core and roll
>our own that takes sign as a separate function argument.

What is your take on that?


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On Tue, 11 Feb 2025 16:57:36 +0100
> 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..1eb945bed77d6 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
>> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,
>
> I think I'd prefer
>
> 	[T]: Index<Idx>,

Is that equivalent?


Best regards,
Andreas Hindborg

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

[Gary Guo]

On Tue, 11 Feb 2025 21:13:10 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> "Gary Guo" <gary@garyguo.net> writes:
> 
> > On Tue, 11 Feb 2025 16:57:39 +0100
> > 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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 111 insertions(+)
> >>
> >> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> >> index c102adac32757..192cd0ff5974f 100644
> >> --- a/rust/kernel/str.rs
> >> +++ b/rust/kernel/str.rs
> >> @@ -945,3 +945,114 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> >>  macro_rules! fmt {
> >>      ($($f:tt)*) => ( core::format_args!($($f)*) )
> >>  }
> >> +
> >> +pub mod parse_int {
> >> +    //! Integer parsing functions for parsing signed and unsigned integers
> >> +    //! potentially prefixed with `0x`, `0o`, or `0b`.
> >> +
> >> +    use crate::alloc::flags;
> >> +    use crate::prelude::*;
> >> +    use crate::str::BStr;
> >> +    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', ..] => (16, &src[2..]),  
> >
> > This can be written as
> >
> > 	[b'0', b'x' | b'X', rest @ ..] => (16, rest),
> >
> > to avoid manual indexing. Same for o and b below.  
> 
> error[E0308]: mismatched types
>    --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:972:52  
>     |
> 972 |             [b'0', b'x' | b'X', rest @ ..] => (16, rest),
>     |                                                    ^^^^ expected `&BStr`, found `&[u8]`
>     |
>     = note: expected reference `&BStr`
>                found reference `&[u8]`
> 
> But I guess I could use the new AsRef impl. Or is it more idiomatic to
> implement `From<&[u8]> for &BStr` and go with `rest.into()`?

Ah, alright, I missed that this function is operating on BStr instead
of slice. Keeping the current form is fine then.

> 
> >  
> >> +            [b'0', b'o' | b'O', ..] => (8, &src[2..]),
> >> +            [b'0', b'b' | b'B', ..] => (2, &src[2..]),
> >> +            [b'0', ..] => (8, src),  
> >
> > Perhaps add a comment saying that this isn't using `src[1..]` so `0`
> > can be parsed.  
> 
> Good idea.
> 
> >  
> >> +            _ => (10, src),
> >> +        }
> >> +    }

> >> +    pub trait ParseInt: FromStrRadix {
> >> +        /// Parse a string according to the description in [`Self`].
> >> +        fn from_str(src: &BStr) -> Result<Self> {
> >> +            match src.iter().next() {
> >> +                None => Err(EINVAL),
> >> +                Some(sign @ b'-') | Some(sign @ b'+') => {
> >> +                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
> >> +                    let mut n_digits: KVec<u8> =
> >> +                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
> >> +                    n_digits.push(*sign, flags::GFP_KERNEL)?;
> >> +                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;  
> >
> > I think my comment from a previous series saying that this shouldn't
> > need allocation is not addressed.  
> 
> Thanks for noticing. This is the discussion from v4:
> 
> >> I don't think we should allocate for parsing. This can trivially be a
> >> non-allocating. Just check that the next byte is an ASCII digit (reject
> >> if so, in case people give multiple signs), and then from_str_radix and
> >> return as is or use `checked_neg`.  
> >
> >The issue with that approach is that 2s complement signed integer types
> >of width `b` can assume values from -2^(b-1) to (2^(b-1))-1. We would
> >reject the value -2^(b-1) when trying to parse as 2^(b-1).
> >
> >We could parse into an unsigned type, but it gets kind of clunky.

I would say either that or just call into kstrto* family.

Best,
Gary

> >
> >Another option is to stop relying on `from_str_radix` from core and roll
> >our own that takes sign as a separate function argument.  
> 
> What is your take on that?
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 

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

[Gary Guo]

On Tue, 11 Feb 2025 21:24:44 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> "Gary Guo" <gary@garyguo.net> writes:
> 
> > On Tue, 11 Feb 2025 16:57:36 +0100
> > 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..1eb945bed77d6 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
> >> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,  
> >
> > I think I'd prefer
> >
> > 	[T]: Index<Idx>,  
> 
> Is that equivalent?

Sorry, I meant `[u8]: Index<Idx>`. This makes more semantic sense that
"what ever can index a byte slice, it can also index BStr". This is
also how our CStr and the array primitive type implements its Index
operation.

They should be equivalent as libcore does

	impl<T, I> Index<I> for [T] where I: SliceIndex<[T]> { ... }

Best,
Gary

> 
> 
> Best regards,
> Andreas Hindborg
> 
> 

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

[Petr Pavlu]

On 2/11/25 16:57, Andreas Hindborg wrote:
> This patch includes changes required for Rust kernel modules to utilize
> module parameters. This code implements read only support for integer
> types without `sysfs` support.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

For what it's worth, this looks reasonable to me from the modules
perspective.

Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective

-- 
Thanks,
Petr

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

[Andreas Hindborg]

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

> On Tue, 11 Feb 2025 21:13:10 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> > On Tue, 11 Feb 2025 16:57:39 +0100
>> > 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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 111 insertions(+)
>> >>
>> >> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> >> index c102adac32757..192cd0ff5974f 100644
>> >> --- a/rust/kernel/str.rs
>> >> +++ b/rust/kernel/str.rs
>> >> @@ -945,3 +945,114 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> >>  macro_rules! fmt {
>> >>      ($($f:tt)*) => ( core::format_args!($($f)*) )
>> >>  }
>> >> +
>> >> +pub mod parse_int {
>> >> +    //! Integer parsing functions for parsing signed and unsigned integers
>> >> +    //! potentially prefixed with `0x`, `0o`, or `0b`.
>> >> +
>> >> +    use crate::alloc::flags;
>> >> +    use crate::prelude::*;
>> >> +    use crate::str::BStr;
>> >> +    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', ..] => (16, &src[2..]),
>> >
>> > This can be written as
>> >
>> > 	[b'0', b'x' | b'X', rest @ ..] => (16, rest),
>> >
>> > to avoid manual indexing. Same for o and b below.
>>
>> error[E0308]: mismatched types
>>    --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:972:52
>>     |
>> 972 |             [b'0', b'x' | b'X', rest @ ..] => (16, rest),
>>     |                                                    ^^^^ expected `&BStr`, found `&[u8]`
>>     |
>>     = note: expected reference `&BStr`
>>                found reference `&[u8]`
>>
>> But I guess I could use the new AsRef impl. Or is it more idiomatic to
>> implement `From<&[u8]> for &BStr` and go with `rest.into()`?
>
> Ah, alright, I missed that this function is operating on BStr instead
> of slice. Keeping the current form is fine then.
>
>>
>> >
>> >> +            [b'0', b'o' | b'O', ..] => (8, &src[2..]),
>> >> +            [b'0', b'b' | b'B', ..] => (2, &src[2..]),
>> >> +            [b'0', ..] => (8, src),
>> >
>> > Perhaps add a comment saying that this isn't using `src[1..]` so `0`
>> > can be parsed.
>>
>> Good idea.
>>
>> >
>> >> +            _ => (10, src),
>> >> +        }
>> >> +    }
>
>> >> +    pub trait ParseInt: FromStrRadix {
>> >> +        /// Parse a string according to the description in [`Self`].
>> >> +        fn from_str(src: &BStr) -> Result<Self> {
>> >> +            match src.iter().next() {
>> >> +                None => Err(EINVAL),
>> >> +                Some(sign @ b'-') | Some(sign @ b'+') => {
>> >> +                    let (radix, digits) = strip_radix(BStr::from_bytes(&src[1..]));
>> >> +                    let mut n_digits: KVec<u8> =
>> >> +                        KVec::with_capacity(digits.len() + 1, flags::GFP_KERNEL)?;
>> >> +                    n_digits.push(*sign, flags::GFP_KERNEL)?;
>> >> +                    n_digits.extend_from_slice(digits, flags::GFP_KERNEL)?;
>> >
>> > I think my comment from a previous series saying that this shouldn't
>> > need allocation is not addressed.
>>
>> Thanks for noticing. This is the discussion from v4:
>>
>> >> I don't think we should allocate for parsing. This can trivially be a
>> >> non-allocating. Just check that the next byte is an ASCII digit (reject
>> >> if so, in case people give multiple signs), and then from_str_radix and
>> >> return as is or use `checked_neg`.
>> >
>> >The issue with that approach is that 2s complement signed integer types
>> >of width `b` can assume values from -2^(b-1) to (2^(b-1))-1. We would
>> >reject the value -2^(b-1) when trying to parse as 2^(b-1).
>> >
>> >We could parse into an unsigned type, but it gets kind of clunky.
>
> I would say either that or just call into kstrto* family.

Right. I'll rather parse into i128 than call into that.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On Tue, 11 Feb 2025 21:24:44 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> > On Tue, 11 Feb 2025 16:57:36 +0100
>> > 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..1eb945bed77d6 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
>> >> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,
>> >
>> > I think I'd prefer
>> >
>> > 	[T]: Index<Idx>,
>>
>> Is that equivalent?
>
> Sorry, I meant `[u8]: Index<Idx>`. This makes more semantic sense that
> "what ever can index a byte slice, it can also index BStr". This is
> also how our CStr and the array primitive type implements its Index
> operation.
>
> They should be equivalent as libcore does
>
> 	impl<T, I> Index<I> for [T] where I: SliceIndex<[T]> { ... }
>

What I originally wrote is `Idx` must be usable as an index for `[u8]`,
yielding `[u8]` when indexing.

The new one you suggest, I parse as `[u8]` should be indexable by `Idx`.
This is less info. The compiler will also complain about the missing info:

error[E0308]: mismatched types
   --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:141:26
    |
141 |         BStr::from_bytes(&self.0[index])
    |         ---------------- ^^^^^^^^^^^^^^ expected `&[u8]`, found `&<[u8] as Index<Idx>>::Output`
    |         |
    |         arguments to this function are incorrect
    |
    = note: expected reference `&[u8]`
               found reference `&<[u8] as Index<Idx>>::Output`
    = help: consider constraining the associated type `<[u8] as Index<Idx>>::Output` to `[u8]`

If I constrain the output it's all fine again:

    [u8]: Index<Idx, Output = [u8]>,


But as I said, I don't think it matters which direction we put this?


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On 18.02.25 12:14, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
> 
>> On Tue, 11 Feb 2025 21:24:44 +0100
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "Gary Guo" <gary@garyguo.net> writes:
>>>
>>>> On Tue, 11 Feb 2025 16:57:36 +0100
>>>> 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..1eb945bed77d6 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
>>>>> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,
>>>>
>>>> I think I'd prefer
>>>>
>>>> 	[T]: Index<Idx>,
>>>
>>> Is that equivalent?
>>
>> Sorry, I meant `[u8]: Index<Idx>`. This makes more semantic sense that
>> "what ever can index a byte slice, it can also index BStr". This is
>> also how our CStr and the array primitive type implements its Index
>> operation.
>>
>> They should be equivalent as libcore does
>>
>> 	impl<T, I> Index<I> for [T] where I: SliceIndex<[T]> { ... }
>>
> 
> What I originally wrote is `Idx` must be usable as an index for `[u8]`,
> yielding `[u8]` when indexing.
> 
> The new one you suggest, I parse as `[u8]` should be indexable by `Idx`.
> This is less info. The compiler will also complain about the missing info:
> 
> error[E0308]: mismatched types
>    --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:141:26
>     |
> 141 |         BStr::from_bytes(&self.0[index])
>     |         ---------------- ^^^^^^^^^^^^^^ expected `&[u8]`, found `&<[u8] as Index<Idx>>::Output`
>     |         |
>     |         arguments to this function are incorrect
>     |
>     = note: expected reference `&[u8]`
>                found reference `&<[u8] as Index<Idx>>::Output`
>     = help: consider constraining the associated type `<[u8] as Index<Idx>>::Output` to `[u8]`
> 
> If I constrain the output it's all fine again:
> 
>     [u8]: Index<Idx, Output = [u8]>,
> 
> 
> But as I said, I don't think it matters which direction we put this?

I think it's better to depend on `Index` compared to `SliceIndex`.

---
Cheers,
Benno

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

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 18.02.25 12:14, Andreas Hindborg wrote:
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Tue, 11 Feb 2025 21:24:44 +0100
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> "Gary Guo" <gary@garyguo.net> writes:
>>>>
>>>>> On Tue, 11 Feb 2025 16:57:36 +0100
>>>>> 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..1eb945bed77d6 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
>>>>>> +    Idx: core::slice::SliceIndex<[u8], Output = [u8]>,
>>>>>
>>>>> I think I'd prefer
>>>>>
>>>>> 	[T]: Index<Idx>,
>>>>
>>>> Is that equivalent?
>>>
>>> Sorry, I meant `[u8]: Index<Idx>`. This makes more semantic sense that
>>> "what ever can index a byte slice, it can also index BStr". This is
>>> also how our CStr and the array primitive type implements its Index
>>> operation.
>>>
>>> They should be equivalent as libcore does
>>>
>>> 	impl<T, I> Index<I> for [T] where I: SliceIndex<[T]> { ... }
>>>
>>
>> What I originally wrote is `Idx` must be usable as an index for `[u8]`,
>> yielding `[u8]` when indexing.
>>
>> The new one you suggest, I parse as `[u8]` should be indexable by `Idx`.
>> This is less info. The compiler will also complain about the missing info:
>>
>> error[E0308]: mismatched types
>>    --> /home/aeh/src/linux-rust/module-params/rust/kernel/str.rs:141:26
>>     |
>> 141 |         BStr::from_bytes(&self.0[index])
>>     |         ---------------- ^^^^^^^^^^^^^^ expected `&[u8]`, found `&<[u8] as Index<Idx>>::Output`
>>     |         |
>>     |         arguments to this function are incorrect
>>     |
>>     = note: expected reference `&[u8]`
>>                found reference `&<[u8] as Index<Idx>>::Output`
>>     = help: consider constraining the associated type `<[u8] as Index<Idx>>::Output` to `[u8]`
>>
>> If I constrain the output it's all fine again:
>>
>>     [u8]: Index<Idx, Output = [u8]>,
>>
>>
>> But as I said, I don't think it matters which direction we put this?
>
> I think it's better to depend on `Index` compared to `SliceIndex`.

I am curious for what reason?


Best regards,
Andreas Hindborg