Re: [PATCH v3 3/5] gpu: nova-core: gsp: add reply/no-reply info to `CommandToGsp`

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

On Thu Mar 5, 2026 at 11:10 AM JST, Alexandre Courbot wrote:
> On Thu Mar 5, 2026 at 10:34 AM JST, Eliot Courtney wrote:
>> On Wed Mar 4, 2026 at 8:56 PM JST, Alexandre Courbot wrote:
>>>> +    /// Sends `command` to the GSP and waits for the reply.
>>>> +    ///
>>>> +    /// # Errors
>>>> +    ///
>>>> +    /// - `ETIMEDOUT` if space does not become available to send the command, or if the reply is
>>>> +    ///   not received within the timeout.
>>>> +    /// - `EIO` if the variable payload requested by the command has not been entirely
>>>> +    ///   written to by its [`CommandToGsp::init_variable_payload`] method.
>>>> +    ///
>>>> +    /// Error codes returned by the command and reply initializers are propagated as-is.
>>>> +    pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result<M::Reply>
>>>> +    where
>>>> +        M: CommandToGsp,
>>>> +        M::Reply: MessageFromGsp,
>>>> +        Error: From<M::InitError>,
>>>> +        Error: From<<M::Reply as MessageFromGsp>::InitError>,
>>>> +    {
>>>> +        self.send_command_internal(bar, command)?;
>>>> +
>>>> +        loop {
>>>> +            match self.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) {
>>>> +                Ok(reply) => break Ok(reply),
>>>> +                Err(ERANGE) => continue,
>>>> +                Err(e) => break Err(e),
>>>> +            }
>>>> +        }
>>>
>>> There is an opportunity for factorize some more code here.
>>>
>>> Notice how the other callers of `receive_msg` (`wait_gsp_init_done` and
>>> `GspSequencer::run`) both use the same kind of loop, down to the same
>>> error handling. We could move that loop logic here and do it in a single
>>> place.
>>>
>>> In the future, we will probably want to add handlers for
>>> unexpected messages from the GSP and it will be easier if we receive all
>>> messages from a single place.
>>>
>>> This can be a separate patch from this one, but I think it makes sense
>>> to have that in this series.
>>>
>>> I expect the last patch to change a bit as a consequence of that - maybe
>>> we will need a `receive_msg_loop` or something in `CmdqInner`.
>>
>> I agree we should migrate all callers and make Cmdq responsible for
>> draining / handling spontaneous messages from the GSP, but I was
>> planning on doing it in a separate patch series until now. I can put it
>> into this one though if you want though no worries.
>
> If it ends up being convulated, let's do that afterwards but since it
> looks like a quick and easy win I thought it would make sense to have it
> here. Your call though.

Another consideration is that if the GSP has some issue, it could cause
this receive loop to run forever. I'm not sure if that practically can
happen or if we want to guard against it, but personally I think we
should and taking care of:

1. Some loop level timeout
2. Future considerations for how to handle spontaneous messages /
   draining
3. Migrating the callers

Seems large enough to do as a follow up series to me. But LMK if you
feel otherwise. Thanks!