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: fsp: wait to consume message before sending another
Date: Sun, 12 Apr 2026 11:19:36 +1000
Message-ID: <review-patch1-20260409-b4-blackwell-unload-v1-1-0f5a2ff838dd@nvidia.com>
In-Reply-To: <20260409-b4-blackwell-unload-v1-1-0f5a2ff838dd@nvidia.com>
References: <20260409-b4-blackwell-unload-v1-0-0f5a2ff838dd@nvidia.com> <20260409-b4-blackwell-unload-v1-1-0f5a2ff838dd@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

**Purpose:** Before sending a new message to FSP via EMEM, poll until the F=
SP's queue head equals its queue tail, indicating the previous message has =
been consumed.

The diff adds:

```rust
+        // Wait for FSP to consume any previous message before sending.
+        read_poll_timeout(
+            || {
+                let head =3D bar.read(regs::NV_PFSP_QUEUE_HEAD).address().=
get();
+                let tail =3D bar.read(regs::NV_PFSP_QUEUE_TAIL).address().=
get();
+                Ok(head =3D=3D tail)
+            },
+            |&ready| ready,
+            Delta::from_millis(1),
+            Delta::from_secs(2),
+        )?;
```

**Observations:**

- **Correct use of `read_poll_timeout`**: The signature is `(op, cond, slee=
p_delta, timeout_delta) -> Result<T>`. The `op` closure returns `Ok(bool)`,=
 the `cond` checks `|&ready| ready`, and the `?` propagates the `ETIMEDOUT`=
 error. This matches the established pattern used in `gfw.rs` and elsewhere=
 in nova-core.

- **Timeout value (2 seconds)**: This is reasonable for FSP message consump=
tion. If the FSP is alive and functioning, it should consume messages quick=
ly. 2 seconds is generous enough to handle edge cases without stalling the =
driver indefinitely.

- **No error context on timeout**: Unlike patch 3 which uses `.inspect_err(=
)` to log a message on timeout, this poll silently returns `ETIMEDOUT`. Con=
sider adding an `.inspect_err()` call to log a message like `"FSP did not c=
onsume previous message"` =E2=80=94 this would help debug unload/reload fai=
lures in the field. Minor nit, not a blocker.

- **Placement before `write_emem`**: This is the correct location =E2=80=94=
 ensuring the queue is drained before writing new data to EMEM and updating=
 the queue tail.

**Verdict:** Looks good. The only suggestion is adding error logging on tim=
eout for consistency with other error paths in the driver.

---

---
Generated by Claude Code Patch Reviewer