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

["Eliot Courtney" <ecourtney@nvidia.com>]

On Tue Oct 20, 2026 at 11:27 PM JST, Alexandre Courbot wrote:
> On Tue Oct 20, 2026 at 7:18 PM JST, sashiko-bot wrote:

[...]

>> 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.

Personally I don't mind the special case in `read_poll_timeout`.
If you say, I want the timeout to be zero and then it sleeps, that
sounds like wonky behaviour to me. I think it feels weird to have
this special case w.r.t. `sleep_delta` only because we are using
constant values and that's a common use case. TBH I think it would
even be fine to change `start.elapsed() > timeout_delta` to
`start.elapsed() >= timeout_delta`, then it's not even really a
special case. I could try sending a patch for this.

I'm also fine with special casing `wait_for_msg`, but personally
(maybe) sleeping when you set timeout to Delta::ZERO sounds busted to
me.