Re: [PATCH 4/4] gpu: nova-core: gsp: cmdq: add the GMC receive path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpu: nova-core: gsp: type mismatch in CmdqInner::send_gmc_command()
- [Medium] gpu: nova-core: gsp: unclaimed replies passed to on_event in await_gmc_reply()
--

commit 54e27415bb52450de6eeae48a370384ffd8a7d77
Author: John Hubbard <jhubbard@nvidia.com>

gpu: nova-core: gsp: cmdq: add the GMC receive path

This commit adds the receive side of the GMC sub-module in nova-core so the
driver can parse GMC messages, handle GMC events, and wait for GMC command
replies from GSP-RM.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq/gmc.rs b/drivers/gpu/nova-core/gsp/cmdq/gmc.rs
> index b035f30479a84..9adba5cbcd5c8 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq/gmc.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq/gmc.rs

[ ... ]

> @@ -150,7 +152,167 @@ fn write(&self, dev: &device::Device, seq: u32, dst: &mut GspCommand<'_>) -> Res

[ ... ]

> +    ///Logs an event, meaning a message that no caller was waiting for.
> +    ///
> +    /// A reply that nobody claimed (e.g. one that arrived after its request timed out) and an
> +    /// unknown command id are logged at warning level. Other events are not logged.
> +    pub(super) fn log(&self, dev: &device::Device) {
> +        let command = self.header.command();
> +        let seq = self.header.sequence();
> +
> +        match command.id() {
> +            Ok(id) if command.is_response() => {
> +                dev_warn!(dev, "unclaimed GSP GMC reply to {:?} (seq {})\n", id, seq);
> +            }
> +            // Nothing to do for the remaining known command ids.
> +            Ok(_) => {}
> +            Err(_) => {
> +                dev_warn!(dev, "unknown GSP GMC command {:?} (seq {})\n", command, seq);
> +            }
> +        }
> +    }
> +}
> +
>  impl Cmdq<'_> {
> +    /// Sends `command` to the GSP and waits for the reply.

[ ... ]

> +    pub(crate) fn send_gmc_command<C>(
> +        &self,
> +        command: C,
> +        on_event: impl Fn(&GmcMessage<'_>) -> Result,
> +    ) -> Result<C::Reply>
> +    where
> +        C: CommandToGsp,
> +        C::Reply: MessageFromGsp,
> +        Error: From<C::InitError>,
> +        Error: From<<C::Reply as MessageFromGsp>::InitError>,
> +    {
> +        let mut inner = self.inner.lock();
> +        let seq = inner.send_gmc_command(command)?;
> +
> +        inner.await_gmc_reply(seq, on_event)
> +    }
> +
>      /// Sends `command` to the GSP without waiting for a reply.
> @@ -191,4 +353,70 @@ fn send_gmc_command<C>(&mut self, command: C) -> Result<u32>
>      {
>          self.send_command_element(GmcCommandElement(command))
>      }

[Severity: High]
Does CmdqInner::send_gmc_command() compile and return the sequence number as
expected here?

Cmdq::send_gmc_command() calls inner.send_gmc_command(command)? to obtain
seq: u32 and pass it to inner.await_gmc_reply(seq, on_event). However,
CmdqInner::send_gmc_command() returns the result of
self.send_command_element(GmcCommandElement(command)), and in
drivers/gpu/nova-core/gsp/cmdq.rs, CmdqInner::send_command_element() has
return type Result (i.e. Result<(), Error>):

drivers/gpu/nova-core/gsp/cmdq.rs:CmdqInner::send_command_element() {
    ...
        let elem_count = dst.header.element_count();
        self.seq += 1;
        self.gsp_mem.advance_cpu_write_ptr(elem_count);
        Cmdq::notify_gsp(self.bar);

        Ok(())
    }

Because rustc type-checks generic function bodies even when unused, won't
this fail to build with a mismatched types error (expected Result<u32, Error>,
found Result<(), Error>) unless send_command_element() or send_gmc_command()
returns the sequence number of the sent element?

> +
> +    /// Receives the reply of type `R` to the command sent with sequence number `seq`, waiting up
> +    /// to [`Cmdq::RECEIVE_TIMEOUT`] from the call.

[ ... ]

> +    fn await_gmc_reply<R: MessageFromGsp>(
> +        &mut self,
> +        seq: u32,
> +        on_event: impl Fn(&GmcMessage<'_>) -> Result,
> +    ) -> Result<R>
> +    where
> +        // This allows all error types, including `Infallible`, to be used for `R::InitError`.
> +        Error: From<R::InitError>,
> +    {
> +        let dev = self.dev;
> +
> +        self.await_msg_with(|message| {
> +            let gmc_message = match message {
> +                GspParsedMessage::Gmc(gmc_message) => gmc_message,
> +                _ => {
> +                    message.log(dev);
> +                    return Err(ENOMSG);
> +                }
> +            };
> +
> +            match gmc_message.is_reply_to(R::COMMAND, seq) {
> +                // Message is not the reply to our command, log it.
> +                None => {
> +                    on_event(&gmc_message)?;
> +                    return Err(ENOMSG);
> +                }

[Severity: Medium]
When gmc_message.is_reply_to(R::COMMAND, seq) returns None in
CmdqInner::await_gmc_reply(), can an unclaimed GMC reply to an earlier
command or an unknown GMC command be passed to on_event() instead of being
logged?

In drivers/gpu/nova-core/gsp/fw.rs, GmcCommandWord::is_response() is set on
responses to requests and not set on events, and GmcMessage::log() checks
command.is_response() and command.id() to log unclaimed replies and unknown
commands at warning level.

Here, the comment says "Message is not the reply to our command, log it.",
but the None arm calls on_event(&gmc_message)? without checking whether the
message is a response or calling gmc_message.log(dev) (which is pub(super)
and not callable from outside the cmdq module). Should unclaimed responses
be logged via gmc_message.log(dev) rather than forwarded to on_event()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-r000_rebase_on_cmdq-rpc-v1-0-189f9ff3bfd2@nvidia.com?part=4