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

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

Patch Review

The core locking patch. The key design decisions:

1. **Inner/Outer split**: `CmdqInner` holds `dev`, `seq`, and `gsp_mem` behind a `Mutex`. `Cmdq` holds `Mutex<CmdqInner>` plus constants.

2. **Static methods moved to outer**: `calculate_checksum` and `notify_gsp` are pure functions / don't access mutable state, so they stay on `Cmdq`. The various offset/size constants also stay on `Cmdq`. This is correct since they don't need mutex protection.

3. **Mutex held across send+receive**: For `send_command`, the mutex is held for both send and receive to prevent reply mismatches. This is documented:

```rust
/// The mutex is held for the entire send+receive cycle to ensure that no other command can
/// be interleaved.
```

This is the right approach — releasing the lock between send and receive would allow another thread to send a command and steal the reply.

4. **`&self` instead of `&mut self`**: All public methods now take `&self`, with interior mutability via the mutex. This is the whole point of the series.

**Concern 1**: `dma_handle()` locks the mutex just to read `gsp_mem`:

```rust
pub(crate) fn dma_handle(&self) -> DmaAddress {
    self.inner.lock().gsp_mem.0.dma_handle()
}
```

The DMA handle is determined at allocation time and never changes. Locking the mutex here adds unnecessary contention. Consider keeping `gsp_mem` (or at least the DMA handle) outside the mutex in `Cmdq` directly. The `gsp_mem` field could be in the outer struct since it's initialized once and never mutated — only `seq` truly needs mutex protection (and the queue read/write pointers accessed through `gsp_mem`). However, since `gsp_mem` provides access to the shared memory that is read/written during send/receive, keeping it inside the mutex is arguably safer. The DMA handle accessor is likely only called during init, so the performance concern is negligible. This is a minor nit.

**Concern 2**: The `boot()` method signature change from `mut self: Pin<&mut Self>` to `self: Pin<&mut Self>` (line 1414-1415) is correct since `cmdq` methods no longer need `&mut self`.

**Concern 3**: `wait_gsp_init_done` and the sequencer's `receive_msg` calls now lock/unlock the mutex on each iteration of their retry loops. For example:

```rust
pub(crate) fn wait_gsp_init_done(cmdq: &Cmdq) -> Result {
    loop {
        match cmdq.receive_msg::<GspInitDone>(Cmdq::RECEIVE_TIMEOUT) {
            ...
            Err(ERANGE) => continue,
        }
    }
}
```

Each call to `cmdq.receive_msg` locks the mutex, receives one message, then unlocks. On `ERANGE`, it loops and re-locks. This means another thread could interleave between iterations and consume the message meant for this caller. This is a potential issue for `wait_gsp_init_done` and `GspSequencer::run`, which use the standalone `receive_msg` pattern (not the `send_command` which holds the lock). However, during boot these are likely single-threaded, so this is not an immediate problem. The cover letter acknowledges this is for the "init done" waiting during boot where there's no concurrent access. Still, this is a design point to be aware of as usage expands.

Overall, this is a clean and well-thought-out series. The type-level distinction between reply/no-reply commands is elegant and the locking strategy is sound for the current use case.

---
Generated by Claude Code Patch Reviewer