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

[Eliot Courtney]

On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
> writes the command, advances the write pointer, and then notifies the GSP.
>
> A LOAD->STORE ordering is needed after reading the read pointer from GSP
> and writing the command. Control dependency exists here which provide the
> needed ordering, but it's best to avoid depending on it.
>
> A STORE->STORE ordering is needed after the command write and before the
> write pointer advance. This is currently incorrectly done after the write
> pointer advance (and before GSP notification), but this can cause issue if
> GSP is still processing ring buffer, as it may observe the write pointer
> advance before command write. Thus move this barrier to be before the write
> pointer advance. Note that barriers are not needed between write pointer
> advance and GSP notification, as MMIO accessors already carries the
> required barrier.
>
> Signed-off-by: Gary Guo <gary@xxxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 6da728201281..70674d2d0f77 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -27,6 +27,11 @@
>      ptr,
>      sync::{
>          aref::ARef,
> +        barrier::{
> +            dma_mb,
> +            Full,
> +            Write, //
> +        },
>          Mutex, //
>      },
>      time::Delta,
> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>              (rx - 1, 0)
>          };
>  
> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
> +        dma_mb(Full);
> +
>          // SAFETY:
>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>          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);
>      }
>  }
>  
> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>              dst.header.length(),
>          );
>  
> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
> +        dma_mb(Write);
> +

Is there a reason this can't go into `advance_cpu_write_ptr`?

>          // All set - update the write pointer and inform the GSP of the new command.
>          let elem_count = dst.header.element_count();
>          self.seq += 1;