Re: [PATCH v3 13/19] rust: io: add I/O backend for system memory with volatile access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing explicit `Send` and `Sync` trait implementations for `SysMem`.
- [High] `RelaxedMmio` lacks `Send` and `Sync` trait implementations.
- [Low] Missing `#[inline]` annotations on small forwarding functions `io_read` and `io_write` in `SysMemBackend`.
--

commit d4531b875e419c5220ee6e82d5fc9e251929f095
Author: Laura Nao <laura.nao@collabora.com>
Subject: rust: io: add I/O backend for system memory with volatile access

This commit adds a SysMem structure and backend for kernel virtual address
ranges. It implements the IoCapable trait for primitive types, utilizing
volatile accesses to provide safe access to shared memory. This provides an
alternative to Coherent memory when another layer handles device mapping.

> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 173f8c0ba2d6c..580ca88c46ccc 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -1040,6 +1040,120 @@ pub fn relaxed(self) -> RelaxedMmio<'a, T> {

[Severity: High]
Does RelaxedMmio lack explicit Send and Sync trait implementations?

Similar to SysMem introduced below, RelaxedMmio was refactored earlier in
this patch series to contain a *mut T inside its ptr field:

rust/kernel/io.rs:
pub struct RelaxedMmio<'a, T: ?Sized> {
    ptr: *mut T,
    phantom: PhantomData<&'a ()>,
}

While Mmio explicitly implements Send and Sync, RelaxedMmio does not.
Because of this, it is now implicitly !Send and !Sync. This seems to be a
regression introduced by this series, as the prior version of RelaxedMmio
implicitly derived Send/Sync from a usize address representation.

Without these traits, RelaxedMmio cannot be shared between threads, stored
in a Mutex, or passed across thread boundaries. Could this limit its usability
in device drivers?

>  #[cfg(CONFIG_64BIT)]
>  impl_mmio_io_capable!(RelaxedMmioBackend, u64, readq_relaxed, writeq_relaxed);

[ ... ]

> +macro_rules! impl_sysmem_io_capable {
> +    ($ty:ty) => {
> +        impl IoCapable<$ty> for SysMemBackend {
> +            fn io_read(view: SysMem<'_, $ty>) -> $ty {

[Severity: Low]
Does this small forwarding function need an #[inline] annotation?

According to the Rust Subsystem guidelines for abstractions, functions that are
small or just forward to an underlying call should be annotated with #[inline].
Since io_read is extremely short and directly wraps read_volatile, does it
require the attribute to prevent suboptimal code generation?

> +                // SAFETY:
> +                // - Per type invariant, `ptr` is valid and aligned.
> +                // - Using read_volatile() here so that race with hardware is well-defined.
> +                // - Using read_volatile() here is not sound if it races with other CPU per Rust
> +                //   rules, but this is allowed per LKMM.
> +                // - The macro is only used on primitives so all bit patterns are valid.
> +                unsafe { view.ptr.read_volatile() }
> +            }
> +
> +            fn io_write(view: SysMem<'_, $ty>, value: $ty) {

[Severity: Low]
Should this small forwarding function also have an #[inline] annotation?

Like io_read above, this directly wraps write_volatile and appears to fall
under the guidelines requiring #[inline] for small forwarding functions.

> +                // SAFETY:
> +                // - Per type invariant, `ptr` is valid and aligned.
> +                // - Using write_volatile() here so that race with hardware is well-defined.
> +                // - Using write_volatile() here is not sound if it races with other CPU per Rust
> +                //   rules, but this is allowed per LKMM.
> +                unsafe { view.ptr.write_volatile(value) }
> +            }
> +        }
> +    };
> +}

[ ... ]

> +/// System memory region.
> +///
> +/// Provides `Io` trait implementation for kernel virtual address ranges,
> +/// using volatile read/write to safely access shared memory that may be
> +/// concurrently accessed by external hardware.
> +///
> +/// # Invariants
> +///
> +/// `self.ptr.addr() .. self.ptr.addr() + KnownSize::size(self.ptr)` is valid and aligned kernel
> +/// accessible memory region for the lifetime `'a`.
> +pub struct SysMem<'a, T: ?Sized> {
> +    ptr: *mut T,
> +    phantom: PhantomData<&'a ()>,
> +}

[Severity: High]
Does SysMem need explicit Send and Sync implementations?

Because SysMem holds a *mut T internal pointer, it is implicitly !Send
and !Sync.

Since it represents an I/O mapping which can be safely shared across threads
(provided T: Sync) and volatile accesses handle hardware races according to
the LKMM, missing these implementations prevents SysMem from being passed
across thread boundaries or embedded into device state structures managed by
Arc or Mutex.

Will this severely restrict its usability in multi-threaded environments,
such as typical device drivers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-io_projection-v3-0-c5cde13a5ec4@garyguo.net?part=13