Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Unconditional 30-second poll for GSP lockdown release in `gh100::boot` even if FSP boot fails synchronously. --
--- Patch [3]: [PATCH v5 03/13] gpu: nova-core: gsp: replace BootUnloadGuard with local handlers --- commit 5f271d8ce96c42826048a3921af9e3b0c91b646e Author: Alexandre Courbot <[email protected]> Subject: gpu: nova-core: gsp: replace BootUnloadGuard with local handlers This commit removes BootUnloadGuard and replaces it with local error handling strategies across the GSP boot process. HALs are now responsible for handling their own errors and returning an unload bundle on success. Gsp::boot now utilizes a ScopeGuard to run Gsp::unload. > diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs > b/drivers/gpu/nova-core/gsp/hal/gh100.rs > index 1d06405a32f61..18c889f9f4130 100644 > --- a/drivers/gpu/nova-core/gsp/hal/gh100.rs > +++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs [ ... ] > @@ -174,11 +169,23 @@ fn boot<'a>( > false, > )?; > > - fsp.boot_fmc(dev, fb_layout, &args)?; > - > - wait_for_gsp_lockdown_release(dev, gsp_falcon, > args.boot_params_dma_handle())?; > - > - Ok(unload_guard) > + // Keep the result as we want to wait for lockdown release even in > case of error, to make > + // sure `args` is not accessed by the GSP anymore. > + let fsp_res = fsp.boot_fmc(dev, fb_layout, &args); > + > + // Wait for GSP-FMC to release the GSP lockdown, indicating that > `args` is not accessed > + // anymore. > + let lockdown_res = > + wait_for_gsp_lockdown_release(dev, gsp_falcon, > args.boot_params_dma_handle()); > + > + match fsp_res.and(lockdown_res) { ^^^^ [Severity: Medium] Will this unconditionally wait for the full timeout if fsp.boot_fmc() fails synchronously? If fsp.boot_fmc() fails early (for example, due to memory allocation failure or invalid firmware signatures) before sending the message to the FSP, the FSP will never be commanded to boot the GSP. Because the early return was removed, wait_for_gsp_lockdown_release() is called anyway and will inevitably block for its full 30-second timeout since the lockdown will never be released. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
