On Mon Jun 22, 2026 at 4:57 PM JST, Eliot Courtney wrote:
> On Fri Jun 19, 2026 at 10:42 PM JST, Alexandre Courbot wrote:
>> The next patch aims at replacing the cumbersome `BootUnloadGuard` with a
>> more local and less intrusive mechanism to run the GSP unload sequence
>> upon GSP boot failure. Doing so requires running the boot code in a
>> local closure, which changes its indentation and would make other
>> changes difficult to track in the diff. Thus, this preparatory patch
>> moves said boot code into a local closure that is run upon construction,
>> so the next patch does not need to re-indent code that changes.
>>
>> This is a mechanical preparatory patch to make the next patch easier to
>> read. No functional change intended.
>>
>> Signed-off-by: Alexandre Courbot <[email protected]>
>> ---
>
> I agree with removing BootUnloadGuard, but I think it's not great to do
> a bunch of lifting into closures then manually handling the result. It's
> error prone imo (we already had several bugs relating to this kind of
> thing). Instead, what about just using ScopeGuard directly? This lets us
> avoid lifting into closures (which is a bit noisy) and avoids manual
> result handling for failures (which is a bit error prone). With the
> `GspBootContext` it's fairly easy to do now:
>
> ```
> let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {
> let _ = gsp.unload(ctx, unload_bundle);
> });
> ```
Yes, initially I wanted to use `ScopeGuard` but ran into issues when
trying to make the references mutable. However it looks like you have
been able to overcome these issues, thanks for taking the time to craft
a patch!
>
> I confirmed that it's also compatible with the v2 of this series that
> has the mutable Fsp - you can stash the context inside the ScopeGuard
> data (then making a &mut reference to the stashed context for brevity)
> or have a separate unload context type that doesn't use FSP or something
> (could later be type parametrized along with Gsp, for example).
>
> For example here is a rough diff on top of this patch series (you can
> change the Result<Option<UnloadBundle>> returns to like
> Result<Result<UnloadBundle>> if you want to centralise teh error
> handling of a failed unloadbundle although currently it can only fail in
> one location):
Yes, looking at it it looks like a cleaner approach than using closures.
The only thing that I saw as a regression is that now each HAL needs to
call `Gsp::unload` itself in its own `ScopeGuard`. I don't think that's
the HAL's work - `Gsp::boot` should be the centralized point where this
happens.
But we can make both work if `unload_bundle` is passed as an output
argument to `GspHal::boot` instead of being returned:
let mut guard = ScopeGuard::new_with_data((ctx, None), ...);
let (ctx, unload_bundle) = &mut *guard;
// `unload_bundle` is a mutable reference to the
// `Option<UnloadBundle>` in `guard`.
hal.boot(&self, ctx, unload_bundle, &fb_layout, &wpr_meta)?;
The boot method can fill `unload_bundle` early, and if it returns an
error then the `ScopeGuard` will be able to use it. Also, and that's
nice, the HALs don't need to use `ScopeGuard`. But output arguments
aren't really something we expect to see in Rust.
Another alternative would be to separate the unload bundle construction
from `GspHal::boot`:
let unload_bundle = hal.build_unload_bundle(ctx, ...);
let guard = ScopeGuard::new_with_data((ctx, unload_bundle), ...);
hal.boot(...)?;
It removes the "1 boot -> 1 unload bundle" symmetry, but on the other
hand it also splits concerns more clearly. And it removes the awkward
return type of `GspHal::boot`, which come to think of it was another
smell that things were not in the right place.
The main drawback I see is that we now need to build `Vbios` twice for
TU102, since it is needed both for the unload bundle and for booting.
But the solution is the same as what the v2 of this series does to
`Fsp`: the BIOS is a GPU-wide subdevice that is likely to be used
elsewhere, not something to be confined in a GSP HAL. So I say, let's
extract it and make it also part of `GspBootContext`.
How does that sound?
