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

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:

quoted

On Fri Sep 4, 2026 at 12:13 PM BST, Alexandre Courbot wrote:

quoted

On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
<...>

quoted

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

quoted

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

quoted

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.