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