[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.

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

[Gary Guo]

On Tue Aug 25, 2026 at 1:38 AM BST, Eliot Courtney wrote:
> 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:
>>>>>> @@ -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.

Frankly I don't like the asymmetry that the advancing code does the barrier,
while the pointer reading code doesn't have the barrier. However, if we move the
barrier to the pointer read function, then the `driver_write_area_size` would
gain a unnecessary barrier. (Actually, `driver_read_area` code have a similar
issue, a failed pool would execute an unnecessary barrier).

As an alternative to move the barrier into the advancing code, alternatively we
can pull the `message.header.length()` to a separate line instead.

I think we should either always have barrier inside the pointer read/update
code, or always on the user side. Given the former would mean unnecessary
barriers, I am erring on the latter.

Best,
Gary

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

[Eliot Courtney]

On Tue Sep 1, 2026 at 2:25 AM JST, Gary Guo wrote:
> On Tue Aug 25, 2026 at 1:38 AM BST, Eliot Courtney wrote:
>> 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:
>>>>>>> @@ -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.
>
> Frankly I don't like the asymmetry that the advancing code does the barrier,
> while the pointer reading code doesn't have the barrier. However, if we move the
> barrier to the pointer read function, then the `driver_write_area_size` would
> gain a unnecessary barrier. (Actually, `driver_read_area` code have a similar
> issue, a failed pool would execute an unnecessary barrier).
>
> As an alternative to move the barrier into the advancing code, alternatively we
> can pull the `message.header.length()` to a separate line instead.
>
> I think we should either always have barrier inside the pointer read/update
> code, or always on the user side. Given the former would mean unnecessary
> barriers, I am erring on the latter.
>
> Best,
> Gary

I see - so you're saying that one side of the maximally consistent
position is to put the memory barriers in additionally `gsp_read_ptr`
and `gsp_write_ptr`, but those don't always need a barrier e.g.
driver_write_area_size because they are not necessarily followed by an
access that needs ordering, and the alternative is to have callers of
those functions handle that responsibility.

I think the differrence is that `advance_cpu_read_ptr` and
`advance_cpu_write_ptr` definitely need barriers, so why push up that
one level? Having barriers in `advance_cpu_read_ptr`,
`advance_cpu_write_ptr`, `driver_read_area`, and `driver_write_area` is
sufficiently consistent since it's the deepest set of functions that
can't avoid memory barriers.

If we want to remove unnecessary memory barriers on polling
`driver_read_area`, we could add an analogous `driver_read_area_size` or
move the memory barrier for driver_read_area up one level. The poll is
only once every millisecond though, so I doubt it makes a difference for
performance. But if we were to change it imo `driver_read_area_size` is
most consistent.

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

[Gary Guo]

On Tue Sep 1, 2026 at 3:47 AM BST, Eliot Courtney wrote:
> On Tue Sep 1, 2026 at 2:25 AM JST, Gary Guo wrote:
>> On Tue Aug 25, 2026 at 1:38 AM BST, Eliot Courtney wrote:
>>> 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:
>>>>>>>> @@ -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.
>>
>> Frankly I don't like the asymmetry that the advancing code does the barrier,
>> while the pointer reading code doesn't have the barrier. However, if we move the
>> barrier to the pointer read function, then the `driver_write_area_size` would
>> gain a unnecessary barrier. (Actually, `driver_read_area` code have a similar
>> issue, a failed pool would execute an unnecessary barrier).
>>
>> As an alternative to move the barrier into the advancing code, alternatively we
>> can pull the `message.header.length()` to a separate line instead.
>>
>> I think we should either always have barrier inside the pointer read/update
>> code, or always on the user side. Given the former would mean unnecessary
>> barriers, I am erring on the latter.
>>
>> Best,
>> Gary
>
> I see - so you're saying that one side of the maximally consistent
> position is to put the memory barriers in additionally `gsp_read_ptr`
> and `gsp_write_ptr`, but those don't always need a barrier e.g.
> driver_write_area_size because they are not necessarily followed by an
> access that needs ordering, and the alternative is to have callers of
> those functions handle that responsibility.
>
> I think the differrence is that `advance_cpu_read_ptr` and
> `advance_cpu_write_ptr` definitely need barriers, so why push up that
> one level? Having barriers in `advance_cpu_read_ptr`,
> `advance_cpu_write_ptr`, `driver_read_area`, and `driver_write_area` is
> sufficiently consistent since it's the deepest set of functions that
> can't avoid memory barriers.

To me conceptually it's best to place memory barriers in places in between two
operations so it's very clear that it provides ordering between two operations.
Hiding memory barrier inside a plain access can be confusing.

Alternatively, we can specifiy that the pointer updater have release semantics
and the pointer reader have acquire semantics. This way it's also very clear,
but does introduce unneeded barrier for the polling case as I mentioned. That
said, given that the polling interval is once a millisecond, extra barrier is
okay (full barrier is ~100ns). I do wish we have acquire/release barriers for
these cases, which would be quite much cheaper!

So unless there's an objection, I'll take the second approach by moving barrrier
to pointer accessors/updaters and mark these methods to have acq/rel semantics.

Best,
Gary

>
> If we want to remove unnecessary memory barriers on polling
> `driver_read_area`, we could add an analogous `driver_read_area_size` or
> move the memory barrier for driver_read_area up one level. The poll is
> only once every millisecond though, so I doubt it makes a difference for
> performance. But if we were to change it imo `driver_read_area_size` is
> most consistent.

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

[Danilo Krummrich]

On 9/1/26 1:43 PM, Gary Guo wrote:
> So unless there's an objection, I'll take the second approach by moving barrrier
> to pointer accessors/updaters and mark these methods to have acq/rel semantics.
Much in favor of that.

Thanks,
Danilo