Claude review: gpu: nova-core: gsp: add mutex locking to Cmdq

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Status: Looks good, a few observations**

This is the main patch, splitting `Cmdq` into an outer struct (holding `dev` and the `Mutex`) and `CmdqInner` (holding mutable state: `seq` and `gsp_mem`).

The split is well-motivated:
```rust
+struct CmdqInner {
+    seq: u32,
+    gsp_mem: DmaGspMem,
+}
+
+pub(crate) struct Cmdq {
+    dev: ARef<device::Device>,
+    #[pin]
+    inner: Mutex<CmdqInner>,
+}
```

Moving `dev` outside the mutex is correct — it's `ARef` (immutable, reference-counted) and doesn't need mutex protection.

The `send_sync_command` correctly holds the lock across send+receive:
```rust
+    pub(crate) fn send_sync_command<M>(&self, bar: &Bar0, command: M) -> Result<M::Reply>
+    ...
+    {
+        let mut inner = self.inner.lock();
+        inner.send_command(&self.dev, bar, command)?;
+        loop {
+            match inner.receive_msg::<M::Reply>(&self.dev, Delta::from_secs(10)) {
```
This ensures no interleaving of commands and their replies. Methods now take `&self` instead of `&mut self`, which is the whole point.

Moving `calculate_checksum` and `notify_gsp` to `Cmdq` (rather than `CmdqInner`) makes sense since they don't use any mutable state — they're effectively static methods.

**Observations:**

1. **`dma_handle()` takes the lock unnecessarily:**
```rust
+    pub(crate) fn dma_handle(&self) -> DmaAddress {
+        self.inner.lock().gsp_mem.0.dma_handle()
+    }
```
The DMA handle is set once during allocation and never changes. This could instead store the DMA handle in the outer `Cmdq` struct at construction time, avoiding the lock. However, this is a minor performance point and correctness is not affected.

2. **`receive_msg` is exposed as `pub(crate)` on `Cmdq`:**
```rust
+    pub(crate) fn receive_msg<M: MessageFromGsp>(&self, timeout: Delta) -> Result<M>
+    ...
+    {
+        self.inner.lock().receive_msg(&self.dev, timeout)
+    }
```
This is used by `wait_gsp_init_done` and `GspSequencer::run` which receive messages without a preceding send. The lock is acquired and released per call, which is correct for these use cases. However, if someone were to call this in a loop (as `wait_gsp_init_done` does), each iteration re-acquires the lock — this is fine for correctness and allows other operations to interleave.

3. **Thread passing `dev` through method parameters:** The `CmdqInner` methods now take `dev: &device::Device` as a parameter instead of accessing `self.dev`, since `dev` lives outside the inner struct. This is a clean pattern that avoids the need for the inner struct to hold a reference back to the outer struct.

4. **Formatting nit** in the constructor:
```rust
+        try_pin_init!(Self {
+            inner <- new_mutex!(CmdqInner {
+                gsp_mem: DmaGspMem::new(dev)?,
+                seq: 0,
+                }),
+            dev: dev.into(),
+        })
```
The closing `})` for `new_mutex!` has unusual indentation — the `)` is at 16 spaces indent while the opening `new_mutex!` is at 24. This is a very minor style nit.

5. The `boot()` signature change from `mut self: Pin<&mut Self>` to `self: Pin<&mut Self>` reflects that `Cmdq` methods no longer require `&mut self`:
```rust
-        mut self: Pin<&mut Self>,
+        self: Pin<&mut Self>,
```

Overall the locking design is correct and well-documented. Already has Reviewed-by from Zhi Wang.

---
Generated by Claude Code Patch Reviewer