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, 25 May 2026 20:54:21 +1000 Message-ID: In-Reply-To: <20260521054539.128651-5-ekansh.gupta@oss.qualcomm.com> References: <20260521054539.128651-1-ekansh.gupta@oss.qualcomm.com> <20260521054539.128651-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 **Status: Needs attention on a few points.** **Issue 1 (medium): Race between polling and glink callback on `is_work_don= e`** The `is_work_done` flag is read in `poll_for_remote_response()` (from the i= nvoking thread context) and written in `fastrpc_rpmsg_callback()` (from the= rpmsg callback context) without any synchronization: ```c // In poll_for_remote_response(): ret =3D read_poll_timeout_atomic(fastrpc_read_poll_addr, val, (val =3D=3D FASTRPC_POLL_RESPONSE) || ctx->i= s_work_done, ...); ``` ```c // In fastrpc_rpmsg_callback(): ctx->retval =3D rsp->retval; ctx->is_work_done =3D true; complete(&ctx->work); ``` While `is_work_done` is a `bool` and the race is "benign" in the sense that= both paths converge to the same outcome (work is done), the `ctx->retval` = assignment in the callback could race with the polling path reading `ctx->r= etval =3D 0`. If the callback sets `is_work_done =3D true` and `ctx->retval= ` to an error, but the polling path already saw `is_work_done =3D true` and= set `ctx->retval =3D 0`, the actual error return value from DSP would be l= ost. Consider using `WRITE_ONCE`/`READ_ONCE` for `is_work_done` at minimum,= and ordering the `retval` write before the flag write with a barrier. **Issue 2 (minor): `read_poll_timeout_atomic` holds preemption disabled for= up to 10ms** ```c #define FASTRPC_POLL_MAX_TIMEOUT_US (10000) ``` `read_poll_timeout_atomic` uses `udelay` internally, meaning it runs with p= reemption disabled for up to 10ms. This is at the upper edge of what's acce= ptable for atomic context. This is a design tradeoff (the whole point is av= oiding scheduling latency), but it's worth noting in the commit message or = comments that this is intentional and the timeout is bounded. **Issue 3 (minor): `-EIO` mapping for timeout is unusual** ```c if (ret =3D=3D -ETIMEDOUT) ret =3D -EIO; ``` When the poll times out, the code remaps `-ETIMEDOUT` to `-EIO` but then fa= lls through to `fastrpc_wait_for_response()` which will wait for the glink = interrupt normally. The error code from `poll_for_remote_response()` is nev= er actually used since `fastrpc_wait_for_completion()` returns the result o= f `fastrpc_wait_for_response()` on poll failure: ```c if (ctx->is_polled) { err =3D poll_for_remote_response(ctx); if (!err) return 0; ctx->is_polled =3D false; } return fastrpc_wait_for_response(ctx, kernel); ``` The `-EIO` mapping is dead code =E2=80=94 `err` is overwritten by `fastrpc_= wait_for_response()`. The `ret` variable and its mapping can be simplified,= or just removed entirely since any non-zero return from `poll_for_remote_r= esponse()` simply triggers the fallback. **Issue 4 (minor): `__maybe_unused` on `fastrpc_poll_supported_machines`** ```c static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_= unused =3D { ``` This `__maybe_unused` is there to suppress warnings when `CONFIG_OF` is dis= abled (since `of_machine_get_match()` returns `NULL` as a static inline and= the array would be unreferenced). This is correct but worth confirming tha= t the whole polling feature should be available even without `CONFIG_OF` = =E2=80=94 in that case `poll_mode_supported` for the exception-list platfor= ms would silently be false, which is fine. **Issue 5 (nit): UAPI extensibility of `fastrpc_ioctl_set_option`** The new UAPI struct: ```c struct fastrpc_ioctl_set_option { __u32 request_id; __u32 value; __s32 reserved[6]; }; ``` The ioctl is defined as `_IOWR` but nothing is written back to userspace. `= _IOW` would be more appropriate unless there's a plan to return data in the= reserved fields. **Issue 6 (nit): `dma_rmb()` placement in polling path** ```c static u32 fastrpc_read_poll_addr(struct fastrpc_invoke_ctx *ctx) { dma_rmb(); return READ_ONCE(*ctx->poll_addr); } ``` The `dma_rmb()` is placed *before* the read, which ensures previous DMA rea= ds are ordered before this one. However, the more typical pattern is to pla= ce the barrier *after* reading the flag and *before* reading the data that = depends on it. Here, when the poll succeeds, the caller in `fastrpc_interna= l_invoke()` proceeds to `fastrpc_put_args()` which reads output buffers =E2= =80=94 the existing `dma_rmb()` at line 1241 in the original code handles t= hat. But note that in the polling path, this `dma_rmb()` was removed from `= fastrpc_internal_invoke()` (it's still there in the original but the patch = replaces the wait code). Let me check... Actually the `dma_rmb()` at the or= iginal line 1241 comes *after* the wait and *before* `fastrpc_put_args()`, = and it's not touched by this patch, so it's still present. The barrier in `= fastrpc_read_poll_addr()` ensures each poll iteration sees fresh DMA data, = which is correct. Overall, this is a solid v12 series. The main concern is the lack of orderi= ng guarantees around `is_work_done`/`retval` between the polling thread and= the rpmsg callback, and the dead `-EIO` mapping code. The rest is in good = shape. --- Generated by Claude Code Patch Reviewer