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

[Gary Guo]

On Fri Sep 4, 2026 at 2:32 PM BST, Alexandre Courbot wrote:
> 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.

I did misread the `receive_msg` code. I read `Err(ERANGE)` as `Err(ERANGE)?` so
I thought the pointer was no incremented in such case.

So everything makes sense to me now.

Best,
Gary

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