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
