Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] gpu: nova-core: unordered put pointer in log buffer snapshot --
commit e3a18fe9cea338c661ee3d185d004b01a5ff3d7d 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 after GPU unbind or probe failure. It creates copies of the buffers in memory owned by the module and exposes them through debugfs, moving the buffer logic to a separate logbuffer.rs 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..b1f912fb3fae1 > --- /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] Since copy_to_slice() reads the entire buffer, including the put pointer at offset 0, and has no internal read ordering guarantees, can the read of the put pointer inside copy_to_slice() be reordered after the data reads? If the GSP is still appending data concurrently (e.g. after a boot timeout), the snapshot might capture a newer put pointer but stale log data. This could cause the log parser to read garbage or fail, defeating the purpose of the explicit dma_mb(Read) barrier. > + > + Ok(snapshot) > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
