Re: [PATCH v2 05/13] gpu: nova-core: gsp: move unload bundle error handling to Gsp::boot

[sashiko-bot@kernel.org]

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 <acourbot@nvidia.com>

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/20260622-nova-bootcontext-v2-0-0ddeafc06f5d@nvidia.com?part=5