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 most substantial patch. It prepares the Booter Unloader and FWSEC-SB firmware at probe time (since filesystem may not be available at unbind time) and runs them during teardown to clear the WPR2 region.

**Observations:**

1. The `FwsecUnloadFirmware` enum handling the with/without bootloader variants is clean:
```rust
enum FwsecUnloadFirmware {
    WithoutBl(FwsecFirmware),
    WithBl(FwsecFirmwareWithBl),
}
```
This avoids conditional logic at run time and makes both paths type-safe.

2. The `Sec2UnloadBundle` correctly pre-loads firmware at boot:
```rust
fn build(...) -> Result<KBox<dyn UnloadBundle>> {
    KBox::new(
        Self {
            fwsec_sb: FwsecUnloadFirmware::new(dev, bar, chipset, bios, gsp_falcon)?,
            booter_unloader: BooterFirmware::new(dev, BooterKind::Unloader, ...)?
        },
        GFP_KERNEL,
    )
    .map(|b| b as KBox<dyn UnloadBundle>)
    .map_err(Into::into)
}
```
Using `KBox<dyn UnloadBundle>` for the trait object is correct for kernel heap allocation.

3. The unload bundle run sequence is well-ordered:
```rust
fn run(&self, dev, bar, gsp_falcon, sec2_falcon) -> Result<()> {
    // 1. Run FWSEC-SB to reset GSP falcon
    self.fwsec_sb.run(dev, bar, gsp_falcon)?;
    // 2. Remove WPR2 via Booter Unloader
    if wpr2_hi.is_wpr2_set() { ... }
}
```
FWSEC-SB first (resets falcon to pre-libos state), then Booter Unloader (removes WPR2). This matches OpenRM's sequence.

4. The mailbox sentinel pattern in the Booter Unloader execution:
```rust
const MAILBOX_SENTINEL: u32 = 0xff;
let (mbox0, _) = sec2_falcon.boot(bar, Some(MAILBOX_SENTINEL), Some(MAILBOX_SENTINEL))?;
if mbox0 != 0 {
    dev_err!(dev, "Booter Unloader returned error 0x{:x}\n", mbox0);
    return Err(EINVAL);
}
```
Writing `0xff` to both mailboxes and then checking mbox0 == 0 for success matches the pattern described in prior versions' changelogs as aligning with OpenRM behavior. Good.

5. **Important design decision**: The unload bundle preparation failure is non-fatal during boot:
```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` via `.ok()` and continuing boot is the right tradeoff — failing to prepare unload firmware shouldn't prevent the GPU from being used, just means manual reset will be needed.

6. The `UnloadBundle` wrapper type in `gsp.rs`:
```rust
pub(crate) struct UnloadBundle(KBox<dyn hal::UnloadBundle>);
```
This provides an opaque boundary between the HAL-internal trait and the public API. Clean encapsulation.

7. In `gpu.rs`, the unload path continues even if GSP shutdown fails:
```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 shutdown fails, the first error is preserved, but we still attempt the unload bundle. This is correct — even if the GSP didn't respond to the shutdown command, the booter unloader / FWSEC-SB may still succeed at resetting hardware state.

8. The `is_wpr2_set` helper is a trivial but useful abstraction:
```rust
pub(crate) fn is_wpr2_set(self) -> bool {
    self.hi_val() != 0
}
```
This replaces `higher_bound() != 0` with a semantically clearer method. Note: `higher_bound()` shifts by 12, so checking `hi_val() != 0` is equivalent to checking `higher_bound() != 0` and avoids the unnecessary shift. Slightly more efficient, though it doesn't matter in practice.

9. Minor: `unload` now warns if no unload bundle is available and returns `Err(EAGAIN)`. `EAGAIN` seems slightly unusual here — the operation won't succeed on retry without external intervention (GPU reset). However, the cover letter explains this situation occurs only when bundle preparation fails, and the user was already warned at boot time. The error code won't propagate to userspace in the normal flow (it's in `PinnedDrop`), so this is mostly for internal tracking. Acceptable.

**No issues.** This is solid work.

---

---
Generated by Claude Code Patch Reviewer