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

[sashiko-bot@kernel.org]

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))?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-cmdq-rpc-v5-0-dceeb8f360ec@nvidia.com?part=11