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: Thu, 04 Jun 2026 16:57:33 +1000
Message-ID: <review-patch3-20260529-nova-unload-v7-3-678f39209e00@nvidia.com>
In-Reply-To: <20260529-nova-unload-v7-3-678f39209e00@nvidia.com>
References: <20260529-nova-unload-v7-0-678f39209e00@nvidia.com> <20260529-nova-unload-v7-3-678f39209e00@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 most substantial patch. It prepares the Booter Unloader and FWS=
EC-SB firmware at probe time (since filesystem may not be available at unbi=
nd time) and runs them during teardown to clear the WPR2 region.

**Observations:**

1. The `FwsecUnloadFirmware` enum handling the with/without bootloader vari=
ants 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 h=
eap 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 (re=
moves WPR2). This matches OpenRM's sequence.

4. The mailbox sentinel pattern in the Booter Unloader execution:
```rust
const MAILBOX_SENTINEL: u32 =3D 0xff;
let (mbox0, _) =3D sec2_falcon.boot(bar, Some(MAILBOX_SENTINEL), Some(MAILB=
OX_SENTINEL))?;
if mbox0 !=3D 0 {
    dev_err!(dev, "Booter Unloader returned error 0x{:x}\n", mbox0);
    return Err(EINVAL);
}
```
Writing `0xff` to both mailboxes and then checking mbox0 =3D=3D 0 for succe=
ss 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 =3D
    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 =E2=80=94 failing to prepare unload firmware shouldn't prevent th=
e 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 pub=
lic API. Clean encapsulation.

7. In `gpu.rs`, the unload path continues even if GSP shutdown fails:
```rust
let mut res =3D Self::shutdown_gsp(...)
    .inspect_err(|e| dev_err!(dev, "GSP shutdown failed: {:?}\n", e));

if let Some(unload_bundle) =3D unload_bundle {
    res =3D 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, bu=
t we still attempt the unload bundle. This is correct =E2=80=94 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() !=3D 0
}
```
This replaces `higher_bound() !=3D 0` with a semantically clearer method. N=
ote: `higher_bound()` shifts by 12, so checking `hi_val() !=3D 0` is equiva=
lent to checking `higher_bound() !=3D 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 =E2=80=94 the operation =
won't succeed on retry without external intervention (GPU reset). However, =
the cover letter explains this situation occurs only when bundle preparatio=
n 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