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: Thu, 23 Apr 2026 07:34:43 +1000
Message-ID: <review-patch5-20260422-nova-unload-v3-5-1d2c81bd3ced@nvidia.com>
In-Reply-To: <20260422-nova-unload-v3-5-1d2c81bd3ced@nvidia.com>
References: <20260422-nova-unload-v3-0-1d2c81bd3ced@nvidia.com> <20260422-nova-unload-v3-5-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 couple of observations**

This is the core functional patch. It adds:

1. A `PowerStateLevel` enum for the unload power state.
2. The `UnloadingGuestDriver` command (both fw-level and GSP-level wrappers=
).
3. `shutdown_gsp()` which sends the command then polls mailbox0 for the sus=
pended bit.
4. `unload()` method on `Gsp` that calls `shutdown_gsp()`.

**Observations:**

The `#[expect(unused)]` on `PowerStateLevel` is appropriate since only `Lev=
el0` is currently used; `Level3` and `Level7` are needed for future suspend=
/hibernate support.

The `shutdown_gsp` polling logic:

```rust
const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 =3D bits::bit_u32(31);
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(|_| ())
```

This correctly checks bit 31 (per the v3 changelog fix) with a 5-second tim=
eout and 10ms polling interval, which seems reasonable.

The `UnloadingGuestDriverReply` uses `type Message =3D ()` which means the =
reply content is ignored =E2=80=94 this is fine if the GSP reply to this co=
mmand carries no meaningful payload.

Minor: the `UnloadingGuestDriver` wrapper in `commands.rs` has the same nam=
e as the fw-level type in `fw::commands::UnloadingGuestDriver`. This is exa=
ctly the naming collision that patch 4 prepared for, and it works cleanly w=
ith the qualified paths.

The `AsBytes` safety comment for `UnloadingGuestDriver` says "Padding is ex=
plicit and will not contain uninitialized data" =E2=80=94 I verified the bi=
nding struct does have explicit `__bindgen_padding_0: [u8; 2usize]`, and th=
e `new()` uses `..Zeroable::zeroed()` which zero-initializes the padding. T=
his is correct.

---
Generated by Claude Code Patch Reviewer