Re: [PATCH v6 17/20] gpu: nova-core: use I/O projection for cleaner encapsulation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing acquire and release memory barriers in the lock-free ring buffer implementation between the CPU and GSP firmware.
--

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

gpu: nova-core: use I/O projection for cleaner encapsulation

This commit refactors PTE arrays and message queues to use io_project!.
It replaces primitive dma_read! and dma_write! usage with io_read!
and io_write! to restore proper encapsulation.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 070de0731e950..c34b489614962 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs

[ ... ]

> @@ -438,12 +441,17 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
>      //
>      // - The returned value is within `0..MSGQ_NUM_PAGES`.
>      fn cpu_write_ptr(&self) -> u32 {
> -        super::fw::gsp_mem::cpu_write_ptr(&self.0)
> +        MsgqTxHeader::write_ptr(io_project!(self.0, .cpuq.tx)) % MSGQ_NUM_PAGES
>      }
>  
>      // Informs the GSP that it can process `elem_count` new pages from the command queue.
>      fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
> -        super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count)
> +        let tx = io_project!(self.0, .cpuq.tx);
> +        let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
> +        MsgqTxHeader::set_write_ptr(tx, wptr);
> +
> +        // Ensure all command data is visible before triggering the GSP read.
> +        fence(Ordering::SeqCst);

[Severity: High]
This is a pre-existing issue, but does placing the fence after the write
pointer update fail to order the preceding command payload writes?

In advance_cpu_write_ptr() in drivers/gpu/nova-core/gsp/cmdq.rs, the driver
updates the TX pointer via io_write!() (inside set_write_ptr) and then issues
a fence(Ordering::SeqCst).

Since io_write!() compiles to a plain write_volatile without an implicit
barrier, placing the fence after the pointer update could allow the
firmware to observe the updated write pointer before the command payload
is fully visible. This could lead to garbage command processing on
architectures with weak memory ordering.

Should this fence be placed before the set_write_ptr() call instead?

Additionally, looking at the wider lock-free ring buffer implementation, do
wait_for_msg() and allocate_command() lack acquire memory barriers?

If wait_for_msg() polls the GSP write pointer via read_volatile and proceeds
to read the payload without an acquire barrier, the CPU might speculatively
read the message payload before the pointer read completes.

>      }
>  }

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