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

[Alexandre Courbot]

On Thu Oct 1, 2026 at 1:48 PM JST, Eliot Courtney wrote:
<...>
>> +/// 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?

Done and fixed, thanks!

>
> [...]
>> -    /// 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.

The rebase on top of the interrupt series (which changed the semantics
on the receive end) should make the next revision less impactful on that
front, although messages are consumed even if they have an unexpected
size because the `RpcMessage::parse` call is now performed inside the
`MessageElement::read` implementation, so the transport layer cannot
tell the difference between an invalid message length and other kinds of
errors.

I would not worry about this detail though, because 1. r000 will put the
message size into the transport layer, removing that issue entirely, and
2. the correct thing to do (which John tackled in his r000 series) is to
mark the command queue as invalid by e.g. setting a poisoned flag and
making further uses of the queue return an error, as such mismatch would
be a firmware bug and thus not a recoverable situation.