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

On Mon Aug 24, 2026 at 2:03 PM BST, Eliot Courtney wrote:
> On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote:
>> 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.
>
> In the second patch, it looks like you're adding the memory barrier
> directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in
> the code advancing the pointer and one not, which seems asymmetric. I
> think it's less error prone to put the barrier in the function so it
> can't be misused (and we already have evidence the barriers are easy to
> get wrong, since this code was already broken).

In the second one `message.header.length()` is read, so if I move the barrier to
before the advance it'll be incorrect.

Best,
Gary