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: Mon, 25 May 2026 20:06:41 +1000
Message-ID: <review-patch3-20260521-nova-unload-v6-3-65f581c812c9@nvidia.com>
In-Reply-To: <20260521-nova-unload-v6-3-65f581c812c9@nvidia.com>
References: <20260521-nova-unload-v6-0-65f581c812c9@nvidia.com> <20260521-nova-unload-v6-3-65f581c812c9@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 functional patch. Several observations:

**`shutdown_gsp` implementation**: The polling approach with `read_poll_tim=
eout` for `LIBOS_INTERRUPT_PROCESSOR_SUSPENDED` (bit 31 of mailbox0) with a=
 5-second timeout and 10ms poll interval is reasonable.

**`PowerStateLevel::is_power_transition`**: Clean design =E2=80=94 only `Le=
vel0` is a full unload, others preserve state:
```rust
pub(crate) fn is_power_transition(self) -> bool {
    self !=3D PowerStateLevel::Level0
}
```

**`UnloadingGuestDriverReply`**: The reply type uses `type Message =3D ()`,=
 meaning the GSP sends no payload in its reply. This is consistent with Ope=
nRM's behavior for this RPC.

**`unbind` in `driver.rs`**: The `unbind` callback receives `dev: &'bound p=
ci::Device<kernel::device::Core>` =E2=80=94 note it's `device::Core`, not `=
device::Bound`. This is intentional since at unbind time the device is bein=
g unbound. The `gpu.rs` `unbind` method correctly calls `pdev.as_ref()` to =
get a `device::Device<device::Bound>` for the `unload` call =E2=80=94 wait,=
 actually `pci::Device<Core>` would yield `device::Device<Core>` via `as_re=
f()`. Let me check:

```rust
pub(crate) fn unbind(&self, pdev: &'bound pci::Device<device::Core>) {
    let _ =3D self
        .gsp
        .unload(pdev.as_ref(), self.bar, &self.gsp_falcon)
```

And `unload` takes `dev: &device::Device<device::Bound>`. This would be a t=
ype mismatch if `pdev.as_ref()` returns `&device::Device<Core>` rather than=
 `&device::Device<Bound>`. However, the Device HRT framework may provide th=
e right conversions here. This could be a compile-time issue but since the =
author presumably tested this, the types likely work out.

**Minor**: The `#[expect(unused)]` on the `PowerStateLevel` enum is good pr=
actice since `Level3` and `Level7` aren't used yet.

---

---
Generated by Claude Code Patch Reviewer