Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The memory barrier in `advance_cpu_write_ptr` is incorrectly placed after the write pointer update, potentially allowing the GSP to read stale command data. -- commit 6cc25fbdd45cd6420a6e1ed539f7745066511455 Author: Gary Guo <[email protected]> Use io_project! for PTE array and message queues to restore the proper encapsulation. This commit migrates the nova-core GSP driver to use the io_project!, io_read!, and io_write! macros for accessing the PTE array and message queues. The previous dma_read! and dma_write! macros are replaced to properly encapsulate IO memory operations on primitives. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 070de0731e95..c34b48961496 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 is the memory barrier placed incorrectly here? Because fence(Ordering::SeqCst) happens after MsgqTxHeader::set_write_ptr() updates the write pointer, could the CPU reorder the preceding command data writes with the write pointer update itself? If the updated write pointer becomes visible to the GSP before the actual command data is fully written to memory, it seems the GSP might read uninitialized or stale commands. Should the fence be moved to immediately before set_write_ptr() to prevent this reordering, similar to how the read pointer is handled in advance_cpu_read_ptr()? > } > } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
