Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] gpu: nova-core: unintended 1ms sleep in CmdqInner::drain() on empty queue --
commit c3321749b8f493965661eed66b6e6bf889cebd66 Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: cmdq: use consume_element to implement drain This commit refactors CmdqInner::drain() to use consume_element() and adds a log() helper method to GspParsedMessage. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 7db3d28e8902c..54e87fd7158cd 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -1054,21 +1063,14 @@ fn await_msg<M: MessageFromGsp>(&mut self) -> > Result<M> > /// `EIO` if a message fails framing or checksum validation, or if a > message's page count > /// overflows a `u32`. > fn drain(&mut self) -> Result { > - while !self.gsp_mem.driver_read_area().0.is_empty() { > - // A message is available, so this returns without waiting. > - let msg = self.wait_for_msg(Delta::ZERO)?; > - > - let pages = > - > u32::try_from(msg.header.msg_length().div_ceil(GSP_PAGE_SIZE)).map_err(|_| { > - dev_err!(&self.dev, "GSP drain: message length > overflow\n"); > - EIO > - })?; > - > - RpcMessage::parse(self.dev, msg)?.log(self.dev); > - > - self.gsp_mem.advance_cpu_read_ptr(pages); > + loop { > + match self.consume_element(Delta::ZERO, |message| > message.log(self.dev)) { > + Ok(()) => (), > + // No more message pending on the queue, drain is complete. > + Err(ETIMEDOUT) => return Ok(()), > + // Other errors are fatal, and stall the queue. > + Err(e) => return Err(e), > + } > } > - > - Ok(()) > } > } [Severity: Medium] Can removing the !self.gsp_mem.driver_read_area().0.is_empty() check cause CmdqInner::drain() to sleep for 1 ms when the queue is empty? Previously, CmdqInner::drain() only called wait_for_msg(Delta::ZERO) after confirming a message was available in driver_read_area(). With the new loop, consume_element(Delta::ZERO, ...) is called until the queue is empty, which invokes wait_for_msg(Delta::ZERO): drivers/gpu/nova-core/gsp/cmdq.rs:CmdqInner::wait_for_msg() 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, ) In rust/kernel/io/poll.rs:read_poll_timeout(), when the condition is false, the timeout check uses a strict greater-than comparison: if start.elapsed() > timeout_delta { // Unlike the C version, we immediately return. // We have just called `op()` so we don't need to call it again. return Err(ETIMEDOUT); } if !sleep_delta.is_zero() { fsleep(sleep_delta); } Because op() only reads two u32 indices from coherent memory in RAM, if the monotonic clock has not advanced by at least 1 ns between Instant::now() and start.elapsed() (for instance on architectures with coarser timer resolution or under virtualization), start.elapsed() is Delta::ZERO. Since Delta::ZERO > Delta::ZERO is false, read_poll_timeout() falls through to fsleep(Delta::from_millis(1)) while holding the Cmdq mutex (in paths like GspInterrupt::handle_threaded() -> Cmdq::drain() and Gpu::new() -> Cmdq::drain()). Would it make sense to keep the while !self.gsp_mem.driver_read_area().0.is_empty() loop condition around self.consume_element(Delta::ZERO, |message| message.log(self.dev))? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
