[PATCH] gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[PATCH] gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Alexandre Courbot]

On Turing and Ampere, resetting the GSP involves running two firmware
images: FWSEC-SB and Booter Unloader. They are independent from one
another, and we should do whatever is possible to restore the GSP's
unloaded state even if a failure occurs along the way.

Thus, keep going and run Booter Unloader even if the execution of
FWSEC-SB failed.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260529-nova-unload-v7-0-678f39209e00%40nvidia.com?part=3
Fixes: adb99ce3cc78 ("gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding")
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
This was caught by Sashiko; I unfortunately noticed it after pushing the
series, but having it as a follow-up is beneficial regardless as it
allows more time for review.
---
 drivers/gpu/nova-core/gsp/hal/tu102.rs | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
index a033bc892066..b10215190257 100644
--- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
+++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
@@ -134,11 +134,19 @@ fn run(
         sec2_falcon: &Falcon<Sec2>,
     ) -> Result {
         // Run FWSEC-SB to reset the GSP falcon to its pre-libos state.
-        self.fwsec_sb.run(dev, bar, gsp_falcon)?;
+        // Log errors but keep going if it fails.
+        let fwsec_sb_res = self
+            .fwsec_sb
+            .run(dev, bar, gsp_falcon)
+            .inspect_err(|e| dev_err!(dev, "FWSEC-SB failed to run: {:?}\n", e));
 
         // Remove WPR2 region if set.
         let wpr2_hi = bar.read(regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI);
-        if wpr2_hi.is_wpr2_set() {
+        let booter_unloader_res = (|| {
+            if !wpr2_hi.is_wpr2_set() {
+                return Ok(());
+            }
+
             sec2_falcon.reset(bar)?;
             sec2_falcon.load(dev, bar, &self.booter_unloader)?;
 
@@ -160,9 +168,11 @@ fn run(
                 );
                 return Err(EBUSY);
             }
-        }
 
-        Ok(())
+            Ok(())
+        })();
+
+        fwsec_sb_res.and(booter_unloader_res)
     }
 }
 

---
base-commit: 75d59327367dc6e2141cf4e11cdf57c55851b5c2
change-id: 20260531-nova-unload-fix-9c6e479a5374

Best regards,
--  
Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH] gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Timur Tabi]

On Sun, 2026-05-31 at 21:37 +0900, Alexandre Courbot wrote:
> On Turing and Ampere, resetting the GSP involves running two firmware
> images: FWSEC-SB and Booter Unloader. They are independent from one
> another, and we should do whatever is possible to restore the GSP's
> unloaded state even if a failure occurs along the way.
> 
> Thus, keep going and run Booter Unloader even if the execution of
> FWSEC-SB failed.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes:
> https://sashiko.dev/#/patchset/20260529-nova-unload-v7-0-678f39209e00%40nvidia.com?part=3
> Fixes: adb99ce3cc78 ("gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding")
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> This was caught by Sashiko; I unfortunately noticed it after pushing the
> series, but having it as a follow-up is beneficial regardless as it
> allows more time for review.
> ---
>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> index a033bc892066..b10215190257 100644
> --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
> +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> @@ -134,11 +134,19 @@ fn run(
>          sec2_falcon: &Falcon<Sec2>,
>      ) -> Result {
>          // Run FWSEC-SB to reset the GSP falcon to its pre-libos state.
> -        self.fwsec_sb.run(dev, bar, gsp_falcon)?;
> +        // Log errors but keep going if it fails.
> +        let fwsec_sb_res = self
> +            .fwsec_sb
> +            .run(dev, bar, gsp_falcon)
> +            .inspect_err(|e| dev_err!(dev, "FWSEC-SB failed to run: {:?}\n", e));

Shouldn't this be dev_warn?

Also, how did you test this?  Have you tried breaking the FWSEC-SB code and telling
booter_unload run anyway, and seeing if you can still reload the driver?  Sashiko said this:

> Since FWSEC-SB (running on gsp_falcon) and the Booter Unloader (running
on sec2_falcon) are independent cleanup steps, returning early here bypasses
the Booter Unloader execution entirely.

Are we sure they really are independent?  What does RM do?

Re: [PATCH] gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Alexandre Courbot]

On Mon Jun 1, 2026 at 3:41 AM JST, Timur Tabi wrote:
> On Sun, 2026-05-31 at 21:37 +0900, Alexandre Courbot wrote:
>> On Turing and Ampere, resetting the GSP involves running two firmware
>> images: FWSEC-SB and Booter Unloader. They are independent from one
>> another, and we should do whatever is possible to restore the GSP's
>> unloaded state even if a failure occurs along the way.
>> 
>> Thus, keep going and run Booter Unloader even if the execution of
>> FWSEC-SB failed.
>> 
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes:
>> https://sashiko.dev/#/patchset/20260529-nova-unload-v7-0-678f39209e00%40nvidia.com?part=3
>> Fixes: adb99ce3cc78 ("gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding")
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> This was caught by Sashiko; I unfortunately noticed it after pushing the
>> series, but having it as a follow-up is beneficial regardless as it
>> allows more time for review.
>> ---
>>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
>> index a033bc892066..b10215190257 100644
>> --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
>> +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
>> @@ -134,11 +134,19 @@ fn run(
>>          sec2_falcon: &Falcon<Sec2>,
>>      ) -> Result {
>>          // Run FWSEC-SB to reset the GSP falcon to its pre-libos state.
>> -        self.fwsec_sb.run(dev, bar, gsp_falcon)?;
>> +        // Log errors but keep going if it fails.
>> +        let fwsec_sb_res = self
>> +            .fwsec_sb
>> +            .run(dev, bar, gsp_falcon)
>> +            .inspect_err(|e| dev_err!(dev, "FWSEC-SB failed to run: {:?}\n", e));
>
> Shouldn't this be dev_warn?

I guess that's subjective, but since it is technically an error that is
likely to prevent the driver the reload I think `dev_err` is the right
level here.

>
> Also, how did you test this?  Have you tried breaking the FWSEC-SB code and telling
> booter_unload run anyway, and seeing if you can still reload the driver?  Sashiko said this:
>
>> Since FWSEC-SB (running on gsp_falcon) and the Booter Unloader (running
> on sec2_falcon) are independent cleanup steps, returning early here bypasses
> the Booter Unloader execution entirely.
>
> Are we sure they really are independent?  What does RM do?

This is not really about being able to reload the driver afterwards if
FWSEC-SB fails - in all likelihood we won't be able to, although that
ultimately depends on where and how hard FWSEC-SB fails.

The reason for doing this is more to stay consistent with our teardown
policy [1], which is to keep going even if one step fails.

[1] https://lore.kernel.org/all/b5fb1462-c409-4ddc-a6c4-a83dcfaeae63@nvidia.com/

Re: [PATCH] gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Eliot Courtney]

On Sun May 31, 2026 at 9:37 PM JST, Alexandre Courbot wrote:
> On Turing and Ampere, resetting the GSP involves running two firmware
> images: FWSEC-SB and Booter Unloader. They are independent from one
> another, and we should do whatever is possible to restore the GSP's
> unloaded state even if a failure occurs along the way.
>
> Thus, keep going and run Booter Unloader even if the execution of
> FWSEC-SB failed.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260529-nova-unload-v7-0-678f39209e00%40nvidia.com?part=3
> Fixes: adb99ce3cc78 ("gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding")
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---

It seems that in openrm and nouveau AFAICT these two cleanups are both
done regardless if either fails.

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>

nit: it seems odd to me to log on fwsec-sb failure but not on booter
unloader failure.

Claude review: gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Claude Code Review Bot]

Patch Review

**The problem:** The original code early-returns on FWSEC-SB failure via `?`:
```rust
self.fwsec_sb.run(dev, bar, gsp_falcon)?;
```
This prevents the Booter Unloader from running, leaving the GSP in a partially-unloaded state.

**The fix has two parts:**

1. **FWSEC-SB error capture:** Instead of propagating immediately, the error is captured and logged:
```rust
let fwsec_sb_res = self
    .fwsec_sb
    .run(dev, bar, gsp_falcon)
    .inspect_err(|e| dev_err!(dev, "FWSEC-SB failed to run: {:?}\n", e));
```
Using `inspect_err` is clean — it logs the error at the point of occurrence while preserving the `Result` for later.

2. **Booter Unloader wrapped in a closure:** The WPR2 check and Booter Unloader execution are wrapped in an immediately-invoked closure to capture its result:
```rust
let booter_unloader_res = (|| {
    if !wpr2_hi.is_wpr2_set() {
        return Ok(());
    }
    // ... falcon reset, load, run ...
    Ok(())
})();
```
The closure inverts the condition from `if wpr2_hi.is_wpr2_set()` to `if !wpr2_hi.is_wpr2_set() { return Ok(()); }` — this is a reasonable transformation to allow `?` usage inside the closure body while keeping the early-exit-on-no-work semantic.

3. **Error combination:** Both results are combined with:
```rust
fwsec_sb_res.and(booter_unloader_res)
```

**One minor observation on error precedence:** `Result::and` returns the first `Err` it encounters (checking `self` first). So if *both* FWSEC-SB and Booter Unloader fail, only the FWSEC-SB error is propagated to the caller. The Booter Unloader error is silently discarded. This is probably acceptable since:
- The FWSEC-SB error is already logged via `inspect_err`
- But the Booter Unloader error is *not* logged before being dropped

It might be worth adding an `inspect_err` on the Booter Unloader closure result too, so that if both fail, the caller sees the FWSEC-SB error (via `and`) but the Booter Unloader failure is at least visible in dmesg. As-is, if both fail, the Booter Unloader error vanishes silently. However, this is a minor nit — the important fix (continuing despite FWSEC-SB failure) is correct.

**Reviewed-by assessment:** The patch is correct and addresses a real bug. The immediately-invoked closure pattern is a pragmatic way to scope `?` usage in Rust without extracting a separate function. The commit message clearly explains the rationale. This is good to go, with the optional suggestion to also log Booter Unloader failures for completeness.

---
Generated by Claude Code Patch Reviewer

Claude review: gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails

[Claude Code Review Bot]

Overall Series Review

Subject: gpu: nova-core: gsp: tu102: keep unloading if FWSEC-SB fails
Author: Alexandre Courbot <acourbot@nvidia.com>
Patches: 4
Reviewed: 2026-06-04T14:51:00.441389

---

This is a single-patch fix for the nova-core GPU driver's GSP unload path on Turing/Ampere hardware. The fix is straightforward and correct: the original code used `?` on the FWSEC-SB call, meaning a failure there would skip the Booter Unloader entirely. Since these two firmware operations are independent, this is wrong — we should attempt both regardless of individual failures.

The patch is well-motivated, properly tagged with `Fixes:`, and the approach is sound. The use of `inspect_err` for logging plus `Result::and` for combining errors is idiomatic Rust.

**Verdict: Patch looks good.**

---
Generated by Claude Code Patch Reviewer