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

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

Patch Review

**Status: Mostly good, a few observations**

This is the final functional patch that completes the unload sequence by running FWSEC-SB and Booter Unloader. The structure mirrors the boot path nicely (FWSEC-FRTS → Booter Loader at boot becomes FWSEC-SB → Booter Unloader at unbind).

The FWSEC-SB section:

```rust
let bios = Vbios::new(dev, bar)?;
let fwsec_sb = FwsecFirmware::new(dev, gsp_falcon, bar, &bios, FwsecCommand::Sb)?;

if chipset.needs_fwsec_bootloader() {
    let fwsec_sb_bl = FwsecFirmwareWithBl::new(fwsec_sb, dev, chipset)?;
    fwsec_sb_bl.run(dev, gsp_falcon, bar)?;
} else {
    fwsec_sb.run(dev, gsp_falcon, bar)?;
}
```

This closely follows the `run_fwsec_frts` pattern from the boot path, which is good.

The Booter Unloader section:

```rust
const MAILBOX_SENTINEL: u32 = 0xff;
let (mbox0, _) = sec2_falcon.boot(bar, Some(MAILBOX_SENTINEL), None)?;
if mbox0 != 0 {
    dev_err!(dev, "Booter Unloader returned error 0x{:x}\n", mbox0);
    return Err(EINVAL);
}
```

The sentinel value `0xff` is documented per the v3 changelog as a value written to mailbox0 before booting to confirm the unloader actually ran and overwrote it. The check `mbox0 != 0` verifies success.

**Observations:**

1. **Error handling in unload path**: The `unload()` method uses `?` to propagate errors from FWSEC-SB and Booter Unloader operations. Back in `gpu.rs`, `unload()` errors are caught by `warn_on_err!` and discarded with `let _ =`. This means that if FWSEC-SB fails after GSP shutdown has already succeeded, the function returns early without reaching `sysmem_flush.unregister()`. This seems intentional — if we can't reset the falcon state, it's unclear whether it's safe to proceed. But it does mean the sysmem flush page might remain registered in a partially-torn-down state. Worth considering whether `sysmem_flush.unregister()` should always run regardless.

2. **WPR2 check uses `hi_val() != 0`**: The `is_wpr2_set()` method checks whether the high address is non-zero. This matches how the boot path checks for WPR2 existence (`higher_bound() != 0` at line 62 of boot.rs). Consistent.

3. **Removing `#[expect(unused)]` and `#[expect(dead_code)]`**: The patch removes the `#[expect(unused)]` from `BooterKind::Unloader` and `#[expect(dead_code)]` from `FwsecCommand::Sb`, since both are now used. Clean.

4. **Re-reading VBIOS during unload**: `Vbios::new(dev, bar)?` is called during unload, which re-reads the VBIOS from the GPU ROM. The VBIOS was already read during boot (in `run_fwsec_frts`). This is a necessary re-read since the original VBIOS object wasn't stored, and it's the correct approach — caching it would add complexity for no meaningful performance benefit during a one-shot unbind operation.

Overall, this is a solid and well-organized series. The main architectural question is item 1 above — whether `sysmem_flush.unregister()` should be reached even if the FWSEC-SB or Booter Unloader steps fail during unload, since leaving it registered in a partially torn-down state could be problematic. The rest is clean and follows existing patterns correctly.

---
Generated by Claude Code Patch Reviewer