Re: [PATCH v4 07/10] gpu: nova-core: gsp: cmdq: split the transport part of the receive path

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

On Fri Oct 9, 2026 at 8:54 PM JST, Alexandre Courbot wrote:
> `receive_msg` currently handles both the transport and RPC layers.
> Introduce the `MessageElement` trait to define how messages are
> processed, independently of their type.
>
> Keep the transport layer in `wait_for_msg`, and introduce
> `consume_element`, a transport-level method which reads the message's
> contents using an implementation of the `MessageElement` trait before
> advancing the CPU read pointer past it. RPC messages are handled by the
> `RpcMessageElement` wrapper, and the RPC part of `receive_msg` is moved
> to its implementation of `MessageElement`. This sets things up for
> moving the RPC code into its own module, leaving the transport agnostic
> of the message type.
>
> As a result of the new abstraction layer, RPC messages whose size is
> unexpected are consumed instead of remaining on the queue.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 102 ++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index a059ce74ba2d..65e1a0b6bb44 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -199,6 +199,15 @@ fn write(&self, dev: &device::Device, seq: u32, dst: &mut GspCommand<'_>) -> Res
>      }
>  }
>  
> +/// Trait implemented by types that can be received as single command queue elements.
> +///
> +/// The command queue validates the element header before calling `read()` to interpret the
> +/// contents.
> +trait MessageElement: Sized {
> +    /// Tries to read `Self` from `message`. `dev` is the queue's device, to be used for logging.
> +    fn read(dev: &device::Device, message: GspMessage<'_>) -> Result<Self>;
> +}
> +
>  /// Trait representing messages received from the GSP.
>  ///
>  /// A reply that [`Cmdq::send_command`] waits for, or an event that [`Cmdq::await_msg`] waits for.
> @@ -224,6 +233,45 @@ fn read(
>      ) -> Result<Self, Self::InitError>;
>  }
>  
> +/// Wrapper type for receiving a RPC message from a command queue element.
> +///
> +/// [`MessageElement`] cannot be directly implemented for all [`MessageFromGsp`] with a blanket
> +/// implementation as it would conflict with other future message types.
> +struct RpcMessageElement<M>(M);
> +
> +impl<M> MessageElement for RpcMessageElement<M>
> +where
> +    M: MessageFromGsp,
> +    Error: From<M::InitError>,
> +{
> +    fn read(dev: &device::Device, message: GspMessage<'_>) -> Result<Self> {
> +        let rpc_message = RpcMessage::parse(dev, message)?;
> +        let function = rpc_message.header.function();
> +
> +        // An early return here would leave the read pointer on this message.

I think this comment is no longer accurate.

> +        let result = if matches!(function, Ok(f) if f == M::FUNCTION) {
> +            let (cmd, contents_1) =
> +                M::Message::from_bytes_prefix(rpc_message.contents.0).ok_or(EIO)?;
> +            let mut sbuffer = SBufferIter::new_reader([contents_1, rpc_message.contents.1]);
> +
> +            M::read(cmd, &mut sbuffer)
> +                .map(Self)
> +                .map_err(|e| e.into())
> +                .inspect(|_| {
> +                    if !sbuffer.is_empty() {
> +                        dev_warn!(dev, "GSP message {:?} has unprocessed data\n", M::FUNCTION);
> +                    }
> +                })
> +        } else {
> +            rpc_message.log(dev);
> +
> +            Err(ENOMSG)
> +        };
> +
> +        result
> +    }
> +}

nit: temporary `result` here is not necessary since you can just have
the if/else as the last expression. Clippy (only at 1.85.0) complains
about this, so might be worth fixing.

> +
>  /// Number of GSP pages making the [`Msgq`].
>  pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>  
> @@ -886,7 +934,7 @@ fn send_command<M>(&mut self, command: M) -> Result
>          }
>      }
>  
> -    /// Wait for a message to become available on the message queue.
> +    /// Wait for the next element to become available on the message queue.
>      ///
>      /// This works purely at the transport layer and does not interpret or validate the message
>      /// beyond the advertised length in its [`GspMsgElement`].
> @@ -933,53 +981,19 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>          })
>      }
>  
> -    /// Receive a message from the GSP.
> +    /// Wait for the next element on the message queue, pass it to [`MessageElement::read`], and
> +    /// advances the read pointer past it.
>      ///
> -    /// A message whose function code is `M::FUNCTION` is decoded and returned. Any other message
> -    /// is logged as an event.
> -    ///
> -    /// The read pointer is always advanced past the message, regardless of whether it matched.
> +    /// The read pointer advances regardless of whether [`MessageElement::read`] succeeds or not.
>      ///
>      /// # Errors
>      ///
> -    /// - `ETIMEDOUT` if `timeout` has elapsed before any message becomes available.
> -    /// - `EIO` if there was some inconsistency (e.g. message shorter than advertised) on the
> -    ///   message queue.
> -    /// - `ENOMSG` if the message was not the awaited reply.
> -    ///
> -    /// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
> -    fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
> -    where
> -        // This allows all error types, including `Infallible`, to be used for `M::InitError`.
> -        Error: From<M::InitError>,
> -    {
> +    /// Errors from [`Self::wait_for_msg`] and from [`MessageElement::read`] are propagated
> +    /// as-is.
> +    fn consume_element<M: MessageElement>(&mut self, timeout: Delta) -> Result<M> {

Ok, so we are changing the semantics to this always consumes the element
in a few more cases. I think this is ok and it's documented so sgtm. And
maybe we do something for poisoning later?

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>