Re: [PATCH v3 6/9] gpu: nova-core: gsp: cmdq: split the transport part of the receive path

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

On Wed Sep 30, 2026 at 11:55 PM JST, Alexandre Courbot wrote:
> `wait_for_msg` mixes two layers: the transport layer which polls the
> queue, extracts the element header and validates the checksum, and the
> RPC layer which reads the RPC header and trims the payload slices to the
> length advertised by the RPC header.
>
> Move the transport layer into `wait_for_element`, 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, and `parse_rpc_message`, which
> validates the RPC layer. This sets things up for moving the RPC code
> into its own module, leaving the transport agnostic of the message type.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 219 +++++++++++++++++++++-----------------
>  1 file changed, 119 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index b8a9e02b76fe..07036972dbec 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -195,6 +195,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 `element`. `dev` is the queue's device, to be used for logging.
> +    fn read(dev: &device::Device, element: GspMessage<'_>) -> Result<Self>;
> +}
> +
>  /// Trait representing messages received from the GSP.
>  ///
>  /// This trait tells [`Cmdq::receive_msg`] how it can receive a given type of message.
> @@ -218,6 +227,94 @@ 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> RpcMessageElement<M>
> +where
> +    M: MessageFromGsp,
> +{
> +    /// Validate the RPC layer of `element` and returns its RPC header and its contents trimmed down
> +    /// to the RPC payload.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EIO` if the element is shorter than the payload length advertised by the RPC header.
> +    fn parse_rpc_message<'a>(
> +        dev: &device::Device,
> +        element: GspMessage<'a>,
> +    ) -> Result<RpcMessage<'a>> {

This doesn't depend on the type M, so it could go on `RpcMessage`
instead.

Also, moving this here breaks some doclinks from other locations (e.g. `
This is the type returned by [`CmdqInner::parse_rpc_message`].`). Can
you fix please?

[...]
> -    /// Receive a message from the GSP.
> -    ///
> -    /// The expected message type is specified using the `M` generic parameter. If the pending
> -    /// message has a different function code, `ERANGE` is returned and the message is consumed.
> -    ///
> -    /// The read pointer is always advanced past the message, regardless of whether it matched.
> -    ///
> -    /// # 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.
> -    /// - `EINVAL` if the function code of the message was not recognized.
> -    /// - `ERANGE` if the message had a recognized but non-matching function code.
> -    ///
> -    /// 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>,
> -    {
> -        let message = self.wait_for_msg(timeout)?;
> -        let function = message.header.function().map_err(|_| EINVAL)?;
> -
> -        // Extract the message. Store the result as we want to advance the read pointer even in
> -        // case of failure.
> -        let result = if function == M::FUNCTION {
> -            let (cmd, contents_1) = M::Message::from_bytes_prefix(message.contents.0).ok_or(EIO)?;
> -            let mut sbuffer = SBufferIter::new_reader([contents_1, message.contents.1]);
> -
> -            M::read(cmd, &mut sbuffer)
> -                .map_err(|e| e.into())
> -                .inspect(|_| {
> -                    if !sbuffer.is_empty() {
> -                        dev_warn!(
> -                            &self.dev,
> -                            "GSP message {:?} has unprocessed data\n",
> -                            function
> -                        );
> -                    }
> -                })
> -        } else {
> -            Err(ERANGE)
> -        };
> -
> -        // Advance the read pointer past this message.
> -        self.gsp_mem.advance_cpu_read_ptr(u32::try_from(
> -            message.header.length().div_ceil(GSP_PAGE_SIZE),
> -        )?);
> +        self.gsp_mem.advance_cpu_read_ptr(elem_count);
>  
>          result

Previously, if we got an unknown function code or the message was too
short for MessageFromGsp::Message or the payload length is too big for
the remaining read area, it wouldn't consume the element, but now it
does. If we had corrupted data that happened to pass checksum, it could
mess up the queue (e.g. wrap the read pointer around in front of the
write pointer).

Since this nests transport, message layer (RPC here), and content layer,
it might be worth saying how each should be handled. Here's the previous
+ semantics with this patch:

Transport:
- Timeout, ETIMEDOUT -> no change
- Bad checksum, EIO, not consumed -> no change

Message:
- Unknown function code, EINVAL: message consumed in this patch

  If we get an unknown function code, we can't know if things are still
  in a valid state, so I think we should not consume the message and
  return an error here.

- Known but unexpected function code, ERANGE, consumed -> no change

  Think we have this since we don't have async GSP message handling
  implemented yet so we use this to drain the cmdq of misc messages.
  So all good here. N.B. we are implicitly relying on the discriminants
  in `MsgFunction` essentially being an allowlist for events we can
  drain, otherwise we hit the case above (in the code previous to this
  patch, at least).

- `slice_1.len() + slice_2.len() < payload_length` hits, EIO, consumed
  in this patch

  This will mess up the read pointer.

Content:
- Payload shorter than MessageFromGsp::Message, consumed in this patch

  This is another weird scenario that shouldn't happen. Arguably we
  shouldn't consume the message here, but this patch changes that
  behaviour.

- MessageFromGsp::read fails, consumed -> no change

  Not sure, but seems a bit weird to consume this here.

- Payload not fully read, warning+Ok -> no change

We could solve this with a custom error type for MessageElement, or
return Result<Result<Self>> -> the Result<Result<Self>> is arguable
since we are returning the result of the content layer.

Send path semantics look unaffected by this series to me.

>      }