Re: [PATCH v3 2/3] gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind

[sashiko-bot@kernel.org]

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 <vladazaharova2018@gmail.com>

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/20260912071842.622696-1-vladazaharova2018@gmail.com?part=2