From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: Add polling mode support for fastRPC driver Date: Mon, 16 Feb 2026 08:26:14 +1000 Message-ID: In-Reply-To: <20260215182136.3995111-5-ekansh.gupta@oss.qualcomm.com> References: <20260215182136.3995111-1-ekansh.gupta@oss.qualcomm.com> <20260215182136.3995111-5-ekansh.gupta@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Assessment: Needs revision =E2=80=94 several issues** **Issue 1 =E2=80=94 Race condition on `is_work_done` and `retval`** `ctx->is_work_done` is set to `true` in two places without any locking or ato= mic operations: ```c // In poll_for_remote_response(): ctx->is_work_done =3D true; ctx->retval =3D 0; // In fastrpc_rpmsg_callback(): ctx->retval =3D rsp->retval; ctx->is_work_done =3D 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 =3D rsp->retval= ` while the poll path sets `ctx->retval =3D 0`, potentially clobbering the ac= tual 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 i= f the callback has already set it. **Issue 2 =E2=80=94 Error propagation in `fastrpc_wait_for_completion`** ```c do { if (ctx->is_polled) { err =3D poll_for_remote_response(ctx); if (err) ctx->is_polled =3D false; } else { err =3D 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 `e= rr` is still `-EIO` from the polling iteration. This means `err` is not clear= ed on success in the wait path =E2=80=94 the return at the end returns the st= ale `-EIO`. This needs to be fixed, either by resetting `err =3D 0` after a s= uccessful `fastrpc_wait_for_response`, or by restructuring the loop. **Issue 3 =E2=80=94 `poll_for_remote_response` maps `-ETIMEDOUT` to `-EIO`** ```c if (ret =3D=3D -ETIMEDOUT) ret =3D -EIO; ``` The intent seems to be that `-ETIMEDOUT` from `read_poll_timeout_atomic` shou= ldn't propagate as a timeout (since it's an expected transition to interrupt-= based waiting). But the function returns `-EIO` on timeout, which is misleadi= ng. 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 t= imed out, fall through to interrupt." **Issue 4 =E2=80=94 UAPI: `_IOWR` vs `_IOW`** ```c #define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_opti= on) ``` The kernel only calls `copy_from_user` on this ioctl =E2=80=94 it never copie= s data back to userspace. This should be `_IOW`, not `_IOWR`. Using `_IOWR` i= mplies bidirectional data transfer. Since this is a UAPI change, it should be= fixed before merging. **Issue 5 =E2=80=94 `fastrpc_poll_op` barrier placement** ```c static inline u32 fastrpc_poll_op(void *p) { struct fastrpc_invoke_ctx *ctx =3D p; dma_rmb(); return READ_ONCE(*ctx->poll); } ``` The `dma_rmb()` is placed *before* `READ_ONCE(*ctx->poll)`. A read memory bar= rier orders prior reads before subsequent reads. Here the intent is presumabl= y 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 not= hing useful =E2=80=94 there's no prior DMA read to order against. Consider mo= ving the `dma_rmb()` to after the poll value is observed to equal `FASTRPC_PO= LL_RESPONSE`, or into the caller after poll success. **Issue 6 =E2=80=94 `FASTRPC_MAX_STATIC_HANDLE` is a new constant without cle= ar 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 =3D=3D USER_PD && fl->poll_mode) ``` The value 20 appears to be a convention from the DSP side. A comment explaini= ng what "static handles" are and why polling shouldn't apply to them would he= lp maintainability. **Issue 7 =E2=80=94 `fastrpc_set_option` style** ```c if (opt.value) fl->poll_mode =3D true; else fl->poll_mode =3D false; ``` This can be simplified to: ```c fl->poll_mode =3D !!opt.value; ``` **Issue 8 =E2=80=94 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_u= ser` as the source for a `struct fastrpc_ioctl_set_option`. This works but wo= uld be cleaner as `void __user *argp` to match the other ioctl handlers in th= is file (checking existing style). **Issue 9 =E2=80=94 Poll memory size accounting** The meta buffer size is increased by `sizeof(u32)` (4 bytes) for the poll wor= d: ```c sizeof(u32) * FASTRPC_MAX_CRCLIST + sizeof(u32); ``` And the poll pointer is computed as: ```c ctx->poll =3D (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLI= ST + sizeof(u32) * FASTRPC_MAX_CRCLIST); ``` This skips past fdlist (16 =C3=97 8 =3D 128 bytes) and crclist (64 =C3=97 4 = =3D 256 bytes). But `ctx->fdlist` is at `(pages + ctx->nscalars)`, and `ctx->= crc` is not explicitly computed here. Comparing with the meta size calculatio= n, the layout is: rpra | invoke_bufs | phy_pages | fdlist | crclist | poll_wo= rd. 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