Re: [PATCH 3/3] gpu: nova-core: fix wrong use of barriers in GSP code

On Thu Apr 2, 2026 at 10:56 PM BST, Joel Fernandes wrote:
> Hi Gary,
>
> On 4/2/2026 11:24 AM, Gary Guo wrote:
>> From: Gary Guo <[email protected]>
>>=20
>> Currently, in the GSP->CPU messaging path, the current code misses a rea=
d
>> barrier before data read. The barrier after read is updated to a DMA
>> barrier (with release ordering desired), instead of the existing (Rust)
>> SeqCst SMP barrier; the location of barrier is also moved to the beginni=
ng
>> of function, because the barrier is needed to synchronizing between data
>> and ring-buffer pointer, the RMW operation does not internally need a
>> barrier (nor it has to be atomic, as CPU pointers are updated by CPU onl=
y).
>>=20
>> In the CPU->GSP messaging path, the current code misses a write barrier
>> after data write and before updating the CPU write pointer. Barrier is n=
ot
>> needed before data write due to control dependency, this fact is documen=
ted
>> explicitly. This could be replaced with an acquire barrier if needed.
>>=20
>> Signed-off-by: Gary Guo <[email protected]>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 19 +++++++++++++++++++
>>  drivers/gpu/nova-core/gsp/fw.rs   | 12 ------------
>>  2 files changed, 19 insertions(+), 12 deletions(-)
>>=20
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/g=
sp/cmdq.rs
>> index 2224896ccc89..7e4315b13984 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -19,6 +19,12 @@
>>      prelude::*,
>>      sync::{
>>          aref::ARef,
>> +        barrier::{
>> +            dma_mb,
>> +            Read,
>> +            Release,
>> +            Write, //
>> +        },
>>          Mutex, //
>>      },
>>      time::Delta,
>> @@ -258,6 +264,9 @@ fn new(dev: &device::Device<device::Bound>) -> Resul=
t<Self> {
>>          let tx =3D self.cpu_write_ptr() as usize;
>>          let rx =3D self.gsp_read_ptr() as usize;
>> =20
>> +        // ORDERING: control dependency provides necessary LOAD->STORE =
ordering.
>> +        // `dma_mb(Acquire)` may be used here if we don't want to rely =
on control dependency.
>
> Just checking, does control dependency on CPU side really apply to orderi=
ng for
> IO (what the device perceives?). IOW, the loads are stores might be order=
ed on
> the CPU side, but the device might be seeing these operations out of orde=
r. If
> that is the case, perhaps the control dependency comment is misleading.

Given that CPU cannot speculate store, I don't see how dependency ordering =
can
be broken even if the other side is a bus-mastering device.

For this to be broken, the device would be able to see the dependency-order=
ed
STORE to be propagated to it before it does it own STORE that propagates to=
 the
CPU to trigger the CPU STORE in the first place? That'll just break casuali=
ty.

I'll say that control dependency is sufficient here. I'm not worried that t=
he
compiler will break this particular control dependency given that if
`gsp_read_ptr =3D=3D cpu_write_ptr` will imply command allocation failure a=
nd this
condition cannot possibly be optimized out by the compiler.

Best,
Gary

>
>
>> +
>>          // SAFETY:
>>          // - We will only access the driver-owned part of the shared me=
mory.
>>          // - Per the safety statement of the function, no concurrent ac=
cess will be performed.
>> @@ -311,6 +320,9 @@ fn driver_write_area_size(&self) -> usize {
>>          let tx =3D self.gsp_write_ptr() as usize;
>>          let rx =3D self.cpu_read_ptr() as usize;
>> =20
>> +        // ORDERING: Ensure data load is ordered after load of GSP writ=
e pointer.
>> +        dma_mb(Read);
>> +
>
> I suggest taking it on a case by case basis, and splitting the patch for =
each
> case, for easier review. There are many patterns AFAICS, load-store, stor=
e-store
> etc.
>
> I do acknowledge the issue you find here though. thanks,

This is pretty much just a common MP pattern, where there's even an example=
 in
Documentation/core-api/circular-buffers.rst (except we're using dma barrier=
s
here as we're talking to devices rather than another SMP CPU), so I think
there's no real need of breaking this into small parts.

(That documentation makes use of smp_load_acquire/smp_store_release which i=
s
another reason I want to at least document the barrrier to be of ACQUIRE/RE=
LEASE
ordering).

Perhaps I can move the removal of redundant barrier after write pointer upd=
ate to
another patch.

Best,
Gary