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 <[email protected]> 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/[email protected]?part=13
