Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `dma_mb(Read)` memory barrier does not protect against data races because the snapshot relies on the `put` pointer re-read by the unordered `copy_to_slice()`. --
commit cb68e0aae9b03cb04a10d2043892acfc8a1fe581 Author: Vladislav Zaharov <[email protected]> gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind Adds a module parameter to retain GSP-RM log buffers after unbind, making them available through debugfs. This copies the contents into module-owned memory since the original buffers are DMA allocations that do not outlive the device. > diff --git a/drivers/gpu/nova-core/gsp/logbuffer.rs > b/drivers/gpu/nova-core/gsp/logbuffer.rs > new file mode 100644 > index 0000000000000..890aa2f9e38e7 > --- /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 unordered copy_to_slice() at the end of this function negate the effect of dma_mb(Read)? If a boot times out, the GSP may still be appending logs when LogBuffer::snapshot() is called. The safely ordered "put" value is dropped here, and the snapshot relies on the "put" pointer being re-read by the unordered copy_to_slice() operation. This means copy_to_slice() can read the log payload before reading the "put" pointer at offset 0. If the GSP concurrently writes new logs and updates the "put" pointer, the snapshot could capture the updated "put" pointer but stale log payload data, causing the user-space decoder to parse garbage data. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
