Re: [PATCH v2 02/13] gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the iterator silently terminates on command parsing errors, causing the GSP boot sequence to incorrectly report success instead of aborting.
--

commit 67a42d6a50b9bd7a1977aebced3e9453d53bb90e
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer

This commit refactors the GSP sequencer by moving the command sequence
data out of the GspSequencer struct and instead passing it directly when
creating the iterator. This resolves borrow-checker limitations and allows
for future mutable borrows within the boot context.

> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index 0da3c35318866..b5e049f76c282 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs

[ ... ]

> @@ -355,7 +351,6 @@ pub(crate) fn run(
>          };
>  
>          let sequencer = GspSequencer {
> -            seq_info,
>              bar: ctx.bar,
>              sec2_falcon: ctx.sec2_falcon,
>              gsp_falcon: ctx.gsp_falcon,
> @@ -366,14 +361,14 @@ pub(crate) fn run(
>  
>          dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n");
>  
> -        for cmd_result in sequencer.iter() {
> +        for cmd_result in GspSeqIter::new(&seq_info, sequencer.dev) {

[Severity: High]
This isn't a bug introduced by this patch, but doesn't the iterator
silently terminate on command parsing errors, causing the GSP boot sequence
to incorrectly report success instead of aborting?

Looking at GspSeqIter::next(), when a command fails to parse, the error
is swallowed and translated to None:

drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::next() {
    ...
    cmd_result.map_or_else(
        |_err| {
            dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
            None
        },
    ...
}

Returning None signals normal exhaustion of the iterator. This causes the
loop here in GspSequencer::run() to cleanly exit without ever hitting the
Err(e) match arm below.

Consequently, GspSequencer::run() will incorrectly log success and return
Ok(()), completely masking the hardware initialization failure and leaving
the hardware in a malformed state.

>              match cmd_result {
>                  Ok(cmd) => cmd.run(&sequencer)?,
>                  Err(e) => {
>                      dev_err!(
>                          sequencer.dev,
>                          "Error running command at index {}\n",
> -                        sequencer.seq_info.cmd_index
> +                        seq_info.cmd_index
>                      );
>                      return Err(e);
>                  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-nova-bootcontext-v2-0-0ddeafc06f5d@nvidia.com?part=2