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

On Mon Aug 24, 2026 at 1:50 PM BST, Eliot Courtney wrote:
> 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 <[email protected]>
>> ---
>>  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`?

I think it's more clear to consider `advance_cpu_write_ptr` to just be the
pointer increment, and the ordering should be visible in code that performs both
memory ops.

Best,
Gary

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