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

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

Patch Review

This is the largest and most complex patch (320 lines added). It has several significant issues:

- **Comparison of unsigned with < 0 is always false:**
  ```c
  +	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
  +	    le32_to_cpu(context->rx->command_execute_resp.response_length) < 0) {
  ```
  `le32_to_cpu` returns `u32`, which is unsigned and can never be `< 0`. This check is dead code.

- **`sahara_command_execute_resp` calls both `sahara_command_execute_data` AND `sahara_command_execute` for the command ID list case:**
  ```c
  	sahara_command_execute_data(context, client_cmd);
  
  	if (client_cmd == SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST) {
  		sahara_command_execute(context, SAHARA_EXEC_CMD_GET_TRAINING_DATA);
  		return;
  	}
  ```
  This sends EXECUTE_DATA for the command list and then immediately sends another EXECUTE for training data, without waiting for the command list data to arrive. This seems like a race -- two TX packets are queued back-to-back without processing the response from the first.

- **GFP_KERNEL allocation from interrupt context.** `sahara_ctrl_trng_get()` is called from `sahara_mhi_dl_xfer_cb()` (the DL transfer callback), which runs in interrupt/softirq context. Inside `sahara_ctrl_trng_get()`:
  ```c
  ct = devres_alloc(sahara_ctrl_trng_release, sizeof(*ct), GFP_KERNEL);
  ```
  `GFP_KERNEL` can sleep, which is invalid in atomic context. Additionally, `mutex_init` and `devres_add` may also have issues here.

- **Race conditions on shared state.** Fields like `context->is_cmd_mode`, `context->receiving_trng_data`, `context->trng_rcvd`, etc. are accessed from both workqueue context and the DL callback (interrupt context) without any synchronization primitives. The `ct->lock` mutex protects the controller-scoped data but a mutex cannot be used from interrupt context -- `mutex_lock()` in `sahara_mhi_dl_xfer_cb` can sleep.

- **`kzalloc(resp_len, GFP_KERNEL)` with device-controlled size.** The `resp_len` comes directly from the device packet. There's no upper bound check, so a malicious or buggy device could request an arbitrarily large allocation:
  ```c
  ct->data = kzalloc(resp_len, GFP_KERNEL);
  ```
  Consider adding a sanity check on `resp_len`.

- **Accessing `ct->data` outside the lock:**
  ```c
  	mutex_unlock(&ct->lock);
  
  	if (!ct->data) {  // <-- outside lock
  ```
  After dropping the lock, another thread could modify `ct->data`. The NULL check should be inside the critical section.

- **`sahara_ctrl_trng_release` takes a mutex in devres release**, which might run during device teardown. Setting fields after `kfree` under the lock serves no purpose since the structure itself is being freed.

---
Generated by Claude Code Patch Reviewer