Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Early boot failures unconditionally trigger false warnings about unload firmware preparation and incorrectly state a GPU reset is required. --
commit 273d5dbf9e1e0ce85069f0026ccecdadd0a2d4fd Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: move unload bundle error handling to Gsp::boot This commit moves the warning for unload bundle creation failure from the Tu102 specific implementation to Gsp::boot so it applies to all boot methods. It changes the HAL boot method to return a Result for the unload bundle. > diff --git a/drivers/gpu/nova-core/gsp/boot.rs > b/drivers/gpu/nova-core/gsp/boot.rs > --- a/drivers/gpu/nova-core/gsp/boot.rs > +++ b/drivers/gpu/nova-core/gsp/boot.rs [ ... ] > @@ -59,6 +59,18 @@ pub(crate) fn boot( > // Perform the chipset-specific boot sequence, and retrieve the > unload bundle. > let (res, unload_bundle) = hal.boot(&self, &ctx, &fb_layout, > &wpr_meta); > > + // Display error for unload bundle if any, and convert to Option. > + let unload_bundle = unload_bundle > + .inspect_err(|e| { [Severity: Medium] If the hal.boot() implementation fails early (for example, if Vbios::new() fails in Tu102::boot()), unload_bundle will retain its initial dummy error value of Err(EAGAIN). Won't inspect_err() then unconditionally print these warnings about firmware preparation and the need for a GPU reset, even though the actual failure occurred before bundle preparation was attempted? > + dev_warn!(dev, "Failed to prepare unload firmware: {:?}\n", > e); > + dev_warn!(dev, "The GSP won't be able to unload properly on > unbind.\n"); > + dev_warn!( > + dev, > + "The GPU will need to be reset before the driver can > bind again.\n" > + ); > + }) > + .ok(); > + > // Run from a closure so we can retrieve the result, and run the > unload sequence of the GSP > // in case of error. > let res = res.and_then(|()| { [ ... ] > diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs > b/drivers/gpu/nova-core/gsp/hal/tu102.rs > --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs > +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs [ ... ] > @@ -269,14 +269,14 @@ fn boot( > ctx: &GspBootContext<'_>, > fb_layout: &FbLayout, > wpr_meta: &Coherent<GspFwWprMeta>, > - ) -> (Result, Option<crate::gsp::UnloadBundle>) { > + ) -> (Result, Result<crate::gsp::UnloadBundle>) { > let dev = ctx.dev(); > let bar = ctx.bar; > let chipset = ctx.chipset; > let gsp_falcon = ctx.gsp_falcon; > let sec2_falcon = ctx.sec2_falcon; > > - let mut unload_bundle = None; > + let mut unload_bundle = Err(EAGAIN); > > let res = (|| { > let bios = Vbios::new(dev, bar)?; [Severity: Medium] Here is where unload_bundle defaults to Err(EAGAIN). Is there a way to differentiate between an early boot failure where bundle creation wasn't reached, versus an actual failure during bundle creation, so that Gsp::boot() doesn't emit false warnings? > @@ -287,15 +287,6 @@ fn boot( -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
