Re: [PATCH v3 09/14] gpu: nova-core: recover the GSP receive path from corrupt framing

[Alexandre Courbot]

On Fri Sep 4, 2026 at 8:17 PM JST, Gary Guo wrote:
> On Fri Sep 4, 2026 at 11:53 AM BST, Alexandre Courbot wrote:
>> On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
>> <...>
>>> @@ -838,23 +871,26 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
>>>          let function = message.header.function();
>>>          let seq = message.header.sequence();
>>>  
>>> -        // Bind the result rather than returning early. The read pointer must advance past this
>>> -        // message on every path.
>>> +        // Every path must advance the read pointer past this message, including a failed decode.
>>>          let result = if matches!(function, Ok(f) if f == 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",
>>> -                            M::FUNCTION
>>> -                        );
>>> -                    }
>>> -                })
>>> +            match M::Message::from_bytes_prefix(message.contents.0) {
>>> +                Some((cmd, contents_1)) => {
>>> +                    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",
>>> +                                    M::FUNCTION
>>> +                                );
>>> +                            }
>>> +                        })
>>> +                }
>>> +                None => Err(EIO),
>>
>> This error path is the only one without a warning. How about:
>>
>>     None => Err(EIO)
>>         .inspect_err(|_| dev_warn!(&self.dev, "GSP message {:?} too short\n", M::FUNCTION)),
>
> I don't see why we want to use `inspect_err` here (just to make it an oneliner?)
>
> Please do
>
>     None => {
>         dev_warn!(&self.dev, "GSP message {:?} too short\n", M::FUNCTION);
>         Err(EIO)
>     }
>
> Instead.

Yeah looking again at my suggestion it seems a bit nonsensical to use
`inspect_err` from an environment where it will trigger unconditionally.
Shorter is not always better. Let's use your version.