Claude review: gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the largest and most important patch. Several points:

**`Cell<Option<gsp::UnloadBundle>>` in `NovaCore`**:
```rust
unload_bundle: Cell<Option<gsp::UnloadBundle>>,
```
As noted in the overall review, `Cell` is `!Sync`. The `Cell::take()` in `unbind()` is used because `unbind` only gets a shared reference (`Pin<&Self::Data>`). This is a pragmatic solution — the unload bundle is consumed exactly once. However, if two unbind calls could race (unlikely but architecturally possible), `Cell::take()` would silently lose the bundle on one path. A `// SAFETY:` or `// INVARIANT:` comment explaining why this is sound would strengthen the patch.

**`UnloadBundle` as opaque wrapper**:
```rust
pub(crate) struct UnloadBundle(KBox<dyn hal::UnloadBundle>);
```

This nicely hides the HAL-specific implementation behind a trait object. The `KBox` ensures heap allocation and dynamic dispatch.

**`Sec2UnloadBundle::build()` error handling**:
```rust
let unload_bundle = Sec2UnloadBundle::build(...)
    .inspect_err(|e| {
        dev_warn!(dev, "Failed to prepare unload firmware: {:?}\n", e);
        ...
    })
    .ok();
```

Converting the error to `None` and continuing boot is the right call — failing to prepare unload firmware shouldn't prevent the driver from loading. The warnings are clear about the consequence.

**`Sec2UnloadBundle::run()` sequence**:
1. Run FWSEC-SB to reset GSP falcon
2. Check WPR2 region, run Booter Unloader if set
3. Verify WPR2 was removed

The Booter Unloader uses sentinel values `0xff` in both mailboxes (unlike Booter Loader which passes WPR metadata addresses). This aligns with OpenRM behavior per the cover letter.

**`unload()` error continuation**:
```rust
let mut res = Self::shutdown_gsp(...)
    .inspect_err(|e| dev_err!(dev, "GSP shutdown failed: {:?}\n", e));

if let Some(unload_bundle) = unload_bundle {
    res = res.and(
        unload_bundle.0.run(...)
            .inspect_err(|e| dev_err!(dev, "Unload bundle failed: {:?}\n", e)),
    );
}
```

Using `res.and()` means: if GSP shutdown failed, the unload bundle still runs, but the overall result preserves the first error. This is excellent teardown logic — continue cleanup despite partial failures, but don't mask errors.

**Missing `Reviewed-by` on this patch**: Unlike patches 1-6, patch 7 has no `Reviewed-by` tag. This is the most critical patch in the series, so it would benefit from additional review.

**`is_wpr2_set()` helper in `regs.rs`**:
```rust
pub(crate) fn is_wpr2_set(self) -> bool {
    self.hi_val() != 0
}
```
This duplicates the logic already in `higher_bound() != 0` checks elsewhere, but having a named predicate is clearer for the unload path.

**`FwsecUnloadFirmware` enum**: Clean abstraction over the with/without-bootloader variants. The `run()` method delegates correctly to the underlying firmware's `run()` method.

**One potential issue**: In `Sec2UnloadBundle::run()`, `self.fwsec_sb.run(dev, bar, gsp_falcon)` is called, but looking at the method signature:
```rust
fn run(&self, dev: ..., bar: &Bar0, gsp_falcon: &Falcon<GspEngine>) -> Result<()>
```
This passes `(dev, bar, gsp_falcon)`, and internally dispatches to `fw.run(dev, gsp_falcon, bar)` — the parameter reordering is intentional and correct, matching the underlying `FwsecFirmware::run(&self, dev, falcon, bar)` signature.

---
Generated by Claude Code Patch Reviewer