Re: [PATCH v5 03/13] gpu: nova-core: gsp: replace BootUnloadGuard with local handlers

["Eliot Courtney" <ecourtney@nvidia.com>]

On Tue Jul 7, 2026 at 4:21 PM JST, Alexandre Courbot wrote:
> When adding the GSP unload capability, we introduced `BootUnloadGuard`
> to automatically call `Gsp::unload` whenever an error occurred during
> the boot process, in order to try to reset the GSP to a valid state.
>
> This approach is not well-suited to the errors that may occur in HALs:
> by definition, an error occurring in the HAL means that the GSP is not
> booted; yet the first thing that `Gsp::unload` does is queue a shutdown
> message to the GSP, which will inevitably result in a timeout when done
> from a HAL.
>
> Furthermore, `BootUnloadGuard` is problematic because it holds
> additional references to the boot context, notably the `Falcon`s. These
> extra references stand in the way of making some of the `Falcon`'s
> methods mutable, since those methods would require exclusive access. As
> this behavior is only needed in one place, introducing dedicated types
> for it is distracting and unnecessary.
>
> Thus, remove `BootUnloadGuard` and adopt a two-level error handling
> strategy:
>
> - HALs are free to handle their errors as they see fit (most likely, by
>   running their unload bundle if it is ready by the time of the error),
> - `Gsp::boot` uses a `ScopeGuard` that runs `Gsp::unload`, since the
>   GSP should be up and running by the time `GspHal::boot` has returned.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/boot.rs      | 67 +++-------------------------------
>  drivers/gpu/nova-core/gsp/hal.rs       | 13 +++----
>  drivers/gpu/nova-core/gsp/hal/gh100.rs | 31 ++++++++++------
>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 23 +++++++-----
>  4 files changed, 44 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index ab0491b57944..536f2e341c01 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -30,66 +30,6 @@
>      },
>  };
>  
> -/// Arguments required to call [`Gsp::unload`](super::Gsp::unload).
> -///
> -/// Stored as their own type to avoid repeating a long and tedious list in [`BootUnloadGuard`].
> -pub(super) struct BootUnloadArgs<'a> {
> -    gsp: &'a super::Gsp,
> -    dev: &'a device::Device<device::Bound>,
> -    bar: Bar0<'a>,
> -    gsp_falcon: &'a Falcon<'a, Gsp>,
> -    sec2_falcon: &'a Falcon<'a, Sec2>,
> -    unload_bundle: Option<super::UnloadBundle>,
> -}
> -
> -/// Guard that calls [`Gsp::unload`](super::Gsp::unload) with a
> -/// [`UnloadBundle`](super::UnloadBundle) when dropped.
> -///
> -/// Used to ensure the `UnloadBundle` is run during failure paths.
> -pub(super) struct BootUnloadGuard<'a> {
> -    guard: ScopeGuard<BootUnloadArgs<'a>, fn(BootUnloadArgs<'a>)>,
> -}
> -
> -impl<'a> BootUnloadGuard<'a> {
> -    /// Wraps `unload_bundle` into a guard that executes it when dropped.
> -    pub(super) fn new(
> -        gsp: &'a super::Gsp,
> -        dev: &'a device::Device<device::Bound>,
> -        bar: Bar0<'a>,
> -        gsp_falcon: &'a Falcon<'a, Gsp>,
> -        sec2_falcon: &'a Falcon<'a, Sec2>,
> -        unload_bundle: Option<super::UnloadBundle>,
> -    ) -> Self {
> -        Self {
> -            guard: ScopeGuard::new_with_data(
> -                BootUnloadArgs {
> -                    gsp,
> -                    dev,
> -                    bar,
> -                    gsp_falcon,
> -                    sec2_falcon,
> -                    unload_bundle,
> -                },
> -                |args| {
> -                    let _ = super::Gsp::unload(
> -                        args.gsp,
> -                        args.dev,
> -                        args.bar,
> -                        args.gsp_falcon,
> -                        args.sec2_falcon,
> -                        args.unload_bundle,
> -                    );
> -                },
> -            ),
> -        }
> -    }
> -
> -    /// Disarms the guard and returns the [`UnloadBundle`](super::UnloadBundle) it contains.
> -    pub(super) fn dismiss(self) -> Option<super::UnloadBundle> {
> -        self.guard.dismiss().unload_bundle
> -    }
> -}
> -
>  impl super::Gsp {
>      /// Attempt to boot the GSP.
>      ///
> @@ -107,6 +47,7 @@ pub(crate) fn boot(
>          let bar = ctx.bar;
>          let chipset = ctx.chipset;
>          let gsp_falcon = ctx.gsp_falcon;
> +        let sec2_falcon = ctx.sec2_falcon;
>          let dev = pdev.as_ref();
>          let hal = super::hal::gsp_hal(chipset);
>  
> @@ -118,7 +59,11 @@ 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_guard = hal.boot(&self, &ctx, &fb_layout, &wpr_meta)?;
> +        let unload_bundle = hal.boot(&self, &ctx, &fb_layout, &wpr_meta)?;
> +
> +        let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {
> +            let _ = self.unload(dev, bar, gsp_falcon, sec2_falcon, unload_bundle);
> +        });
>  
>          gsp_falcon.write_os_version(gsp_fw.bootloader.app_version);
>  
> diff --git a/drivers/gpu/nova-core/gsp/hal.rs b/drivers/gpu/nova-core/gsp/hal.rs
> index d3e47ef206de..851d1f24c137 100644
> --- a/drivers/gpu/nova-core/gsp/hal.rs
> +++ b/drivers/gpu/nova-core/gsp/hal.rs
> @@ -24,7 +24,6 @@
>          Chipset, //
>      },
>      gsp::{
> -        boot::BootUnloadGuard,
>          Gsp,
>          GspBootContext,
>          GspFwWprMeta, //
> @@ -51,15 +50,15 @@ fn run(
>  pub(super) trait GspHal: Send {
>      /// Performs the GSP boot process, loading and running the required firmwares as needed.
>      ///
> -    /// Upon success, returns a guard that runs the GSP unload sequence if GSP boot does not
> -    /// complete.
> -    fn boot<'a>(
> +    /// Upon success, returns the [`crate::gsp::UnloadBundle`] to use with [`Gsp::unload`], if one
> +    /// could be created.
> +    fn boot(
>          &self,
> -        gsp: &'a Gsp,
> -        ctx: &GspBootContext<'a>,
> +        gsp: &Gsp,
> +        ctx: &GspBootContext<'_>,
>          fb_layout: &FbLayout,
>          wpr_meta: &Coherent<GspFwWprMeta>,
> -    ) -> Result<BootUnloadGuard<'a>>;
> +    ) -> Result<Option<crate::gsp::UnloadBundle>>;
>  
>      /// Performs HAL-specific post-GSP boot tasks.
>      ///
> diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
> index 1d06405a32f6..18c889f9f413 100644
> --- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
> +++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
> @@ -23,7 +23,6 @@
>          Fsp, //
>      },
>      gsp::{
> -        boot::BootUnloadGuard,
>          hal::{
>              GspHal,
>              UnloadBundle, //
> @@ -143,13 +142,13 @@ impl GspHal for Gh100 {
>      ///
>      /// This path uses FSP to establish a chain of trust and boot GSP-FMC. FSP handles
>      /// the GSP boot internally - no manual GSP reset/boot is needed.
> -    fn boot<'a>(
> +    fn boot(
>          &self,
> -        gsp: &'a Gsp,
> -        ctx: &GspBootContext<'a>,
> +        gsp: &Gsp,
> +        ctx: &GspBootContext<'_>,
>          fb_layout: &FbLayout,
>          wpr_meta: &Coherent<GspFwWprMeta>,
> -    ) -> Result<BootUnloadGuard<'a>> {
> +    ) -> Result<Option<crate::gsp::UnloadBundle>> {
>          let dev = ctx.dev();
>          let bar = ctx.bar;
>          let chipset = ctx.chipset;
> @@ -160,10 +159,6 @@ fn boot<'a>(
>              KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle>
>          );
>  
> -        // Wrap the unload bundle into a drop guard so it is automatically run upon failure.
> -        let unload_guard =
> -            BootUnloadGuard::new(gsp, dev, bar, gsp_falcon, sec2_falcon, Some(unload_bundle));
> -
>          let mut fsp = Fsp::wait_secure_boot(dev, bar, chipset)?;
>  
>          let args = FmcBootArgs::new(
> @@ -174,11 +169,23 @@ fn boot<'a>(
>              false,
>          )?;
>  
> -        fsp.boot_fmc(dev, fb_layout, &args)?;
> +        // 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_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle())?;
> +        // 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());
>  
> -        Ok(unload_guard)
> +        match fsp_res.and(lockdown_res) {
> +            Ok(()) => Ok(Some(unload_bundle)),
> +            Err(e) => {
> +                // Wait for the GSP RISC-V core to halt in case of error.
> +                let _ = unload_bundle.0.run(dev, bar, gsp_falcon, sec2_falcon);
> +                Err(e)
> +            }
> +        }

IMO we should just ScopeGuard here too (after args creation) to run the
unload bundle. It will avoid manual manipulation of results which is
error prone (since a ? added later will break things) and also halt
happens-after gsp lockdown, so it's a natural sequence point. Also I am
not 100% sure but I am not fully convinced that
wait_for_gsp_lockdown_release will correctly detect all errors if they
occur.

With ^ change,
Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>


>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> index 87ceb8878f01..f78e2489f5a6 100644
> --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
> +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> @@ -6,7 +6,8 @@
>  use kernel::{
>      device,
>      dma::Coherent,
> -    io::Io, //
> +    io::Io,
> +    types::ScopeGuard, //
>  };
>  
>  use crate::{
> @@ -32,7 +33,6 @@
>      },
>      gpu::Chipset,
>      gsp::{
> -        boot::BootUnloadGuard,
>          hal::{
>              GspHal,
>              UnloadBundle, //
> @@ -259,13 +259,13 @@ fn run_fwsec_frts(
>  struct Tu102;
>  
>  impl GspHal for Tu102 {
> -    fn boot<'a>(
> +    fn boot(
>          &self,
> -        gsp: &'a Gsp,
> -        ctx: &GspBootContext<'a>,
> +        gsp: &Gsp,
> +        ctx: &GspBootContext<'_>,
>          fb_layout: &FbLayout,
>          wpr_meta: &Coherent<GspFwWprMeta>,
> -    ) -> Result<BootUnloadGuard<'a>> {
> +    ) -> Result<Option<crate::gsp::UnloadBundle>> {
>          let dev = ctx.dev();
>          let bar = ctx.bar;
>          let chipset = ctx.chipset;
> @@ -290,9 +290,12 @@ fn boot<'a>(
>              .ok()
>              .map(crate::gsp::UnloadBundle);
>  
> -        // Wrap the unload bundle into a drop guard so it is automatically run upon failure.
> -        let unload_guard =
> -            BootUnloadGuard::new(gsp, dev, bar, gsp_falcon, sec2_falcon, unload_bundle);
> +        // Run the unload bundle to try and recover the GSP if an error occurs.
> +        let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {
> +            if let Some(unload_bundle) = unload_bundle {
> +                let _ = unload_bundle.0.run(dev, bar, gsp_falcon, sec2_falcon);
> +            }
> +        });
>  
>          // FWSEC-FRTS is not executed on chips where the FRTS region size is 0 (e.g. GA100).
>          if !fb_layout.frts.is_empty() {
> @@ -319,7 +322,7 @@ fn boot<'a>(
>          )?
>          .run(dev, sec2_falcon, wpr_meta)?;
>  
> -        Ok(unload_guard)
> +        Ok(unload_guard.dismiss())
>      }
>  
>      fn post_boot(&self, gsp: &Gsp, ctx: &GspBootContext<'_>, gsp_fw: &GspFirmware) -> Result {