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 08:52:56 +1000 Message-ID: In-Reply-To: <20260421-nova-unload-v2-5-2fe54963af8b@nvidia.com> References: <20260421-nova-unload-v2-0-2fe54963af8b@nvidia.com> <20260421-nova-unload-v2-5-2fe54963af8b@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 completes the unload sequence by tearing down the WPR2 region. 1. **Removing dead_code/unused annotations** (`firmware/booter.rs`, `firmwa= re/fwsec.rs`): ```rust - #[expect(unused)] Unloader, ``` ```rust - #[expect(dead_code)] Sb, ``` These variants were pre-declared but unused until now. Clean removal. 2. **WPR2 check helper** (`regs.rs`): ```rust + pub(crate) fn is_wpr2_set(self) -> bool { + self.hi_val() !=3D 0 + } ``` Simple and correct =E2=80=94 the existing comment on `higher_bound()` says = "A value of zero means the WPR2 region is not set." 3. **Full unload sequence** (`gsp/boot.rs`): The unload now does: - Shut down GSP (from patch 4) - Run FWSEC-SB to reset the GSP falcon to pre-libos state - If WPR2 is still set, run Booter Unloader to remove it - Verify WPR2 was cleared The FWSEC-SB and Booter Unloader loading mirrors the boot-time pattern in `= boot()` / `run_fwsec_frts()`, using the same `needs_fwsec_bootloader()` dis= patch. This symmetry with the boot path is good. ```rust + let bios =3D Vbios::new(dev, bar)?; + let fwsec_sb =3D FwsecFirmware::new(dev, gsp_falcon, bar, &bios, F= wsecCommand::Sb)?; ``` One thing to note: `Vbios::new()`, `FwsecFirmware::new()`, and `BooterFirmw= are::new()` all request firmware from the filesystem. If firmware loading f= ails during unbind (e.g., firmware files have been removed), the unload wil= l fail with an error and the WPR2 region will remain set, preventing re-pro= be. The `?` propagation means these failures are reported via `warn_on_err!= ` in the caller. This is probably the best that can be done =E2=80=94 you c= an't unload without the firmware =E2=80=94 but it's a dependency worth docu= menting. ```rust + let _ =3D sec2_falcon.boot(bar, Some(0xff), Some(0xff))?; ``` The `let _ =3D` discards the boot return value (a mailbox read result). The= `Some(0xff)` arguments for both mailbox values appear to be "don't care" s= entinel values (matching the pattern used during boot in the sequencer). Th= e `?` propagation means if SEC2 fails to boot the unloader, we bail out. ```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= ran\n"); + return Err(EBUSY); + } ``` Good defensive check. If the booter unloader ran but didn't actually clear = WPR2, returning `EBUSY` is appropriate. 4. **Updated unload signature** (`gpu.rs`): ```rust + let _ =3D kernel::warn_on_err!(self.gsp.unload( + dev, + bar, + self.spec.chipset, + &self.gsp_falcon, + &self.sec2_falcon, + )); ``` Passing chipset and sec2_falcon through is clean =E2=80=94 they're needed f= or the FWSEC-SB bootloader dispatch and Booter Unloader respectively. **Minor observation on error handling in unbind path:** Both the FWSEC-SB a= nd Booter Unloader steps use `?` internally, which means any failure in the= unload sequence will abort the remaining steps but still continue to `sysm= em_flush.unregister()` thanks to the `let _ =3D warn_on_err!(...)` in `unbi= nd()`. This is reasonable best-effort teardown. If FWSEC-SB fails, attempti= ng the Booter Unloader would likely also fail, so the early return within `= unload()` makes sense. No blocking issues. --- Generated by Claude Code Patch Reviewer