Claude review: bus: mhi: Capture DDR training data using command mode

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

Patch Review

This is the most substantial and most problematic patch (328 lines added).

**Bugs:**
1. **Tautological comparison — always true:**
   ```c
   le32_to_cpu(context->rx->command_execute_resp.response_length) < 0
   ```
   `le32_to_cpu` returns a `u32`, which is unsigned and can never be less than 0. This check does nothing. If the intent is to validate the response length, it should check against a maximum or ensure it's non-zero.

2. **Race condition in `sahara_command_execute_resp`:** The function sends `sahara_command_execute_data()` BEFORE processing the response. For `GET_COMMAND_ID_LIST`, it sends the execute_data request and then *also* sends a new `sahara_command_execute(TRAINING_DATA)` request. This means two TX packets are queued back-to-back using `context->tx[0]` — the second write to `tx[0]` will overwrite the first before it may have been DMA'd. This is a potential data corruption issue since `tx[0]` is used as the buffer for both sends.

3. **`ct->data` NULL check outside lock:** After calling `kzalloc` under the lock and releasing it:
   ```c
   mutex_lock(&ct->lock);
   kfree(ct->data);
   ct->data = kzalloc(resp_len, GFP_KERNEL);
   ...
   mutex_unlock(&ct->lock);

   if (!ct->data) {
   ```
   The `ct->data` check is done *after* releasing the lock. Another thread could theoretically modify `ct->data` between the unlock and the check. The NULL check should be inside the lock, or the return value should be saved.

4. **`mutex_lock` in DL callback context:** The `sahara_mhi_dl_xfer_cb` callback takes `ct->lock` (a mutex). MHI DL transfer callbacks may be called in softirq/tasklet context (depending on the MHI implementation). If so, `mutex_lock` would be illegal in that context — it should use a spinlock instead, or the data copy should be deferred to a workqueue.

5. **Training data larger than `SAHARA_NUM_CMD_BUF * SAHARA_PACKET_MAX_SIZE`:** If `resp_len` exceeds the total capacity of the pre-allocated cmd buffers, the loop at line 752-762 will stop early, `remaining` will be non-zero, and the data transfer will be incomplete. No error is reported for this case, and `trng_size` still reflects the full requested size, leading to a partial/corrupt capture.

6. **`cancel_work_sync(&context->read_data_work)` missing in remove:** The original code has `read_data_work` but the remove function only cancels `fw_work`, `dump_work`, and `cmd_work`. The `read_data_work` cancel is missing (pre-existing bug, but patch 7 adds `cmd_work` cancel without fixing this).

**Design concerns:**
- The DL callback does pointer comparison (`mhi_result->buf_addr == context->cmd_buff[i]`) to distinguish training data from control packets. This is fragile and relies on MHI always returning the exact same pointer.

- The command mode state machine is not explicitly modeled. The various boolean flags (`is_cmd_mode`, `receiving_trng_data`) create implicit states that are easy to get wrong.

---
Generated by Claude Code Patch Reviewer