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: send UNLOADING_GUEST_DRIVER GSP command upon unloading
Date: Tue, 28 Apr 2026 15:01:47 +1000
Message-ID: <review-patch4-20260427-nova-unload-v4-4-e145ccddae66@nvidia.com>
In-Reply-To: <20260427-nova-unload-v4-4-e145ccddae66@nvidia.com>
References: <20260427-nova-unload-v4-0-e145ccddae66@nvidia.com> <20260427-nova-unload-v4-4-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 core behavioral change. Key observations:

**GSP shutdown implementation** in `boot.rs`:
```rust
+    fn shutdown_gsp(
+        cmdq: &Cmdq,
+        bar: &Bar0,
+        gsp_falcon: &Falcon<Gsp>,
+        mode: commands::PowerStateLevel,
+    ) -> Result<()> {
+        cmdq.send_command(bar, commands::UnloadingGuestDriver::new(mode))?;
+        const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 =3D bits::bit_u32(3=
1);
+        read_poll_timeout(
+            || Ok(gsp_falcon.read_mailbox0(bar)),
+            |&mb0| mb0 & LIBOS_INTERRUPT_PROCESSOR_SUSPENDED !=3D 0,
+            Delta::from_millis(10),
+            Delta::from_secs(5),
+        )
+        .map(|_| ())
+    }
```

The 5-second timeout for GSP suspension seems reasonable. The poll checks b=
it 31 of mailbox0, which aligns with OpenRM's `LIBOS_INTERRUPT_PROCESSOR_SU=
SPENDED`.

**Error handling in `unload()`** =E2=80=94 uses `.inspect_err()` then `?`, =
meaning a GSP shutdown failure prevents sysmem flush unregistration. But in=
 `gpu.rs`:
```rust
+        let _ =3D self
+            .gsp
+            .unload(dev, bar, &self.gsp_falcon)
+            .inspect_err(|e| dev_err!(dev, "failed to unload GSP: {:?}\n",=
 e));
+        self.sysmem_flush.unregister(bar);
```
The `let _ =3D` ensures sysmem flush unregistration always happens regardle=
ss of unload failure. This is correct and the comment in the diff confirms =
this intent.

**PowerStateLevel enum** =E2=80=94 uses `#[expect(unused)]` on the enum but=
 only `Level0` is used. The `Level3` and `Level7` variants are for future s=
uspend/hibernate support. This is fine.

**UnloadingGuestDriver command** =E2=80=94 the `fw::commands::UnloadingGues=
tDriver` wrapper uses `#[repr(transparent)]` over the binding struct, which=
 is correct. The `is_power_transition()` method returning `self !=3D Level0=
` is clear.

**Minor note:** The `UnloadingGuestDriverReply` in `commands.rs` has `type =
Message =3D ()` =E2=80=94 this means the reply carries no payload. This mat=
ches OpenRM behavior where the reply is just an acknowledgment.

No blocking issues. Well implemented.

---

---
Generated by Claude Code Patch Reviewer