Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The safely-read `put` pointer is discarded and overwritten by an unsynchronized `put` pointer during the bulk log data copy, undermining the preceding DMA barrier. --
commit f151b2c49feb508fe7910dc5dccc6fabd8696fdb Author: Vladislav Zaharov <[email protected]> gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind This commit adds a gsp_keep_logs module parameter to retain GSP-RM log buffers in memory and debugfs after the GPU is unbound. This is useful for debugging probe failures or timeouts. It also moves the log buffer code into its own file. [ ... ] > diff --git a/drivers/gpu/nova-core/gsp/logbuffer.rs > b/drivers/gpu/nova-core/gsp/logbuffer.rs > new file mode 100644 > index 0000000000000..22c47f1991698 > --- /dev/null > +++ b/drivers/gpu/nova-core/gsp/logbuffer.rs [ ... ] > + fn snapshot(&self) -> Result<VVec<u8>> { > + // Offset 0 holds the "put" pointer, which the GSP advances as it > appends entries. It is > + // still zero if nothing was ever logged, which is all that is > tested here: a buffer that > + // was written to is copied whole, and making sense of "put" is left > to the decoder. > + let put = io_project!(self.0, [build: > ..size_of::<u64>()]).try_cast::<u64>()?; > + if put.read_val() == 0 { > + return Ok(VVec::new()); > + } > + > + // ORDERING: LOAD->LOAD ordering needed to order the "put" read > before the data read. The > + // GSP has normally been stopped by the time this runs, but a boot > that timed out can leave > + // it still appending. > + dma_mb(Read); > + > + let mut snapshot = VVec::zeroed(LOG_BUFFER_SIZE, GFP_KERNEL)?; > + io_project!(self.0, [build: ..]).copy_to_slice(&mut snapshot); [Severity: Medium] Does the bulk copy overwrite the synchronized put pointer with an unsafe read? The code reads the put pointer and correctly issues a DMA read barrier. However, the copy_to_slice operation reads the entire buffer starting from offset 0, which includes the put pointer. During a hardware timeout where the GSP is still actively appending to the log, could this bulk copy capture a newer put pointer that gets paired with stale log data read earlier in the copy sequence? If this happens, a userspace parser reading the snapshot might read past the actually valid data and parse garbage, defeating the purpose of the barrier. Could the copy_to_slice operation be adjusted to only copy the data region, or should the safely-read put pointer be explicitly written into the snapshot after the bulk copy completes? > + > + Ok(snapshot) > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
