Re: [PATCH v4 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] The memory barrier in `advance_cpu_write_ptr` is incorrectly placed after the write pointer update, potentially allowing the GSP to read stale command data.
--

commit 6cc25fbdd45cd6420a6e1ed539f7745066511455
Author: Gary Guo <gary@garyguo.net>
Use io_project! for PTE array and message queues to restore the proper encapsulation.

This commit migrates the nova-core GSP driver to use the io_project!,
io_read!, and io_write! macros for accessing the PTE array and message queues.
The previous dma_read! and dma_write! macros are replaced to properly
encapsulate IO memory operations on primitives.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 070de0731e95..c34b48961496 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 is the memory barrier placed incorrectly
here? 

Because fence(Ordering::SeqCst) happens after MsgqTxHeader::set_write_ptr()
updates the write pointer, could the CPU reorder the preceding command data
writes with the write pointer update itself?

If the updated write pointer becomes visible to the GSP before the actual
command data is fully written to memory, it seems the GSP might read
uninitialized or stale commands. 

Should the fence be moved to immediately before set_write_ptr() to prevent
this reordering, similar to how the read pointer is handled in
advance_cpu_read_ptr()?

>      }
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-io_projection-v4-0-1f7224b02dcb@garyguo.net?part=17