Re: [PATCH v3 10/14] gpu: nova-core: bound a GSP wait by a single deadline

[Alexandre Courbot]

On Fri Sep 4, 2026 at 8:26 PM JST, Gary Guo wrote:
> On Fri Sep 4, 2026 at 12:13 PM BST, Alexandre Courbot wrote:
>> On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
>> <...>
>>> @@ -611,15 +621,39 @@ pub(crate) fn send_command_no_wait<M>(&self, bar: Bar0<'_>, command: M) -> Resul
>>>          self.inner.lock().send_command(bar, command)
>>>      }
>>>  
>>> -    /// Receive a message from the GSP.
>>> +    /// Waits for an unsolicited GSP event of type `M`, logging any other event that arrives
>>> +    /// first.
>>> +    ///
>>> +    /// The queue is locked for the whole wait, for up to [`Self::RECEIVE_TIMEOUT`], so a
>>> +    /// concurrent command cannot consume the awaited event.
>>>      ///
>>> -    /// See [`CmdqInner::receive_msg`] for details.
>>> -    pub(crate) fn receive_msg<M: MessageFromGsp>(&self, timeout: Delta) -> Result<M>
>>> +    /// # Errors
>>> +    ///
>>> +    /// - `ETIMEDOUT` if the event does not arrive within [`Self::RECEIVE_TIMEOUT`] of the call,
>>> +    ///   however many other events arrive while waiting.
>>> +    /// - `EIO` if the queue is poisoned or a message fails framing or checksum validation (see
>>> +    ///   [`CmdqInner::wait_for_msg`]).
>>
>> Let's not mention private methods in public documentation.
>>
>>> +    ///
>>> +    /// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
>>> +    pub(crate) fn await_msg<M: MessageFromGsp>(&self) -> Result<M>
>>>      where
>>>          // This allows all error types, including `Infallible`, to be used for `M::InitError`.
>>>          Error: From<M::InitError>,
>>>      {
>>> -        self.inner.lock().receive_msg(timeout)
>>> +        let mut inner = self.inner.lock();
>>> +
>>> +        let deadline = Instant::<Monotonic>::now() + Self::RECEIVE_TIMEOUT;
>>> +        loop {
>>> +            let remaining = deadline - Instant::<Monotonic>::now();
>>> +            if remaining.is_negative() {
>>> +                break Err(ETIMEDOUT);
>>> +            }
>>> +            match inner.receive_msg::<M>(remaining) {
>>> +                Ok(msg) => break Ok(msg),
>>> +                Err(ERANGE) => continue,
>>> +                Err(e) => break Err(e),
>>> +            }
>>> +        }
>>
>> This block and the one from `send_command` are strictly identical - we
>> should factor them out.
>>
>> The right place for this seems to be a new method in `CmdqInner`:
>>
>>     fn await_msg<M: MessageFromGsp>(&mut self) -> Result<M> ...
>>
>> Then this `await_msg` simply becomes:
>>
>>     self.inner.lock().await_msg()
>>
>> While `send_command` is simplified to:
>>
>>     let mut inner = self.inner.lock();
>>     inner.send_command(bar, command)?;
>>     inner.await_msg()
>
> Unless I misunderstand the GSP code, the unmatched message is not discarded, but
> rather the caller returns from inner code, and drops the lock so other waiters
> of GSP message can have a chance to take the inner lock and receive the message
> so then get the non-matched message out of the way. So your suggestion would
> cause `await_msg` to never complete in such cases?
>
> If my understanding of this is correct, then this code should just be moved to
> the outer `receive_msg`, because all callers of it have the same loop and I
> think it's a wanted behaviour anyway.

I don't really understand what you mean here. There is no concept of
other waiters at the moment, and `receive_msg` unconditionally advances
the read pointer. In effect, the queue is working in a synchronous
manner (which is the design of the queue itself, not a Nova limitation)
so there can be only one expected reply after a message has been
successfully sent.

I think once we move to the newer firmware we will want to add more
sophisticated message dispatchers, but for now this simple
implementation does what we need.

As for my comment, please check with the code - it's really about
factoring out a block of code without any runtime side-effect.

>
> BTW, ERANGE is a very bad error code to mean "the message had a recognized but
> non-matching function code".

Maybe we can change this to `ENOMSG`. This will need to be its own patch
though.