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