Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Diagnostic warnings about a missing unload bundle and the need to reset the GPU are silently dropped if `hal.boot()` fails after a failed unload bundle creation. --
--- Patch [5]: [PATCH v5 05/13] gpu: nova-core: gsp: centralize missing unload bundle warnings --- commit 5c2f2654dd3e426ece23d356efc67204a24f8fc0 Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: centralize missing unload bundle warnings This commit centralizes the warning emitted when the unload bundle cannot be constructed by moving it from the Tu102-specific boot sequence to the common Gsp::boot method. > diff --git a/drivers/gpu/nova-core/gsp/boot.rs > b/drivers/gpu/nova-core/gsp/boot.rs > index 75054424a9516..430ffc57b1366 100644 > --- a/drivers/gpu/nova-core/gsp/boot.rs > +++ b/drivers/gpu/nova-core/gsp/boot.rs > @@ -56,7 +56,15 @@ pub(crate) fn boot( > let wpr_meta = Coherent::init(dev, GFP_KERNEL, > GspFwWprMeta::new(&gsp_fw, &fb_layout))?; > > // Perform the chipset-specific boot sequence, and retrieve the > unload bundle. > - let unload_bundle = hal.boot(&self, &ctx, &fb_layout, &wpr_meta)?; > + let unload_bundle = hal.boot(&self, &ctx, &fb_layout, > &wpr_meta)?.or_else(|| { > + 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" > + ); > + > + None > + }); [Severity: Medium] Does the `?` operator here inadvertently skip these warnings if a subsequent step in `hal.boot()` fails? In the previous implementation, the warnings were printed immediately when `Sec2UnloadBundle::build()` failed in `Tu102::boot()`. With this change, if the unload bundle creation fails, but a subsequent operation inside `hal.boot()` also fails and returns an `Err`, the `?` operator will immediately propagate that error. This entirely bypasses the `.or_else()` block, dropping the diagnostic warnings about the GPU needing a reset. > diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs > b/drivers/gpu/nova-core/gsp/hal/tu102.rs > index c0956fb1c9cff..ef465b99af051 100644 > --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs > +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs [ ... ] > @@ -278,14 +278,7 @@ fn boot( > // If the unload bundle creation fails, the GPU will need to be > reset before the driver can > // be probed again. > let unload_bundle = Sec2UnloadBundle::build(dev, chipset, &bios, > gsp_falcon, sec2_falcon) > - .inspect_err(|e| { > - 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" > - ); > - }) > + .inspect_err(|e| dev_warn!(dev, "Failed to prepare unload > firmware: {:?}\n", e)) > .ok() > .map(crate::gsp::UnloadBundle); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
