Re: [PATCH v5 11/14] gpu: nova-core: gsp: cmdq: use consume_element to implement drain

["Alexandre Courbot" <acourbot@nvidia.com>]

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 <acourbot@nvidia.com>
>
> 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.