Re: [PATCH v3 03/16] rust: mem: add `AsRepr` and `AsReprMut`

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Incorrect use of `#[inline(always)]` instead of `#[inline]` for abstraction trait methods.
- [Low] The commit message asserts that 'all repr types are normalized to unsigned integers', but pointer types use `*const c_void` as their representation.
--

--- Patch [3]: [PATCH v3 03/16] rust: mem: add `AsRepr` and `AsReprMut` ---
commit 0638c4f7e52f9eb87681654296d18ff15c916f15
Author: Gary Guo <[email protected]>

rust: mem: add AsRepr and AsReprMut

This commit adds AsRepr and AsReprMut traits to indicate that a type
can be turned into a primitive for operations like atomics and I/O. It
also provides implementations for booleans, pointers, and integers.

> To avoid duplicating implementation, all repr types are normalized to
> unsigned integers.

[Severity: Low]
This isn't a bug, but the commit message states that all repr types are
normalized to unsigned integers. However, the implementations for *mut T
and *const T use *const c_void as their representation instead of an
unsigned integer. Should the commit message or the pointer implementations
be updated to match?

> diff --git a/rust/kernel/mem.rs b/rust/kernel/mem.rs
> index a0901cbe1b2d6..5bce381d88952 100644
> --- a/rust/kernel/mem.rs
> +++ b/rust/kernel/mem.rs
[ ... ]
> +pub unsafe trait AsRepr: Sized {
> +    /// Primitive representation of this type.
> +    type Repr;
> +
> +    /// Convert from [`AsRepr::Repr`] to `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `repr` must be a valid bit patern of `Self`. If `repr` is previously obtained using
> +    /// [`AsRepr::into_repr`], then it will always be safe.
> +    #[inline(always)]
> +    unsafe fn from_repr_unchecked(repr: Self::Repr) -> Self {

[Severity: Low]
This isn't a bug, but is #[inline(always)] the intended attribute here?
The Rust subsystem guidelines state that functions for abstractions should
be annotated with #[inline] unless they use build_assert!() dependent on
function parameters.

> +        // SAFETY: Per safety requirement of the trait.
> +        unsafe { transmute_unchecked(repr) }
> +    }
> +
> +    /// Convert from `Self` to [`AsRepr::Repr`].
> +    #[inline(always)]
> +    fn into_repr(this: Self) -> Self::Repr {

[Severity: Low]
This isn't a bug, but this method might also need to use #[inline] rather
than #[inline(always)] for the same reason.

> +        // SAFETY: Per safety requirement of the trait.
> +        unsafe { transmute_unchecked(this) }
> +    }
> +}
> +
> +/// Type that is bi-directionally transmutable with a primitive representation.
> +///
> +/// # Safety
> +///
> +/// - [`Self`] must be [transmutable] from [`Self::Repr`].
> +/// - Note that [`Self::Repr`] must be [transmutable] from `Self` as well, however that is a
> +///   requirement of the [`AsRepr`] super trait already.
> +///
> +/// [`transmutable`]: core::mem::transmute
> +pub unsafe trait AsReprMut: AsRepr {
> +    /// Convert from [`AsRepr::Repr`] to `Self`.
> +    #[inline(always)]
> +    fn from_repr(repr: Self::Repr) -> Self {

[Severity: Low]
This isn't a bug, but would it be better to use #[inline] here to follow
the abstraction guidelines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3