Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing DMA write memory barrier before updating the GSP command queue write pointer. - [High] Missing DMA read memory barrier after reading the GSP message queue write pointer. -- commit 899e3ba6af7f4cc7e2c28b0f4f3f73db18de1f16 Author: Alexandre Courbot <[email protected]> gpu: nova-core: use FromSafeCast wherever possible Replaces various *_as_* casting functions with FromSafeCast in non-const contexts to simplify conversions. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index f85fde09aa6e3..658d0a9b2cfb2 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -343,10 +346,10 @@ fn driver_write_area_size(&self) -> usize { > unsafe { > ( > core::slice::from_raw_parts( > - data.add(casts::u32_as_usize(rx)), > - casts::u32_as_usize(tail_end - rx), > + data.add(usize::from_safe_cast(rx)), > + usize::from_safe_cast(tail_end - rx), > ), > - core::slice::from_raw_parts(data, > casts::u32_as_usize(wrap_end)), > + core::slice::from_raw_parts(data, > usize::from_safe_cast(wrap_end)), > ) > } > } [Severity: High] This isn't a bug introduced by this patch, but while reviewing this file I noticed a potential memory barrier issue in advance_cpu_write_ptr(): > fn advance_cpu_write_ptr(&mut self, elem_count: u32) { > 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); > } Because there is no memory barrier between the payload writes (done prior to calling this function) and the write pointer volatile write in MsgqTxHeader::set_write_ptr(), the CPU can reorder them. If the GSP observes the updated write pointer before the payload writes are globally visible, could it read stale command data? [Severity: High] This is also a pre-existing issue, but is there a missing read memory barrier after reading the message queue write pointer in wait_for_msg()? > fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> { > // Wait for a message to arrive from the GSP. > let (slice_1, slice_2) = read_poll_timeout( > || Ok(self.gsp_mem.driver_read_area()), > |driver_area| !driver_area.0.is_empty(), > Delta::from_millis(1), > timeout, > ) > .map(|(slice_1, slice_2)| (slice_1.as_flattened(), > slice_2.as_flattened()))?; > > // Extract the `GspMsgElement`. > let (header, slice_1) = > GspMsgElement::from_bytes_prefix(slice_1).ok_or(EIO)?; When polling for a new message, read_poll_timeout repeatedly evaluates driver_read_area(), which performs a volatile read of the GSP write pointer. Once a new message is indicated, the payload memory slices are immediately accessed. Since there is no read memory barrier between the volatile read of the write pointer and the payload access, could modern CPUs speculatively execute the data loads before the write pointer read completes, causing the driver to read stale payload data? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
