Re: [PATCH v3 07/19] rust: io: implement `Mmio` as view type

[sashiko-bot@kernel.org]

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 <gary@garyguo.net>

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/20260608-io_projection-v3-0-c5cde13a5ec4@garyguo.net?part=7