Re: [PATCH v5 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Command parsing errors silently terminate execution while reporting success.
- [High] Truncated commands are executed with zeroed payloads instead of failing.
- [High] `seq_info.cmd_index` is improperly used as the total command count.
--

--- Patch [2]: [PATCH v5 02/13] gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer ---
commit b9f8dfb196e815ccff55e396434b9fbb45c0cc05
Author: Alexandre Courbot <acourbot@nvidia.com>

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

Moves the sequence out of the GspSequencer struct to avoid borrow-checker
limitations. Iterator creation is moved to GspSeqIter::new so it no longer
borrows the entire GspSequencer.

> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index f55205bd61f35..ddce32cc4e308 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs

[ ... ]

> @@ -281,6 +279,18 @@ pub(crate) struct GspSeqIter<'a> {
>      dev: &'a device::Device,
>  }
>  
> +impl<'a> GspSeqIter<'a> {
> +    fn new(seq: &'a GspSequence, dev: &'a device::Device) -> Self {
> +        Self {
> +            cmd_data: &seq.cmd_data,
> +            current_offset: 0,
> +            total_cmds: seq.cmd_index,

[Severity: High]
This is a pre-existing issue, but does this incorrectly use the DWord size
as the command count?

If seq.cmd_index represents the buffer length in DWords, assigning it to
total_cmds breaks the iteration bounds check.

In GspSeqIter::next(), cmds_processed is incremented by 1 per command,
which means the condition cmds_processed >= total_cmds won't trigger at
the actual end of valid commands.

Could this cause the iterator to parse and execute padding bytes as
commands?

> +            cmds_processed: 0,
> +            dev,
> +        }
> +    }
> +}

[ ... ]

> @@ -363,14 +358,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 is a pre-existing issue, but is it possible for truncated commands to
be parsed and executed with zeroed payloads?

Looking at GspSeqIter::next(), if the remaining buffer is smaller than
CMD_SIZE, it appears to zero-pad the data into a buffer of size CMD_SIZE
before passing it to GspSeqCmd::new():

    let cmd_result = GspSeqCmd::new(&buffer, self.dev);

Inside GspSeqCmd::new(), the safety check uses data.len():

    if data.len() < size {
        dev_err!(dev, "Data is not enough for command\n");
        return Err(EINVAL);
    }

Since the padded buffer length is always exactly CMD_SIZE, this check might
act as dead code. Does this mean truncated commands would succeed using the
zero padding as their payload rather than failing with an error?

[Severity: High]
This is another pre-existing issue, but does command parsing silently
terminate on errors while reporting success to the caller?

In GspSeqIter::next(), when parsing fails, the error closure logs the
failure but returns None:

    cmd_result.map_or_else(
        |_err| {
            dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
            None
        },

Since None signals normal iterator termination, this loop in
GspSequencer::run() will exit cleanly and return Ok(()) without triggering
an error.

Could this leave the hardware sequence incomplete while the driver
incorrectly assumes the boot sequence succeeded?

>              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/20260707-nova-bootcontext-v5-0-ecad9346387f@nvidia.com?part=2