Claude review: misc: fastrpc: Add polling mode support for fastRPC driver

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

Patch Review

**Overall: Needs attention on several points.**

#### 1. Concurrency issue with `is_work_done` (important)

`ctx->is_work_done` is written from two concurrent contexts without synchronization:
- The rpmsg callback (interrupt context): `ctx->is_work_done = true;` then `complete(&ctx->work);`
- The polling loop: reads `ctx->is_work_done` in `read_poll_timeout_atomic`, writes `ctx->is_work_done = true` in `poll_for_remote_response`

```c
ctx->is_work_done = true;    // in rpmsg_callback
complete(&ctx->work);
```

```c
ret = read_poll_timeout_atomic(fastrpc_read_poll_addr, val,
    (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, ...);
if (!ret && val == FASTRPC_POLL_RESPONSE) {
    ctx->is_work_done = true;
    ctx->retval = 0;
}
```

`is_work_done` is a plain `bool` — it should be accessed with `READ_ONCE`/`WRITE_ONCE` or be an `atomic_t` to avoid data races. The compiler could cache the value in a register during the poll loop, causing the callback's write to be missed. While `read_poll_timeout_atomic` does use `READ_ONCE` on the poll value, the `ctx->is_work_done` read in the condition expression may not get the same treatment.

#### 2. Memory barrier placement in `fastrpc_read_poll_addr`

```c
static u32 fastrpc_read_poll_addr(struct fastrpc_invoke_ctx *ctx)
{
    dma_rmb();
    return READ_ONCE(*ctx->poll_addr);
}
```

The `dma_rmb()` is called before every single read inside `read_poll_timeout_atomic`. While technically correct for ordering, this is a busy-poll loop with 1 µs delay — the barrier on every iteration may add unnecessary overhead. More importantly, the `dma_rmb()` ensures ordering of DMA reads, but a matching `dma_wmb()` on the DSP side is assumed. This is fine as a convention, just worth noting.

#### 3. Race between poll completion and rpmsg callback

When `poll_for_remote_response` succeeds (val == FASTRPC_POLL_RESPONSE), it sets:
```c
ctx->is_work_done = true;
ctx->retval = 0;
```

But the rpmsg callback may still fire later (the DSP sends both a poll response AND an rpmsg callback), and overwrite `ctx->retval` with `rsp->retval`:
```c
ctx->retval = rsp->retval;
ctx->is_work_done = true;
complete(&ctx->work);
```

Then `schedule_work(&ctx->put_work)` is called in the callback, which may race with the normal cleanup path after `fastrpc_internal_invoke` returns. The `put_work` path does `fastrpc_context_put()` which decrements the refcount, so there shouldn't be a use-after-free if refcounting is correct — but this deserves verification. Does the context hold enough refcount references to survive both the poll completion path and the deferred callback?

#### 4. `poll_addr` alignment and buffer layout

```c
ctx->poll_addr = (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLIST +
                     sizeof(u32) * FASTRPC_MAX_CRCLIST);
```

The `sizeof(u32)` added to `fastrpc_get_meta_size` accounts for the poll address word. The fdlist is `u64[]` (8-byte aligned), FASTRPC_MAX_FDLIST=16, FASTRPC_MAX_CRCLIST=64, so the offset is `16*8 + 64*4 = 128 + 256 = 384` bytes from fdlist, which is 4-byte aligned. This is fine for a `u32*`.

However, this `poll_addr` is within the DMA-coherent buffer (`ctx->buf`). The DSP writes `FASTRPC_POLL_RESPONSE` to this address. Is there a guarantee that the DSP knows the layout and will write to this exact offset? The commit message and code don't document how the DSP discovers this address. Presumably this is part of the FastRPC protocol, but it would be good to document.

#### 5. UAPI design: `__IOWR` vs `__IOW`

```c
#define FASTRPC_IOCTL_SET_OPTION  _IOWR('R', 12, struct fastrpc_ioctl_set_option)
```

This uses `_IOWR` (read+write), but `fastrpc_set_option` only does `copy_from_user` — it never writes back to userspace. `_IOW` would be more appropriate. Using `_IOWR` is not incorrect but is misleading about the direction.

#### 6. UAPI: gap in ioctl numbering

The new ioctl uses number 12, and the existing `FASTRPC_IOCTL_GET_DSP_INFO` uses 13. This fills the gap (number 12 was previously unused), which is fine.

#### 7. `__maybe_unused` on `fastrpc_poll_supported_machines`

```c
static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unused = {
```

The `__maybe_unused` is needed because `of_machine_get_match()` is a no-op returning NULL when `CONFIG_OF` is disabled, and the compiler would warn about the unused array. This is correct.

#### 8. `is_polled` set after `fastrpc_invoke_send`

```c
if (err)
    goto bail;

if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD && fl->poll_mode)
    ctx->is_polled = true;

err = fastrpc_wait_for_completion(ctx, kernel);
```

The `is_polled` flag is set *after* `fastrpc_invoke_send` has already sent the message. This means the DSP may have already started writing to `poll_addr` before the flag is checked. Since `poll_addr` is initialized to 0 (DMA buffer is zeroed) and the DSP writes `FASTRPC_POLL_RESPONSE` on completion, this ordering is safe — the poll loop will just pick up the response. But it does mean there's a small window where the rpmsg callback could arrive and set `is_work_done` before the poll even starts, which is handled correctly since the poll condition checks `ctx->is_work_done`.

#### 9. Error code conversion

```c
if (ret == -ETIMEDOUT)
    ret = -EIO;
```

Converting `ETIMEDOUT` to `EIO` in `poll_for_remote_response` means the poll timeout is not distinguishable from an actual I/O error. Since the caller falls back to the completion-based path on any error from polling, this is acceptable, but the `-EIO` return is never actually used since the fallback path overwrites `err`:

```c
err = poll_for_remote_response(ctx);
if (!err)
    return 0;
ctx->is_polled = false;
return fastrpc_wait_for_response(ctx, kernel);
```

The `-EIO` conversion is dead code since the error value is discarded. Consider just returning any non-zero value or keeping `ETIMEDOUT`.

#### 10. No locking on `fl->poll_mode`

`fl->poll_mode` is set by the ioctl path and read from `fastrpc_internal_invoke`. There's no locking between these. If one thread is doing `SET_OPTION` while another is invoking, there's a data race on `poll_mode`. Since it's a `bool` and the worst case is enabling/disabling polling for one invocation, this is benign in practice but should use `READ_ONCE`/`WRITE_ONCE` for correctness.

---

**Summary:** Patches 1-3 are clean and ready. Patch 4 needs attention on:
- The `is_work_done` data race (use `READ_ONCE`/`WRITE_ONCE` or `atomic_t`)
- The `fl->poll_mode` data race (minor, same fix)
- The dead `-EIO` conversion
- Consider `_IOW` instead of `_IOWR` for the set option ioctl

---
Generated by Claude Code Patch Reviewer