* [PATCH v12 0/4] misc: fastrpc: Add polling mode support
@ 2026-05-21 5:45 Ekansh Gupta
2026-05-21 5:45 ` [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure Ekansh Gupta
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Ekansh Gupta @ 2026-05-21 5:45 UTC (permalink / raw)
To: srini, linux-arm-msm
Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
dmitry.baryshkov
This patch series adds polling mode feature that have been missing in
upstream FastRPC driver.
- Add changes to move fdlist to ctx structure to avoid code duplicacy.
- Update context mask to support polling mode.
- Add changes to support polling feature.
Userspace change: https://github.com/qualcomm/fastrpc/pull/258
Patch [v11]: https://lore.kernel.org/all/20260520065047.3415790-1-ekansh.gupta@oss.qualcomm.com/
Changes in v12:
- Fixed poll_mode_supported check.
Changes in v11:
- Moved back to read_poll*.
- Improved error handling.
Changes in v10:
- Add milos and sar2130p to fastrpc_poll_supported_machines.
- Updated comment for supported platform list.
Changes in v9:
- Added platform support check.
- Moved to readl as per Luben's suggestion.
- Cleaned up fastrpc_wait_for_completion().
Changes in v8:
- Added more comments.
Changes in v7:
- Fixed warnings.
- Fixed commit text.
- Addressed clean-up comments.
Changes in v6:
- Fixed poll memory calculation.
- Added few formatting changes.
Changes in v5:
- Add more details in commit text.
Changes in v4:
- Replace hardcoded ctxid mask with GENMASK.
- Fixed commit text.
Changes in v3:
- Resolve compilation warning.
Changes in v2:
- Added comments and fixed commit text.
- Defined context id position as a macro.
- Added new IOCTL to control polling mode as always enabling
it might cause excess power consumption.
- Cleaned up polling mode implementation.
Ekansh Gupta (4):
misc: fastrpc: Move fdlist to invoke context structure
misc: fastrpc: Replace hardcoded ctxid mask with GENMASK
misc: fastrpc: Expand context ID mask for DSP polling mode support
misc: fastrpc: Add polling mode support for fastRPC driver
drivers/misc/fastrpc.c | 189 ++++++++++++++++++++++++++++++++----
include/uapi/misc/fastrpc.h | 29 ++++++
2 files changed, 198 insertions(+), 20 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta @ 2026-05-21 5:45 ` Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK Ekansh Gupta ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-05-21 5:45 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov The fdlist is currently part of the meta buffer which is set during fastrpc_get_args(), this fdlist is getting recalculated during fastrpc_put_args(). Move fdlist to the invoke context structure to improve maintainability and reduce redundancy. This centralizes its handling and simplifies meta buffer preparation and reading logic. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 1080f9acf70a..a9f507a88c67 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -233,6 +233,7 @@ struct fastrpc_invoke_ctx { int pid; int client_id; u32 sc; + u64 *fdlist; u32 *crc; u64 ctxid; u64 msg_sz; @@ -1016,6 +1017,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx) rpra = ctx->buf->virt; list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); + ctx->fdlist = (u64 *)(pages + ctx->nscalars); args = (uintptr_t)ctx->buf->virt + metalen; rlen = pkt_size - metalen; ctx->rpra = rpra; @@ -1118,18 +1120,11 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx, union fastrpc_remote_arg *rpra = ctx->rpra; struct fastrpc_user *fl = ctx->fl; struct fastrpc_map *mmap = NULL; - struct fastrpc_invoke_buf *list; - struct fastrpc_phy_page *pages; - u64 *fdlist; - int i, inbufs, outbufs, handles; + u64 *fdlist = ctx->fdlist; + int i, inbufs; int ret = 0; inbufs = REMOTE_SCALARS_INBUFS(ctx->sc); - outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc); - handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc); - list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); - pages = fastrpc_phy_page_start(list, ctx->nscalars); - fdlist = (uint64_t *)(pages + inbufs + outbufs + handles); for (i = inbufs; i < ctx->nbufs; ++i) { if (!ctx->maps[i]) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Move fdlist to invoke context structure 2026-05-21 5:45 ` [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure Ekansh Gupta @ 2026-05-25 10:54 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 10:54 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Status: Looks good.** This moves the `fdlist` pointer from being locally computed in both `fastrpc_get_args()` and `fastrpc_put_args()` into the `struct fastrpc_invoke_ctx`. This eliminates redundant computation and is also necessary for patch 4 to calculate `poll_addr` relative to the end of the metadata buffer. One subtle correctness point: the old code in `fastrpc_put_args()` computed `fdlist` as: ```c fdlist = (uint64_t *)(pages + inbufs + outbufs + handles); ``` while the new code in `fastrpc_get_args()` computes it as: ```c ctx->fdlist = (u64 *)(pages + ctx->nscalars); ``` These are equivalent because `nscalars = inbufs + outbufs + handles` (see `REMOTE_SCALARS_LENGTH`), so this is correct and also simpler. Has R-b from Dmitry Baryshkov. No issues. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v12 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta 2026-05-21 5:45 ` [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure Ekansh Gupta @ 2026-05-21 5:45 ` Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support Ekansh Gupta ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-05-21 5:45 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov, Konrad Dybcio Replace the hardcoded context ID mask (0xFF0) with GENMASK(11, 4) to improve readability and follow kernel bitfield conventions. Use FIELD_PREP and FIELD_GET instead of manual shifts for setting and extracting ctxid values. Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index a9f507a88c67..3f5d5d73be5a 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -23,6 +23,7 @@ #include <uapi/misc/fastrpc.h> #include <linux/of_reserved_mem.h> #include <linux/bits.h> +#include <linux/bitops.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -37,7 +38,7 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 -#define FASTRPC_CTXID_MASK (0xFF0) +#define FASTRPC_CTXID_MASK GENMASK(11, 4) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) #define FASTRPC_DEVICE_NAME "fastrpc" @@ -515,7 +516,7 @@ static void fastrpc_context_free(struct kref *ref) fastrpc_buf_free(ctx->buf); spin_lock_irqsave(&cctx->lock, flags); - idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4); + idr_remove(&cctx->ctx_idr, FIELD_GET(FASTRPC_CTXID_MASK, ctx->ctxid)); spin_unlock_irqrestore(&cctx->lock, flags); kfree(ctx->maps); @@ -649,7 +650,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc( spin_unlock_irqrestore(&cctx->lock, flags); goto err_idr; } - ctx->ctxid = ret << 4; + ctx->ctxid = FIELD_PREP(FASTRPC_CTXID_MASK, ret); spin_unlock_irqrestore(&cctx->lock, flags); kref_init(&ctx->refcount); @@ -2508,7 +2509,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data, if (len < sizeof(*rsp)) return -EINVAL; - ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4); + ctxid = FIELD_GET(FASTRPC_CTXID_MASK, rsp->ctx); spin_lock_irqsave(&cctx->lock, flags); ctx = idr_find(&cctx->ctx_idr, ctxid); -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Replace hardcoded ctxid mask with GENMASK 2026-05-21 5:45 ` [PATCH v12 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK Ekansh Gupta @ 2026-05-25 10:54 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 10:54 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Status: Looks good.** Straightforward cleanup replacing `0xFF0` with `GENMASK(11, 4)` and manual shifts with `FIELD_PREP`/`FIELD_GET`. The `#include <linux/bitops.h>` is added but `FIELD_PREP`/`FIELD_GET` are from `<linux/bitfield.h>` — however, `bitfield.h` is likely pulled in transitively already since the code compiles. Minor nit but not a blocker if it builds. Has R-b from both Konrad Dybcio and Dmitry Baryshkov. No issues. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v12 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta 2026-05-21 5:45 ` [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure Ekansh Gupta 2026-05-21 5:45 ` [PATCH v12 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK Ekansh Gupta @ 2026-05-21 5:45 ` Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta 2026-05-25 10:54 ` Claude review: misc: fastrpc: Add polling mode support Claude Code Review Bot 4 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-05-21 5:45 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov Current FastRPC context uses a 12-bit mask: [ID(8 bits)][PD type(4 bits)] = GENMASK(11, 4) This works for normal calls but fails for DSP polling mode. Polling mode expects a 16-bit layout: [15:8] = context ID (8 bits) [7:5] = reserved [4] = async mode bit [3:0] = PD type (4 bits) If async bit (bit 4) is set, DSP disables polling. With current mask, odd IDs can set this bit, causing DSP to skip poll updates. Update FASTRPC_CTXID_MASK to GENMASK(15, 8) so IDs occupy upper byte and lower byte is left for DSP flags and PD type. Reserved bits remain unused. This change is compatible with polling mode and does not break non-polling behavior. Bit layout: [15:8] = CCCCCCCC (context ID) [7:5] = xxx (reserved) [4] = A (async mode) [3:0] = PPPP (PD type) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 3f5d5d73be5a..c4a3547a5c7f 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -38,7 +38,7 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 -#define FASTRPC_CTXID_MASK GENMASK(11, 4) +#define FASTRPC_CTXID_MASK GENMASK(15, 8) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) #define FASTRPC_DEVICE_NAME "fastrpc" -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Expand context ID mask for DSP polling mode support 2026-05-21 5:45 ` [PATCH v12 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support Ekansh Gupta @ 2026-05-25 10:54 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 10:54 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Status: Looks good, well-documented.** The commit message clearly explains *why* the mask needs to expand: with `GENMASK(11, 4)`, odd context IDs can inadvertently set bit 4 (the "async mode" bit), causing the DSP to skip polling updates. Moving to `GENMASK(15, 8)` places the 8-bit context ID cleanly into the upper byte. This is a wire-protocol change, but it's described as backward-compatible (lower bits are reserved/unused on non-polling paths). The bit layout documentation in the commit message is excellent. Has R-b from Dmitry Baryshkov. No issues. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v12 4/4] misc: fastrpc: Add polling mode support for fastRPC driver 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta ` (2 preceding siblings ...) 2026-05-21 5:45 ` [PATCH v12 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support Ekansh Gupta @ 2026-05-21 5:45 ` Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-25 10:54 ` Claude review: misc: fastrpc: Add polling mode support Claude Code Review Bot 4 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-05-21 5:45 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov For any remote call to DSP, after sending an invocation message, fastRPC driver waits for glink response and during this time the CPU can go into low power modes. This adds latency to overall fastrpc call as CPU wakeup and scheduling latencies are included. Add polling mode support with which fastRPC driver will poll continuously on a memory after sending a message to remote subsystem which will eliminate CPU wakeup and scheduling latencies and reduce fastRPC overhead. In case poll timeout happens, the call will fallback to normal RPC mode. Poll mode can be enabled by user by using FASTRPC_IOCTL_SET_OPTION ioctl request with FASTRPC_POLL_MODE request id. Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 167 ++++++++++++++++++++++++++++++++++-- include/uapi/misc/fastrpc.h | 29 +++++++ 2 files changed, 189 insertions(+), 7 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index c4a3547a5c7f..f0311395a686 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -24,6 +24,8 @@ #include <linux/of_reserved_mem.h> #include <linux/bits.h> #include <linux/bitops.h> +#include <linux/compiler.h> +#include <linux/iopoll.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -38,6 +40,12 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 +/* + * Maximum handle value for static handles. + * Static handles are pre-defined, fixed numeric values statically assigned + * in the IDL file or FastRPC framework. + */ +#define FASTRPC_MAX_STATIC_HANDLE (20) #define FASTRPC_CTXID_MASK GENMASK(15, 8) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) @@ -106,6 +114,12 @@ #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) +/* Poll response number from remote processor for call completion */ +#define FASTRPC_POLL_RESPONSE (0xdecaf) + +/* Polling mode timeout limit */ +#define FASTRPC_POLL_MAX_TIMEOUT_US (10000) + struct fastrpc_phy_page { dma_addr_t addr; /* dma address */ u64 size; /* size of contiguous region */ @@ -236,8 +250,14 @@ struct fastrpc_invoke_ctx { u32 sc; u64 *fdlist; u32 *crc; + /* Poll memory that DSP updates */ + u32 *poll_addr; u64 ctxid; u64 msg_sz; + /* work done status flag */ + bool is_work_done; + /* process updates poll memory instead of glink response */ + bool is_polled; struct kref refcount; struct list_head node; /* list of ctxs */ struct completion work; @@ -263,6 +283,7 @@ struct fastrpc_soc_data { u32 sid_pos; u32 dma_addr_bits_cdsp; u32 dma_addr_bits_default; + bool poll_mode_supported; }; struct fastrpc_channel_ctx { @@ -285,6 +306,7 @@ struct fastrpc_channel_ctx { struct list_head invoke_interrupted_mmaps; bool secure; bool unsigned_support; + bool poll_mode_supported; u64 dma_mask; const struct fastrpc_soc_data *soc_data; }; @@ -308,6 +330,8 @@ struct fastrpc_user { int client_id; int pd; bool is_secure_dev; + /* Flags poll mode state */ + bool poll_mode; /* Lock for lists */ spinlock_t lock; /* lock for allocations */ @@ -923,7 +947,8 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx) sizeof(struct fastrpc_invoke_buf) + sizeof(struct fastrpc_phy_page)) * ctx->nscalars + sizeof(u64) * FASTRPC_MAX_FDLIST + - sizeof(u32) * FASTRPC_MAX_CRCLIST; + sizeof(u32) * FASTRPC_MAX_CRCLIST + + sizeof(u32); return size; } @@ -1019,6 +1044,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx) list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); ctx->fdlist = (u64 *)(pages + ctx->nscalars); + ctx->poll_addr = (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLIST + + sizeof(u32) * FASTRPC_MAX_CRCLIST); + args = (uintptr_t)ctx->buf->virt + metalen; rlen = pkt_size - metalen; ctx->rpra = rpra; @@ -1188,6 +1216,71 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx, } +static u32 fastrpc_read_poll_addr(struct fastrpc_invoke_ctx *ctx) +{ + dma_rmb(); + return READ_ONCE(*ctx->poll_addr); +} + +static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx) +{ + u32 val; + int ret; + + /* + * Poll until DSP writes FASTRPC_POLL_RESPONSE into *ctx->poll_addr + * or until another path marks the work done. + */ + ret = read_poll_timeout_atomic(fastrpc_read_poll_addr, val, + (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, 1, + FASTRPC_POLL_MAX_TIMEOUT_US, false, ctx); + + if (!ret && val == FASTRPC_POLL_RESPONSE) { + /* + * DSP writes FASTRPC_POLL_RESPONSE to signal successful + * completion via the poll path. + */ + ctx->is_work_done = true; + ctx->retval = 0; + } + + if (ret == -ETIMEDOUT) + ret = -EIO; + + return ret; +} + +static inline int fastrpc_wait_for_response(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err = 0; + + if (kernel) { + if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) + err = -ETIMEDOUT; + } else { + err = wait_for_completion_interruptible(&ctx->work); + } + + return err; +} + +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err; + + if (ctx->is_polled) { + err = poll_for_remote_response(ctx); + if (!err) + return 0; + /* If polling timed out or failed, move to normal response mode */ + ctx->is_polled = false; + } + + return fastrpc_wait_for_response(ctx, kernel); +} + static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, u32 handle, u32 sc, struct fastrpc_invoke_args *args) @@ -1223,13 +1316,14 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, if (err) goto bail; - if (kernel) { - if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) - err = -ETIMEDOUT; - } else { - err = wait_for_completion_interruptible(&ctx->work); - } + /* + * Set message context as polled if the call is for a user PD + * dynamic module and user has enabled poll mode. + */ + if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD && fl->poll_mode) + ctx->is_polled = true; + err = fastrpc_wait_for_completion(ctx, kernel); if (err) goto bail; @@ -1813,6 +1907,35 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, return 0; } +static int fastrpc_set_option(struct fastrpc_user *fl, char __user *argp) +{ + struct fastrpc_ioctl_set_option opt = {0}; + int i; + + if (copy_from_user(&opt, argp, sizeof(opt))) + return -EFAULT; + + for (i = 0; i < ARRAY_SIZE(opt.reserved); i++) { + if (opt.reserved[i] != 0) + return -EINVAL; + } + + if (opt.request_id != FASTRPC_POLL_MODE) + return -EINVAL; + + if (!fl->cctx->poll_mode_supported) + return -EOPNOTSUPP; + + if (opt.value == FASTRPC_POLL_MODE_ENABLE) + fl->poll_mode = true; + else if (opt.value == FASTRPC_POLL_MODE_DISABLE) + fl->poll_mode = false; + else + return -EINVAL; + + return 0; +} + static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_ioctl_capability cap = {0}; @@ -2168,6 +2291,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, case FASTRPC_IOCTL_MEM_UNMAP: err = fastrpc_req_mem_unmap(fl, argp); break; + case FASTRPC_IOCTL_SET_OPTION: + err = fastrpc_set_option(fl, argp); + break; case FASTRPC_IOCTL_GET_DSP_INFO: err = fastrpc_get_dsp_info(fl, argp); break; @@ -2323,6 +2449,7 @@ static const struct fastrpc_soc_data kaanapali_soc_data = { .sid_pos = 56, .dma_addr_bits_cdsp = 34, .dma_addr_bits_default = 32, + .poll_mode_supported = true, }; static const struct fastrpc_soc_data default_soc_data = { @@ -2331,6 +2458,29 @@ static const struct fastrpc_soc_data default_soc_data = { .dma_addr_bits_default = 32, }; +/* + * Exception list for older platforms that use default_soc_data but whose + * DSP firmware supports FastRPC polling mode. + * + * NOTE: This list is intentionally closed. + * Do NOT add new platforms here. New SoCs must advertise polling mode + * support via their soc_data. + */ + +static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unused = { + { .compatible = "qcom,milos" }, + { .compatible = "qcom,qcs8300" }, + { .compatible = "qcom,sa8775p" }, + { .compatible = "qcom,sar2130p" }, + { .compatible = "qcom,sm8450" }, + { .compatible = "qcom,sm8550" }, + { .compatible = "qcom,sm8650" }, + { .compatible = "qcom,sm8750" }, + { .compatible = "qcom,x1e80100" }, + { .compatible = "qcom,x1p42100" }, + {}, +}; + static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *rdev = &rpdev->dev; @@ -2397,6 +2547,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain")); data->secure = secure_dsp; data->soc_data = soc_data; + data->poll_mode_supported = soc_data->poll_mode_supported || + of_machine_get_match(fastrpc_poll_supported_machines); switch (domain_id) { case ADSP_DOMAIN_ID: @@ -2521,6 +2673,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data, } ctx->retval = rsp->retval; + ctx->is_work_done = true; complete(&ctx->work); /* diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h index c6e2925f47e6..ba1ea5ed426c 100644 --- a/include/uapi/misc/fastrpc.h +++ b/include/uapi/misc/fastrpc.h @@ -16,6 +16,7 @@ #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static) #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map) #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap) +#define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_option) #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability) /** @@ -67,6 +68,28 @@ enum fastrpc_proc_attr { /* Fastrpc attribute for memory protection of buffers */ #define FASTRPC_ATTR_SECUREMAP (1) +/** + * FASTRPC_POLL_MODE - Enable/disable poll mode for FastRPC invocations + * + * Poll mode is an optimization that allows the CPU to poll shared memory + * for completion instead of waiting for an interrupt-based response. + * This reduces latency for fast-completing operations. + * + * Restrictions: + * - Only supported for USER_PD (User Protection Domain) + * - Only applies to dynamic modules (handle > 20) + * - Static modules always use interrupt-based completion + * + * Values: + * - 0: Disable poll mode (use interrupt-based completion) + * - 1: Enable poll mode (poll shared memory for completion) + */ +#define FASTRPC_POLL_MODE (1) + +/* Values for FASTRPC_POLL_MODE request */ +#define FASTRPC_POLL_MODE_DISABLE 0 +#define FASTRPC_POLL_MODE_ENABLE 1 + struct fastrpc_invoke_args { __u64 ptr; __u64 length; @@ -133,6 +156,12 @@ struct fastrpc_mem_unmap { __s32 reserved[5]; }; +struct fastrpc_ioctl_set_option { + __u32 request_id; /* Request type (e.g., FASTRPC_POLL_MODE) */ + __u32 value; /* Request-specific value */ + __s32 reserved[6]; +}; + struct fastrpc_ioctl_capability { __u32 unused; /* deprecated, ignored by the kernel */ __u32 attribute_id; -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Add polling mode support for fastRPC driver 2026-05-21 5:45 ` [PATCH v12 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta @ 2026-05-25 10:54 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 10:54 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Status: Needs attention on a few points.** **Issue 1 (medium): Race between polling and glink callback on `is_work_done`** The `is_work_done` flag is read in `poll_for_remote_response()` (from the invoking thread context) and written in `fastrpc_rpmsg_callback()` (from the rpmsg callback context) without any synchronization: ```c // In poll_for_remote_response(): ret = read_poll_timeout_atomic(fastrpc_read_poll_addr, val, (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, ...); ``` ```c // In fastrpc_rpmsg_callback(): ctx->retval = rsp->retval; ctx->is_work_done = 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->retval = 0`. If the callback sets `is_work_done = true` and `ctx->retval` to an error, but the polling path already saw `is_work_done = true` and set `ctx->retval = 0`, the actual error return value from DSP would be lost. 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 preemption disabled for up to 10ms. This is at the upper edge of what's acceptable for atomic context. This is a design tradeoff (the whole point is avoiding 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 == -ETIMEDOUT) ret = -EIO; ``` When the poll times out, the code remaps `-ETIMEDOUT` to `-EIO` but then falls through to `fastrpc_wait_for_response()` which will wait for the glink interrupt normally. The error code from `poll_for_remote_response()` is never actually used since `fastrpc_wait_for_completion()` returns the result of `fastrpc_wait_for_response()` on poll failure: ```c if (ctx->is_polled) { err = poll_for_remote_response(ctx); if (!err) return 0; ctx->is_polled = false; } return fastrpc_wait_for_response(ctx, kernel); ``` The `-EIO` mapping is dead code — `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_response()` 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 = { ``` This `__maybe_unused` is there to suppress warnings when `CONFIG_OF` is disabled (since `of_machine_get_match()` returns `NULL` as a static inline and the array would be unreferenced). This is correct but worth confirming that the whole polling feature should be available even without `CONFIG_OF` — in that case `poll_mode_supported` for the exception-list platforms 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 reads are ordered before this one. However, the more typical pattern is to place the barrier *after* reading the flag and *before* reading the data that depends on it. Here, when the poll succeeds, the caller in `fastrpc_internal_invoke()` proceeds to `fastrpc_put_args()` which reads output buffers — the existing `dma_rmb()` at line 1241 in the original code handles that. 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 original 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 ordering 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Add polling mode support 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta ` (3 preceding siblings ...) 2026-05-21 5:45 ` [PATCH v12 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta @ 2026-05-25 10:54 ` Claude Code Review Bot 4 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 10:54 UTC (permalink / raw) To: dri-devel-reviews Overall Series Review Subject: misc: fastrpc: Add polling mode support Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> Patches: 5 Reviewed: 2026-05-25T20:54:20.772819 --- This is a 4-patch series (v12) adding polling mode support to the Qualcomm FastRPC driver. The series is structured well: patches 1-3 are preparatory refactors (moving `fdlist` to context, switching to `GENMASK`/`FIELD_GET`, and expanding the context ID mask), and patch 4 adds the actual polling mode feature. The overall approach is sound — polling on shared memory to avoid CPU wakeup latency is a well-understood optimization for latency-sensitive RPC paths, with a fallback to interrupt-based completion on timeout. The preparatory patches (1-3) are clean, well-reviewed (R-b from Dmitry Baryshkov and Konrad Dybcio), and look correct. Patch 4 has a few issues worth discussing, ranging from a potential data race to UAPI design considerations. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v11 0/4] misc: fastrpc: Add polling mode support
@ 2026-05-20 6:50 Ekansh Gupta
2026-05-20 6:50 ` [PATCH v11 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta
0 siblings, 1 reply; 13+ messages in thread
From: Ekansh Gupta @ 2026-05-20 6:50 UTC (permalink / raw)
To: srini, linux-arm-msm
Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
dmitry.baryshkov
This patch series adds polling mode feature that have been missing in
upstream FastRPC driver.
- Add changes to move fdlist to ctx structure to avoid code duplicacy.
- Update context mask to support polling mode.
- Add changes to support polling feature.
Userspace change: https://github.com/qualcomm/fastrpc/pull/258
Patch [v10]: https://lore.kernel.org/all/20260424095903.1622565-1-ekansh.gupta@oss.qualcomm.com/
Changes in v11:
- Moved back to read_poll*.
- Improved error handling.
Changes in v10:
- Add milos and sar2130p to fastrpc_poll_supported_machines.
- Updated comment for supported platform list.
Changes in v9:
- Added platform support check.
- Moved to readl as per Luben's suggestion.
- Cleaned up fastrpc_wait_for_completion().
Changes in v8:
- Added more comments.
Changes in v7:
- Fixed warnings.
- Fixed commit text.
- Addressed clean-up comments.
Changes in v6:
- Fixed poll memory calculation.
- Added few formatting changes.
Changes in v5:
- Add more details in commit text.
Changes in v4:
- Replace hardcoded ctxid mask with GENMASK.
- Fixed commit text.
Changes in v3:
- Resolve compilation warning.
Changes in v2:
- Added comments and fixed commit text.
- Defined context id position as a macro.
- Added new IOCTL to control polling mode as always enabling
it might cause excess power consumption.
- Cleaned up polling mode implementation.
Ekansh Gupta (4):
misc: fastrpc: Move fdlist to invoke context structure
misc: fastrpc: Replace hardcoded ctxid mask with GENMASK
misc: fastrpc: Expand context ID mask for DSP polling mode support
misc: fastrpc: Add polling mode support for fastRPC driver
drivers/misc/fastrpc.c | 189 ++++++++++++++++++++++++++++++++----
include/uapi/misc/fastrpc.h | 29 ++++++
2 files changed, 198 insertions(+), 20 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v11 4/4] misc: fastrpc: Add polling mode support for fastRPC driver 2026-05-20 6:50 [PATCH v11 0/4] " Ekansh Gupta @ 2026-05-20 6:50 ` Ekansh Gupta 2026-05-25 12:12 ` Claude review: " Claude Code Review Bot 0 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-05-20 6:50 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov For any remote call to DSP, after sending an invocation message, fastRPC driver waits for glink response and during this time the CPU can go into low power modes. This adds latency to overall fastrpc call as CPU wakeup and scheduling latencies are included. Add polling mode support with which fastRPC driver will poll continuously on a memory after sending a message to remote subsystem which will eliminate CPU wakeup and scheduling latencies and reduce fastRPC overhead. In case poll timeout happens, the call will fallback to normal RPC mode. Poll mode can be enabled by user by using FASTRPC_IOCTL_SET_OPTION ioctl request with FASTRPC_POLL_MODE request id. Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 167 ++++++++++++++++++++++++++++++++++-- include/uapi/misc/fastrpc.h | 29 +++++++ 2 files changed, 189 insertions(+), 7 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index c4a3547a5c7f..aab70947482d 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -24,6 +24,8 @@ #include <linux/of_reserved_mem.h> #include <linux/bits.h> #include <linux/bitops.h> +#include <linux/compiler.h> +#include <linux/iopoll.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -38,6 +40,12 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 +/* + * Maximum handle value for static handles. + * Static handles are pre-defined, fixed numeric values statically assigned + * in the IDL file or FastRPC framework. + */ +#define FASTRPC_MAX_STATIC_HANDLE (20) #define FASTRPC_CTXID_MASK GENMASK(15, 8) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) @@ -106,6 +114,12 @@ #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) +/* Poll response number from remote processor for call completion */ +#define FASTRPC_POLL_RESPONSE (0xdecaf) + +/* Polling mode timeout limit */ +#define FASTRPC_POLL_MAX_TIMEOUT_US (10000) + struct fastrpc_phy_page { dma_addr_t addr; /* dma address */ u64 size; /* size of contiguous region */ @@ -236,8 +250,14 @@ struct fastrpc_invoke_ctx { u32 sc; u64 *fdlist; u32 *crc; + /* Poll memory that DSP updates */ + u32 *poll_addr; u64 ctxid; u64 msg_sz; + /* work done status flag */ + bool is_work_done; + /* process updates poll memory instead of glink response */ + bool is_polled; struct kref refcount; struct list_head node; /* list of ctxs */ struct completion work; @@ -263,6 +283,7 @@ struct fastrpc_soc_data { u32 sid_pos; u32 dma_addr_bits_cdsp; u32 dma_addr_bits_default; + bool poll_mode_supported; }; struct fastrpc_channel_ctx { @@ -285,6 +306,7 @@ struct fastrpc_channel_ctx { struct list_head invoke_interrupted_mmaps; bool secure; bool unsigned_support; + bool poll_mode_supported; u64 dma_mask; const struct fastrpc_soc_data *soc_data; }; @@ -308,6 +330,8 @@ struct fastrpc_user { int client_id; int pd; bool is_secure_dev; + /* Flags poll mode state */ + bool poll_mode; /* Lock for lists */ spinlock_t lock; /* lock for allocations */ @@ -923,7 +947,8 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx) sizeof(struct fastrpc_invoke_buf) + sizeof(struct fastrpc_phy_page)) * ctx->nscalars + sizeof(u64) * FASTRPC_MAX_FDLIST + - sizeof(u32) * FASTRPC_MAX_CRCLIST; + sizeof(u32) * FASTRPC_MAX_CRCLIST + + sizeof(u32); return size; } @@ -1019,6 +1044,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx) list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); ctx->fdlist = (u64 *)(pages + ctx->nscalars); + ctx->poll_addr = (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLIST + + sizeof(u32) * FASTRPC_MAX_CRCLIST); + args = (uintptr_t)ctx->buf->virt + metalen; rlen = pkt_size - metalen; ctx->rpra = rpra; @@ -1188,6 +1216,71 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx, } +static u32 fastrpc_read_poll_addr(struct fastrpc_invoke_ctx *ctx) +{ + dma_rmb(); + return READ_ONCE(*ctx->poll_addr); +} + +static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx) +{ + u32 val; + int ret; + + /* + * Poll until DSP writes FASTRPC_POLL_RESPONSE into *ctx->poll_addr + * or until another path marks the work done. + */ + ret = read_poll_timeout_atomic(fastrpc_read_poll_addr, val, + (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, 1, + FASTRPC_POLL_MAX_TIMEOUT_US, false, ctx); + + if (!ret && val == FASTRPC_POLL_RESPONSE) { + /* + * DSP writes FASTRPC_POLL_RESPONSE to signal successful + * completion via the poll path. + */ + ctx->is_work_done = true; + ctx->retval = 0; + } + + if (ret == -ETIMEDOUT) + ret = -EIO; + + return ret; +} + +static inline int fastrpc_wait_for_response(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err = 0; + + if (kernel) { + if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) + err = -ETIMEDOUT; + } else { + err = wait_for_completion_interruptible(&ctx->work); + } + + return err; +} + +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err; + + if (ctx->is_polled) { + err = poll_for_remote_response(ctx); + if (!err) + return 0; + /* If polling timed out or failed, move to normal response mode */ + ctx->is_polled = false; + } + + return fastrpc_wait_for_response(ctx, kernel); +} + static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, u32 handle, u32 sc, struct fastrpc_invoke_args *args) @@ -1223,13 +1316,14 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, if (err) goto bail; - if (kernel) { - if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) - err = -ETIMEDOUT; - } else { - err = wait_for_completion_interruptible(&ctx->work); - } + /* + * Set message context as polled if the call is for a user PD + * dynamic module and user has enabled poll mode. + */ + if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD && fl->poll_mode) + ctx->is_polled = true; + err = fastrpc_wait_for_completion(ctx, kernel); if (err) goto bail; @@ -1813,6 +1907,35 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, return 0; } +static int fastrpc_set_option(struct fastrpc_user *fl, char __user *argp) +{ + struct fastrpc_ioctl_set_option opt = {0}; + int i; + + if (!fl->cctx->poll_mode_supported) + return -EOPNOTSUPP; + + if (copy_from_user(&opt, argp, sizeof(opt))) + return -EFAULT; + + for (i = 0; i < ARRAY_SIZE(opt.reserved); i++) { + if (opt.reserved[i] != 0) + return -EINVAL; + } + + if (opt.request_id != FASTRPC_POLL_MODE) + return -EINVAL; + + if (opt.value == FASTRPC_POLL_MODE_ENABLE) + fl->poll_mode = true; + else if (opt.value == FASTRPC_POLL_MODE_DISABLE) + fl->poll_mode = false; + else + return -EINVAL; + + return 0; +} + static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_ioctl_capability cap = {0}; @@ -2168,6 +2291,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, case FASTRPC_IOCTL_MEM_UNMAP: err = fastrpc_req_mem_unmap(fl, argp); break; + case FASTRPC_IOCTL_SET_OPTION: + err = fastrpc_set_option(fl, argp); + break; case FASTRPC_IOCTL_GET_DSP_INFO: err = fastrpc_get_dsp_info(fl, argp); break; @@ -2323,6 +2449,7 @@ static const struct fastrpc_soc_data kaanapali_soc_data = { .sid_pos = 56, .dma_addr_bits_cdsp = 34, .dma_addr_bits_default = 32, + .poll_mode_supported = true, }; static const struct fastrpc_soc_data default_soc_data = { @@ -2331,6 +2458,29 @@ static const struct fastrpc_soc_data default_soc_data = { .dma_addr_bits_default = 32, }; +/* + * Exception list for older platforms that use default_soc_data but whose + * DSP firmware supports FastRPC polling mode. + * + * NOTE: This list is intentionally closed. + * Do NOT add new platforms here. New SoCs must advertise polling mode + * support via their soc_data. + */ + +static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unused = { + { .compatible = "qcom,milos" }, + { .compatible = "qcom,qcs8300" }, + { .compatible = "qcom,sa8775p" }, + { .compatible = "qcom,sar2130p" }, + { .compatible = "qcom,sm8450" }, + { .compatible = "qcom,sm8550" }, + { .compatible = "qcom,sm8650" }, + { .compatible = "qcom,sm8750" }, + { .compatible = "qcom,x1e80100" }, + { .compatible = "qcom,x1p42100" }, + {}, +}; + static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *rdev = &rpdev->dev; @@ -2397,6 +2547,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain")); data->secure = secure_dsp; data->soc_data = soc_data; + data->poll_mode_supported = soc_data->poll_mode_supported || + of_machine_get_match(fastrpc_poll_supported_machines); switch (domain_id) { case ADSP_DOMAIN_ID: @@ -2521,6 +2673,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data, } ctx->retval = rsp->retval; + ctx->is_work_done = true; complete(&ctx->work); /* diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h index c6e2925f47e6..ba1ea5ed426c 100644 --- a/include/uapi/misc/fastrpc.h +++ b/include/uapi/misc/fastrpc.h @@ -16,6 +16,7 @@ #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static) #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map) #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap) +#define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_option) #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability) /** @@ -67,6 +68,28 @@ enum fastrpc_proc_attr { /* Fastrpc attribute for memory protection of buffers */ #define FASTRPC_ATTR_SECUREMAP (1) +/** + * FASTRPC_POLL_MODE - Enable/disable poll mode for FastRPC invocations + * + * Poll mode is an optimization that allows the CPU to poll shared memory + * for completion instead of waiting for an interrupt-based response. + * This reduces latency for fast-completing operations. + * + * Restrictions: + * - Only supported for USER_PD (User Protection Domain) + * - Only applies to dynamic modules (handle > 20) + * - Static modules always use interrupt-based completion + * + * Values: + * - 0: Disable poll mode (use interrupt-based completion) + * - 1: Enable poll mode (poll shared memory for completion) + */ +#define FASTRPC_POLL_MODE (1) + +/* Values for FASTRPC_POLL_MODE request */ +#define FASTRPC_POLL_MODE_DISABLE 0 +#define FASTRPC_POLL_MODE_ENABLE 1 + struct fastrpc_invoke_args { __u64 ptr; __u64 length; @@ -133,6 +156,12 @@ struct fastrpc_mem_unmap { __s32 reserved[5]; }; +struct fastrpc_ioctl_set_option { + __u32 request_id; /* Request type (e.g., FASTRPC_POLL_MODE) */ + __u32 value; /* Request-specific value */ + __s32 reserved[6]; +}; + struct fastrpc_ioctl_capability { __u32 unused; /* deprecated, ignored by the kernel */ __u32 attribute_id; -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Add polling mode support for fastRPC driver 2026-05-20 6:50 ` [PATCH v11 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta @ 2026-05-25 12:12 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-05-25 12:12 UTC (permalink / raw) To: dri-devel-reviews 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 0/4] misc: fastrpc: Add polling mode support
@ 2026-04-22 9:24 Ekansh Gupta
2026-04-22 9:24 ` [PATCH v9 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta
0 siblings, 1 reply; 13+ messages in thread
From: Ekansh Gupta @ 2026-04-22 9:24 UTC (permalink / raw)
To: srini, linux-arm-msm
Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
dmitry.baryshkov, ltuikov89, konrad.dybcio, andersson
This patch series adds polling mode feature that have been missing in
upstream FastRPC driver.
- Add changes to move fdlist to ctx structure to avoid code duplicacy.
- Update context mask to support polling mode.
- Add changes to support polling feature.
Userspace change: https://github.com/qualcomm/fastrpc/pull/258
Patch [v8]: https://lore.kernel.org/all/20260415112530.4083240-1-ekansh.gupta@oss.qualcomm.com/
Changes in v9:
- Added platform support check.
- Moved to readl as per Luben's suggestion.
- Cleaned up fastrpc_wait_for_completion().
Changes in v8:
- Added more comments.
Changes in v7:
- Fixed warnings.
- Fixed commit text.
- Addressed clean-up comments.
Changes in v6:
- Fixed poll memory calculation.
- Added few formatting changes.
Changes in v5:
- Add more details in commit text.
Changes in v4:
- Replace hardcoded ctxid mask with GENMASK.
- Fixed commit text.
Changes in v3:
- Resolve compilation warning.
Changes in v2:
- Added comments and fixed commit text.
- Defined context id position as a macro.
- Added new IOCTL to control polling mode as always enabling
it might cause excess power consumption.
- Cleaned up polling mode implementation.
Ekansh Gupta (4):
misc: fastrpc: Move fdlist to invoke context structure
misc: fastrpc: Replace hardcoded ctxid mask with GENMASK
misc: fastrpc: Expand context ID mask for DSP polling mode support
misc: fastrpc: Add polling mode support for fastRPC driver
drivers/misc/fastrpc.c | 170 +++++++++++++++++++++++++++++++-----
include/uapi/misc/fastrpc.h | 25 ++++++
2 files changed, 175 insertions(+), 20 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v9 4/4] misc: fastrpc: Add polling mode support for fastRPC driver 2026-04-22 9:24 [PATCH v9 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta @ 2026-04-22 9:24 ` Ekansh Gupta 2026-04-22 21:50 ` Claude review: " Claude Code Review Bot 0 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-04-22 9:24 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov, ltuikov89, konrad.dybcio, andersson For any remote call to DSP, after sending an invocation message, fastRPC driver waits for glink response and during this time the CPU can go into low power modes. This adds latency to overall fastrpc call as CPU wakeup and scheduling latencies are included. Add polling mode support with which fastRPC driver will poll continuously on a memory after sending a message to remote subsystem which will eliminate CPU wakeup and scheduling latencies and reduce fastRPC overhead. In case poll timeout happens, the call will fallback to normal RPC mode. Poll mode can be enabled by user by using FASTRPC_IOCTL_SET_OPTION ioctl request with FASTRPC_POLL_MODE request id. Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 148 ++++++++++++++++++++++++++++++++++-- include/uapi/misc/fastrpc.h | 25 ++++++ 2 files changed, 166 insertions(+), 7 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index c4a3547a5c7f..7445e2aa3c3c 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -24,6 +24,8 @@ #include <linux/of_reserved_mem.h> #include <linux/bits.h> #include <linux/bitops.h> +#include <linux/compiler.h> +#include <linux/iopoll.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -38,6 +40,12 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 +/* + * Maximum handle value for static handles. + * Static handles are pre-defined, fixed numeric values statically assigned + * in the IDL file or FastRPC framework. + */ +#define FASTRPC_MAX_STATIC_HANDLE (20) #define FASTRPC_CTXID_MASK GENMASK(15, 8) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) @@ -106,6 +114,12 @@ #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) +/* Poll response number from remote processor for call completion */ +#define FASTRPC_POLL_RESPONSE (0xdecaf) + +/* Polling mode timeout limit */ +#define FASTRPC_POLL_MAX_TIMEOUT_US (10000) + struct fastrpc_phy_page { dma_addr_t addr; /* dma address */ u64 size; /* size of contiguous region */ @@ -236,8 +250,14 @@ struct fastrpc_invoke_ctx { u32 sc; u64 *fdlist; u32 *crc; + /* Poll memory that DSP updates */ + u32 *poll_addr; u64 ctxid; u64 msg_sz; + /* work done status flag */ + bool is_work_done; + /* process updates poll memory instead of glink response */ + bool is_polled; struct kref refcount; struct list_head node; /* list of ctxs */ struct completion work; @@ -263,6 +283,7 @@ struct fastrpc_soc_data { u32 sid_pos; u32 dma_addr_bits_cdsp; u32 dma_addr_bits_default; + bool poll_mode_supported; }; struct fastrpc_channel_ctx { @@ -285,6 +306,7 @@ struct fastrpc_channel_ctx { struct list_head invoke_interrupted_mmaps; bool secure; bool unsigned_support; + bool poll_mode_supported; u64 dma_mask; const struct fastrpc_soc_data *soc_data; }; @@ -308,6 +330,8 @@ struct fastrpc_user { int client_id; int pd; bool is_secure_dev; + /* Flags poll mode state */ + bool poll_mode; /* Lock for lists */ spinlock_t lock; /* lock for allocations */ @@ -923,7 +947,8 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx) sizeof(struct fastrpc_invoke_buf) + sizeof(struct fastrpc_phy_page)) * ctx->nscalars + sizeof(u64) * FASTRPC_MAX_FDLIST + - sizeof(u32) * FASTRPC_MAX_CRCLIST; + sizeof(u32) * FASTRPC_MAX_CRCLIST + + sizeof(u32); return size; } @@ -1019,6 +1044,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx) list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); ctx->fdlist = (u64 *)(pages + ctx->nscalars); + ctx->poll_addr = (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLIST + + sizeof(u32) * FASTRPC_MAX_CRCLIST); + args = (uintptr_t)ctx->buf->virt + metalen; rlen = pkt_size - metalen; ctx->rpra = rpra; @@ -1188,6 +1216,61 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx, } +static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx) +{ + u32 val; + int ret; + + /* + * Poll until DSP writes FASTRPC_POLL_RESPONSE into *ctx->poll_addr + * or until another path marks the work done. + */ + ret = readl_poll_timeout_atomic(ctx->poll_addr, val, + (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, 1, + FASTRPC_POLL_MAX_TIMEOUT_US); + + if (!ret && val == FASTRPC_POLL_RESPONSE) { + ctx->is_work_done = true; + ctx->retval = 0; + } + + if (ret == -ETIMEDOUT) + ret = -EIO; + + return ret; +} + +static inline int fastrpc_wait_for_response(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err = 0; + + if (kernel) { + if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) + err = -ETIMEDOUT; + } else { + err = wait_for_completion_interruptible(&ctx->work); + } + + return err; +} + +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err; + + if (ctx->is_polled) { + err = poll_for_remote_response(ctx); + if (!err) + return 0; + /* If polling timed out or failed, move to normal response mode */ + ctx->is_polled = false; + } + + return fastrpc_wait_for_response(ctx, kernel); +} + static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, u32 handle, u32 sc, struct fastrpc_invoke_args *args) @@ -1223,13 +1306,14 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, if (err) goto bail; - if (kernel) { - if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) - err = -ETIMEDOUT; - } else { - err = wait_for_completion_interruptible(&ctx->work); - } + /* + * Set message context as polled if the call is for a user PD + * dynamic module and user has enabled poll mode. + */ + if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD && fl->poll_mode) + ctx->is_polled = true; + err = fastrpc_wait_for_completion(ctx, kernel); if (err) goto bail; @@ -1813,6 +1897,33 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, return 0; } +static int fastrpc_set_option(struct fastrpc_user *fl, char __user *argp) +{ + struct fastrpc_ioctl_set_option opt = {0}; + int i; + + if (copy_from_user(&opt, argp, sizeof(opt))) + return -EFAULT; + + for (i = 0; i < ARRAY_SIZE(opt.reserved); i++) { + if (opt.reserved[i] != 0) + return -EINVAL; + } + + if (opt.request_id != FASTRPC_POLL_MODE) + return -EINVAL; + + if (opt.value) { + if (!fl->cctx->poll_mode_supported) + return -EOPNOTSUPP; + fl->poll_mode = true; + } else { + fl->poll_mode = false; + } + + return 0; +} + static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_ioctl_capability cap = {0}; @@ -2168,6 +2279,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, case FASTRPC_IOCTL_MEM_UNMAP: err = fastrpc_req_mem_unmap(fl, argp); break; + case FASTRPC_IOCTL_SET_OPTION: + err = fastrpc_set_option(fl, argp); + break; case FASTRPC_IOCTL_GET_DSP_INFO: err = fastrpc_get_dsp_info(fl, argp); break; @@ -2323,6 +2437,7 @@ static const struct fastrpc_soc_data kaanapali_soc_data = { .sid_pos = 56, .dma_addr_bits_cdsp = 34, .dma_addr_bits_default = 32, + .poll_mode_supported = true, }; static const struct fastrpc_soc_data default_soc_data = { @@ -2331,6 +2446,22 @@ static const struct fastrpc_soc_data default_soc_data = { .dma_addr_bits_default = 32, }; +/* + * Older platforms that use default_soc_data but whose DSP firmware + * supports the FastRPC polling mode. + */ +static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unused = { + { .compatible = "qcom,qcs8300" }, + { .compatible = "qcom,qcs9100" }, + { .compatible = "qcom,sm8450" }, + { .compatible = "qcom,sm8550" }, + { .compatible = "qcom,sm8650" }, + { .compatible = "qcom,sm8750" }, + { .compatible = "qcom,x1e80100" }, + { .compatible = "qcom,x1p42100" }, + {}, +}; + static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *rdev = &rpdev->dev; @@ -2397,6 +2528,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain")); data->secure = secure_dsp; data->soc_data = soc_data; + data->poll_mode_supported = soc_data->poll_mode_supported || + of_machine_get_match(fastrpc_poll_supported_machines); switch (domain_id) { case ADSP_DOMAIN_ID: @@ -2521,6 +2654,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data, } ctx->retval = rsp->retval; + ctx->is_work_done = true; complete(&ctx->work); /* diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h index c6e2925f47e6..63346e27d5e9 100644 --- a/include/uapi/misc/fastrpc.h +++ b/include/uapi/misc/fastrpc.h @@ -16,6 +16,7 @@ #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static) #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map) #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap) +#define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_option) #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability) /** @@ -67,6 +68,24 @@ enum fastrpc_proc_attr { /* Fastrpc attribute for memory protection of buffers */ #define FASTRPC_ATTR_SECUREMAP (1) +/** + * FASTRPC_POLL_MODE - Enable/disable poll mode for FastRPC invocations + * + * Poll mode is an optimization that allows the CPU to poll shared memory + * for completion instead of waiting for an interrupt-based response. + * This reduces latency for fast-completing operations. + * + * Restrictions: + * - Only supported for USER_PD (User Protection Domain) + * - Only applies to dynamic modules (handle > 20) + * - Static modules always use interrupt-based completion + * + * Values: + * - 0: Disable poll mode (use interrupt-based completion) + * - 1: Enable poll mode (poll shared memory for completion) + */ +#define FASTRPC_POLL_MODE (1) + struct fastrpc_invoke_args { __u64 ptr; __u64 length; @@ -133,6 +152,12 @@ struct fastrpc_mem_unmap { __s32 reserved[5]; }; +struct fastrpc_ioctl_set_option { + __u32 request_id; /* Request type (e.g., FASTRPC_POLL_MODE) */ + __u32 value; /* Request-specific value */ + __s32 reserved[6]; +}; + struct fastrpc_ioctl_capability { __u32 unused; /* deprecated, ignored by the kernel */ __u32 attribute_id; -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Add polling mode support for fastRPC driver 2026-04-22 9:24 ` [PATCH v9 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta @ 2026-04-22 21:50 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-04-22 21:50 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Assessment: Has several issues that need addressing.** **Issue 1 (Build break): `of_machine_get_match` does not exist** ```c data->poll_mode_supported = soc_data->poll_mode_supported || of_machine_get_match(fastrpc_poll_supported_machines); ``` The kernel has `of_machine_device_match()` (returns `bool`) and `of_machine_get_match_data()` (returns `const void *`). Since you only need a boolean "is this machine in the list", use `of_machine_device_match()`: ```c data->poll_mode_supported = soc_data->poll_mode_supported || of_machine_device_match(fastrpc_poll_supported_machines); ``` **Issue 2: `readl_poll_timeout_atomic` on DMA buffer memory** ```c ret = readl_poll_timeout_atomic(ctx->poll_addr, val, (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done, 1, FASTRPC_POLL_MAX_TIMEOUT_US); ``` `ctx->poll_addr` points into `ctx->buf->virt`, which is a DMA coherent buffer allocated via `dma_alloc_coherent()`, not an MMIO register. `readl()` is intended for MMIO and may apply `__iomem` annotations or arch-specific I/O accessors that are inappropriate for regular memory. Use `read_poll_timeout(READ_ONCE, ...)` or a custom polling loop with `READ_ONCE()` instead. This also avoids the atomic context restriction (see Issue 3). **Issue 3: 10ms atomic busy-wait** `readl_poll_timeout_atomic` uses `udelay()` and disables preemption for the entire duration. With `FASTRPC_POLL_MAX_TIMEOUT_US = 10000` (10ms), this is a very long time to hold a CPU non-preemptible. The non-atomic variant (`readl_poll_timeout` / `read_poll_timeout`) uses `usleep_range()` which is much more scheduler-friendly, and this code path is in process context (ioctl handler), so sleeping is fine. **Issue 4: Data race on `ctx->is_work_done`** `ctx->is_work_done` is read in the polling condition and written from `fastrpc_rpmsg_callback` (interrupt context) without any synchronization: ```c // In poll_for_remote_response (process context): (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done // In fastrpc_rpmsg_callback (interrupt context): ctx->is_work_done = true; ``` A plain `bool` is not guaranteed to be atomically visible across CPUs. This should use `READ_ONCE(ctx->is_work_done)` in the polling condition and `WRITE_ONCE(ctx->is_work_done, true)` in the callback (or use an atomic flag). Similarly in `poll_for_remote_response`: ```c if (!ret && val == FASTRPC_POLL_RESPONSE) { ctx->is_work_done = true; ``` This write should also use `WRITE_ONCE`. **Issue 5: `ctx->retval = 0` in poll path may race with callback** In `poll_for_remote_response`: ```c if (!ret && val == FASTRPC_POLL_RESPONSE) { ctx->is_work_done = true; ctx->retval = 0; } ``` The callback also sets `ctx->retval = rsp->retval`. If the DSP writes the poll response *and* sends an rpmsg response, both paths could write `retval` concurrently. The poll path hardcodes `retval = 0`, which may not match the actual DSP return value. If the DSP puts the real return value in the rpmsg response, blindly setting it to 0 in the poll path loses error information. **Issue 6: Missing `dma_rmb()` after polling** The existing code has a `dma_rmb()` barrier between receiving the completion and calling `fastrpc_put_args()`: ```c /* make sure that all memory writes by DSP are seen by CPU */ dma_rmb(); ``` After the polling path succeeds, the same barrier is needed to ensure the CPU sees all DSP writes to the output buffers. The current code does fall through to `fastrpc_put_args()` after `fastrpc_wait_for_completion()` returns, but the `dma_rmb()` is placed after the original wait code. Verify the barrier is still hit on the polling-success path — looking at the patched flow, `fastrpc_wait_for_completion()` returns 0, then execution continues to the existing `dma_rmb()` + `fastrpc_put_args()`, so this should be fine. **Issue 7: `__maybe_unused` on the match table** ```c static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unused = { ``` This suggests it may be unused when `CONFIG_OF` is off. But `of_machine_device_match()` has a stub returning `false` when `!CONFIG_OF`, so the table would indeed be unused. A cleaner approach would be to wrap the call with `IS_ENABLED(CONFIG_OF)` or just accept that this driver practically requires OF. **Issue 8: IOCTL number 12 skipped for `SET_OPTION`** The existing header has ioctl numbers 1-11 and then 13. The new `FASTRPC_IOCTL_SET_OPTION` is assigned number 12, which fills the gap. This is fine and intentional. **Issue 9: `FASTRPC_POLL_MODE` value documentation says "handle > 20" but code uses `FASTRPC_MAX_STATIC_HANDLE`** The UAPI header comment says: ```c * - Only applies to dynamic modules (handle > 20) ``` And the code uses: ```c if (handle > FASTRPC_MAX_STATIC_HANDLE && ...) ``` Where `FASTRPC_MAX_STATIC_HANDLE` is defined as 20. This is consistent, but hardcoding "20" in the UAPI comment is fragile. Consider referencing the macro name instead. **Minor: `fastrpc_wait_for_response` is marked `inline`** ```c static inline int fastrpc_wait_for_response(...) ``` The `inline` keyword is unnecessary — the compiler will decide. Kernel style generally avoids explicit `inline` on static functions unless there's a specific reason. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 0/4] misc: fastrpc: Add polling mode support
@ 2026-02-15 18:21 Ekansh Gupta
2026-02-15 18:21 ` [PATCH v6 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta
0 siblings, 1 reply; 13+ messages in thread
From: Ekansh Gupta @ 2026-02-15 18:21 UTC (permalink / raw)
To: srini, linux-arm-msm
Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
dmitry.baryshkov
This patch series adds polling mode feature that have been missing in
upstream FastRPC driver.
- Add changes to move fdlist to ctx structure to avoid code duplicacy.
- Update context mask to support polling mode.
- Add changes to support polling feature.
Userspace change: https://github.com/qualcomm/fastrpc/pull/258
Patch [v5]: https://lore.kernel.org/all/20251230062831.1195116-1-ekansh.gupta@oss.qualcomm.com/
Changes in v5:
- Fixed poll memory calculation.
- Added few formatting changes.
Changes in v5:
- Add more details in commit text.
Changes in v4:
- Replace hardcoded ctxid mask with GENMASK.
- Fixed commit text.
Changes in v3:
- Resolve compilation warning.
Changes in v2:
- Added comments and fixed commit text.
- Defined context id position as a macro.
- Added new IOCTL to control polling mode as always enabling
it might cause excess power consumption.
- Cleaned up polling mode implementation.
Ekansh Gupta (4):
misc: fastrpc: Move fdlist to invoke context structure
misc: fastrpc: Replace hardcoded ctxid mask with GENMASK
misc: fastrpc: Expand context ID mask for DSP polling mode support
misc: fastrpc: Add polling mode support for fastRPC driver
drivers/misc/fastrpc.c | 166 +++++++++++++++++++++++++++++++-----
include/uapi/misc/fastrpc.h | 10 +++
2 files changed, 154 insertions(+), 22 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v6 4/4] misc: fastrpc: Add polling mode support for fastRPC driver 2026-02-15 18:21 [PATCH v6 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta @ 2026-02-15 18:21 ` Ekansh Gupta 2026-02-15 22:26 ` Claude review: " Claude Code Review Bot 0 siblings, 1 reply; 13+ messages in thread From: Ekansh Gupta @ 2026-02-15 18:21 UTC (permalink / raw) To: srini, linux-arm-msm Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd, dmitry.baryshkov For any remote call to DSP, after sending an invocation message, fastRPC driver waits for glink response and during this time the CPU can go into low power modes. This adds latency to overall fastrpc call as CPU wakeup and scheduling latencies are included. Add polling mode support with which fastRPC driver will poll continuously on a memory after sending a message to remote subsystem which will eliminate CPU wakeup and scheduling latencies and reduce fastRPC overhead. Poll mode can be enabled by user by using FASTRPC_IOCTL_SET_OPTION ioctl request with FASTRPC_POLL_MODE request id. Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 142 ++++++++++++++++++++++++++++++++++-- include/uapi/misc/fastrpc.h | 10 +++ 2 files changed, 145 insertions(+), 7 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index e935ae3776b4..c1e67dbacf2c 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -23,6 +23,8 @@ #include <uapi/misc/fastrpc.h> #include <linux/of_reserved_mem.h> #include <linux/bits.h> +#include <linux/compiler.h> +#include <linux/iopoll.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -37,6 +39,7 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 +#define FASTRPC_MAX_STATIC_HANDLE (20) #define FASTRPC_CTXID_MASK GENMASK(15, 8) #define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) @@ -105,6 +108,12 @@ #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) +/* Poll response number from remote processor for call completion */ +#define FASTRPC_POLL_RESPONSE (0xdecaf) + +/* Polling mode timeout limit */ +#define FASTRPC_POLL_MAX_TIMEOUT_US (10000) + struct fastrpc_phy_page { dma_addr_t addr; /* dma address */ u64 size; /* size of contiguous region */ @@ -235,8 +244,14 @@ struct fastrpc_invoke_ctx { u32 sc; u64 *fdlist; u32 *crc; + /* Poll memory that DSP updates */ + u32 *poll; u64 ctxid; u64 msg_sz; + /* work done status flag */ + bool is_work_done; + /* process updates poll memory instead of glink response */ + bool is_polled; struct kref refcount; struct list_head node; /* list of ctxs */ struct completion work; @@ -307,6 +322,8 @@ struct fastrpc_user { int client_id; int pd; bool is_secure_dev; + /* Flags poll mode state */ + bool poll_mode; /* Lock for lists */ spinlock_t lock; /* lock for allocations */ @@ -924,7 +941,8 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx) sizeof(struct fastrpc_invoke_buf) + sizeof(struct fastrpc_phy_page)) * ctx->nscalars + sizeof(u64) * FASTRPC_MAX_FDLIST + - sizeof(u32) * FASTRPC_MAX_CRCLIST; + sizeof(u32) * FASTRPC_MAX_CRCLIST + + sizeof(u32); return size; } @@ -1020,6 +1038,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx) list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); ctx->fdlist = (u64 *)(pages + ctx->nscalars); + ctx->poll = (u32 *)((uintptr_t)ctx->fdlist + sizeof(u64) * FASTRPC_MAX_FDLIST + + sizeof(u32) * FASTRPC_MAX_CRCLIST); + args = (uintptr_t)ctx->buf->virt + metalen; rlen = pkt_size - metalen; ctx->rpra = rpra; @@ -1188,6 +1209,75 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx, } +static inline u32 fastrpc_poll_op(void *p) +{ + struct fastrpc_invoke_ctx *ctx = p; + + dma_rmb(); + return READ_ONCE(*ctx->poll); +} + +static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx) +{ + u32 val; + int ret; + + /* + * Poll until DSP writes FASTRPC_POLL_RESPONSE into *ctx->poll + * or until another path marks the work done. + */ + ret = read_poll_timeout_atomic(fastrpc_poll_op, val, + (val == FASTRPC_POLL_RESPONSE) || + ctx->is_work_done, 1, + FASTRPC_POLL_MAX_TIMEOUT_US, false, ctx); + + if (!ret && val == FASTRPC_POLL_RESPONSE) { + ctx->is_work_done = true; + ctx->retval = 0; + } + + if (ret == -ETIMEDOUT) + ret = -EIO; + + return ret; +} + +static inline int fastrpc_wait_for_response(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err = 0; + + if (kernel) { + if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) + err = -ETIMEDOUT; + } else { + err = wait_for_completion_interruptible(&ctx->work); + } + + return err; +} + +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx, + u32 kernel) +{ + int err; + + do { + if (ctx->is_polled) { + err = poll_for_remote_response(ctx); + /* If polling timed out, move to normal response mode */ + 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; +} + static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, u32 handle, u32 sc, struct fastrpc_invoke_args *args) @@ -1223,16 +1313,26 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, if (err) goto bail; - if (kernel) { - if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) - err = -ETIMEDOUT; - } else { - err = wait_for_completion_interruptible(&ctx->work); - } + /* + * Set message context as polled if the call is for a user PD + * dynamic module and user has enabled poll mode. + */ + if (handle > FASTRPC_MAX_STATIC_HANDLE && fl->pd == USER_PD && + fl->poll_mode) + ctx->is_polled = true; + + err = fastrpc_wait_for_completion(ctx, kernel); if (err) goto bail; + if (!ctx->is_work_done) { + err = -ETIMEDOUT; + dev_dbg(fl->sctx->dev, "Invalid workdone state for handle 0x%x, sc 0x%x\n", + handle, sc); + goto bail; + } + /* make sure that all memory writes by DSP are seen by CPU */ dma_rmb(); /* populate all the output buffers with results */ @@ -1812,6 +1912,30 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, return 0; } +static int fastrpc_set_option(struct fastrpc_user *fl, char __user *argp) +{ + struct fastrpc_ioctl_set_option opt = {0}; + int i; + + if (copy_from_user(&opt, argp, sizeof(opt))) + return -EFAULT; + + for (i = 0; i < ARRAY_SIZE(opt.reserved); i++) { + if (opt.reserved[i] != 0) + return -EINVAL; + } + + if (opt.req != FASTRPC_POLL_MODE) + return -EINVAL; + + if (opt.value) + fl->poll_mode = true; + else + fl->poll_mode = false; + + return 0; +} + static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_ioctl_capability cap = {0}; @@ -2167,6 +2291,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, case FASTRPC_IOCTL_MEM_UNMAP: err = fastrpc_req_mem_unmap(fl, argp); break; + case FASTRPC_IOCTL_SET_OPTION: + err = fastrpc_set_option(fl, argp); + break; case FASTRPC_IOCTL_GET_DSP_INFO: err = fastrpc_get_dsp_info(fl, argp); break; @@ -2518,6 +2645,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data, } ctx->retval = rsp->retval; + ctx->is_work_done = true; complete(&ctx->work); /* diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h index c6e2925f47e6..c37e24a764ae 100644 --- a/include/uapi/misc/fastrpc.h +++ b/include/uapi/misc/fastrpc.h @@ -16,6 +16,7 @@ #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static) #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map) #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap) +#define FASTRPC_IOCTL_SET_OPTION _IOWR('R', 12, struct fastrpc_ioctl_set_option) #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability) /** @@ -67,6 +68,9 @@ enum fastrpc_proc_attr { /* Fastrpc attribute for memory protection of buffers */ #define FASTRPC_ATTR_SECUREMAP (1) +/* Set option request ID to enable poll mode */ +#define FASTRPC_POLL_MODE (1) + struct fastrpc_invoke_args { __u64 ptr; __u64 length; @@ -133,6 +137,12 @@ struct fastrpc_mem_unmap { __s32 reserved[5]; }; +struct fastrpc_ioctl_set_option { + __u32 req; /* request id */ + __u32 value; /* value */ + __s32 reserved[6]; +}; + struct fastrpc_ioctl_capability { __u32 unused; /* deprecated, ignored by the kernel */ __u32 attribute_id; -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Claude review: misc: fastrpc: Add polling mode support for fastRPC driver 2026-02-15 18:21 ` [PATCH v6 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta @ 2026-02-15 22:26 ` Claude Code Review Bot 0 siblings, 0 replies; 13+ messages in thread From: Claude Code Review Bot @ 2026-02-15 22:26 UTC (permalink / raw) To: dri-devel-reviews 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-05-25 12:12 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-21 5:45 [PATCH v12 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta 2026-05-21 5:45 ` [PATCH v12 1/4] misc: fastrpc: Move fdlist to invoke context structure Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-21 5:45 ` [PATCH v12 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta 2026-05-25 10:54 ` Claude review: " Claude Code Review Bot 2026-05-25 10:54 ` Claude review: misc: fastrpc: Add polling mode support Claude Code Review Bot -- strict thread matches above, loose matches on Subject: below -- 2026-05-20 6:50 [PATCH v11 0/4] " Ekansh Gupta 2026-05-20 6:50 ` [PATCH v11 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta 2026-05-25 12:12 ` Claude review: " Claude Code Review Bot 2026-04-22 9:24 [PATCH v9 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta 2026-04-22 9:24 ` [PATCH v9 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta 2026-04-22 21:50 ` Claude review: " Claude Code Review Bot 2026-02-15 18:21 [PATCH v6 0/4] misc: fastrpc: Add polling mode support Ekansh Gupta 2026-02-15 18:21 ` [PATCH v6 4/4] misc: fastrpc: Add polling mode support for fastRPC driver Ekansh Gupta 2026-02-15 22:26 ` Claude review: " Claude Code Review Bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox