From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding Date: Thu, 23 Apr 2026 07:34:43 +1000 Message-ID: In-Reply-To: <20260422-nova-unload-v3-6-1d2c81bd3ced@nvidia.com> References: <20260422-nova-unload-v3-0-1d2c81bd3ced@nvidia.com> <20260422-nova-unload-v3-6-1d2c81bd3ced@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 **Status: Mostly good, a few observations** This is the final functional patch that completes the unload sequence by ru= nning FWSEC-SB and Booter Unloader. The structure mirrors the boot path nic= ely (FWSEC-FRTS =E2=86=92 Booter Loader at boot becomes FWSEC-SB =E2=86=92 = Booter Unloader at unbind). The FWSEC-SB section: ```rust let bios =3D Vbios::new(dev, bar)?; let fwsec_sb =3D FwsecFirmware::new(dev, gsp_falcon, bar, &bios, FwsecComma= nd::Sb)?; if chipset.needs_fwsec_bootloader() { let fwsec_sb_bl =3D 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 =3D 0xff; let (mbox0, _) =3D sec2_falcon.boot(bar, Some(MAILBOX_SENTINEL), None)?; if mbox0 !=3D 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 wri= tten to mailbox0 before booting to confirm the unloader actually ran and ov= erwrote it. The check `mbox0 !=3D 0` verifies success. **Observations:** 1. **Error handling in unload path**: The `unload()` method uses `?` to pro= pagate errors from FWSEC-SB and Booter Unloader operations. Back in `gpu.rs= `, `unload()` errors are caught by `warn_on_err!` and discarded with `let _= =3D`. This means that if FWSEC-SB fails after GSP shutdown has already suc= ceeded, the function returns early without reaching `sysmem_flush.unregiste= r()`. This seems intentional =E2=80=94 if we can't reset the falcon state, = it's unclear whether it's safe to proceed. But it does mean the sysmem flus= h page might remain registered in a partially-torn-down state. Worth consid= ering whether `sysmem_flush.unregister()` should always run regardless. 2. **WPR2 check uses `hi_val() !=3D 0`**: The `is_wpr2_set()` method checks= whether the high address is non-zero. This matches how the boot path check= s for WPR2 existence (`higher_bound() !=3D 0` at line 62 of boot.rs). Consi= stent. 3. **Removing `#[expect(unused)]` and `#[expect(dead_code)]`**: The patch r= emoves the `#[expect(unused)]` from `BooterKind::Unloader` and `#[expect(de= ad_code)]` from `FwsecCommand::Sb`, since both are now used. Clean. 4. **Re-reading VBIOS during unload**: `Vbios::new(dev, bar)?` is called du= ring unload, which re-reads the VBIOS from the GPU ROM. The VBIOS was alrea= dy read during boot (in `run_fwsec_frts`). This is a necessary re-read sinc= e the original VBIOS object wasn't stored, and it's the correct approach = =E2=80=94 caching it would add complexity for no meaningful performance ben= efit during a one-shot unbind operation. Overall, this is a solid and well-organized series. The main architectural = question is item 1 above =E2=80=94 whether `sysmem_flush.unregister()` shou= ld be reached even if the FWSEC-SB or Booter Unloader steps fail during unl= oad, since leaving it registered in a partially torn-down state could be pr= oblematic. The rest is clean and follows existing patterns correctly. --- Generated by Claude Code Patch Reviewer