[PATCH v3 0/2] gpu: nova: fix incorrect GSP barrier usage

[PATCH v3 0/2] gpu: nova: fix incorrect GSP barrier usage

[Gary Guo]

Currently Nova GSP code uses Rust barriers, which equivalent to LKMM SMP
barriers. This does not provide sufficient ordering for DMA operations on
AArch64. Fix this to be using the newly introduced `dma_mb(Ordering)` API.

Also, there is an incorrect placement of barrier, which is also fixed.

This should wait until v7.3-rc1 backmerge to drm-rust-next occurs.

---
Changes in v3:
- Drop abstraction changes which are already picked.
- Improve ordering comment messages.
- Replace control dependency reliance with explicit barrier.
- Link to v2: https://patch.msgid.link/20260609-rust-barrier-v2-0-30fcc48e1cd0@garyguo.net

Changes in v2:
- Dropped `Acquire` and `Release` aliases of `Full` (Joel)
- Use macros to implement most `MemoryBarrier`
- Split Nova change to GSP->CPU commit and CPU->GSP commit (Joel)
- Link to v1: https://patch.msgid.link/20260402152443.1059634-2-gary@kernel.org

To: Danilo Krummrich <dakr@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Alexandre Courbot <acourbot@nvidia.com>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
Cc: nova-gpu@lists.linux.dev
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

---
Gary Guo (2):
      gpu: nova-core: fix barrier usage in CPU->GSP messaging path
      gpu: nova-core: fix barrier usage in GSP->CPU messaging path

 drivers/gpu/nova-core/gsp/cmdq.rs | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
---
base-commit: e6664f2b33db9b6811eb4cec109f06cb2b4f458d
change-id: 20260609-rust-barrier-63078ea76216

Best regards,
--  
Gary Guo <gary@garyguo.net>

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

[Gary Guo]

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@garyguo.net>
---
 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);
+
         // All set - update the write pointer and inform the GSP of the new command.
         let elem_count = dst.header.element_count();
         self.seq += 1;

-- 
2.54.0

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

[Gary Guo]

In the GSP->CPU messaging path, the code reads the write pointer from GSP,
reads the response and advances the read pointer.

A LOAD->LOAD ordering is required after the write pointer read and the data
read. Add it as this is currently missing.

A LOAD->STORE ordering is required after the data read and the advance of
read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
to `smp_mb(Full)`; this however does not order DMA operations (notably on
ARM, the generate barrier orders inner shareable and not outer shareable,
which is ordered by `dma_mb`). This ordering does not need to be in between
read pointer read and write, because it's for ordering between the ring
buffer data and the pointer; the RMW operation does not internally need a
barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
so move it to before the RMW sequence for clarity.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 70674d2d0f77..9fe393da6b10 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -2,13 +2,7 @@
 
 mod continuation;
 
-use core::{
-    mem,
-    sync::atomic::{
-        fence,
-        Ordering, //
-    },
-};
+use core::mem;
 
 use kernel::{
     device,
@@ -30,6 +24,7 @@
         barrier::{
             dma_mb,
             Full,
+            Read,
             Write, //
         },
         Mutex, //
@@ -339,6 +334,9 @@ fn driver_write_area_size(&self) -> usize {
             (MSGQ_NUM_PAGES, tx)
         };
 
+        // ORDERING: LOAD->LOAD ordering needed to order `gsp_write_ptr` read before data read.
+        dma_mb(Read);
+
         // SAFETY:
         // - `data` was created from a valid pointer, and `rx` and `tx` are in the
         //   `0..MSGQ_NUM_PAGES` range per the invariants of `gsp_write_ptr` and `cpu_read_ptr`,
@@ -436,12 +434,11 @@ fn cpu_read_ptr(&self) -> u32 {
 
     // Informs the GSP that it can send `elem_count` new pages into the message queue.
     fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
+        // ORDERING: LOAD->STORE ordering needed to order `cpu_read_ptr` write after data read.
+        dma_mb(Full);
+
         let rx = io_project!(self.0, .cpuq.rx);
         let rptr = MsgqRxHeader::read_ptr(rx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
-
-        // Ensure read pointer is properly ordered.
-        fence(Ordering::SeqCst);
-
         MsgqRxHeader::set_read_ptr(rx, rptr)
     }
 

-- 
2.54.0

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@garyguo.net>
> ---
>  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;

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

[Eliot Courtney]

On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
> In the GSP->CPU messaging path, the code reads the write pointer from GSP,
> reads the response and advances the read pointer.
>
> A LOAD->LOAD ordering is required after the write pointer read and the data
> read. Add it as this is currently missing.
>
> A LOAD->STORE ordering is required after the data read and the advance of
> read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
> to `smp_mb(Full)`; this however does not order DMA operations (notably on
> ARM, the generate barrier orders inner shareable and not outer shareable,
> which is ordered by `dma_mb`). This ordering does not need to be in between
> read pointer read and write, because it's for ordering between the ring
> buffer data and the pointer; the RMW operation does not internally need a
> barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
> so move it to before the RMW sequence for clarity.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>

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

[Gary Guo]

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 <gary@garyguo.net>
>> ---
>>  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;

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

[Eliot Courtney]

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 <gary@garyguo.net>
>>> ---
>>>  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).

>
> 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;

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

[Gary Guo]

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 <gary@garyguo.net>
>>>> ---
>>>>  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

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

[Eliot Courtney]

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 <gary@garyguo.net>
>>>>> ---
>>>>>  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.