On Thu Mar 5, 2026 at 4:44 PM JST, Eliot Courtney wrote:
> 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!
Follow-up is fine, especially if the fix ends up being a bit more
involved than I first expected.
