Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing memory barrier before write pointer update allows GSP to read stale command data. - [High] Unrelated asynchronous messages are silently dropped while waiting for a command reply or initialization event. -- --- Patch [1]: [PATCH] gpu: nova-core: fix packed registry table size --- commit 7a27da8622da09265a549e78737e3707931209c2 Author: Alexandre Courbot <[email protected]> gpu: nova-core: fix packed registry table size PACKED_REGISTRY_TABLE::size describes the entire table, including its fixed-size header. SetRegistry currently initializes it with only the variable payload length, omitting the 8 bytes header. Fix this by using CommandToGsp::size to obtain the actual command size, including its header. Fixes: 19b0a6e7c2be ("gpu: nova-core: gsp: Add SetRegistry command") Reported-by: Sashiko <[email protected]> Closes: https://lore.kernel.org/r/[email protected] Signed-off-by: Alexandre Courbot <[email protected]> > diff --git a/drivers/gpu/nova-core/gsp/commands.rs > b/drivers/gpu/nova-core/gsp/commands.rs > index 08380de39048a..0f8bd77acddf7 100644 > --- a/drivers/gpu/nova-core/gsp/commands.rs > +++ b/drivers/gpu/nova-core/gsp/commands.rs > @@ -115,10 +115,7 @@ impl CommandToGsp for SetRegistry { > type InitError = Infallible; > > fn init(&self) -> impl Init<Self::Command, Self::InitError> { > - Self::Command::init( > - self.entries.len() as u32, > - self.variable_payload_len() as u32, > - ) > + Self::Command::init(self.entries.len() as u32, self.size() as u32) > } > > fn variable_payload_len(&self) -> usize { [Severity: High] This isn't a bug introduced by this patch, but looking at advance_cpu_write_ptr() in drivers/gpu/nova-core/gsp/cmdq.rs: drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_write_ptr() { MsgqTxHeader::set_write_ptr(tx, wptr); // Ensure all command data is visible before triggering the GSP read. fence(Ordering::SeqCst); } Does the fence(Ordering::SeqCst) need to be placed before set_write_ptr()? Since it is executed after the write pointer update, could a weakly ordered architecture reorder the payload stores to become globally visible after the pointer update, allowing the GSP to read stale or zeroed command data? [Severity: High] This is a pre-existing issue, but in drivers/gpu/nova-core/gsp/cmdq.rs, receive_msg() unconditionally advances the read pointer even when returning ERANGE for an unmatched message: drivers/gpu/nova-core/gsp/cmdq.rs:receive_msg() { ... } else { Err(ERANGE) }; // Advance the read pointer past this message. self.gsp_mem.advance_cpu_read_ptr(u32::try_from( message.header.length().div_ceil(GSP_PAGE_SIZE), )?); result } When this ERANGE is caught in send_command() or wait_gsp_init_done(), the loop simply continues: drivers/gpu/nova-core/gsp/cmdq.rs:send_command() { loop { match inner.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) { Ok(reply) => break Ok(reply), Err(ERANGE) => continue, Err(e) => break Err(e), } } } Will this permanently discard unrelated asynchronous messages (like GPU thermal alerts or hardware faults) arriving from the GSP during this wait, rather than processing or queuing them? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
