Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] `RelaxedMmio` is missing `Send` and `Sync` trait implementations, preventing it from being shared or sent across threads. - [Low] The `Mmio::relaxed` method is missing the required `#[inline]` annotation. - [Low] The doc comment for `Mmio::relaxed` incorrectly claims to return a reference. --
commit 2af28174275bf1ccd9fc4579aef4a245e6ff315d Author: Gary Guo <[email protected]> rust: io: implement `Mmio` as view type This commit implements Mmio as a view type and converts RelaxedMmio to a view type. I/O implementations of MmioOwned are changed to delegate to the Mmio view type. Documentation users of MmioOwned are updated to reflect these semantics. > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index d5c233a66846d..771372a8aa36b 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs [ ... ] > @@ -888,32 +968,53 @@ pub unsafe fn from_raw(raw: &MmioRaw<Region<SIZE>>) -> > &Self { [ ... ] > +pub struct RelaxedMmio<'a, T: ?Sized> { > + ptr: *mut T, > + phantom: PhantomData<&'a ()>, > +} [Severity: Medium] Because RelaxedMmio now contains a raw pointer (*mut T), it is implicitly !Send and !Sync. While the newly introduced Mmio type received explicit Send and Sync implementations, they seem to be omitted here. Should RelaxedMmio also implement Send and Sync so it can be shared or sent across threads by drivers? [ ... ] > -impl<const SIZE: usize> MmioOwned<SIZE> { > +impl<'a, T: ?Sized> Mmio<'a, T> { > /// Returns a [`RelaxedMmio`] reference that performs relaxed I/O > operations. [Severity: Low] This isn't a bug, but the doc comment says it returns a reference, while the function signature below now returns the view type by value. Could this be updated to reflect the new semantics? > /// > /// Relaxed accessors do not provide ordering guarantees with respect to > DMA or memory accesses > @@ -924,20 +1025,23 @@ impl<const SIZE: usize> MmioOwned<SIZE> { [ ... ] > - pub fn relaxed(&self) -> &RelaxedMmio<SIZE> { > - // SAFETY: `RelaxedMmio` is `#[repr(transparent)]` over `MmioOwned`, > so `MmioOwned<SIZE>` > - // and `RelaxedMmio<SIZE>` have identical layout. > - unsafe { core::mem::transmute(self) } > + pub fn relaxed(self) -> RelaxedMmio<'a, T> { > + // INVARIANT: `RelaxedMmio` has the same invariant as `Mmio`. > + RelaxedMmio { > + ptr: self.ptr, > + phantom: PhantomData, > + } > } [Severity: Low] This isn't a bug, but should this small abstraction function be annotated with #[inline]? According to the Rust subsystem coding guidelines, functions in abstractions that are small or just forwarding to a binding call should be inlined. This would match other view-creation methods like Mmio::from_raw. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
