On Tue Oct 20, 2026 at 7:18 PM JST, sashiko-bot wrote: > 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))?
Mmm, that's an interesting one, in that I am not sure there are any concrete implications but I'd like to fix it anyway. I was thinking that maybe we should add a special-case to `read_poll_timeout` so it returns unconditionally after checking the condition once if `timeout_delta == Delta::ZERO`, but doing so would make the `sleep_delta` argument moot so I guess that's not adequate (and the current behavior is technically correct). So I think I'll special-case nova-core's `wait_for_msg` instead.
