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

On Mon Aug 24, 2026 at 10:07 PM JST, Gary Guo wrote:
> 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

Yerp I mean move the barrier into `advance_cpu_write_ptr` not move the
barrier out of `advance_cpu_read_ptr` - I agree that'd be incorrect. On
clearness, it feels very odd to me to have these two functions
(advance_cpu_read_ptr, advance_cpu_write_ptr) where one controls the
memory barrier and one doesn't, purely based off the structure of the
callers. And I still think it's less error prone (for future changes) to
do it this way too.