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

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

Patch Review

**Assessment: Needs revision — several issues**

**Issue 1 — Race condition on `is_work_done` and `retval`**

`ctx->is_work_done` is set to `true` in two places without any locking or atomic operations:

```c
// In poll_for_remote_response():
ctx->is_work_done = true;
ctx->retval = 0;

// In fastrpc_rpmsg_callback():
ctx->retval = rsp->retval;
ctx->is_work_done = true;
complete(&ctx->work);
```

If the DSP writes the poll response and the rpmsg callback fires concurrently, both paths could race. The rpmsg callback sets `ctx->retval = rsp->retval` while the poll path sets `ctx->retval = 0`, potentially clobbering the actual return value. Consider using `atomic_t` or `smp_store_release`/`smp_load_acquire` for `is_work_done`, and not overwriting `retval` in the poll path if the callback has already set it.

**Issue 2 — Error propagation in `fastrpc_wait_for_completion`**

```c
do {
    if (ctx->is_polled) {
        err = poll_for_remote_response(ctx);
        if (err)
            ctx->is_polled = false;
    } else {
        err = fastrpc_wait_for_response(ctx, kernel);
        if (err)
            return err;
    }
} while (!ctx->is_work_done);

return err;
```

When polling times out, `err` is set to `-EIO`, `is_polled` is set to `false`, and the loop continues. On the next iteration, `fastrpc_wait_for_response` succeeds (returns 0), `is_work_done` becomes true, and the loop exits. But `err` is still `-EIO` from the polling iteration. This means `err` is not cleared on success in the wait path — the return at the end returns the stale `-EIO`. This needs to be fixed, either by resetting `err = 0` after a successful `fastrpc_wait_for_response`, or by restructuring the loop.

**Issue 3 — `poll_for_remote_response` maps `-ETIMEDOUT` to `-EIO`**

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

The intent seems to be that `-ETIMEDOUT` from `read_poll_timeout_atomic` shouldn't propagate as a timeout (since it's an expected transition to interrupt-based waiting). But the function returns `-EIO` on timeout, which is misleading. The caller (`fastrpc_wait_for_completion`) treats any non-zero return as "switch to interrupt mode". Consider just returning `-ETIMEDOUT` directly and handling it in the caller, or returning 0 if the design intent is "polling timed out, fall through to interrupt."

**Issue 4 — UAPI: `_IOWR` vs `_IOW`**

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

The kernel only calls `copy_from_user` on this ioctl — it never copies data back to userspace. This should be `_IOW`, not `_IOWR`. Using `_IOWR` implies bidirectional data transfer. Since this is a UAPI change, it should be fixed before merging.

**Issue 5 — `fastrpc_poll_op` barrier placement**

```c
static inline u32 fastrpc_poll_op(void *p)
{
    struct fastrpc_invoke_ctx *ctx = p;
    dma_rmb();
    return READ_ONCE(*ctx->poll);
}
```

The `dma_rmb()` is placed *before* `READ_ONCE(*ctx->poll)`. A read memory barrier orders prior reads before subsequent reads. Here the intent is presumably to ensure that after seeing the poll response, subsequent reads (of output buffers) see the DSP's writes. But the barrier should be *after* the read of the poll word for that purpose, not before. The current placement ensures nothing useful — there's no prior DMA read to order against. Consider moving the `dma_rmb()` to after the poll value is observed to equal `FASTRPC_POLL_RESPONSE`, or into the caller after poll success.

**Issue 6 — `FASTRPC_MAX_STATIC_HANDLE` is a new constant without clear justification**

```c
#define FASTRPC_MAX_STATIC_HANDLE (20)
```

The patch adds this and uses it to restrict polling to dynamic handles:
```c
if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD &&
    fl->poll_mode)
```

The value 20 appears to be a convention from the DSP side. A comment explaining what "static handles" are and why polling shouldn't apply to them would help maintainability.

**Issue 7 — `fastrpc_set_option` style**

```c
if (opt.value)
    fl->poll_mode = true;
else
    fl->poll_mode = false;
```

This can be simplified to:
```c
fl->poll_mode = !!opt.value;
```

**Issue 8 — Missing `__user` annotation correctness**

```c
static int fastrpc_set_option(struct fastrpc_user *fl, char __user *argp)
```

The function parameter is `char __user *argp` but it's passed to `copy_from_user` as the source for a `struct fastrpc_ioctl_set_option`. This works but would be cleaner as `void __user *argp` to match the other ioctl handlers in this file (checking existing style).

**Issue 9 — Poll memory size accounting**

The meta buffer size is increased by `sizeof(u32)` (4 bytes) for the poll word:

```c
sizeof(u32) * FASTRPC_MAX_CRCLIST +
sizeof(u32);
```

And the poll pointer is computed as:

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

This skips past fdlist (16 × 8 = 128 bytes) and crclist (64 × 4 = 256 bytes). But `ctx->fdlist` is at `(pages + ctx->nscalars)`, and `ctx->crc` is not explicitly computed here. Comparing with the meta size calculation, the layout is: rpra | invoke_bufs | phy_pages | fdlist | crclist | poll_word. The computation looks correct, but note that `ctx->crc` pointer from the original code is stored in the struct but never set in this patch series. The `crc` field initialization should be verified for correctness.

---
Generated by Claude Code Patch Reviewer