Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing acquire and release memory barriers in the lock-free ring buffer implementation between the CPU and GSP firmware. -- commit 0033519811b496066fb6090d793690e7fd6d3d45 Author: Gary Guo <[email protected]> gpu: nova-core: use I/O projection for cleaner encapsulation This commit refactors PTE arrays and message queues to use io_project!. It replaces primitive dma_read! and dma_write! usage with io_read! and io_write! to restore proper encapsulation. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 070de0731e950..c34b489614962 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -438,12 +441,17 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) { > // > // - The returned value is within `0..MSGQ_NUM_PAGES`. > fn cpu_write_ptr(&self) -> u32 { > - super::fw::gsp_mem::cpu_write_ptr(&self.0) > + MsgqTxHeader::write_ptr(io_project!(self.0, .cpuq.tx)) % > MSGQ_NUM_PAGES > } > > // Informs the GSP that it can process `elem_count` new pages from the > command queue. > fn advance_cpu_write_ptr(&mut self, elem_count: u32) { > - super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count) > + 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); [Severity: High] This is a pre-existing issue, but does placing the fence after the write pointer update fail to order the preceding command payload writes? In advance_cpu_write_ptr() in drivers/gpu/nova-core/gsp/cmdq.rs, the driver updates the TX pointer via io_write!() (inside set_write_ptr) and then issues a fence(Ordering::SeqCst). Since io_write!() compiles to a plain write_volatile without an implicit barrier, placing the fence after the pointer update could allow the firmware to observe the updated write pointer before the command payload is fully visible. This could lead to garbage command processing on architectures with weak memory ordering. Should this fence be placed before the set_write_ptr() call instead? Additionally, looking at the wider lock-free ring buffer implementation, do wait_for_msg() and allocate_command() lack acquire memory barriers? If wait_for_msg() polls the GSP write pointer via read_volatile and proceeds to read the payload without an acquire barrier, the CPU might speculatively read the message payload before the pointer read completes. > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
