From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding
Date: Tue, 28 Apr 2026 15:01:48 +1000
Message-ID: <review-patch8-20260427-nova-unload-v4-8-e145ccddae66@nvidia.com>
In-Reply-To: <20260427-nova-unload-v4-8-e145ccddae66@nvidia.com>
References: <20260427-nova-unload-v4-0-e145ccddae66@nvidia.com> <20260427-nova-unload-v4-8-e145ccddae66@nvidia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the culmination of the series. Key points:

**UnloadBundle trait:**
```rust
pub(super) trait UnloadBundle: Send {
    fn run(&self, dev: ..., bar: ..., gsp_falcon: ..., sec2_falcon: ...) ->=
 Result;
}
```

The `Send` bound is important since the bundle is stored in the `Gsp` struc=
t and may be accessed from different contexts.

**FwsecUnloadFirmware enum** =E2=80=94 abstracts over with/without bootload=
er variants. Clean approach:
```rust
enum FwsecUnloadFirmware {
    WithoutBl(FwsecFirmware),
    WithBl(FwsecFirmwareWithBl),
}
```

**Sec2UnloadBundle::run() implementation:**
```rust
    fn run(&self, ...) -> Result<()> {
        self.fwsec_sb.run(dev, bar, gsp_falcon)?;
        let wpr2_hi =3D bar.read(regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI);
        if wpr2_hi.is_wpr2_set() {
            sec2_falcon.reset(bar)?;
            sec2_falcon.load(dev, bar, &self.booter_unloader)?;
            const MAILBOX_SENTINEL: u32 =3D 0xff;
            let (mbox0, _) =3D sec2_falcon.boot(bar, Some(MAILBOX_SENTINEL)=
, Some(MAILBOX_SENTINEL))?;
            ...
        }
        Ok(())
    }
```

The sequence is: (1) run FWSEC-SB to reset GSP falcon state, (2) run Booter=
 Unloader via SEC2 to remove WPR2. The sentinel value `0xff` in both mailbo=
xes aligns with the v4 changelog noting alignment with OpenRM.

**Checking WPR2 removal after Booter Unloader:**
```rust
            let wpr2_hi =3D bar.read(regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI);
            if wpr2_hi.is_wpr2_set() {
                dev_err!(dev, "WPR2 region still set after Booter Unloader =
returned\n");
                return Err(EBUSY);
            }
```
Good defensive check.

**Non-fatal unload bundle preparation:**
```rust
        let unload_bundle =3D Sec2UnloadBundle::build(...)
            .inspect_err(|e| { dev_warn!(...); })
            .ok();
```
This converts errors to `None`, meaning the driver loads successfully even =
if unload firmware can't be prepared. The three `dev_warn!` messages clearl=
y communicate the degraded state. This is a reasonable tradeoff =E2=80=94 f=
ailing to load entirely because the unload firmware is unavailable would be=
 too aggressive.

**Pin projection concern:** In `boot.rs`:
```rust
+        *self.as_mut().project().unload =3D unload_bundle;
```
This requires the `#[pin_data]` macro to generate a `project()` method and =
`unload` must not be a pinned field. Since `unload` is `Option<KBox<dyn hal=
::UnloadBundle>>` and is NOT marked `#[pin]` in the struct definition, this=
 should work correctly =E2=80=94 `project()` will give direct `&mut` access=
 to non-pinned fields.

**`is_wpr2_set()` helper** added to `regs.rs`:
```rust
    pub(crate) fn is_wpr2_set(self) -> bool {
        self.hi_val() !=3D 0
    }
```
This duplicates the logic already present in `higher_bound()` (which return=
s 0 when not set), but is more semantically clear. Fine addition.

**The `#[expect(unused)]` and `#[expect(dead_code)]` removals** from `Boote=
rKind::Unloader` and `FwsecCommand::Sb` are correct =E2=80=94 these variant=
s are now used.

**One observation:** The unload path in `boot.rs` calls `shutdown_gsp()` th=
en `unload_bundle.run()`. If `shutdown_gsp` succeeds but `unload_bundle.run=
()` fails (e.g., Booter Unloader errors), the GSP is stopped but WPR2 is no=
t removed. The GPU would then need a PCI reset before rebinding. This seems=
 unavoidable given the sequential nature of the unload, and the error is pr=
opagated to the caller.

No blocking issues. The implementation is solid and well-considered.

---
Generated by Claude Code Patch Reviewer