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 <[email protected]> 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/[email protected]?part=2
