Re: [PATCH v6 19/20] rust: io: add copying methods

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Exposing `copy_read` and `copy_write` for arbitrary generic types (`T: FromBytes`/`IntoBytes`) on `MmioBackend` maps to stream accessors (`memcpy_{from,to}io`), which bypasses CPU-to-device byte-swapping. This conflates stream and register I/O semantics, creating a severe footgun for MMIO register structs on big-endian architectures.

Pre-existing issues:
- [High] The use of `fence(Ordering::SeqCst)` emits a CPU-domain memory barrier, which is insufficient for synchronizing memory between the CPU and the device (GSP) when updating command queues.
--

commit b39304c44ce9723636b250bacb9f2e99203d514d
Author: Gary Guo <gary@garyguo.net>

rust: io: add copying methods

This commit introduces copy_read and copy_write methods to the Io trait
to allow reading and writing structures and slices. It implements the
IoCopyable trait for I/O backends, mapping the MMIO operations to
memcpy_fromio and memcpy_toio.

> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index c423125870b75..9df4e982c5d8a 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs

[ ... ]

> @@ -442,6 +524,121 @@ fn write_val(self, value: Self::Target)
>          Self::Backend::io_write(self.as_view(), value)
>      }
>  
> +    /// Copy-read from I/O memory.
> +    ///
> +    /// This is equivalent to reading from the I/O memory with byte-wise copy, although the actual
> +    /// implementation might be more efficient. There is no atomicity guarantee. Note that for some
> +    /// backends (e.g. `Mmio`), this can read different value compared to [`read_val`] as
> +    /// byte-swapping is not performed.
> +    ///
> +    /// [`read_val`]: Io::read_val
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```no_run
> +    /// # use kernel::io::*;
> +    /// # fn test_copy_read(mmio: Mmio<'_, [u8; 6]>) {
> +    /// // let mmio: Mmio<'_, [u8; 6]>;
> +    /// let val: [u8; 6] = mmio.copy_read();
> +    /// # }
> +    /// ```
> +    #[inline]
> +    fn copy_read(self) -> Self::Target
> +    where
> +        Self::Backend: IoCopyable,
> +        Self::Target: Sized + FromBytes,
> +    {
> +        Self::Backend::copy_read(self.as_view())
> +    }

[Severity: High]
Does exposing this generic copy_read() for any FromBytes target conflate 
stream and register I/O semantics?

When used with a struct representing MMIO control registers, this method
eventually maps to stream accessors like memcpy_fromio() in MmioBackend,
which bypasses CPU-to-device byte-swapping.

Could this lead to data corruption on big-endian systems, where drivers might
expect the byteswapped register layout but receive unswapped data instead?
Would it be safer to restrict bulk stream copying strictly to [u8] targets
via copy_from_slice and copy_to_slice?

[ ... ]

While reviewing these I/O accessor changes, I also noticed an issue in
the GPU driver code introduced earlier in the series.

drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_read_ptr() {
    ...
    // Ensure read pointer is properly ordered.
    fence(Ordering::SeqCst);
    ...
}

drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_write_ptr() {
    ...
    // Ensure all command data is visible before triggering the GSP read.
    fence(Ordering::SeqCst);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is this CPU-domain memory
barrier sufficient for synchronizing memory between the CPU and the device
when updating command queues?

Rust's atomic fences map to CPU-domain memory barriers (e.g., dmb ish on
ARM64), which only guarantee ordering within the inner-shareable domain
between CPUs.

To properly order memory accesses visible to a device in the outer-shareable
domain, a device DMA barrier like dma_wmb() or dma_rmb() is strictly
required.

Can using a CPU fence here lead to data corruption or lost commands on weakly
ordered architectures, since the GSP could read stale command payloads before
the CPU writes are visible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-io_projection-v6-0-72cd5d055d54@garyguo.net?part=19