Re: [PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Early returns in message parsing skip advancing the CPU read pointer, permanently wedging the command queue.
--

--- Patch [2]: [PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path ---
commit fab176fb9e47452c5f023d1bbf02cec260583fbf
Author: Gary Guo <gary@garyguo.net>

gpu: nova-core: fix barrier usage in GSP->CPU messaging path

In the GSP->CPU messaging path, the code reads the write pointer from GSP,
reads the response and advances the read pointer.

A LOAD->LOAD ordering is required after the write pointer read and the data
read. Add it as this is currently missing.

A LOAD->STORE ordering is required after the data read and the advance of
read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
to `smp_mb(Full)`; this however does not order DMA operations (notably on
ARM, the generate barrier orders inner shareable and not outer shareable,
which is ordered by `dma_mb`). This ordering does not need to be in between
read pointer read and write, because it's for ordering between the ring
buffer data and the pointer; the RMW operation does not internally need a
barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
so move it to before the RMW sequence for clarity.

Signed-off-by: Gary Guo <gary@garyguo.net>

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 70674d2d0f77e..9fe393da6b10c 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs

[ ... ]

> @@ -436,12 +434,11 @@ fn cpu_read_ptr(&self) -> u32 {
>  
>      // Informs the GSP that it can send `elem_count` new pages into the message queue.
>      fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
> +        // ORDERING: LOAD->STORE ordering needed to order `cpu_read_ptr` write after data read.
> +        dma_mb(Full);
> +
>          let rx = io_project!(self.0, .cpuq.rx);
>          let rptr = MsgqRxHeader::read_ptr(rx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
> -
> -        // Ensure read pointer is properly ordered.
> -        fence(Ordering::SeqCst);
> -
>          MsgqRxHeader::set_read_ptr(rx, rptr)
>      }

[Severity: High]
This isn't a bug introduced by this patch, but does CmdqInner::receive_msg()
skip advancing the CPU read pointer on parsing errors?

Looking at CmdqInner::receive_msg(), if the GSP sends a message with an
unrecognized function code or a truncated prefix, do the `?` operators
cause an early return?

drivers/gpu/nova-core/gsp/cmdq.rs:CmdqInner::receive_msg() {
        let message = self.wait_for_msg(timeout)?;
        let function = message.header.function().map_err(|_| EINVAL)?;

        // Extract the message. Store the result as we want to advance the read pointer even in
        // case of failure.
        let result = if function == M::FUNCTION {
            let (cmd, contents_1) = M::Message::from_bytes_prefix(message.contents.0).ok_or(EIO)?;
        ...

If an early return happens here, self.gsp_mem.advance_cpu_read_ptr() at the end
of the function is never called, despite the explicit comment's intent.

Will the unhandled message remain at the head of the queue, causing
subsequent calls to continually read the same bad message and fail again,
permanently wedging the message queue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust-barrier-v3-0-d5b7bd7e6624@garyguo.net?part=2