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 22:12:30 +1000 Message-ID: In-Reply-To: <20260520065047.3415790-5-ekansh.gupta@oss.qualcomm.com> References: <20260520065047.3415790-1-ekansh.gupta@oss.qualcomm.com> <20260520065047.3415790-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 **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 synchro= nization: - The rpmsg callback (interrupt context): `ctx->is_work_done =3D true;` the= n `complete(&ctx->work);` - The polling loop: reads `ctx->is_work_done` in `read_poll_timeout_atomic`= , writes `ctx->is_work_done =3D true` in `poll_for_remote_response` ```c ctx->is_work_done =3D true; // in rpmsg_callback complete(&ctx->work); ``` ```c ret =3D read_poll_timeout_atomic(fastrpc_read_poll_addr, val, (val =3D=3D FASTRPC_POLL_RESPONSE) || ctx->is_work_done, ...); if (!ret && val =3D=3D FASTRPC_POLL_RESPONSE) { ctx->is_work_done =3D true; ctx->retval =3D 0; } ``` `is_work_done` is a plain `bool` =E2=80=94 it should be accessed with `READ= _ONCE`/`WRITE_ONCE` or be an `atomic_t` to avoid data races. The compiler c= ould cache the value in a register during the poll loop, causing the callba= ck's write to be missed. While `read_poll_timeout_atomic` does use `READ_ON= CE` on the poll value, the `ctx->is_work_done` read in the condition expres= sion 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_timeou= t_atomic`. While technically correct for ordering, this is a busy-poll loop= with 1 =C2=B5s delay =E2=80=94 the barrier on every iteration may add unne= cessary 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 =3D=3D FASTRPC_POLL_RESPONSE)= , it sets: ```c ctx->is_work_done =3D true; ctx->retval =3D 0; ``` But the rpmsg callback may still fire later (the DSP sends both a poll resp= onse AND an rpmsg callback), and overwrite `ctx->retval` with `rsp->retval`: ```c ctx->retval =3D rsp->retval; ctx->is_work_done =3D true; complete(&ctx->work); ``` Then `schedule_work(&ctx->put_work)` is called in the callback, which may r= ace with the normal cleanup path after `fastrpc_internal_invoke` returns. T= he `put_work` path does `fastrpc_context_put()` which decrements the refcou= nt, so there shouldn't be a use-after-free if refcounting is correct =E2=80= =94 but this deserves verification. Does the context hold enough refcount r= eferences to survive both the poll completion path and the deferred callbac= k? #### 4. `poll_addr` alignment and buffer layout ```c ctx->poll_addr =3D (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_M= AX_FDLIST + sizeof(u32) * FASTRPC_MAX_CRCLIST); ``` The `sizeof(u32)` added to `fastrpc_get_meta_size` accounts for the poll ad= dress word. The fdlist is `u64[]` (8-byte aligned), FASTRPC_MAX_FDLIST=3D16= , FASTRPC_MAX_CRCLIST=3D64, so the offset is `16*8 + 64*4 =3D 128 + 256 =3D= 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`). T= he 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 com= mit message and code don't document how the DSP discovers this address. Pre= sumably this is part of the FastRPC protocol, but it would be good to docum= ent. #### 5. UAPI design: `__IOWR` vs `__IOW` ```c #define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_o= ption) ``` This uses `_IOWR` (read+write), but `fastrpc_set_option` only does `copy_fr= om_user` =E2=80=94 it never writes back to userspace. `_IOW` would be more = appropriate. Using `_IOWR` is not incorrect but is misleading about the dir= ection. #### 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 fi= ne. #### 7. `__maybe_unused` on `fastrpc_poll_supported_machines` ```c static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_= unused =3D { ``` 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 ab= out 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 =3D=3D USER_PD && fl->poll= _mode) ctx->is_polled =3D true; err =3D 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_a= ddr` 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 =E2=80=94 the poll loop will just pick up the respo= nse. 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 handle= d correctly since the poll condition checks `ctx->is_work_done`. #### 9. Error code conversion ```c if (ret =3D=3D -ETIMEDOUT) ret =3D -EIO; ``` Converting `ETIMEDOUT` to `EIO` in `poll_for_remote_response` means the pol= l 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 fallbac= k path overwrites `err`: ```c 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` conversion is dead code since the error value is discarded. Cons= ider 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_in= voke`. 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 invocati= on, 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