[PATCH v1 0/4] Add invokeV2 to support new features

[PATCH v1 0/4] Add invokeV2 to support new features

[Ekansh Gupta]

This patch series adds the listed features that have been missing
in upstream fastRPC driver.

- Add changes to support new enhanced invocation ioctl request.
- Add support for CRC check.
- Add support for DSP and kernel performance counters.
- Add polling mode support.

Userspace change: https://github.com/quic/fastrpc/pull/73

Ekansh Gupta (4):
  misc: fastrpc: Add CRC support using invokeV2 request
  misc: fastrpc: Capture kernel and DSP performance counters
  misc: fastrpc: Modify context id calculation for poll mode
  misc: fastrpc: Add polling mode support for fastRPC driver

 drivers/misc/fastrpc.c      | 435 ++++++++++++++++++++++++++++++------
 include/uapi/misc/fastrpc.h |  10 +
 2 files changed, 376 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

InvokeV2 request is intended to support multiple enhanced invoke
requests like CRC check, performance counter enablement and polling
mode for RPC invocations. CRC check is getting enabled as part of
this patch. CRC check for input and output argument helps in ensuring
data consistency over a remote call. If user intends to enable CRC
check, first local user CRC is calculated at user end and a CRC buffer
is passed to DSP to capture remote CRC values. DSP is expected to
write to the remote CRC buffer which is then compared at user level
with the local CRC values.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
 include/uapi/misc/fastrpc.h |   7 ++
 2 files changed, 116 insertions(+), 52 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 74181b8c386b..8e817a763d1d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
 
 static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 			struct fastrpc_user *user, u32 kernel, u32 sc,
-			struct fastrpc_invoke_args *args)
+			struct fastrpc_invoke_v2 *inv2)
 {
 	struct fastrpc_channel_ctx *cctx = user->cctx;
 	struct fastrpc_invoke_ctx *ctx = NULL;
+	struct fastrpc_invoke_args *args = NULL;
 	unsigned long flags;
 	int ret;
 
+	args = (struct fastrpc_invoke_args *)inv2->inv.args;
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return ERR_PTR(-ENOMEM);
@@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	/* Released in fastrpc_context_put() */
 	fastrpc_channel_ctx_get(cctx);
 
+	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
 	ctx->sc = sc;
 	ctx->retval = -1;
 	ctx->pid = current->pid;
@@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 	struct fastrpc_invoke_buf *list;
 	struct fastrpc_phy_page *pages;
 	u64 *fdlist;
+	u32 *crclist;
 	int i, inbufs, outbufs, handles;
 
 	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
@@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
 	pages = fastrpc_phy_page_start(list, ctx->nscalars);
 	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
+	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
 
 	for (i = inbufs; i < ctx->nbufs; ++i) {
 		if (!ctx->maps[i]) {
@@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 			fastrpc_map_put(mmap);
 	}
 
+	if (ctx->crc && crclist && rpra) {
+		if (copy_to_user((void __user *)ctx->crc, crclist,
+				FASTRPC_MAX_CRCLIST * sizeof(u32)))
+			return -EFAULT;
+	}
+
 	return 0;
 }
 
@@ -1137,13 +1148,12 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
 
 }
 
-static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
-				   u32 handle, u32 sc,
-				   struct fastrpc_invoke_args *args)
+static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
 {
 	struct fastrpc_invoke_ctx *ctx = NULL;
 	struct fastrpc_buf *buf, *b;
-
+	struct fastrpc_invoke inv;
+	u32 handle, sc;
 	int err = 0;
 
 	if (!fl->sctx)
@@ -1152,12 +1162,15 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	if (!fl->cctx->rpdev)
 		return -EPIPE;
 
+	inv = inv2->inv;
+	handle = inv.handle;
+	sc = inv.sc;
 	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
 		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
 		return -EPERM;
 	}
 
-	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
+	ctx = fastrpc_context_alloc(fl, kernel, sc, inv2);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
@@ -1239,6 +1252,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 {
 	struct fastrpc_init_create_static init;
 	struct fastrpc_invoke_args *args;
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_phy_page pages[1];
 	char *name;
 	int err;
@@ -1248,7 +1262,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		u32 namelen;
 		u32 pageslen;
 	} inbuf;
-	u32 sc;
 
 	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
 	if (!args)
@@ -1313,10 +1326,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	args[2].length = sizeof(*pages);
 	args[2].fd = -1;
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
-
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
-				      sc, args);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (err)
 		goto err_invoke;
 
@@ -1357,6 +1370,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 {
 	struct fastrpc_init_create init;
 	struct fastrpc_invoke_args *args;
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_phy_page pages[1];
 	struct fastrpc_map *map = NULL;
 	struct fastrpc_buf *imem = NULL;
@@ -1370,7 +1384,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		u32 attrs;
 		u32 siglen;
 	} inbuf;
-	u32 sc;
 	bool unsigned_module = false;
 
 	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
@@ -1444,12 +1457,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 	args[5].length = sizeof(inbuf.siglen);
 	args[5].fd = -1;
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
 	if (init.attrs)
-		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
-
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
-				      sc, args);
+		ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (err)
 		goto err_invoke;
 
@@ -1501,17 +1514,18 @@ static void fastrpc_session_free(struct fastrpc_channel_ctx *cctx,
 static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
 {
 	struct fastrpc_invoke_args args[1];
+	struct fastrpc_invoke_v2 ioctl = {0};
 	int tgid = 0;
-	u32 sc;
 
 	tgid = fl->tgid;
 	args[0].ptr = (u64)(uintptr_t) &tgid;
 	args[0].length = sizeof(tgid);
 	args[0].fd = -1;
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
 
-	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
-				       sc, &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
+	ioctl.inv.args = (u64)args;
+	return fastrpc_internal_invoke(fl, true, &ioctl);
 }
 
 static int fastrpc_device_release(struct inode *inode, struct file *file)
@@ -1647,45 +1661,77 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 {
 	struct fastrpc_invoke_args args[1];
+	struct fastrpc_invoke_v2 ioctl = {0};
 	int tgid = fl->tgid;
-	u32 sc;
 
 	args[0].ptr = (u64)(uintptr_t) &tgid;
 	args[0].length = sizeof(tgid);
 	args[0].fd = -1;
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
 	fl->pd = pd;
 
-	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
-				       sc, &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
+	ioctl.inv.args = (u64)args;
+	return fastrpc_internal_invoke(fl, true, &ioctl);
 }
 
-static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
+static int fastrpc_copy_args(struct fastrpc_invoke *inv)
 {
 	struct fastrpc_invoke_args *args = NULL;
-	struct fastrpc_invoke inv;
 	u32 nscalars;
-	int err;
-
-	if (copy_from_user(&inv, argp, sizeof(inv)))
-		return -EFAULT;
 
 	/* nscalars is truncated here to max supported value */
-	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
+	nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
 	if (nscalars) {
 		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
 		if (!args)
 			return -ENOMEM;
 
-		if (copy_from_user(args, (void __user *)(uintptr_t)inv.args,
+		if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
 				   nscalars * sizeof(*args))) {
 			kfree(args);
 			return -EFAULT;
 		}
 	}
+	inv->args = args;
 
-	err = fastrpc_internal_invoke(fl, false, inv.handle, inv.sc, args);
-	kfree(args);
+	return 0;
+}
+
+static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
+{
+	struct fastrpc_invoke_v2 ioctl = {0};
+	struct fastrpc_invoke inv;
+	int err;
+
+	if (copy_from_user(&inv, argp, sizeof(inv)))
+		return -EFAULT;
+
+	err = fastrpc_copy_args(&inv);
+	if (err)
+		return err;
+
+	ioctl.inv = inv;
+	err = fastrpc_internal_invoke(fl, false, &ioctl);
+	kfree(inv.args);
+
+	return err;
+}
+
+static int fastrpc_invokev2(struct fastrpc_user *fl, char __user *argp)
+{
+	struct fastrpc_invoke_v2 inv2 = {0};
+	int err;
+
+	if (copy_from_user(&inv2, argp, sizeof(inv2)))
+		return -EFAULT;
+
+	err = fastrpc_copy_args(&inv2.inv);
+	if (err)
+		return err;
+
+	err = fastrpc_internal_invoke(fl, false, &inv2);
+	kfree(inv2.inv.args);
 
 	return err;
 }
@@ -1694,6 +1740,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
 				     uint32_t dsp_attr_buf_len)
 {
 	struct fastrpc_invoke_args args[2] = { 0 };
+	struct fastrpc_invoke_v2 ioctl = {0};
 
 	/*
 	 * Capability filled in userspace. This carries the information
@@ -1710,8 +1757,10 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
 	args[1].length = dsp_attr_buf_len * sizeof(u32);
 	args[1].fd = -1;
 
-	return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
-				       FASTRPC_SCALARS(0, 1, 1), args);
+	ioctl.inv.handle = FASTRPC_DSP_UTILITIES_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(0, 1, 1);
+	ioctl.inv.args = (u64)args;
+	return fastrpc_internal_invoke(fl, true, &ioctl);
 }
 
 static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
@@ -1798,10 +1847,10 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
 {
 	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_munmap_req_msg req_msg;
 	struct device *dev = fl->sctx->dev;
 	int err;
-	u32 sc;
 
 	req_msg.pgid = fl->tgid;
 	req_msg.size = buf->size;
@@ -1810,9 +1859,10 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 	args[0].ptr = (u64) (uintptr_t) &req_msg;
 	args[0].length = sizeof(req_msg);
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
-				      &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (!err) {
 		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
 		spin_lock(&fl->lock);
@@ -1856,6 +1906,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 {
 	struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_buf *buf = NULL;
 	struct fastrpc_mmap_req_msg req_msg;
 	struct fastrpc_mmap_rsp_msg rsp_msg;
@@ -1863,7 +1914,6 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 	struct fastrpc_req_mmap req;
 	struct device *dev = fl->sctx->dev;
 	int err;
-	u32 sc;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -1906,9 +1956,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 	args[2].ptr = (u64) (uintptr_t) &rsp_msg;
 	args[2].length = sizeof(rsp_msg);
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
-				      &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (err) {
 		dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
 		fastrpc_buf_free(buf);
@@ -1957,10 +2008,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
 {
 	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_map *map = NULL, *iter, *m;
 	struct fastrpc_mem_unmap_req_msg req_msg = { 0 };
 	int err = 0;
-	u32 sc;
 	struct device *dev = fl->sctx->dev;
 
 	spin_lock(&fl->lock);
@@ -1986,9 +2037,10 @@ static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_me
 	args[0].ptr = (u64) (uintptr_t) &req_msg;
 	args[0].length = sizeof(req_msg);
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0);
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
-				      &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (err) {
 		dev_err(dev, "unmmap\tpt fd = %d, 0x%09llx error\n",  map->fd, map->raddr);
 		return err;
@@ -2011,6 +2063,7 @@ static int fastrpc_req_mem_unmap(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
 {
 	struct fastrpc_invoke_args args[4] = { [0 ... 3] = { 0 } };
+	struct fastrpc_invoke_v2 ioctl = {0};
 	struct fastrpc_mem_map_req_msg req_msg = { 0 };
 	struct fastrpc_mmap_rsp_msg rsp_msg = { 0 };
 	struct fastrpc_mem_unmap req_unmap = { 0 };
@@ -2019,7 +2072,6 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
 	struct device *dev = fl->sctx->dev;
 	struct fastrpc_map *map = NULL;
 	int err;
-	u32 sc;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -2055,8 +2107,10 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
 	args[3].ptr = (u64) (uintptr_t) &rsp_msg;
 	args[3].length = sizeof(rsp_msg);
 
-	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_MAP, 3, 1);
-	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]);
+	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
+	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_MAP, 3, 1);
+	ioctl.inv.args = (u64)args;
+	err = fastrpc_internal_invoke(fl, true, &ioctl);
 	if (err) {
 		dev_err(dev, "mem mmap error, fd %d, vaddr %llx, size %lld\n",
 			req.fd, req.vaddrin, map->size);
@@ -2096,6 +2150,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 	case FASTRPC_IOCTL_INVOKE:
 		err = fastrpc_invoke(fl, argp);
 		break;
+	case FASTRPC_IOCTL_INVOKEV2:
+		err = fastrpc_invokev2(fl, argp);
+		break;
 	case FASTRPC_IOCTL_INIT_ATTACH:
 		err = fastrpc_init_attach(fl, ROOT_PD);
 		break;
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index f33d914d8f46..406b80555d41 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -17,6 +17,7 @@
 #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_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
+#define FASTRPC_IOCTL_INVOKEV2		_IOWR('R', 14, struct fastrpc_invoke_v2)
 
 /**
  * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
@@ -80,6 +81,12 @@ struct fastrpc_invoke {
 	__u64 args;
 };
 
+struct fastrpc_invoke_v2 {
+	struct fastrpc_invoke inv;
+	__u64 crc;
+	__u32 reserved[16];
+};
+
 struct fastrpc_init_create {
 	__u32 filelen;	/* elf file length */
 	__s32 filefd;	/* fd for the file */
-- 
2.34.1

[PATCH v1 2/4] misc: fastrpc: Capture kernel and DSP performance counters

[Ekansh Gupta]

Add support to capture kernel performance counters for different
kernel level operations. These counters collects the information
for remote call and copies the information to a buffer shared
by user.

Collection of DSP performance counters is also added as part of
this change. DSP updates the performance information in the
metadata which is then copied to a buffer passed by the users.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c      | 143 ++++++++++++++++++++++++++++++++++--
 include/uapi/misc/fastrpc.h |   4 +-
 2 files changed, 139 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 8e817a763d1d..54a562fc94fb 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -19,6 +19,7 @@
 #include <linux/rpmsg.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <uapi/misc/fastrpc.h>
 #include <linux/of_reserved_mem.h>
@@ -38,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 (0xFF0)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
 #define INIT_FILE_NAMELEN_MAX (128)
@@ -106,6 +108,9 @@
 
 #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
 
+#define FASTRPC_KERNEL_PERF_LIST (PERF_KEY_MAX)
+#define FASTRPC_DSP_PERF_LIST 12
+
 static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
 						"sdsp", "cdsp", "cdsp1" };
 struct fastrpc_phy_page {
@@ -229,6 +234,18 @@ struct fastrpc_map {
 	struct kref refcount;
 };
 
+struct fastrpc_perf {
+	u64 count;
+	u64 flush;
+	u64 map;
+	u64 copy;
+	u64 link;
+	u64 getargs;
+	u64 putargs;
+	u64 invargs;
+	u64 invoke;
+};
+
 struct fastrpc_invoke_ctx {
 	int nscalars;
 	int nbufs;
@@ -237,6 +254,8 @@ struct fastrpc_invoke_ctx {
 	int tgid;
 	u32 sc;
 	u32 *crc;
+	u64 *perf_kernel;
+	u64 *perf_dsp;
 	u64 ctxid;
 	u64 msg_sz;
 	struct kref refcount;
@@ -251,6 +270,7 @@ struct fastrpc_invoke_ctx {
 	struct fastrpc_invoke_args *args;
 	struct fastrpc_buf_overlap *olaps;
 	struct fastrpc_channel_ctx *cctx;
+	struct fastrpc_perf *perf;
 };
 
 struct fastrpc_session_ctx {
@@ -308,6 +328,51 @@ struct fastrpc_user {
 	struct mutex mutex;
 };
 
+enum fastrpc_perfkeys {
+	PERF_COUNT = 0,
+	PERF_FLUSH = 1,
+	PERF_MAP = 2,
+	PERF_COPY = 3,
+	PERF_LINK = 4,
+	PERF_GETARGS = 5,
+	PERF_PUTARGS = 6,
+	PERF_INVARGS = 7,
+	PERF_INVOKE = 8,
+	PERF_KEY_MAX = 9,
+};
+
+#define PERF_END ((void)0)
+
+#define PERF(enb, cnt, ff) \
+	{\
+		struct timespec64 startT = {0};\
+		u64 *counter = cnt;\
+		if (enb && counter) {\
+			ktime_get_boottime_ts64(&startT);\
+		} \
+		ff ;\
+		if (enb && counter) {\
+			*counter += getnstimediff(&startT);\
+		} \
+	}
+
+#define GET_COUNTER(perf_ptr, offset)  \
+	(perf_ptr != NULL ?\
+		(((offset >= 0) && (offset < PERF_KEY_MAX)) ?\
+			(u64 *)(perf_ptr + offset)\
+				: (u64 *)NULL) : (u64 *)NULL)
+
+static inline s64 getnstimediff(struct timespec64 *start)
+{
+	s64 ns;
+	struct timespec64 ts, b;
+
+	ktime_get_boottime_ts64(&ts);
+	b = timespec64_sub(ts, *start);
+	ns = timespec64_to_ns(&b);
+	return ns;
+}
+
 static void fastrpc_free_map(struct kref *ref)
 {
 	struct fastrpc_map *map;
@@ -497,6 +562,7 @@ static void fastrpc_context_free(struct kref *ref)
 	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
+	kfree(ctx->perf);
 	kfree(ctx->maps);
 	kfree(ctx->olaps);
 	kfree(ctx);
@@ -614,6 +680,13 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	fastrpc_channel_ctx_get(cctx);
 
 	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
+	ctx->perf_dsp = (u64 *)(uintptr_t)inv2->perf_dsp;
+	ctx->perf_kernel = (u64 *)(uintptr_t)inv2->perf_kernel;
+	if (ctx->perf_kernel) {
+		ctx->perf = kzalloc(sizeof(*(ctx->perf)), GFP_KERNEL);
+		if (!ctx->perf)
+			return ERR_PTR(-ENOMEM);
+	}
 	ctx->sc = sc;
 	ctx->retval = -1;
 	ctx->pid = current->pid;
@@ -877,7 +950,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) + sizeof(u64) * FASTRPC_DSP_PERF_LIST;
 
 	return size;
 }
@@ -951,7 +1025,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 	metalen = fastrpc_get_meta_size(ctx);
 	pkt_size = fastrpc_get_payload_size(ctx, metalen);
 
+	PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_MAP),
 	err = fastrpc_create_maps(ctx);
+	PERF_END);
 	if (err)
 		return err;
 
@@ -987,6 +1063,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			continue;
 
 		if (ctx->maps[i]) {
+			PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_MAP),
 			struct vm_area_struct *vma = NULL;
 
 			rpra[i].buf.pv = (u64) ctx->args[i].ptr;
@@ -1003,9 +1080,9 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			pg_end = ((ctx->args[i].ptr + len - 1) & PAGE_MASK) >>
 				  PAGE_SHIFT;
 			pages[i].size = (pg_end - pg_start + 1) * PAGE_SIZE;
-
+			PERF_END);
 		} else {
-
+			PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_COPY),
 			if (ctx->olaps[oix].offset == 0) {
 				rlen -= ALIGN(args, FASTRPC_ALIGN) - args;
 				args = ALIGN(args, FASTRPC_ALIGN);
@@ -1027,12 +1104,14 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			pages[i].size = (pg_end - pg_start + 1) * PAGE_SIZE;
 			args = args + mlen;
 			rlen -= mlen;
+			PERF_END);
 		}
 
 		if (i < inbufs && !ctx->maps[i]) {
 			void *dst = (void *)(uintptr_t)rpra[i].buf.pv;
 			void *src = (void *)(uintptr_t)ctx->args[i].ptr;
 
+			PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_COPY),
 			if (!kernel) {
 				if (copy_from_user(dst, (void __user *)src,
 						   len)) {
@@ -1042,6 +1121,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			} else {
 				memcpy(dst, src, len);
 			}
+			PERF_END);
 		}
 	}
 
@@ -1072,9 +1152,9 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 	struct fastrpc_map *mmap = NULL;
 	struct fastrpc_invoke_buf *list;
 	struct fastrpc_phy_page *pages;
-	u64 *fdlist;
-	u32 *crclist;
-	int i, inbufs, outbufs, handles;
+	u64 *fdlist, *perf_dsp_list;
+	u32 *crclist, *poll;
+	int i, inbufs, outbufs, handles, perferr;
 
 	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
 	outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
@@ -1083,6 +1163,8 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 	pages = fastrpc_phy_page_start(list, ctx->nscalars);
 	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
 	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
+	poll = (u32 *)(crclist + FASTRPC_MAX_CRCLIST);
+	perf_dsp_list = (u64 *)(poll + 1);
 
 	for (i = inbufs; i < ctx->nbufs; ++i) {
 		if (!ctx->maps[i]) {
@@ -1113,6 +1195,14 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 			return -EFAULT;
 	}
 
+	if (ctx->perf_dsp && perf_dsp_list) {
+		perferr = copy_to_user((void __user *)ctx->perf_dsp,
+				      perf_dsp_list,
+				      FASTRPC_DSP_PERF_LIST * sizeof(u64));
+		if (perferr)
+			dev_dbg(fl->sctx->dev, "Warning: failed to copy perf data %d\n", perferr);
+	}
+
 	return 0;
 }
 
@@ -1148,13 +1238,35 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
 
 }
 
+static void fastrpc_update_invoke_count(u32 handle, u64 *perf_counter,
+					struct timespec64 *invoket)
+{
+	/* update invoke count for dynamic handles */
+	u64 *invcount, *count;
+
+	if (handle <= FASTRPC_MAX_STATIC_HANDLE)
+		return;
+
+	invcount = GET_COUNTER(perf_counter, PERF_INVOKE);
+	if (invcount)
+		*invcount += getnstimediff(invoket);
+
+	count = GET_COUNTER(perf_counter, PERF_COUNT);
+	if (count)
+		*count++;
+}
+
 static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
 {
 	struct fastrpc_invoke_ctx *ctx = NULL;
 	struct fastrpc_buf *buf, *b;
 	struct fastrpc_invoke inv;
 	u32 handle, sc;
-	int err = 0;
+	int err = 0, perferr = 0;
+	struct timespec64 invoket = {0};
+
+	if (inv2->perf_kernel)
+		ktime_get_boottime_ts64(&invoket);
 
 	if (!fl->sctx)
 		return -EINVAL;
@@ -1174,14 +1286,18 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
+	PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_GETARGS),
 	err = fastrpc_get_args(kernel, ctx);
+	PERF_END);
 	if (err)
 		goto bail;
 
 	/* make sure that all CPU memory writes are seen by DSP */
 	dma_wmb();
+	PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_LINK),
 	/* Send invoke buffer to remote dsp */
 	err = fastrpc_invoke_send(fl->sctx, ctx, kernel, handle);
+	PERF_END);
 	if (err)
 		goto bail;
 
@@ -1197,8 +1313,10 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct
 
 	/* make sure that all memory writes by DSP are seen by CPU */
 	dma_rmb();
+	PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_PUTARGS),
 	/* populate all the output buffers with results */
 	err = fastrpc_put_args(ctx, kernel);
+	PERF_END);
 	if (err)
 		goto bail;
 
@@ -1209,6 +1327,17 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct
 
 bail:
 	if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
+		if (ctx) {
+			if (ctx->perf_kernel && !err)
+				fastrpc_update_invoke_count(handle, (u64 *)ctx->perf, &invoket);
+			if (ctx->perf_kernel && ctx->perf && ctx->perf_kernel) {
+				perferr = copy_to_user((void __user *)ctx->perf_kernel,
+							ctx->perf,
+							FASTRPC_KERNEL_PERF_LIST * sizeof(u64));
+				if (perferr)
+					dev_dbg(fl->sctx->dev, "Warning: failed to copy perf data %d\n", perferr);
+			}
+		}
 		/* We are done with this compute context */
 		spin_lock(&fl->lock);
 		list_del(&ctx->node);
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index 406b80555d41..1edc7c04b171 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -84,7 +84,9 @@ struct fastrpc_invoke {
 struct fastrpc_invoke_v2 {
 	struct fastrpc_invoke inv;
 	__u64 crc;
-	__u32 reserved[16];
+	__u64 perf_kernel;
+	__u64 perf_dsp;
+	__u32 reserved[12];
 };
 
 struct fastrpc_init_create {
-- 
2.34.1

[PATCH v1 3/4] misc: fastrpc: Modify context id calculation for poll mode

[Ekansh Gupta]

Poll mode is a feature to be introduced which improves the fastrpc
remote calls latency. For this feature, DSP expects the 4th to 7th
bit of context id to be zero. Prepare context id in such a way that
polling mode can be supported.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 54a562fc94fb..c008fcd95e15 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -40,7 +40,7 @@
 #define FASTRPC_INIT_HANDLE	1
 #define FASTRPC_DSP_UTILITIES_HANDLE	2
 #define FASTRPC_MAX_STATIC_HANDLE (20)
-#define FASTRPC_CTXID_MASK (0xFF0)
+#define FASTRPC_CTXID_MASK (0xFF0000)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
 #define INIT_FILE_NAMELEN_MAX (128)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
@@ -559,7 +559,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, ctx->ctxid >> 16);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	kfree(ctx->perf);
@@ -706,7 +706,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 		spin_unlock_irqrestore(&cctx->lock, flags);
 		goto err_idr;
 	}
-	ctx->ctxid = ret << 4;
+	ctx->ctxid = ret << 16;
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	kref_init(&ctx->refcount);
@@ -2621,7 +2621,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 = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 16);
 
 	spin_lock_irqsave(&cctx->lock, flags);
 	ctx = idr_find(&cctx->ctx_idr, ctxid);
-- 
2.34.1

[PATCH v1 4/4] misc: fastrpc: Add polling mode support for fastRPC driver

[Ekansh Gupta]

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. Adding a 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.
With this change, DSP always sends a glink response which will
get ignored if polling mode didn't time out.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c      | 127 +++++++++++++++++++++++++++++++++---
 include/uapi/misc/fastrpc.h |   3 +-
 2 files changed, 121 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c008fcd95e15..a25673c22db8 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -111,6 +111,19 @@
 #define FASTRPC_KERNEL_PERF_LIST (PERF_KEY_MAX)
 #define FASTRPC_DSP_PERF_LIST 12
 
+/* Poll response number from remote processor for call completion */
+#define FASTRPC_POLL_RESPONSE (0xdecaf)
+/* timeout in us for polling until memory barrier */
+#define FASTRPC_POLL_TIME_MEM_UPDATE (500)
+
+/* Response types supported for RPC calls */
+enum fastrpc_response_flags {
+	/* normal job completion glink response */
+	NORMAL_RESPONSE = 0,
+	/* process updates poll memory instead of glink response */
+	POLL_MODE = 1,
+};
+
 static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
 						"sdsp", "cdsp", "cdsp1" };
 struct fastrpc_phy_page {
@@ -258,6 +271,12 @@ struct fastrpc_invoke_ctx {
 	u64 *perf_dsp;
 	u64 ctxid;
 	u64 msg_sz;
+	/* Threads poll for specified timeout and fall back to glink wait */
+	u64 poll_timeout;
+	/* work done status flag */
+	bool is_work_done;
+	/* response flags from remote processor */
+	enum fastrpc_response_flags rsp_flags;
 	struct kref refcount;
 	struct list_head node; /* list of ctxs */
 	struct completion work;
@@ -682,6 +701,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
 	ctx->perf_dsp = (u64 *)(uintptr_t)inv2->perf_dsp;
 	ctx->perf_kernel = (u64 *)(uintptr_t)inv2->perf_kernel;
+	ctx->poll_timeout = (u64)inv2->poll_timeout;
 	if (ctx->perf_kernel) {
 		ctx->perf = kzalloc(sizeof(*(ctx->perf)), GFP_KERNEL);
 		if (!ctx->perf)
@@ -692,6 +712,8 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	ctx->pid = current->pid;
 	ctx->tgid = user->tgid;
 	ctx->cctx = cctx;
+	ctx->rsp_flags = NORMAL_RESPONSE;
+	ctx->is_work_done = false;
 	init_completion(&ctx->work);
 	INIT_WORK(&ctx->put_work, fastrpc_context_put_wq);
 
@@ -1256,6 +1278,87 @@ static void fastrpc_update_invoke_count(u32 handle, u64 *perf_counter,
 		*count++;
 }
 
+static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx, u64 timeout)
+{
+	int err = -EIO, i, j;
+	u32 sc = ctx->sc;
+	struct fastrpc_invoke_buf *list;
+	struct fastrpc_phy_page *pages;
+	u64 *fdlist = NULL;
+	u32 *crclist = NULL, *poll = NULL;
+	unsigned int inbufs, outbufs, handles;
+
+	/* calculate poll memory location */
+	inbufs = REMOTE_SCALARS_INBUFS(sc);
+	outbufs = REMOTE_SCALARS_OUTBUFS(sc);
+	handles = REMOTE_SCALARS_INHANDLES(sc) + REMOTE_SCALARS_OUTHANDLES(sc);
+	list = fastrpc_invoke_buf_start(ctx->rpra, ctx->nscalars);
+	pages = fastrpc_phy_page_start(list, ctx->nscalars);
+	fdlist = (u64 *)(pages + inbufs + outbufs + handles);
+	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
+	poll = (u32 *)(crclist + FASTRPC_MAX_CRCLIST);
+
+	/* poll on memory for DSP response. Return failure on timeout */
+	for (i = 0, j = 0; i < timeout; i++, j++) {
+		if (*poll == FASTRPC_POLL_RESPONSE) {
+			err = 0;
+			ctx->is_work_done = true;
+			ctx->retval = 0;
+			break;
+		}
+		if (j == FASTRPC_POLL_TIME_MEM_UPDATE) {
+			/* make sure that all poll memory writes by DSP are seen by CPU */
+			dma_rmb();
+			j = 0;
+		}
+		udelay(1);
+	}
+	return err;
+}
+
+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 {
+		switch (ctx->rsp_flags) {
+		case NORMAL_RESPONSE:
+			err = fastrpc_wait_for_response(ctx, kernel);
+			if (err || ctx->is_work_done)
+				return err;
+			break;
+		case POLL_MODE:
+			err = poll_for_remote_response(ctx, ctx->poll_timeout);
+			/* If polling timed out, move to normal response mode */
+			if (err)
+				ctx->rsp_flags = NORMAL_RESPONSE;
+			break;
+		default:
+			err = -EBADR;
+			dev_dbg(ctx->fl->sctx->dev, "unsupported response type:0x%x\n", ctx->rsp_flags);
+			break;
+		}
+	} while (!ctx->is_work_done);
+
+	return err;
+}
+
 static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
 {
 	struct fastrpc_invoke_ctx *ctx = NULL;
@@ -1301,16 +1404,22 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct
 	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);
-	}
+	if (ctx->poll_timeout != 0 && handle > FASTRPC_MAX_STATIC_HANDLE &&
+		fl->cctx->domain_id == CDSP_DOMAIN_ID &&
+		fl->pd == USER_PD)
+		ctx->rsp_flags = POLL_MODE;
 
+	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();
 	PERF(ctx->perf_kernel, GET_COUNTER((u64 *)ctx->perf, PERF_PUTARGS),
@@ -2627,12 +2736,14 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 	ctx = idr_find(&cctx->ctx_idr, ctxid);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
+	/* Ignore this failure as context returned will be NULL for polling mode */
 	if (!ctx) {
-		dev_err(&rpdev->dev, "No context ID matches response\n");
-		return -ENOENT;
+		dev_dbg(&rpdev->dev, "No context ID matches response\n");
+		return 0;
 	}
 
 	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 1edc7c04b171..dde684554011 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -86,7 +86,8 @@ struct fastrpc_invoke_v2 {
 	__u64 crc;
 	__u64 perf_kernel;
 	__u64 perf_dsp;
-	__u32 reserved[12];
+	__u64 poll_timeout;
+	__u32 reserved[10];
 };
 
 struct fastrpc_init_create {
-- 
2.34.1

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Greg KH]

On Mon, Oct 07, 2024 at 02:15:15PM +0530, Ekansh Gupta wrote:
> +struct fastrpc_invoke_v2 {
> +	struct fastrpc_invoke inv;
> +	__u64 crc;
> +	__u32 reserved[16];

I'm sure we have been over this before, but if you have a reserved
field, you HAVE to check that it is zero if you ever want to use it in
the future.  As you did not check it, just remove it from this structure
as obviously it can never be used for anything.

Your internal review should have caught this, if not, please go fix it
AND get some internal review and signed-off-by before submitting this
series again.

thanks,

greg k-h

Re: [PATCH v1 2/4] misc: fastrpc: Capture kernel and DSP performance counters

[Greg KH]

On Mon, Oct 07, 2024 at 02:15:16PM +0530, Ekansh Gupta wrote:
> Add support to capture kernel performance counters for different
> kernel level operations. These counters collects the information
> for remote call and copies the information to a buffer shared
> by user.
> 
> Collection of DSP performance counters is also added as part of
> this change. DSP updates the performance information in the
> metadata which is then copied to a buffer passed by the users.

"also" usually means "should be broken up into a new patch", right?

Also, where is any of these new "performance counters" documented?  Why
not use the normal kernel apis for performance counters?

thanks,

greg k-h

Re: [PATCH v1 0/4] Add invokeV2 to support new features

[Dmitry Baryshkov]

On Mon, Oct 07, 2024 at 02:15:14PM GMT, Ekansh Gupta wrote:
> This patch series adds the listed features that have been missing
> in upstream fastRPC driver.

please use sensible subject for the cover letter too. Ideally it should
start with the corresponding subsys name or otherwise point out the
target.

> 
> - Add changes to support new enhanced invocation ioctl request.
> - Add support for CRC check.
> - Add support for DSP and kernel performance counters.
> - Add polling mode support.
> 
> Userspace change: https://github.com/quic/fastrpc/pull/73

Up to now we have been using [1].  There is no way to switch to the
mentioned repo with no changelog information between them.

And anyway, as pointed out earlier, please steer your activities towards
implementation at [2]. Both "fastrpc" implementation are ugly,
AOSP-centric and must be dropped as soon as possible.

[1] https://git.codelinaro.org/linaro/qcomlt/fastrpc.git
[2] https://github.com/linux-msm/hexagonrpc

> 
> Ekansh Gupta (4):
>   misc: fastrpc: Add CRC support using invokeV2 request
>   misc: fastrpc: Capture kernel and DSP performance counters
>   misc: fastrpc: Modify context id calculation for poll mode
>   misc: fastrpc: Add polling mode support for fastRPC driver
> 
>  drivers/misc/fastrpc.c      | 435 ++++++++++++++++++++++++++++++------
>  include/uapi/misc/fastrpc.h |  10 +
>  2 files changed, 376 insertions(+), 69 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Dmitry Baryshkov]

On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
> InvokeV2 request is intended to support multiple enhanced invoke
> requests like CRC check, performance counter enablement and polling
> mode for RPC invocations. CRC check is getting enabled as part of
> this patch. CRC check for input and output argument helps in ensuring
> data consistency over a remote call. If user intends to enable CRC
> check, first local user CRC is calculated at user end and a CRC buffer
> is passed to DSP to capture remote CRC values. DSP is expected to
> write to the remote CRC buffer which is then compared at user level
> with the local CRC values.

This doesn't explain why this is necessary. Why do you need to checksum
arguments?

Also, what if the DSP firmware doesn't support CRC? How should userspace
know that?

> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
>  include/uapi/misc/fastrpc.h |   7 ++
>  2 files changed, 116 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 74181b8c386b..8e817a763d1d 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>  
>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  			struct fastrpc_user *user, u32 kernel, u32 sc,
> -			struct fastrpc_invoke_args *args)
> +			struct fastrpc_invoke_v2 *inv2)
>  {
>  	struct fastrpc_channel_ctx *cctx = user->cctx;
>  	struct fastrpc_invoke_ctx *ctx = NULL;
> +	struct fastrpc_invoke_args *args = NULL;

Why do you need to init to NULL if you are going to set it two lines
below?

>  	unsigned long flags;
>  	int ret;
>  
> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;

Why does it need a typecast?

>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return ERR_PTR(-ENOMEM);
> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  	/* Released in fastrpc_context_put() */
>  	fastrpc_channel_ctx_get(cctx);
>  
> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;

Oh, but why? Also is it a user pointer or in-kernel data? If it's a
user-based pointer, where is the accessiblity check? Why isn't it
annotated properly?

>  	ctx->sc = sc;
>  	ctx->retval = -1;
>  	ctx->pid = current->pid;
> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>  	struct fastrpc_invoke_buf *list;
>  	struct fastrpc_phy_page *pages;
>  	u64 *fdlist;
> +	u32 *crclist;
>  	int i, inbufs, outbufs, handles;
>  
>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);

I think we should rewrite this parsing somehow. Is the format of data
documented somewhere?

>  
>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>  		if (!ctx->maps[i]) {
> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>  			fastrpc_map_put(mmap);
>  	}
>  
> +	if (ctx->crc && crclist && rpra) {
> +		if (copy_to_user((void __user *)ctx->crc, crclist,
> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))

Oh, so it's a user pointer. Then u32* was completely incorrect.
Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
filled? Or are we leaking some data to userspace?

> +			return -EFAULT;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1137,13 +1148,12 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
>  
>  }
>  
> -static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
> -				   u32 handle, u32 sc,
> -				   struct fastrpc_invoke_args *args)
> +static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)

Please don't touch what doesn't need to be touched. You are replacing
handle/sc/args with inv2, not touching the first line.

>  {
>  	struct fastrpc_invoke_ctx *ctx = NULL;
>  	struct fastrpc_buf *buf, *b;
> -
> +	struct fastrpc_invoke inv;
> +	u32 handle, sc;
>  	int err = 0;
>  
>  	if (!fl->sctx)
> @@ -1152,12 +1162,15 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  	if (!fl->cctx->rpdev)
>  		return -EPIPE;
>  
> +	inv = inv2->inv;
> +	handle = inv.handle;
> +	sc = inv.sc;
>  	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
>  		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
>  		return -EPERM;
>  	}
>  
> -	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
> +	ctx = fastrpc_context_alloc(fl, kernel, sc, inv2);
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> @@ -1239,6 +1252,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  {
>  	struct fastrpc_init_create_static init;
>  	struct fastrpc_invoke_args *args;
> +	struct fastrpc_invoke_v2 ioctl = {0};

Why do you need to init it?

>  	struct fastrpc_phy_page pages[1];
>  	char *name;
>  	int err;
> @@ -1248,7 +1262,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  		u32 namelen;
>  		u32 pageslen;
>  	} inbuf;
> -	u32 sc;
>  
>  	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>  	if (!args)
> @@ -1313,10 +1326,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	args[2].length = sizeof(*pages);
>  	args[2].fd = -1;
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> -
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> -				      sc, args);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> +	ioctl.inv.args = (u64)args;

Can you pass it as is, without typecasts?

> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (err)
>  		goto err_invoke;
>  
> @@ -1357,6 +1370,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  {
>  	struct fastrpc_init_create init;
>  	struct fastrpc_invoke_args *args;
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	struct fastrpc_phy_page pages[1];
>  	struct fastrpc_map *map = NULL;
>  	struct fastrpc_buf *imem = NULL;
> @@ -1370,7 +1384,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  		u32 attrs;
>  		u32 siglen;
>  	} inbuf;
> -	u32 sc;
>  	bool unsigned_module = false;
>  
>  	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> @@ -1444,12 +1457,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  	args[5].length = sizeof(inbuf.siglen);
>  	args[5].fd = -1;
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>  	if (init.attrs)
> -		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> -
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> -				      sc, args);
> +		ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);

if (init.attrs)
    ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
else
    ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);

> +	ioctl.inv.args = (u64)args;
> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (err)
>  		goto err_invoke;
>  
> @@ -1501,17 +1514,18 @@ static void fastrpc_session_free(struct fastrpc_channel_ctx *cctx,
>  static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
>  {
>  	struct fastrpc_invoke_args args[1];
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	int tgid = 0;
> -	u32 sc;
>  
>  	tgid = fl->tgid;
>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>  	args[0].length = sizeof(tgid);
>  	args[0].fd = -1;
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
>  
> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> -				       sc, &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
> +	ioctl.inv.args = (u64)args;
> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>  }
>  
>  static int fastrpc_device_release(struct inode *inode, struct file *file)
> @@ -1647,45 +1661,77 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>  static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>  {
>  	struct fastrpc_invoke_args args[1];
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	int tgid = fl->tgid;
> -	u32 sc;
>  
>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>  	args[0].length = sizeof(tgid);
>  	args[0].fd = -1;
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>  	fl->pd = pd;
>  
> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> -				       sc, &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> +	ioctl.inv.args = (u64)args;
> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>  }
>  
> -static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
> +static int fastrpc_copy_args(struct fastrpc_invoke *inv)
>  {
>  	struct fastrpc_invoke_args *args = NULL;
> -	struct fastrpc_invoke inv;
>  	u32 nscalars;
> -	int err;
> -
> -	if (copy_from_user(&inv, argp, sizeof(inv)))
> -		return -EFAULT;
>  
>  	/* nscalars is truncated here to max supported value */
> -	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
> +	nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
>  	if (nscalars) {
>  		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
>  		if (!args)
>  			return -ENOMEM;
>  
> -		if (copy_from_user(args, (void __user *)(uintptr_t)inv.args,
> +		if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,

Wait... So inv->args is a user pointer? Then how can you assign a
kernel-based pointer to the same field? I think you need to sanitize
your structures. One is userspace-facing. It should be using userspace
data pointers, etc. Another one is a kernel representation of the ioctl
args. It might have a different structure, it shouldn't contain __user
data, etc.

>  				   nscalars * sizeof(*args))) {
>  			kfree(args);
>  			return -EFAULT;
>  		}
>  	}
> +	inv->args = args;
>  
> -	err = fastrpc_internal_invoke(fl, false, inv.handle, inv.sc, args);
> -	kfree(args);
> +	return 0;
> +}

Looking at the rest of the code, I think the patch needs to be split.
CRC is the minor issue at this point, please focus on getting existing
data being handled correctly while refactoring the code to use new
structure. I'd suggest seeing two struct definitions: one for the
userspace and another one for the kernel space.

> +
> +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
> +{
> +	struct fastrpc_invoke_v2 ioctl = {0};
> +	struct fastrpc_invoke inv;
> +	int err;
> +
> +	if (copy_from_user(&inv, argp, sizeof(inv)))
> +		return -EFAULT;
> +
> +	err = fastrpc_copy_args(&inv);
> +	if (err)
> +		return err;
> +
> +	ioctl.inv = inv;
> +	err = fastrpc_internal_invoke(fl, false, &ioctl);
> +	kfree(inv.args);
> +
> +	return err;
> +}
> +
> +static int fastrpc_invokev2(struct fastrpc_user *fl, char __user *argp)
> +{
> +	struct fastrpc_invoke_v2 inv2 = {0};
> +	int err;
> +
> +	if (copy_from_user(&inv2, argp, sizeof(inv2)))
> +		return -EFAULT;
> +
> +	err = fastrpc_copy_args(&inv2.inv);
> +	if (err)
> +		return err;
> +
> +	err = fastrpc_internal_invoke(fl, false, &inv2);
> +	kfree(inv2.inv.args);
>  
>  	return err;
>  }
> @@ -1694,6 +1740,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>  				     uint32_t dsp_attr_buf_len)
>  {
>  	struct fastrpc_invoke_args args[2] = { 0 };
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  
>  	/*
>  	 * Capability filled in userspace. This carries the information
> @@ -1710,8 +1757,10 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>  	args[1].length = dsp_attr_buf_len * sizeof(u32);
>  	args[1].fd = -1;
>  
> -	return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> -				       FASTRPC_SCALARS(0, 1, 1), args);
> +	ioctl.inv.handle = FASTRPC_DSP_UTILITIES_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(0, 1, 1);
> +	ioctl.inv.args = (u64)args;
> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>  }
>  
>  static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> @@ -1798,10 +1847,10 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>  static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
>  {
>  	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	struct fastrpc_munmap_req_msg req_msg;
>  	struct device *dev = fl->sctx->dev;
>  	int err;
> -	u32 sc;
>  
>  	req_msg.pgid = fl->tgid;
>  	req_msg.size = buf->size;
> @@ -1810,9 +1859,10 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>  	args[0].ptr = (u64) (uintptr_t) &req_msg;
>  	args[0].length = sizeof(req_msg);
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> -				      &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MUNMAP, 1, 0);
> +	ioctl.inv.args = (u64)args;
> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (!err) {
>  		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
>  		spin_lock(&fl->lock);
> @@ -1856,6 +1906,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  {
>  	struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	struct fastrpc_buf *buf = NULL;
>  	struct fastrpc_mmap_req_msg req_msg;
>  	struct fastrpc_mmap_rsp_msg rsp_msg;
> @@ -1863,7 +1914,6 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  	struct fastrpc_req_mmap req;
>  	struct device *dev = fl->sctx->dev;
>  	int err;
> -	u32 sc;
>  
>  	if (copy_from_user(&req, argp, sizeof(req)))
>  		return -EFAULT;
> @@ -1906,9 +1956,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  	args[2].ptr = (u64) (uintptr_t) &rsp_msg;
>  	args[2].length = sizeof(rsp_msg);
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> -				      &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> +	ioctl.inv.args = (u64)args;
> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (err) {
>  		dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
>  		fastrpc_buf_free(buf);
> @@ -1957,10 +2008,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
>  {
>  	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	struct fastrpc_map *map = NULL, *iter, *m;
>  	struct fastrpc_mem_unmap_req_msg req_msg = { 0 };
>  	int err = 0;
> -	u32 sc;
>  	struct device *dev = fl->sctx->dev;
>  
>  	spin_lock(&fl->lock);
> @@ -1986,9 +2037,10 @@ static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_me
>  	args[0].ptr = (u64) (uintptr_t) &req_msg;
>  	args[0].length = sizeof(req_msg);
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0);
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> -				      &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0);
> +	ioctl.inv.args = (u64)args;
> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (err) {
>  		dev_err(dev, "unmmap\tpt fd = %d, 0x%09llx error\n",  map->fd, map->raddr);
>  		return err;
> @@ -2011,6 +2063,7 @@ static int fastrpc_req_mem_unmap(struct fastrpc_user *fl, char __user *argp)
>  static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>  {
>  	struct fastrpc_invoke_args args[4] = { [0 ... 3] = { 0 } };
> +	struct fastrpc_invoke_v2 ioctl = {0};
>  	struct fastrpc_mem_map_req_msg req_msg = { 0 };
>  	struct fastrpc_mmap_rsp_msg rsp_msg = { 0 };
>  	struct fastrpc_mem_unmap req_unmap = { 0 };
> @@ -2019,7 +2072,6 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>  	struct device *dev = fl->sctx->dev;
>  	struct fastrpc_map *map = NULL;
>  	int err;
> -	u32 sc;
>  
>  	if (copy_from_user(&req, argp, sizeof(req)))
>  		return -EFAULT;
> @@ -2055,8 +2107,10 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>  	args[3].ptr = (u64) (uintptr_t) &rsp_msg;
>  	args[3].length = sizeof(rsp_msg);
>  
> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_MAP, 3, 1);
> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]);
> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_MAP, 3, 1);
> +	ioctl.inv.args = (u64)args;
> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>  	if (err) {
>  		dev_err(dev, "mem mmap error, fd %d, vaddr %llx, size %lld\n",
>  			req.fd, req.vaddrin, map->size);
> @@ -2096,6 +2150,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>  	case FASTRPC_IOCTL_INVOKE:
>  		err = fastrpc_invoke(fl, argp);
>  		break;
> +	case FASTRPC_IOCTL_INVOKEV2:
> +		err = fastrpc_invokev2(fl, argp);
> +		break;
>  	case FASTRPC_IOCTL_INIT_ATTACH:
>  		err = fastrpc_init_attach(fl, ROOT_PD);
>  		break;
> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..406b80555d41 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -17,6 +17,7 @@
>  #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_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
> +#define FASTRPC_IOCTL_INVOKEV2		_IOWR('R', 14, struct fastrpc_invoke_v2)
>  
>  /**
>   * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
> @@ -80,6 +81,12 @@ struct fastrpc_invoke {
>  	__u64 args;
>  };
>  
> +struct fastrpc_invoke_v2 {
> +	struct fastrpc_invoke inv;
> +	__u64 crc;
> +	__u32 reserved[16];
> +};
> +
>  struct fastrpc_init_create {
>  	__u32 filelen;	/* elf file length */
>  	__s32 filefd;	/* fd for the file */
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[kernel test robot]

Hi Ekansh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ekansh-Gupta/misc-fastrpc-Add-CRC-support-using-invokeV2-request/20241007-164734
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241007084518.3649876-2-quic_ekangupt%40quicinc.com
patch subject: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request
config: arc-randconfig-001-20241008 (https://download.01.org/0day-ci/archive/20241008/202410080906.2kCMkY5c-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410080906.2kCMkY5c-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410080906.2kCMkY5c-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/misc/fastrpc.c: In function 'fastrpc_context_alloc':
>> drivers/misc/fastrpc.c:584:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     584 |         args = (struct fastrpc_invoke_args *)inv2->inv.args;
         |                ^
   drivers/misc/fastrpc.c: In function 'fastrpc_init_create_static_process':
>> drivers/misc/fastrpc.c:1331:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1331 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_init_create_process':
   drivers/misc/fastrpc.c:1464:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1464 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_release_current_dsp_process':
   drivers/misc/fastrpc.c:1527:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1527 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_init_attach':
   drivers/misc/fastrpc.c:1674:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1674 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_copy_args':
>> drivers/misc/fastrpc.c:1696:19: warning: assignment to '__u64' {aka 'long long unsigned int'} from 'struct fastrpc_invoke_args *' makes integer from pointer without a cast [-Wint-conversion]
    1696 |         inv->args = args;
         |                   ^
   drivers/misc/fastrpc.c: In function 'fastrpc_invoke':
>> drivers/misc/fastrpc.c:1716:18: warning: passing argument 1 of 'kfree' makes pointer from integer without a cast [-Wint-conversion]
    1716 |         kfree(inv.args);
         |               ~~~^~~~~
         |                  |
         |                  __u64 {aka long long unsigned int}
   In file included from include/linux/mm.h:33,
                    from include/linux/scatterlist.h:8,
                    from include/linux/dma-buf.h:19,
                    from drivers/misc/fastrpc.c:7:
   include/linux/slab.h:446:24: note: expected 'const void *' but argument is of type '__u64' {aka 'long long unsigned int'}
     446 | void kfree(const void *objp);
         |            ~~~~~~~~~~~~^~~~
   drivers/misc/fastrpc.c: In function 'fastrpc_invokev2':
   drivers/misc/fastrpc.c:1734:23: warning: passing argument 1 of 'kfree' makes pointer from integer without a cast [-Wint-conversion]
    1734 |         kfree(inv2.inv.args);
         |               ~~~~~~~~^~~~~
         |                       |
         |                       __u64 {aka long long unsigned int}
   include/linux/slab.h:446:24: note: expected 'const void *' but argument is of type '__u64' {aka 'long long unsigned int'}
     446 | void kfree(const void *objp);
         |            ~~~~~~~~~~~~^~~~
   drivers/misc/fastrpc.c: In function 'fastrpc_get_info_from_dsp':
   drivers/misc/fastrpc.c:1762:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1762 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_req_munmap_impl':
   drivers/misc/fastrpc.c:1864:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1864 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_req_mmap':
   drivers/misc/fastrpc.c:1961:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1961 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_req_mem_unmap_impl':
   drivers/misc/fastrpc.c:2042:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    2042 |         ioctl.inv.args = (u64)args;
         |                          ^
   drivers/misc/fastrpc.c: In function 'fastrpc_req_mem_map':
   drivers/misc/fastrpc.c:2112:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    2112 |         ioctl.inv.args = (u64)args;
         |                          ^


vim +584 drivers/misc/fastrpc.c

   573	
   574	static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
   575				struct fastrpc_user *user, u32 kernel, u32 sc,
   576				struct fastrpc_invoke_v2 *inv2)
   577	{
   578		struct fastrpc_channel_ctx *cctx = user->cctx;
   579		struct fastrpc_invoke_ctx *ctx = NULL;
   580		struct fastrpc_invoke_args *args = NULL;
   581		unsigned long flags;
   582		int ret;
   583	
 > 584		args = (struct fastrpc_invoke_args *)inv2->inv.args;
   585		ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
   586		if (!ctx)
   587			return ERR_PTR(-ENOMEM);
   588	
   589		INIT_LIST_HEAD(&ctx->node);
   590		ctx->fl = user;
   591		ctx->nscalars = REMOTE_SCALARS_LENGTH(sc);
   592		ctx->nbufs = REMOTE_SCALARS_INBUFS(sc) +
   593			     REMOTE_SCALARS_OUTBUFS(sc);
   594	
   595		if (ctx->nscalars) {
   596			ctx->maps = kcalloc(ctx->nscalars,
   597					    sizeof(*ctx->maps), GFP_KERNEL);
   598			if (!ctx->maps) {
   599				kfree(ctx);
   600				return ERR_PTR(-ENOMEM);
   601			}
   602			ctx->olaps = kcalloc(ctx->nscalars,
   603					    sizeof(*ctx->olaps), GFP_KERNEL);
   604			if (!ctx->olaps) {
   605				kfree(ctx->maps);
   606				kfree(ctx);
   607				return ERR_PTR(-ENOMEM);
   608			}
   609			ctx->args = args;
   610			fastrpc_get_buff_overlaps(ctx);
   611		}
   612	
   613		/* Released in fastrpc_context_put() */
   614		fastrpc_channel_ctx_get(cctx);
   615	
   616		ctx->crc = (u32 *)(uintptr_t)inv2->crc;
   617		ctx->sc = sc;
   618		ctx->retval = -1;
   619		ctx->pid = current->pid;
   620		ctx->tgid = user->tgid;
   621		ctx->cctx = cctx;
   622		init_completion(&ctx->work);
   623		INIT_WORK(&ctx->put_work, fastrpc_context_put_wq);
   624	
   625		spin_lock(&user->lock);
   626		list_add_tail(&ctx->node, &user->pending);
   627		spin_unlock(&user->lock);
   628	
   629		spin_lock_irqsave(&cctx->lock, flags);
   630		ret = idr_alloc_cyclic(&cctx->ctx_idr, ctx, 1,
   631				       FASTRPC_CTX_MAX, GFP_ATOMIC);
   632		if (ret < 0) {
   633			spin_unlock_irqrestore(&cctx->lock, flags);
   634			goto err_idr;
   635		}
   636		ctx->ctxid = ret << 4;
   637		spin_unlock_irqrestore(&cctx->lock, flags);
   638	
   639		kref_init(&ctx->refcount);
   640	
   641		return ctx;
   642	err_idr:
   643		spin_lock(&user->lock);
   644		list_del(&ctx->node);
   645		spin_unlock(&user->lock);
   646		fastrpc_channel_ctx_put(cctx);
   647		kfree(ctx->maps);
   648		kfree(ctx->olaps);
   649		kfree(ctx);
   650	
   651		return ERR_PTR(ret);
   652	}
   653	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[kernel test robot]

Hi Ekansh,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ekansh-Gupta/misc-fastrpc-Add-CRC-support-using-invokeV2-request/20241007-164734
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241007084518.3649876-2-quic_ekangupt%40quicinc.com
patch subject: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request
config: arm-randconfig-002-20241008 (https://download.01.org/0day-ci/archive/20241008/202410080940.S7Rar8Zi-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project fef3566a25ff0e34fb87339ba5e13eca17cec00f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410080940.S7Rar8Zi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410080940.S7Rar8Zi-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/misc/fastrpc.c:7:
   In file included from include/linux/dma-buf.h:19:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/misc/fastrpc.c:1696:12: error: incompatible pointer to integer conversion assigning to '__u64' (aka 'unsigned long long') from 'struct fastrpc_invoke_args *' [-Wint-conversion]
    1696 |         inv->args = args;
         |                   ^ ~~~~
>> drivers/misc/fastrpc.c:1716:8: error: incompatible integer to pointer conversion passing '__u64' (aka 'unsigned long long') to parameter of type 'const void *' [-Wint-conversion]
    1716 |         kfree(inv.args);
         |               ^~~~~~~~
   include/linux/slab.h:446:24: note: passing argument to parameter 'objp' here
     446 | void kfree(const void *objp);
         |                        ^
   drivers/misc/fastrpc.c:1734:8: error: incompatible integer to pointer conversion passing '__u64' (aka 'unsigned long long') to parameter of type 'const void *' [-Wint-conversion]
    1734 |         kfree(inv2.inv.args);
         |               ^~~~~~~~~~~~~
   include/linux/slab.h:446:24: note: passing argument to parameter 'objp' here
     446 | void kfree(const void *objp);
         |                        ^
   1 warning and 3 errors generated.


vim +1696 drivers/misc/fastrpc.c

  1677	
  1678	static int fastrpc_copy_args(struct fastrpc_invoke *inv)
  1679	{
  1680		struct fastrpc_invoke_args *args = NULL;
  1681		u32 nscalars;
  1682	
  1683		/* nscalars is truncated here to max supported value */
  1684		nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
  1685		if (nscalars) {
  1686			args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
  1687			if (!args)
  1688				return -ENOMEM;
  1689	
  1690			if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
  1691					   nscalars * sizeof(*args))) {
  1692				kfree(args);
  1693				return -EFAULT;
  1694			}
  1695		}
> 1696		inv->args = args;
  1697	
  1698		return 0;
  1699	}
  1700	
  1701	static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
  1702	{
  1703		struct fastrpc_invoke_v2 ioctl = {0};
  1704		struct fastrpc_invoke inv;
  1705		int err;
  1706	
  1707		if (copy_from_user(&inv, argp, sizeof(inv)))
  1708			return -EFAULT;
  1709	
  1710		err = fastrpc_copy_args(&inv);
  1711		if (err)
  1712			return err;
  1713	
  1714		ioctl.inv = inv;
  1715		err = fastrpc_internal_invoke(fl, false, &ioctl);
> 1716		kfree(inv.args);
  1717	
  1718		return err;
  1719	}
  1720	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[kernel test robot]

Hi Ekansh,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ekansh-Gupta/misc-fastrpc-Add-CRC-support-using-invokeV2-request/20241007-164734
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241007084518.3649876-2-quic_ekangupt%40quicinc.com
patch subject: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20241008/202410081016.Nuyub4XK-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410081016.Nuyub4XK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410081016.Nuyub4XK-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/misc/fastrpc.c: In function 'fastrpc_copy_args':
>> drivers/misc/fastrpc.c:1696:19: error: assignment to '__u64' {aka 'long long unsigned int'} from 'struct fastrpc_invoke_args *' makes integer from pointer without a cast [-Wint-conversion]
    1696 |         inv->args = args;
         |                   ^
   drivers/misc/fastrpc.c: In function 'fastrpc_invoke':
>> drivers/misc/fastrpc.c:1716:18: error: passing argument 1 of 'kfree' makes pointer from integer without a cast [-Wint-conversion]
    1716 |         kfree(inv.args);
         |               ~~~^~~~~
         |                  |
         |                  __u64 {aka long long unsigned int}
   In file included from include/linux/fs.h:45,
                    from arch/loongarch/include/asm/elf.h:9,
                    from include/linux/elf.h:6,
                    from include/linux/module.h:19,
                    from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from drivers/misc/fastrpc.c:6:
   include/linux/slab.h:446:24: note: expected 'const void *' but argument is of type '__u64' {aka 'long long unsigned int'}
     446 | void kfree(const void *objp);
         |            ~~~~~~~~~~~~^~~~
   drivers/misc/fastrpc.c: In function 'fastrpc_invokev2':
   drivers/misc/fastrpc.c:1734:23: error: passing argument 1 of 'kfree' makes pointer from integer without a cast [-Wint-conversion]
    1734 |         kfree(inv2.inv.args);
         |               ~~~~~~~~^~~~~
         |                       |
         |                       __u64 {aka long long unsigned int}
   include/linux/slab.h:446:24: note: expected 'const void *' but argument is of type '__u64' {aka 'long long unsigned int'}
     446 | void kfree(const void *objp);
         |            ~~~~~~~~~~~~^~~~


vim +1696 drivers/misc/fastrpc.c

  1677	
  1678	static int fastrpc_copy_args(struct fastrpc_invoke *inv)
  1679	{
  1680		struct fastrpc_invoke_args *args = NULL;
  1681		u32 nscalars;
  1682	
  1683		/* nscalars is truncated here to max supported value */
  1684		nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
  1685		if (nscalars) {
  1686			args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
  1687			if (!args)
  1688				return -ENOMEM;
  1689	
  1690			if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
  1691					   nscalars * sizeof(*args))) {
  1692				kfree(args);
  1693				return -EFAULT;
  1694			}
  1695		}
> 1696		inv->args = args;
  1697	
  1698		return 0;
  1699	}
  1700	
  1701	static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
  1702	{
  1703		struct fastrpc_invoke_v2 ioctl = {0};
  1704		struct fastrpc_invoke inv;
  1705		int err;
  1706	
  1707		if (copy_from_user(&inv, argp, sizeof(inv)))
  1708			return -EFAULT;
  1709	
  1710		err = fastrpc_copy_args(&inv);
  1711		if (err)
  1712			return err;
  1713	
  1714		ioctl.inv = inv;
  1715		err = fastrpc_internal_invoke(fl, false, &ioctl);
> 1716		kfree(inv.args);
  1717	
  1718		return err;
  1719	}
  1720	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[kernel test robot]

Hi Ekansh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc2 next-20241008]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ekansh-Gupta/misc-fastrpc-Add-CRC-support-using-invokeV2-request/20241007-164734
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241007084518.3649876-2-quic_ekangupt%40quicinc.com
patch subject: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request
config: alpha-randconfig-r112-20241008 (https://download.01.org/0day-ci/archive/20241009/202410090402.lospEFvZ-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce: (https://download.01.org/0day-ci/archive/20241009/202410090402.lospEFvZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410090402.lospEFvZ-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/misc/fastrpc.c:1696:19: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] args @@     got struct fastrpc_invoke_args *[assigned] args @@
   drivers/misc/fastrpc.c:1696:19: sparse:     expected unsigned long long [usertype] args
   drivers/misc/fastrpc.c:1696:19: sparse:     got struct fastrpc_invoke_args *[assigned] args
>> drivers/misc/fastrpc.c:1716:18: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected void const *objp @@     got unsigned long long [addressable] [usertype] args @@
   drivers/misc/fastrpc.c:1716:18: sparse:     expected void const *objp
   drivers/misc/fastrpc.c:1716:18: sparse:     got unsigned long long [addressable] [usertype] args
   drivers/misc/fastrpc.c:1734:23: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected void const *objp @@     got unsigned long long [addressable] [usertype] args @@
   drivers/misc/fastrpc.c:1734:23: sparse:     expected void const *objp
   drivers/misc/fastrpc.c:1734:23: sparse:     got unsigned long long [addressable] [usertype] args
   drivers/misc/fastrpc.c: note: in included file (through include/linux/swait.h, include/linux/completion.h):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +1696 drivers/misc/fastrpc.c

  1677	
  1678	static int fastrpc_copy_args(struct fastrpc_invoke *inv)
  1679	{
  1680		struct fastrpc_invoke_args *args = NULL;
  1681		u32 nscalars;
  1682	
  1683		/* nscalars is truncated here to max supported value */
  1684		nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
  1685		if (nscalars) {
  1686			args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
  1687			if (!args)
  1688				return -ENOMEM;
  1689	
  1690			if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
  1691					   nscalars * sizeof(*args))) {
  1692				kfree(args);
  1693				return -EFAULT;
  1694			}
  1695		}
> 1696		inv->args = args;
  1697	
  1698		return 0;
  1699	}
  1700	
  1701	static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
  1702	{
  1703		struct fastrpc_invoke_v2 ioctl = {0};
  1704		struct fastrpc_invoke inv;
  1705		int err;
  1706	
  1707		if (copy_from_user(&inv, argp, sizeof(inv)))
  1708			return -EFAULT;
  1709	
  1710		err = fastrpc_copy_args(&inv);
  1711		if (err)
  1712			return err;
  1713	
  1714		ioctl.inv = inv;
  1715		err = fastrpc_internal_invoke(fl, false, &ioctl);
> 1716		kfree(inv.args);
  1717	
  1718		return err;
  1719	}
  1720	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

On 10/7/2024 2:23 PM, Greg KH wrote:
> On Mon, Oct 07, 2024 at 02:15:15PM +0530, Ekansh Gupta wrote:
>> +struct fastrpc_invoke_v2 {
>> +	struct fastrpc_invoke inv;
>> +	__u64 crc;
>> +	__u32 reserved[16];
> I'm sure we have been over this before, but if you have a reserved
> field, you HAVE to check that it is zero if you ever want to use it in
> the future.  As you did not check it, just remove it from this structure
> as obviously it can never be used for anything.
>
> Your internal review should have caught this, if not, please go fix it
> AND get some internal review and signed-off-by before submitting this
> series again.
Thanks for your insights. I'll fix this in the next patch. My apologies for missing it.

--ekansh
>
> thanks,
>
> greg k-h

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
>> InvokeV2 request is intended to support multiple enhanced invoke
>> requests like CRC check, performance counter enablement and polling
>> mode for RPC invocations. CRC check is getting enabled as part of
>> this patch. CRC check for input and output argument helps in ensuring
>> data consistency over a remote call. If user intends to enable CRC
>> check, first local user CRC is calculated at user end and a CRC buffer
>> is passed to DSP to capture remote CRC values. DSP is expected to
>> write to the remote CRC buffer which is then compared at user level
>> with the local CRC values.
> This doesn't explain why this is necessary. Why do you need to checksum
> arguments?
This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
remote call. This is not enabled by default and user can enable it as per their reqirement.
I'll add this information.
>
> Also, what if the DSP firmware doesn't support CRC? How should userspace
> know that?
CRC support on DSP is there since long time(>6years). From user space CRC check failure is
not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
call failure. Should I keep it as fatal considering that ever very old DSP support this or should
I consider the copy failure as non-fatal as userspace is treating this as a warning?
>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
>>  include/uapi/misc/fastrpc.h |   7 ++
>>  2 files changed, 116 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 74181b8c386b..8e817a763d1d 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>>  
>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
>> -			struct fastrpc_invoke_args *args)
>> +			struct fastrpc_invoke_v2 *inv2)
>>  {
>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>> +	struct fastrpc_invoke_args *args = NULL;
> Why do you need to init to NULL if you are going to set it two lines
> below?
>
>>  	unsigned long flags;
>>  	int ret;
>>  
>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
> Why does it need a typecast?
>
>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>  	if (!ctx)
>>  		return ERR_PTR(-ENOMEM);
>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>  	/* Released in fastrpc_context_put() */
>>  	fastrpc_channel_ctx_get(cctx);
>>  
>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
> user-based pointer, where is the accessiblity check? Why isn't it
> annotated properly?
This is a user pointer where the crc data is expected to be copied. There is no
other access to this pointer from kernel. I'm planning to change the data type
for crc as (void __user*) inside fastrpc_invoke_ctx structure.
>
>>  	ctx->sc = sc;
>>  	ctx->retval = -1;
>>  	ctx->pid = current->pid;
>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>  	struct fastrpc_invoke_buf *list;
>>  	struct fastrpc_phy_page *pages;
>>  	u64 *fdlist;
>> +	u32 *crclist;
>>  	int i, inbufs, outbufs, handles;
>>  
>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
> I think we should rewrite this parsing somehow. Is the format of data
> documented somewhere?
fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
to store these pointers to ctx structure and directly use it wherever needed. This will
clean-up this unnecessary calculations at multiple places.
>
>>  
>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>>  		if (!ctx->maps[i]) {
>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>  			fastrpc_map_put(mmap);
>>  	}
>>  
>> +	if (ctx->crc && crclist && rpra) {
>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
> Oh, so it's a user pointer. Then u32* was completely incorrect.
> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
> filled? Or are we leaking some data to userspace?
Yes, right. Planning clean-up in next patch.

All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
>
>> +			return -EFAULT;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1137,13 +1148,12 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
>>  
>>  }
>>  
>> -static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>> -				   u32 handle, u32 sc,
>> -				   struct fastrpc_invoke_args *args)
>> +static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
> Please don't touch what doesn't need to be touched. You are replacing
> handle/sc/args with inv2, not touching the first line.
Ack.
>
>>  {
>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>>  	struct fastrpc_buf *buf, *b;
>> -
>> +	struct fastrpc_invoke inv;
>> +	u32 handle, sc;
>>  	int err = 0;
>>  
>>  	if (!fl->sctx)
>> @@ -1152,12 +1162,15 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>>  	if (!fl->cctx->rpdev)
>>  		return -EPIPE;
>>  
>> +	inv = inv2->inv;
>> +	handle = inv.handle;
>> +	sc = inv.sc;
>>  	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
>>  		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
>>  		return -EPERM;
>>  	}
>>  
>> -	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
>> +	ctx = fastrpc_context_alloc(fl, kernel, sc, inv2);
>>  	if (IS_ERR(ctx))
>>  		return PTR_ERR(ctx);
>>  
>> @@ -1239,6 +1252,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  {
>>  	struct fastrpc_init_create_static init;
>>  	struct fastrpc_invoke_args *args;
>> +	struct fastrpc_invoke_v2 ioctl = {0};
> Why do you need to init it?
Ack.
>
>>  	struct fastrpc_phy_page pages[1];
>>  	char *name;
>>  	int err;
>> @@ -1248,7 +1262,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  		u32 namelen;
>>  		u32 pageslen;
>>  	} inbuf;
>> -	u32 sc;
>>  
>>  	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>>  	if (!args)
>> @@ -1313,10 +1326,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  	args[2].length = sizeof(*pages);
>>  	args[2].fd = -1;
>>  
>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
>> -
>> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> -				      sc, args);
>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
>> +	ioctl.inv.args = (u64)args;
> Can you pass it as is, without typecasts?
Cleaned up in refactoring change.
>
>> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>>  	if (err)
>>  		goto err_invoke;
>>  
>> @@ -1357,6 +1370,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  {
>>  	struct fastrpc_init_create init;
>>  	struct fastrpc_invoke_args *args;
>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>  	struct fastrpc_phy_page pages[1];
>>  	struct fastrpc_map *map = NULL;
>>  	struct fastrpc_buf *imem = NULL;
>> @@ -1370,7 +1384,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  		u32 attrs;
>>  		u32 siglen;
>>  	} inbuf;
>> -	u32 sc;
>>  	bool unsigned_module = false;
>>  
>>  	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>> @@ -1444,12 +1457,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  	args[5].length = sizeof(inbuf.siglen);
>>  	args[5].fd = -1;
>>  
>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>>  	if (init.attrs)
>> -		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
>> -
>> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> -				      sc, args);
>> +		ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> if (init.attrs)
>     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> else
>     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>
>> +	ioctl.inv.args = (u64)args;
>> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>>  	if (err)
>>  		goto err_invoke;
>>  
>> @@ -1501,17 +1514,18 @@ static void fastrpc_session_free(struct fastrpc_channel_ctx *cctx,
>>  static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
>>  {
>>  	struct fastrpc_invoke_args args[1];
>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>  	int tgid = 0;
>> -	u32 sc;
>>  
>>  	tgid = fl->tgid;
>>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>>  	args[0].length = sizeof(tgid);
>>  	args[0].fd = -1;
>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
>>  
>> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> -				       sc, &args[0]);
>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
>> +	ioctl.inv.args = (u64)args;
>> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>>  }
>>  
>>  static int fastrpc_device_release(struct inode *inode, struct file *file)
>> @@ -1647,45 +1661,77 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>>  static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>>  {
>>  	struct fastrpc_invoke_args args[1];
>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>  	int tgid = fl->tgid;
>> -	u32 sc;
>>  
>>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>>  	args[0].length = sizeof(tgid);
>>  	args[0].fd = -1;
>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>>  	fl->pd = pd;
>>  
>> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> -				       sc, &args[0]);
>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>> +	ioctl.inv.args = (u64)args;
>> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>>  }
>>  
>> -static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
>> +static int fastrpc_copy_args(struct fastrpc_invoke *inv)
>>  {
>>  	struct fastrpc_invoke_args *args = NULL;
>> -	struct fastrpc_invoke inv;
>>  	u32 nscalars;
>> -	int err;
>> -
>> -	if (copy_from_user(&inv, argp, sizeof(inv)))
>> -		return -EFAULT;
>>  
>>  	/* nscalars is truncated here to max supported value */
>> -	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
>> +	nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
>>  	if (nscalars) {
>>  		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
>>  		if (!args)
>>  			return -ENOMEM;
>>  
>> -		if (copy_from_user(args, (void __user *)(uintptr_t)inv.args,
>> +		if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
> Wait... So inv->args is a user pointer? Then how can you assign a
> kernel-based pointer to the same field? I think you need to sanitize
> your structures. One is userspace-facing. It should be using userspace
> data pointers, etc. Another one is a kernel representation of the ioctl
> args. It might have a different structure, it shouldn't contain __user
> data, etc.
I'm planning to have a new structure to carry ctx specific data which will be saved in
fastrpc_invoke_ctx structure during fastrpc_context_alloc.

Something like this:
struct fastrpc_ctx_args {
    struct fastrpc_invoke_args *args;  /* Carries args that is copied from ioctl structure */
    void __user *crc; /* Carries CRC user pointer where the crcdata from DSP will be copied for user to read */
    u64 poll_timeout; /* Carried context specific poll_timeout information */
};

Do you see any problem with this?
>
>>  				   nscalars * sizeof(*args))) {
>>  			kfree(args);
>>  			return -EFAULT;
>>  		}
>>  	}
>> +	inv->args = args;
>>  
>> -	err = fastrpc_internal_invoke(fl, false, inv.handle, inv.sc, args);
>> -	kfree(args);
>> +	return 0;
>> +}
> Looking at the rest of the code, I think the patch needs to be split.
> CRC is the minor issue at this point, please focus on getting existing
> data being handled correctly while refactoring the code to use new
> structure. I'd suggest seeing two struct definitions: one for the
> userspace and another one for the kernel space.
I've made changes for refactoring where instead of using userspace structure, I'm
planning to use fastrpc_ctx_args structure mentioned above.
>
>> +
>> +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
>> +{
>> +	struct fastrpc_invoke_v2 ioctl = {0};
>> +	struct fastrpc_invoke inv;
>> +	int err;
>> +
>> +	if (copy_from_user(&inv, argp, sizeof(inv)))
>> +		return -EFAULT;
>> +
>> +	err = fastrpc_copy_args(&inv);
>> +	if (err)
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Dmitry Baryshkov]

On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
> 
> 
> 
> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
> > On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
> >> InvokeV2 request is intended to support multiple enhanced invoke
> >> requests like CRC check, performance counter enablement and polling
> >> mode for RPC invocations. CRC check is getting enabled as part of
> >> this patch. CRC check for input and output argument helps in ensuring
> >> data consistency over a remote call. If user intends to enable CRC
> >> check, first local user CRC is calculated at user end and a CRC buffer
> >> is passed to DSP to capture remote CRC values. DSP is expected to
> >> write to the remote CRC buffer which is then compared at user level
> >> with the local CRC values.
> > This doesn't explain why this is necessary. Why do you need to checksum
> > arguments?
> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
> remote call. This is not enabled by default and user can enable it as per their reqirement.
> I'll add this information.

An inconsistency where? Between the kernel and the DSP? Between the user
and the DSP? Does it cover buffer contents or just the addresses?

> >
> > Also, what if the DSP firmware doesn't support CRC? How should userspace
> > know that?
> CRC support on DSP is there since long time(>6years).

This doesn't give us a lot. Upstream kernel supports fastrpc since
MSM8916 and MSM8996. Do those platforms support CRC?
And if they do, why do we need the invoke_v2? Can we modify existing
code instead?

> From user space CRC check failure is
> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
> I consider the copy failure as non-fatal as userspace is treating this as a warning?

warnings can remain unseen for a long time. Consider a GUI app. Nobody
is there to view kernel warnings or library output.

> >
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
> >>  include/uapi/misc/fastrpc.h |   7 ++
> >>  2 files changed, 116 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index 74181b8c386b..8e817a763d1d 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
> >>  
> >>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>  			struct fastrpc_user *user, u32 kernel, u32 sc,
> >> -			struct fastrpc_invoke_args *args)
> >> +			struct fastrpc_invoke_v2 *inv2)
> >>  {
> >>  	struct fastrpc_channel_ctx *cctx = user->cctx;
> >>  	struct fastrpc_invoke_ctx *ctx = NULL;
> >> +	struct fastrpc_invoke_args *args = NULL;
> > Why do you need to init to NULL if you are going to set it two lines
> > below?
> >
> >>  	unsigned long flags;
> >>  	int ret;
> >>  
> >> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
> > Why does it need a typecast?
> >
> >>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> >>  	if (!ctx)
> >>  		return ERR_PTR(-ENOMEM);
> >> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>  	/* Released in fastrpc_context_put() */
> >>  	fastrpc_channel_ctx_get(cctx);
> >>  
> >> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
> > Oh, but why? Also is it a user pointer or in-kernel data? If it's a
> > user-based pointer, where is the accessiblity check? Why isn't it
> > annotated properly?
> This is a user pointer where the crc data is expected to be copied. There is no
> other access to this pointer from kernel. I'm planning to change the data type
> for crc as (void __user*) inside fastrpc_invoke_ctx structure.

Yes, please. Also make sure that sparse doesn't add any warnings
regarding pointer conversions.

> >
> >>  	ctx->sc = sc;
> >>  	ctx->retval = -1;
> >>  	ctx->pid = current->pid;
> >> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>  	struct fastrpc_invoke_buf *list;
> >>  	struct fastrpc_phy_page *pages;
> >>  	u64 *fdlist;
> >> +	u32 *crclist;
> >>  	int i, inbufs, outbufs, handles;
> >>  
> >>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> >> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> >>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
> >>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
> >> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
> > I think we should rewrite this parsing somehow. Is the format of data
> > documented somewhere?
> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
> to store these pointers to ctx structure and directly use it wherever needed. This will
> clean-up this unnecessary calculations at multiple places.


Please do. Nevertheless, the format also must be documented.

> >
> >>  
> >>  	for (i = inbufs; i < ctx->nbufs; ++i) {
> >>  		if (!ctx->maps[i]) {
> >> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>  			fastrpc_map_put(mmap);
> >>  	}
> >>  
> >> +	if (ctx->crc && crclist && rpra) {
> >> +		if (copy_to_user((void __user *)ctx->crc, crclist,
> >> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
> > Oh, so it's a user pointer. Then u32* was completely incorrect.
> > Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
> > filled? Or are we leaking some data to userspace?
> Yes, right. Planning clean-up in next patch.
> 
> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.

Huh? I definitely want to see documentation for function arguments.

> >
> >> +			return -EFAULT;
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -1137,13 +1148,12 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
> >>  
> >>  }
> >>  
> >> -static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
> >> -				   u32 handle, u32 sc,
> >> -				   struct fastrpc_invoke_args *args)
> >> +static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
> > Please don't touch what doesn't need to be touched. You are replacing
> > handle/sc/args with inv2, not touching the first line.
> Ack.
> >
> >>  {
> >>  	struct fastrpc_invoke_ctx *ctx = NULL;
> >>  	struct fastrpc_buf *buf, *b;
> >> -
> >> +	struct fastrpc_invoke inv;
> >> +	u32 handle, sc;
> >>  	int err = 0;
> >>  
> >>  	if (!fl->sctx)
> >> @@ -1152,12 +1162,15 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
> >>  	if (!fl->cctx->rpdev)
> >>  		return -EPIPE;
> >>  
> >> +	inv = inv2->inv;
> >> +	handle = inv.handle;
> >> +	sc = inv.sc;
> >>  	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
> >>  		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
> >>  		return -EPERM;
> >>  	}
> >>  
> >> -	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
> >> +	ctx = fastrpc_context_alloc(fl, kernel, sc, inv2);
> >>  	if (IS_ERR(ctx))
> >>  		return PTR_ERR(ctx);
> >>  
> >> @@ -1239,6 +1252,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  {
> >>  	struct fastrpc_init_create_static init;
> >>  	struct fastrpc_invoke_args *args;
> >> +	struct fastrpc_invoke_v2 ioctl = {0};
> > Why do you need to init it?
> Ack.
> >
> >>  	struct fastrpc_phy_page pages[1];
> >>  	char *name;
> >>  	int err;
> >> @@ -1248,7 +1262,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  		u32 namelen;
> >>  		u32 pageslen;
> >>  	} inbuf;
> >> -	u32 sc;
> >>  
> >>  	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> >>  	if (!args)
> >> @@ -1313,10 +1326,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  	args[2].length = sizeof(*pages);
> >>  	args[2].fd = -1;
> >>  
> >> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> >> -
> >> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> >> -				      sc, args);
> >> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> >> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
> >> +	ioctl.inv.args = (u64)args;
> > Can you pass it as is, without typecasts?
> Cleaned up in refactoring change.
> >
> >> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
> >>  	if (err)
> >>  		goto err_invoke;
> >>  
> >> @@ -1357,6 +1370,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >>  {
> >>  	struct fastrpc_init_create init;
> >>  	struct fastrpc_invoke_args *args;
> >> +	struct fastrpc_invoke_v2 ioctl = {0};
> >>  	struct fastrpc_phy_page pages[1];
> >>  	struct fastrpc_map *map = NULL;
> >>  	struct fastrpc_buf *imem = NULL;
> >> @@ -1370,7 +1384,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >>  		u32 attrs;
> >>  		u32 siglen;
> >>  	} inbuf;
> >> -	u32 sc;
> >>  	bool unsigned_module = false;
> >>  
> >>  	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> >> @@ -1444,12 +1457,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >>  	args[5].length = sizeof(inbuf.siglen);
> >>  	args[5].fd = -1;
> >>  
> >> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
> >> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> >> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
> >>  	if (init.attrs)
> >> -		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> >> -
> >> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> >> -				      sc, args);
> >> +		ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> > if (init.attrs)
> >     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
> > else
> >     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
> >
> >> +	ioctl.inv.args = (u64)args;
> >> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
> >>  	if (err)
> >>  		goto err_invoke;
> >>  
> >> @@ -1501,17 +1514,18 @@ static void fastrpc_session_free(struct fastrpc_channel_ctx *cctx,
> >>  static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
> >>  {
> >>  	struct fastrpc_invoke_args args[1];
> >> +	struct fastrpc_invoke_v2 ioctl = {0};
> >>  	int tgid = 0;
> >> -	u32 sc;
> >>  
> >>  	tgid = fl->tgid;
> >>  	args[0].ptr = (u64)(uintptr_t) &tgid;
> >>  	args[0].length = sizeof(tgid);
> >>  	args[0].fd = -1;
> >> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
> >>  
> >> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> >> -				       sc, &args[0]);
> >> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> >> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
> >> +	ioctl.inv.args = (u64)args;
> >> +	return fastrpc_internal_invoke(fl, true, &ioctl);
> >>  }
> >>  
> >>  static int fastrpc_device_release(struct inode *inode, struct file *file)
> >> @@ -1647,45 +1661,77 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
> >>  static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> >>  {
> >>  	struct fastrpc_invoke_args args[1];
> >> +	struct fastrpc_invoke_v2 ioctl = {0};
> >>  	int tgid = fl->tgid;
> >> -	u32 sc;
> >>  
> >>  	args[0].ptr = (u64)(uintptr_t) &tgid;
> >>  	args[0].length = sizeof(tgid);
> >>  	args[0].fd = -1;
> >> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> >>  	fl->pd = pd;
> >>  
> >> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> >> -				       sc, &args[0]);
> >> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
> >> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> >> +	ioctl.inv.args = (u64)args;
> >> +	return fastrpc_internal_invoke(fl, true, &ioctl);
> >>  }
> >>  
> >> -static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
> >> +static int fastrpc_copy_args(struct fastrpc_invoke *inv)
> >>  {
> >>  	struct fastrpc_invoke_args *args = NULL;
> >> -	struct fastrpc_invoke inv;
> >>  	u32 nscalars;
> >> -	int err;
> >> -
> >> -	if (copy_from_user(&inv, argp, sizeof(inv)))
> >> -		return -EFAULT;
> >>  
> >>  	/* nscalars is truncated here to max supported value */
> >> -	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
> >> +	nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
> >>  	if (nscalars) {
> >>  		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
> >>  		if (!args)
> >>  			return -ENOMEM;
> >>  
> >> -		if (copy_from_user(args, (void __user *)(uintptr_t)inv.args,
> >> +		if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
> > Wait... So inv->args is a user pointer? Then how can you assign a
> > kernel-based pointer to the same field? I think you need to sanitize
> > your structures. One is userspace-facing. It should be using userspace
> > data pointers, etc. Another one is a kernel representation of the ioctl
> > args. It might have a different structure, it shouldn't contain __user
> > data, etc.
> I'm planning to have a new structure to carry ctx specific data which will be saved in
> fastrpc_invoke_ctx structure during fastrpc_context_alloc.
> 
> Something like this:
> struct fastrpc_ctx_args {
>     struct fastrpc_invoke_args *args;  /* Carries args that is copied from ioctl structure */
>     void __user *crc; /* Carries CRC user pointer where the crcdata from DSP will be copied for user to read */
>     u64 poll_timeout; /* Carried context specific poll_timeout information */
> };
> 
> Do you see any problem with this?

Not yet. But I'd like to see patches first.

> >
> >>  				   nscalars * sizeof(*args))) {
> >>  			kfree(args);
> >>  			return -EFAULT;
> >>  		}
> >>  	}
> >> +	inv->args = args;
> >>  
> >> -	err = fastrpc_internal_invoke(fl, false, inv.handle, inv.sc, args);
> >> -	kfree(args);
> >> +	return 0;
> >> +}
> > Looking at the rest of the code, I think the patch needs to be split.
> > CRC is the minor issue at this point, please focus on getting existing
> > data being handled correctly while refactoring the code to use new
> > structure. I'd suggest seeing two struct definitions: one for the
> > userspace and another one for the kernel space.
> I've made changes for refactoring where instead of using userspace structure, I'm
> planning to use fastrpc_ctx_args structure mentioned above.
> >
> >> +
> >> +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
> >> +{
> >> +	struct fastrpc_invoke_v2 ioctl = {0};
> >> +	struct fastrpc_invoke inv;
> >> +	int err;
> >> +
> >> +	if (copy_from_user(&inv, argp, sizeof(inv)))
> >> +		return -EFAULT;
> >> +
> >> +	err = fastrpc_copy_args(&inv);
> >> +	if (err)
> >>
> >> -- 
> >> 2.34.1
> >>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

On 1/23/2025 1:18 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
>>
>>
>> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
>>> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
>>>> InvokeV2 request is intended to support multiple enhanced invoke
>>>> requests like CRC check, performance counter enablement and polling
>>>> mode for RPC invocations. CRC check is getting enabled as part of
>>>> this patch. CRC check for input and output argument helps in ensuring
>>>> data consistency over a remote call. If user intends to enable CRC
>>>> check, first local user CRC is calculated at user end and a CRC buffer
>>>> is passed to DSP to capture remote CRC values. DSP is expected to
>>>> write to the remote CRC buffer which is then compared at user level
>>>> with the local CRC values.
>>> This doesn't explain why this is necessary. Why do you need to checksum
>>> arguments?
>> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
>> remote call. This is not enabled by default and user can enable it as per their reqirement.
>> I'll add this information.
> An inconsistency where? Between the kernel and the DSP? Between the user
> and the DSP? Does it cover buffer contents or just the addresses?
Inconsistency between user and DSP. crc_user is calculated at user library before
making ioctl call and it is compared against the crc data which is filled by DSP and
copied to user.
This covers inconsistency in buffer contents.
>
>>> Also, what if the DSP firmware doesn't support CRC? How should userspace
>>> know that?
>> CRC support on DSP is there since long time(>6years).
> This doesn't give us a lot. Upstream kernel supports fastrpc since
> MSM8916 and MSM8996. Do those platforms support CRC?
The metadata buffer as of today also carries space for CRC information:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n877

So this is common across all platforms.

In case CRC is not supported on any older platform, it would result in crc mismatch at user library.
As of now a warning is getting logged there, I can add the information suggesting the failure might
also occur if CRC is not supported.
> And if they do, why do we need the invoke_v2? Can we modify existing
> code instead?
invoke_v2 is needed because there is a need to pass user crc pointer over ioctl call which
cannot be achieved using existing code. Also there are plans to add more features to this
invoke_v2 request which will carry some information from user.
>
>> From user space CRC check failure is
>> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
>> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
>> I consider the copy failure as non-fatal as userspace is treating this as a warning?
> warnings can remain unseen for a long time. Consider a GUI app. Nobody
> is there to view kernel warnings or library output.
Let me see if this can be done. Are you suggesting that the app will be somewhat tracking
if there is any crc check mismatch failures?
>
>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
>>>>  include/uapi/misc/fastrpc.h |   7 ++
>>>>  2 files changed, 116 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 74181b8c386b..8e817a763d1d 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>>>>  
>>>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
>>>> -			struct fastrpc_invoke_args *args)
>>>> +			struct fastrpc_invoke_v2 *inv2)
>>>>  {
>>>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
>>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>>>> +	struct fastrpc_invoke_args *args = NULL;
>>> Why do you need to init to NULL if you are going to set it two lines
>>> below?
>>>
>>>>  	unsigned long flags;
>>>>  	int ret;
>>>>  
>>>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
>>> Why does it need a typecast?
>>>
>>>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>>>  	if (!ctx)
>>>>  		return ERR_PTR(-ENOMEM);
>>>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>  	/* Released in fastrpc_context_put() */
>>>>  	fastrpc_channel_ctx_get(cctx);
>>>>  
>>>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
>>> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
>>> user-based pointer, where is the accessiblity check? Why isn't it
>>> annotated properly?
>> This is a user pointer where the crc data is expected to be copied. There is no
>> other access to this pointer from kernel. I'm planning to change the data type
>> for crc as (void __user*) inside fastrpc_invoke_ctx structure.
> Yes, please. Also make sure that sparse doesn't add any warnings
> regarding pointer conversions.
Ack.
>
>>>>  	ctx->sc = sc;
>>>>  	ctx->retval = -1;
>>>>  	ctx->pid = current->pid;
>>>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>  	struct fastrpc_invoke_buf *list;
>>>>  	struct fastrpc_phy_page *pages;
>>>>  	u64 *fdlist;
>>>> +	u32 *crclist;
>>>>  	int i, inbufs, outbufs, handles;
>>>>  
>>>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
>>>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>>>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
>>>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>>>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
>>> I think we should rewrite this parsing somehow. Is the format of data
>>> documented somewhere?
>> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
>> to store these pointers to ctx structure and directly use it wherever needed. This will
>> clean-up this unnecessary calculations at multiple places.
>
> Please do. Nevertheless, the format also must be documented.
Ack.
>
>>>>  
>>>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>>>>  		if (!ctx->maps[i]) {
>>>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>  			fastrpc_map_put(mmap);
>>>>  	}
>>>>  
>>>> +	if (ctx->crc && crclist && rpra) {
>>>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
>>>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
>>> Oh, so it's a user pointer. Then u32* was completely incorrect.
>>> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
>>> filled? Or are we leaking some data to userspace?
>> Yes, right. Planning clean-up in next patch.
>>
>> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
> Huh? I definitely want to see documentation for function arguments.
Sure. I'll also modify the metadata layout doc here to add fdlist, CRC and other planned contents.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n842
>
>>>> +			return -EFAULT;
>>>> +	}
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -1137,13 +1148,12 @@ static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
>>>>  
>>>>  }
>>>>  
>>>> -static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>>>> -				   u32 handle, u32 sc,
>>>> -				   struct fastrpc_invoke_args *args)
>>>> +static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel, struct fastrpc_invoke_v2 *inv2)
>>> Please don't touch what doesn't need to be touched. You are replacing
>>> handle/sc/args with inv2, not touching the first line.
>> Ack.
>>>>  {
>>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>>>>  	struct fastrpc_buf *buf, *b;
>>>> -
>>>> +	struct fastrpc_invoke inv;
>>>> +	u32 handle, sc;
>>>>  	int err = 0;
>>>>  
>>>>  	if (!fl->sctx)
>>>> @@ -1152,12 +1162,15 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>>>>  	if (!fl->cctx->rpdev)
>>>>  		return -EPIPE;
>>>>  
>>>> +	inv = inv2->inv;
>>>> +	handle = inv.handle;
>>>> +	sc = inv.sc;
>>>>  	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
>>>>  		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
>>>>  		return -EPERM;
>>>>  	}
>>>>  
>>>> -	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
>>>> +	ctx = fastrpc_context_alloc(fl, kernel, sc, inv2);
>>>>  	if (IS_ERR(ctx))
>>>>  		return PTR_ERR(ctx);
>>>>  
>>>> @@ -1239,6 +1252,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>  {
>>>>  	struct fastrpc_init_create_static init;
>>>>  	struct fastrpc_invoke_args *args;
>>>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>> Why do you need to init it?
>> Ack.
>>>>  	struct fastrpc_phy_page pages[1];
>>>>  	char *name;
>>>>  	int err;
>>>> @@ -1248,7 +1262,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>  		u32 namelen;
>>>>  		u32 pageslen;
>>>>  	} inbuf;
>>>> -	u32 sc;
>>>>  
>>>>  	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>>>>  	if (!args)
>>>> @@ -1313,10 +1326,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>>  	args[2].length = sizeof(*pages);
>>>>  	args[2].fd = -1;
>>>>  
>>>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
>>>> -
>>>> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>>> -				      sc, args);
>>>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>>>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0);
>>>> +	ioctl.inv.args = (u64)args;
>>> Can you pass it as is, without typecasts?
>> Cleaned up in refactoring change.
>>>> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>>>>  	if (err)
>>>>  		goto err_invoke;
>>>>  
>>>> @@ -1357,6 +1370,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>>>  {
>>>>  	struct fastrpc_init_create init;
>>>>  	struct fastrpc_invoke_args *args;
>>>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>>>  	struct fastrpc_phy_page pages[1];
>>>>  	struct fastrpc_map *map = NULL;
>>>>  	struct fastrpc_buf *imem = NULL;
>>>> @@ -1370,7 +1384,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>>>  		u32 attrs;
>>>>  		u32 siglen;
>>>>  	} inbuf;
>>>> -	u32 sc;
>>>>  	bool unsigned_module = false;
>>>>  
>>>>  	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>>>> @@ -1444,12 +1457,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>>>  	args[5].length = sizeof(inbuf.siglen);
>>>>  	args[5].fd = -1;
>>>>  
>>>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>>>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>>>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>>>>  	if (init.attrs)
>>>> -		sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
>>>> -
>>>> -	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>>> -				      sc, args);
>>>> +		ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
>>> if (init.attrs)
>>>     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_ATTR, 4, 0);
>>> else
>>>     ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE, 4, 0);
>>>
>>>> +	ioctl.inv.args = (u64)args;
>>>> +	err = fastrpc_internal_invoke(fl, true, &ioctl);
>>>>  	if (err)
>>>>  		goto err_invoke;
>>>>  
>>>> @@ -1501,17 +1514,18 @@ static void fastrpc_session_free(struct fastrpc_channel_ctx *cctx,
>>>>  static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
>>>>  {
>>>>  	struct fastrpc_invoke_args args[1];
>>>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>>>  	int tgid = 0;
>>>> -	u32 sc;
>>>>  
>>>>  	tgid = fl->tgid;
>>>>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>>>>  	args[0].length = sizeof(tgid);
>>>>  	args[0].fd = -1;
>>>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
>>>>  
>>>> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>>> -				       sc, &args[0]);
>>>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>>>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_RELEASE, 1, 0);
>>>> +	ioctl.inv.args = (u64)args;
>>>> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>>>>  }
>>>>  
>>>>  static int fastrpc_device_release(struct inode *inode, struct file *file)
>>>> @@ -1647,45 +1661,77 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>>>>  static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>>>>  {
>>>>  	struct fastrpc_invoke_args args[1];
>>>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>>>  	int tgid = fl->tgid;
>>>> -	u32 sc;
>>>>  
>>>>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>>>>  	args[0].length = sizeof(tgid);
>>>>  	args[0].fd = -1;
>>>> -	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>>>>  	fl->pd = pd;
>>>>  
>>>> -	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>>> -				       sc, &args[0]);
>>>> +	ioctl.inv.handle = FASTRPC_INIT_HANDLE;
>>>> +	ioctl.inv.sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>>>> +	ioctl.inv.args = (u64)args;
>>>> +	return fastrpc_internal_invoke(fl, true, &ioctl);
>>>>  }
>>>>  
>>>> -static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
>>>> +static int fastrpc_copy_args(struct fastrpc_invoke *inv)
>>>>  {
>>>>  	struct fastrpc_invoke_args *args = NULL;
>>>> -	struct fastrpc_invoke inv;
>>>>  	u32 nscalars;
>>>> -	int err;
>>>> -
>>>> -	if (copy_from_user(&inv, argp, sizeof(inv)))
>>>> -		return -EFAULT;
>>>>  
>>>>  	/* nscalars is truncated here to max supported value */
>>>> -	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
>>>> +	nscalars = REMOTE_SCALARS_LENGTH(inv->sc);
>>>>  	if (nscalars) {
>>>>  		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
>>>>  		if (!args)
>>>>  			return -ENOMEM;
>>>>  
>>>> -		if (copy_from_user(args, (void __user *)(uintptr_t)inv.args,
>>>> +		if (copy_from_user(args, (void __user *)(uintptr_t)inv->args,
>>> Wait... So inv->args is a user pointer? Then how can you assign a
>>> kernel-based pointer to the same field? I think you need to sanitize
>>> your structures. One is userspace-facing. It should be using userspace
>>> data pointers, etc. Another one is a kernel representation of the ioctl
>>> args. It might have a different structure, it shouldn't contain __user
>>> data, etc.
>> I'm planning to have a new structure to carry ctx specific data which will be saved in
>> fastrpc_invoke_ctx structure during fastrpc_context_alloc.
>>
>> Something like this:
>> struct fastrpc_ctx_args {
>>     struct fastrpc_invoke_args *args;  /* Carries args that is copied from ioctl structure */
>>     void __user *crc; /* Carries CRC user pointer where the crcdata from DSP will be copied for user to read */
>>     u64 poll_timeout; /* Carried context specific poll_timeout information */
>> };
>>
>> Do you see any problem with this?
> Not yet. But I'd like to see patches first.
Sure, thanks.

--ekansh
>
>>>>  				   nscalars * sizeof(*args))) {
>>>>  			kfree(args);
>>>>  			return -EFAULT;
>>>>  		}
>>>>  	}
>>>> +	inv->args = args;
>>>>  
>>>> -	err = fastrpc_internal_invoke(fl, false, inv.handle, inv.sc, args);
>>>> -	kfree(args);
>>>> +	return 0;
>>>> +}
>>> Looking at the rest of the code, I think the patch needs to be split.
>>> CRC is the minor issue at this point, please focus on getting existing
>>> data being handled correctly while refactoring the code to use new
>>> structure. I'd suggest seeing two struct definitions: one for the
>>> userspace and another one for the kernel space.
>> I've made changes for refactoring where instead of using userspace structure, I'm
>> planning to use fastrpc_ctx_args structure mentioned above.
>>>> +
>>>> +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
>>>> +{
>>>> +	struct fastrpc_invoke_v2 ioctl = {0};
>>>> +	struct fastrpc_invoke inv;
>>>> +	int err;
>>>> +
>>>> +	if (copy_from_user(&inv, argp, sizeof(inv)))
>>>> +		return -EFAULT;
>>>> +
>>>> +	err = fastrpc_copy_args(&inv);
>>>> +	if (err)
>>>>
>>>> -- 
>>>> 2.34.1
>>>>

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Dmitry Baryshkov]

On Thu, Jan 23, 2025 at 03:19:21PM +0530, Ekansh Gupta wrote:
> 
> 
> 
> On 1/23/2025 1:18 PM, Dmitry Baryshkov wrote:
> > On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
> >>
> >>
> >> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
> >>> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
> >>>> InvokeV2 request is intended to support multiple enhanced invoke
> >>>> requests like CRC check, performance counter enablement and polling
> >>>> mode for RPC invocations. CRC check is getting enabled as part of
> >>>> this patch. CRC check for input and output argument helps in ensuring
> >>>> data consistency over a remote call. If user intends to enable CRC
> >>>> check, first local user CRC is calculated at user end and a CRC buffer
> >>>> is passed to DSP to capture remote CRC values. DSP is expected to
> >>>> write to the remote CRC buffer which is then compared at user level
> >>>> with the local CRC values.
> >>> This doesn't explain why this is necessary. Why do you need to checksum
> >>> arguments?
> >> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
> >> remote call. This is not enabled by default and user can enable it as per their reqirement.
> >> I'll add this information.
> > An inconsistency where? Between the kernel and the DSP? Between the user
> > and the DSP? Does it cover buffer contents or just the addresses?
> Inconsistency between user and DSP. crc_user is calculated at user library before
> making ioctl call and it is compared against the crc data which is filled by DSP and
> copied to user.
> This covers inconsistency in buffer contents.

What is the reason for possible inconsistencies? Is it a debugging
feature?

> >
> >>> Also, what if the DSP firmware doesn't support CRC? How should userspace
> >>> know that?
> >> CRC support on DSP is there since long time(>6years).
> > This doesn't give us a lot. Upstream kernel supports fastrpc since
> > MSM8916 and MSM8996. Do those platforms support CRC?
> The metadata buffer as of today also carries space for CRC information:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n877
> 
> So this is common across all platforms.
> 
> In case CRC is not supported on any older platform, it would result in crc mismatch at user library.
> As of now a warning is getting logged there, I can add the information suggesting the failure might
> also occur if CRC is not supported.

Logs go to /dev/null, they are ignored by users, etc. So either there
should be an actual error being returned by the kernel / library, or it
can be completely ignored and skipped.

So, do MSM8916 / MSM8996 / SDM845 support CRC? If not, that must be
handled somehow.

> > And if they do, why do we need the invoke_v2? Can we modify existing
> > code instead?
> invoke_v2 is needed because there is a need to pass user crc pointer over ioctl call which
> cannot be achieved using existing code. Also there are plans to add more features to this
> invoke_v2 request which will carry some information from user.

Is it really extensible without breaking the ABI?

> >
> >> From user space CRC check failure is
> >> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
> >> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
> >> I consider the copy failure as non-fatal as userspace is treating this as a warning?
> > warnings can remain unseen for a long time. Consider a GUI app. Nobody
> > is there to view kernel warnings or library output.
> Let me see if this can be done. Are you suggesting that the app will be somewhat tracking
> if there is any crc check mismatch failures?

I suggest returning -EIO to the app.

> >
> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>> ---
> >>>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
> >>>>  include/uapi/misc/fastrpc.h |   7 ++
> >>>>  2 files changed, 116 insertions(+), 52 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>> index 74181b8c386b..8e817a763d1d 100644
> >>>> --- a/drivers/misc/fastrpc.c
> >>>> +++ b/drivers/misc/fastrpc.c
> >>>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
> >>>>  
> >>>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
> >>>> -			struct fastrpc_invoke_args *args)
> >>>> +			struct fastrpc_invoke_v2 *inv2)
> >>>>  {
> >>>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
> >>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
> >>>> +	struct fastrpc_invoke_args *args = NULL;
> >>> Why do you need to init to NULL if you are going to set it two lines
> >>> below?
> >>>
> >>>>  	unsigned long flags;
> >>>>  	int ret;
> >>>>  
> >>>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
> >>> Why does it need a typecast?
> >>>
> >>>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> >>>>  	if (!ctx)
> >>>>  		return ERR_PTR(-ENOMEM);
> >>>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>>>  	/* Released in fastrpc_context_put() */
> >>>>  	fastrpc_channel_ctx_get(cctx);
> >>>>  
> >>>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
> >>> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
> >>> user-based pointer, where is the accessiblity check? Why isn't it
> >>> annotated properly?
> >> This is a user pointer where the crc data is expected to be copied. There is no
> >> other access to this pointer from kernel. I'm planning to change the data type
> >> for crc as (void __user*) inside fastrpc_invoke_ctx structure.
> > Yes, please. Also make sure that sparse doesn't add any warnings
> > regarding pointer conversions.
> Ack.
> >
> >>>>  	ctx->sc = sc;
> >>>>  	ctx->retval = -1;
> >>>>  	ctx->pid = current->pid;
> >>>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>  	struct fastrpc_invoke_buf *list;
> >>>>  	struct fastrpc_phy_page *pages;
> >>>>  	u64 *fdlist;
> >>>> +	u32 *crclist;
> >>>>  	int i, inbufs, outbufs, handles;
> >>>>  
> >>>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> >>>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> >>>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
> >>>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
> >>>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
> >>> I think we should rewrite this parsing somehow. Is the format of data
> >>> documented somewhere?
> >> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
> >> to store these pointers to ctx structure and directly use it wherever needed. This will
> >> clean-up this unnecessary calculations at multiple places.
> >
> > Please do. Nevertheless, the format also must be documented.
> Ack.
> >
> >>>>  
> >>>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
> >>>>  		if (!ctx->maps[i]) {
> >>>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>  			fastrpc_map_put(mmap);
> >>>>  	}
> >>>>  
> >>>> +	if (ctx->crc && crclist && rpra) {
> >>>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
> >>>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
> >>> Oh, so it's a user pointer. Then u32* was completely incorrect.
> >>> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
> >>> filled? Or are we leaking some data to userspace?
> >> Yes, right. Planning clean-up in next patch.
> >>
> >> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
> > Huh? I definitely want to see documentation for function arguments.
> Sure. I'll also modify the metadata layout doc here to add fdlist, CRC and other planned contents.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n842

This is not a documentation. E.g. I can not write code using that
description. For example, it mentions neither FDLIST nor CRC.

> >
> >>>> +			return -EFAULT;
> >>>> +	}
> >>>> +
> >>>>  	return 0;
> >>>>  }
> >>>>  

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

On 1/23/2025 4:43 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 03:19:21PM +0530, Ekansh Gupta wrote:
>>
>>
>> On 1/23/2025 1:18 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
>>>>
>>>> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
>>>>>> InvokeV2 request is intended to support multiple enhanced invoke
>>>>>> requests like CRC check, performance counter enablement and polling
>>>>>> mode for RPC invocations. CRC check is getting enabled as part of
>>>>>> this patch. CRC check for input and output argument helps in ensuring
>>>>>> data consistency over a remote call. If user intends to enable CRC
>>>>>> check, first local user CRC is calculated at user end and a CRC buffer
>>>>>> is passed to DSP to capture remote CRC values. DSP is expected to
>>>>>> write to the remote CRC buffer which is then compared at user level
>>>>>> with the local CRC values.
>>>>> This doesn't explain why this is necessary. Why do you need to checksum
>>>>> arguments?
>>>> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
>>>> remote call. This is not enabled by default and user can enable it as per their reqirement.
>>>> I'll add this information.
>>> An inconsistency where? Between the kernel and the DSP? Between the user
>>> and the DSP? Does it cover buffer contents or just the addresses?
>> Inconsistency between user and DSP. crc_user is calculated at user library before
>> making ioctl call and it is compared against the crc data which is filled by DSP and
>> copied to user.
>> This covers inconsistency in buffer contents.
> What is the reason for possible inconsistencies? Is it a debugging
> feature?
This is a debugging feature. Buffer data corruption might result in inconsistency.
>
>>>>> Also, what if the DSP firmware doesn't support CRC? How should userspace
>>>>> know that?
>>>> CRC support on DSP is there since long time(>6years).
>>> This doesn't give us a lot. Upstream kernel supports fastrpc since
>>> MSM8916 and MSM8996. Do those platforms support CRC?
>> The metadata buffer as of today also carries space for CRC information:
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n877
>>
>> So this is common across all platforms.
>>
>> In case CRC is not supported on any older platform, it would result in crc mismatch at user library.
>> As of now a warning is getting logged there, I can add the information suggesting the failure might
>> also occur if CRC is not supported.
> Logs go to /dev/null, they are ignored by users, etc. So either there
> should be an actual error being returned by the kernel / library, or it
> can be completely ignored and skipped.
>
> So, do MSM8916 / MSM8996 / SDM845 support CRC? If not, that must be
> handled somehow.
I see it's supported on SDM845 but not on MSM89##. I'll just send the new patch version for now
as CRC mismatch failures are getting ignored.
>
>>> And if they do, why do we need the invoke_v2? Can we modify existing
>>> code instead?
>> invoke_v2 is needed because there is a need to pass user crc pointer over ioctl call which
>> cannot be achieved using existing code. Also there are plans to add more features to this
>> invoke_v2 request which will carry some information from user.
> Is it really extensible without breaking the ABI?
I'm planning to keep reserved bits in uapi struct for the same. Do you see any
problem with this?
>>>> From user space CRC check failure is
>>>> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
>>>> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
>>>> I consider the copy failure as non-fatal as userspace is treating this as a warning?
>>> warnings can remain unseen for a long time. Consider a GUI app. Nobody
>>> is there to view kernel warnings or library output.
>> Let me see if this can be done. Are you suggesting that the app will be somewhat tracking
>> if there is any crc check mismatch failures?
> I suggest returning -EIO to the app.
I'll check this.
>
>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>> ---
>>>>>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
>>>>>>  include/uapi/misc/fastrpc.h |   7 ++
>>>>>>  2 files changed, 116 insertions(+), 52 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>>> index 74181b8c386b..8e817a763d1d 100644
>>>>>> --- a/drivers/misc/fastrpc.c
>>>>>> +++ b/drivers/misc/fastrpc.c
>>>>>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>>>>>>  
>>>>>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
>>>>>> -			struct fastrpc_invoke_args *args)
>>>>>> +			struct fastrpc_invoke_v2 *inv2)
>>>>>>  {
>>>>>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
>>>>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>>>>>> +	struct fastrpc_invoke_args *args = NULL;
>>>>> Why do you need to init to NULL if you are going to set it two lines
>>>>> below?
>>>>>
>>>>>>  	unsigned long flags;
>>>>>>  	int ret;
>>>>>>  
>>>>>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
>>>>> Why does it need a typecast?
>>>>>
>>>>>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>>>>>  	if (!ctx)
>>>>>>  		return ERR_PTR(-ENOMEM);
>>>>>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>>>  	/* Released in fastrpc_context_put() */
>>>>>>  	fastrpc_channel_ctx_get(cctx);
>>>>>>  
>>>>>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
>>>>> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
>>>>> user-based pointer, where is the accessiblity check? Why isn't it
>>>>> annotated properly?
>>>> This is a user pointer where the crc data is expected to be copied. There is no
>>>> other access to this pointer from kernel. I'm planning to change the data type
>>>> for crc as (void __user*) inside fastrpc_invoke_ctx structure.
>>> Yes, please. Also make sure that sparse doesn't add any warnings
>>> regarding pointer conversions.
>> Ack.
>>>>>>  	ctx->sc = sc;
>>>>>>  	ctx->retval = -1;
>>>>>>  	ctx->pid = current->pid;
>>>>>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>  	struct fastrpc_invoke_buf *list;
>>>>>>  	struct fastrpc_phy_page *pages;
>>>>>>  	u64 *fdlist;
>>>>>> +	u32 *crclist;
>>>>>>  	int i, inbufs, outbufs, handles;
>>>>>>  
>>>>>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
>>>>>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>>>>>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
>>>>>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>>>>>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
>>>>> I think we should rewrite this parsing somehow. Is the format of data
>>>>> documented somewhere?
>>>> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
>>>> to store these pointers to ctx structure and directly use it wherever needed. This will
>>>> clean-up this unnecessary calculations at multiple places.
>>> Please do. Nevertheless, the format also must be documented.
>> Ack.
>>>>>>  
>>>>>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>>>>>>  		if (!ctx->maps[i]) {
>>>>>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>  			fastrpc_map_put(mmap);
>>>>>>  	}
>>>>>>  
>>>>>> +	if (ctx->crc && crclist && rpra) {
>>>>>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
>>>>>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
>>>>> Oh, so it's a user pointer. Then u32* was completely incorrect.
>>>>> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
>>>>> filled? Or are we leaking some data to userspace?
>>>> Yes, right. Planning clean-up in next patch.
>>>>
>>>> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
>>> Huh? I definitely want to see documentation for function arguments.
>> Sure. I'll also modify the metadata layout doc here to add fdlist, CRC and other planned contents.
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n842
> This is not a documentation. E.g. I can not write code using that
> description. For example, it mentions neither FDLIST nor CRC.
I'm planning to add complete documentation for CRC and polling mode in user library project.
If I need to add documentation in driver, can you pls suggest what is the right place to add
the information?

Thanks for your comments.

--ekansh
>
>>>>>> +			return -EFAULT;
>>>>>> +	}
>>>>>> +
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Dmitry Baryshkov]

On Thu, Jan 23, 2025 at 05:34:00PM +0530, Ekansh Gupta wrote:
> 
> 
> 
> On 1/23/2025 4:43 PM, Dmitry Baryshkov wrote:
> > On Thu, Jan 23, 2025 at 03:19:21PM +0530, Ekansh Gupta wrote:
> >>
> >>
> >> On 1/23/2025 1:18 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
> >>>>
> >>>> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
> >>>>>> InvokeV2 request is intended to support multiple enhanced invoke
> >>>>>> requests like CRC check, performance counter enablement and polling
> >>>>>> mode for RPC invocations. CRC check is getting enabled as part of
> >>>>>> this patch. CRC check for input and output argument helps in ensuring
> >>>>>> data consistency over a remote call. If user intends to enable CRC
> >>>>>> check, first local user CRC is calculated at user end and a CRC buffer
> >>>>>> is passed to DSP to capture remote CRC values. DSP is expected to
> >>>>>> write to the remote CRC buffer which is then compared at user level
> >>>>>> with the local CRC values.
> >>>>> This doesn't explain why this is necessary. Why do you need to checksum
> >>>>> arguments?
> >>>> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
> >>>> remote call. This is not enabled by default and user can enable it as per their reqirement.
> >>>> I'll add this information.
> >>> An inconsistency where? Between the kernel and the DSP? Between the user
> >>> and the DSP? Does it cover buffer contents or just the addresses?
> >> Inconsistency between user and DSP. crc_user is calculated at user library before
> >> making ioctl call and it is compared against the crc data which is filled by DSP and
> >> copied to user.
> >> This covers inconsistency in buffer contents.
> > What is the reason for possible inconsistencies? Is it a debugging
> > feature?
> This is a debugging feature. Buffer data corruption might result in inconsistency.
> >
> >>>>> Also, what if the DSP firmware doesn't support CRC? How should userspace
> >>>>> know that?
> >>>> CRC support on DSP is there since long time(>6years).
> >>> This doesn't give us a lot. Upstream kernel supports fastrpc since
> >>> MSM8916 and MSM8996. Do those platforms support CRC?
> >> The metadata buffer as of today also carries space for CRC information:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n877
> >>
> >> So this is common across all platforms.
> >>
> >> In case CRC is not supported on any older platform, it would result in crc mismatch at user library.
> >> As of now a warning is getting logged there, I can add the information suggesting the failure might
> >> also occur if CRC is not supported.
> > Logs go to /dev/null, they are ignored by users, etc. So either there
> > should be an actual error being returned by the kernel / library, or it
> > can be completely ignored and skipped.
> >
> > So, do MSM8916 / MSM8996 / SDM845 support CRC? If not, that must be
> > handled somehow.
> I see it's supported on SDM845 but not on MSM89##. I'll just send the new patch version for now
> as CRC mismatch failures are getting ignored.

Please clearly document which platforms don't support CRC feature, the
implications and the possible (userspace) impact. E.g. if the kernel
logs a message for each call with failed CRC case, then this might cause
a serious slowdown. Likewise a userspace lib might cause a slowdown.

In fact, if CRC is a debugging feature, is it going to be enabled or
disabled by default?

> >
> >>> And if they do, why do we need the invoke_v2? Can we modify existing
> >>> code instead?
> >> invoke_v2 is needed because there is a need to pass user crc pointer over ioctl call which
> >> cannot be achieved using existing code. Also there are plans to add more features to this
> >> invoke_v2 request which will carry some information from user.
> > Is it really extensible without breaking the ABI?
> I'm planning to keep reserved bits in uapi struct for the same. Do you see any
> problem with this?

Please keep Greg's comment in mind - verify that all reserved fields are
zero.

> >>>> From user space CRC check failure is
> >>>> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
> >>>> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
> >>>> I consider the copy failure as non-fatal as userspace is treating this as a warning?
> >>> warnings can remain unseen for a long time. Consider a GUI app. Nobody
> >>> is there to view kernel warnings or library output.
> >> Let me see if this can be done. Are you suggesting that the app will be somewhat tracking
> >> if there is any crc check mismatch failures?
> > I suggest returning -EIO to the app.
> I'll check this.
> >
> >>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>>>> ---
> >>>>>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
> >>>>>>  include/uapi/misc/fastrpc.h |   7 ++
> >>>>>>  2 files changed, 116 insertions(+), 52 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>>>> index 74181b8c386b..8e817a763d1d 100644
> >>>>>> --- a/drivers/misc/fastrpc.c
> >>>>>> +++ b/drivers/misc/fastrpc.c
> >>>>>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
> >>>>>>  
> >>>>>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>>>>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
> >>>>>> -			struct fastrpc_invoke_args *args)
> >>>>>> +			struct fastrpc_invoke_v2 *inv2)
> >>>>>>  {
> >>>>>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
> >>>>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
> >>>>>> +	struct fastrpc_invoke_args *args = NULL;
> >>>>> Why do you need to init to NULL if you are going to set it two lines
> >>>>> below?
> >>>>>
> >>>>>>  	unsigned long flags;
> >>>>>>  	int ret;
> >>>>>>  
> >>>>>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
> >>>>> Why does it need a typecast?
> >>>>>
> >>>>>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> >>>>>>  	if (!ctx)
> >>>>>>  		return ERR_PTR(-ENOMEM);
> >>>>>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>>>>>  	/* Released in fastrpc_context_put() */
> >>>>>>  	fastrpc_channel_ctx_get(cctx);
> >>>>>>  
> >>>>>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
> >>>>> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
> >>>>> user-based pointer, where is the accessiblity check? Why isn't it
> >>>>> annotated properly?
> >>>> This is a user pointer where the crc data is expected to be copied. There is no
> >>>> other access to this pointer from kernel. I'm planning to change the data type
> >>>> for crc as (void __user*) inside fastrpc_invoke_ctx structure.
> >>> Yes, please. Also make sure that sparse doesn't add any warnings
> >>> regarding pointer conversions.
> >> Ack.
> >>>>>>  	ctx->sc = sc;
> >>>>>>  	ctx->retval = -1;
> >>>>>>  	ctx->pid = current->pid;
> >>>>>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>>>  	struct fastrpc_invoke_buf *list;
> >>>>>>  	struct fastrpc_phy_page *pages;
> >>>>>>  	u64 *fdlist;
> >>>>>> +	u32 *crclist;
> >>>>>>  	int i, inbufs, outbufs, handles;
> >>>>>>  
> >>>>>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> >>>>>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> >>>>>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
> >>>>>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
> >>>>>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
> >>>>> I think we should rewrite this parsing somehow. Is the format of data
> >>>>> documented somewhere?
> >>>> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
> >>>> to store these pointers to ctx structure and directly use it wherever needed. This will
> >>>> clean-up this unnecessary calculations at multiple places.
> >>> Please do. Nevertheless, the format also must be documented.
> >> Ack.
> >>>>>>  
> >>>>>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
> >>>>>>  		if (!ctx->maps[i]) {
> >>>>>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >>>>>>  			fastrpc_map_put(mmap);
> >>>>>>  	}
> >>>>>>  
> >>>>>> +	if (ctx->crc && crclist && rpra) {
> >>>>>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
> >>>>>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
> >>>>> Oh, so it's a user pointer. Then u32* was completely incorrect.
> >>>>> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
> >>>>> filled? Or are we leaking some data to userspace?
> >>>> Yes, right. Planning clean-up in next patch.
> >>>>
> >>>> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
> >>> Huh? I definitely want to see documentation for function arguments.
> >> Sure. I'll also modify the metadata layout doc here to add fdlist, CRC and other planned contents.
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n842
> > This is not a documentation. E.g. I can not write code using that
> > description. For example, it mentions neither FDLIST nor CRC.
> I'm planning to add complete documentation for CRC and polling mode in user library project.
> If I need to add documentation in driver, can you pls suggest what is the right place to add
> the information?

Library should be fine. We don't require documenting all hardware in the
kernel. But the uAPI of the driver should be properly described.

> 
> Thanks for your comments.
> 
> --ekansh
> >
> >>>>>> +			return -EFAULT;
> >>>>>> +	}
> >>>>>> +
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>  
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/4] misc: fastrpc: Add CRC support using invokeV2 request

[Ekansh Gupta]

On 1/23/2025 6:03 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 05:34:00PM +0530, Ekansh Gupta wrote:
>>
>>
>> On 1/23/2025 4:43 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 23, 2025 at 03:19:21PM +0530, Ekansh Gupta wrote:
>>>>
>>>> On 1/23/2025 1:18 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Jan 23, 2025 at 11:16:41AM +0530, Ekansh Gupta wrote:
>>>>>> On 10/7/2024 7:27 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Oct 07, 2024 at 02:15:15PM GMT, Ekansh Gupta wrote:
>>>>>>>> InvokeV2 request is intended to support multiple enhanced invoke
>>>>>>>> requests like CRC check, performance counter enablement and polling
>>>>>>>> mode for RPC invocations. CRC check is getting enabled as part of
>>>>>>>> this patch. CRC check for input and output argument helps in ensuring
>>>>>>>> data consistency over a remote call. If user intends to enable CRC
>>>>>>>> check, first local user CRC is calculated at user end and a CRC buffer
>>>>>>>> is passed to DSP to capture remote CRC values. DSP is expected to
>>>>>>>> write to the remote CRC buffer which is then compared at user level
>>>>>>>> with the local CRC values.
>>>>>>> This doesn't explain why this is necessary. Why do you need to checksum
>>>>>>> arguments?
>>>>>> This helps if the user suspects any data inconsistencies in the buffers passed to DSP over
>>>>>> remote call. This is not enabled by default and user can enable it as per their reqirement.
>>>>>> I'll add this information.
>>>>> An inconsistency where? Between the kernel and the DSP? Between the user
>>>>> and the DSP? Does it cover buffer contents or just the addresses?
>>>> Inconsistency between user and DSP. crc_user is calculated at user library before
>>>> making ioctl call and it is compared against the crc data which is filled by DSP and
>>>> copied to user.
>>>> This covers inconsistency in buffer contents.
>>> What is the reason for possible inconsistencies? Is it a debugging
>>> feature?
>> This is a debugging feature. Buffer data corruption might result in inconsistency.
>>>>>>> Also, what if the DSP firmware doesn't support CRC? How should userspace
>>>>>>> know that?
>>>>>> CRC support on DSP is there since long time(>6years).
>>>>> This doesn't give us a lot. Upstream kernel supports fastrpc since
>>>>> MSM8916 and MSM8996. Do those platforms support CRC?
>>>> The metadata buffer as of today also carries space for CRC information:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n877
>>>>
>>>> So this is common across all platforms.
>>>>
>>>> In case CRC is not supported on any older platform, it would result in crc mismatch at user library.
>>>> As of now a warning is getting logged there, I can add the information suggesting the failure might
>>>> also occur if CRC is not supported.
>>> Logs go to /dev/null, they are ignored by users, etc. So either there
>>> should be an actual error being returned by the kernel / library, or it
>>> can be completely ignored and skipped.
>>>
>>> So, do MSM8916 / MSM8996 / SDM845 support CRC? If not, that must be
>>> handled somehow.
>> I see it's supported on SDM845 but not on MSM89##. I'll just send the new patch version for now
>> as CRC mismatch failures are getting ignored.
> Please clearly document which platforms don't support CRC feature, the
> implications and the possible (userspace) impact. E.g. if the kernel
> logs a message for each call with failed CRC case, then this might cause
> a serious slowdown. Likewise a userspace lib might cause a slowdown.
>
> In fact, if CRC is a debugging feature, is it going to be enabled or
> disabled by default?
>
>>>>> And if they do, why do we need the invoke_v2? Can we modify existing
>>>>> code instead?
>>>> invoke_v2 is needed because there is a need to pass user crc pointer over ioctl call which
>>>> cannot be achieved using existing code. Also there are plans to add more features to this
>>>> invoke_v2 request which will carry some information from user.
>>> Is it really extensible without breaking the ABI?
>> I'm planning to keep reserved bits in uapi struct for the same. Do you see any
>> problem with this?
> Please keep Greg's comment in mind - verify that all reserved fields are
> zero.
Yes, I'll be ensuring checks for reserved fields.
>
>>>>>> From user space CRC check failure is
>>>>>> not fatal and is printed as a warning. But if copy of CRC to user fails, it will result in remote
>>>>>> call failure. Should I keep it as fatal considering that ever very old DSP support this or should
>>>>>> I consider the copy failure as non-fatal as userspace is treating this as a warning?
>>>>> warnings can remain unseen for a long time. Consider a GUI app. Nobody
>>>>> is there to view kernel warnings or library output.
>>>> Let me see if this can be done. Are you suggesting that the app will be somewhat tracking
>>>> if there is any crc check mismatch failures?
>>> I suggest returning -EIO to the app.
>> I'll check this.
>>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>>>> ---
>>>>>>>>  drivers/misc/fastrpc.c      | 161 ++++++++++++++++++++++++------------
>>>>>>>>  include/uapi/misc/fastrpc.h |   7 ++
>>>>>>>>  2 files changed, 116 insertions(+), 52 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>>>>> index 74181b8c386b..8e817a763d1d 100644
>>>>>>>> --- a/drivers/misc/fastrpc.c
>>>>>>>> +++ b/drivers/misc/fastrpc.c
>>>>>>>> @@ -573,13 +573,15 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>>>>>>>>  
>>>>>>>>  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>>>>>  			struct fastrpc_user *user, u32 kernel, u32 sc,
>>>>>>>> -			struct fastrpc_invoke_args *args)
>>>>>>>> +			struct fastrpc_invoke_v2 *inv2)
>>>>>>>>  {
>>>>>>>>  	struct fastrpc_channel_ctx *cctx = user->cctx;
>>>>>>>>  	struct fastrpc_invoke_ctx *ctx = NULL;
>>>>>>>> +	struct fastrpc_invoke_args *args = NULL;
>>>>>>> Why do you need to init to NULL if you are going to set it two lines
>>>>>>> below?
>>>>>>>
>>>>>>>>  	unsigned long flags;
>>>>>>>>  	int ret;
>>>>>>>>  
>>>>>>>> +	args = (struct fastrpc_invoke_args *)inv2->inv.args;
>>>>>>> Why does it need a typecast?
>>>>>>>
>>>>>>>>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>>>>>>>  	if (!ctx)
>>>>>>>>  		return ERR_PTR(-ENOMEM);
>>>>>>>> @@ -611,6 +613,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>>>>>  	/* Released in fastrpc_context_put() */
>>>>>>>>  	fastrpc_channel_ctx_get(cctx);
>>>>>>>>  
>>>>>>>> +	ctx->crc = (u32 *)(uintptr_t)inv2->crc;
>>>>>>> Oh, but why? Also is it a user pointer or in-kernel data? If it's a
>>>>>>> user-based pointer, where is the accessiblity check? Why isn't it
>>>>>>> annotated properly?
>>>>>> This is a user pointer where the crc data is expected to be copied. There is no
>>>>>> other access to this pointer from kernel. I'm planning to change the data type
>>>>>> for crc as (void __user*) inside fastrpc_invoke_ctx structure.
>>>>> Yes, please. Also make sure that sparse doesn't add any warnings
>>>>> regarding pointer conversions.
>>>> Ack.
>>>>>>>>  	ctx->sc = sc;
>>>>>>>>  	ctx->retval = -1;
>>>>>>>>  	ctx->pid = current->pid;
>>>>>>>> @@ -1070,6 +1073,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>>>  	struct fastrpc_invoke_buf *list;
>>>>>>>>  	struct fastrpc_phy_page *pages;
>>>>>>>>  	u64 *fdlist;
>>>>>>>> +	u32 *crclist;
>>>>>>>>  	int i, inbufs, outbufs, handles;
>>>>>>>>  
>>>>>>>>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
>>>>>>>> @@ -1078,6 +1082,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>>>  	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>>>>>>>>  	pages = fastrpc_phy_page_start(list, ctx->nscalars);
>>>>>>>>  	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>>>>>>>> +	crclist = (u32 *)(fdlist + FASTRPC_MAX_FDLIST);
>>>>>>> I think we should rewrite this parsing somehow. Is the format of data
>>>>>>> documented somewhere?
>>>>>> fdlist, crclist and poll(planned) are the only pointers that is being used. I'm planning
>>>>>> to store these pointers to ctx structure and directly use it wherever needed. This will
>>>>>> clean-up this unnecessary calculations at multiple places.
>>>>> Please do. Nevertheless, the format also must be documented.
>>>> Ack.
>>>>>>>>  
>>>>>>>>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>>>>>>>>  		if (!ctx->maps[i]) {
>>>>>>>> @@ -1102,6 +1107,12 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>>>>>>>>  			fastrpc_map_put(mmap);
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>> +	if (ctx->crc && crclist && rpra) {
>>>>>>>> +		if (copy_to_user((void __user *)ctx->crc, crclist,
>>>>>>>> +				FASTRPC_MAX_CRCLIST * sizeof(u32)))
>>>>>>> Oh, so it's a user pointer. Then u32* was completely incorrect.
>>>>>>> Also you are copying FASTRPC_MAX_CRCLIST elements. Are all of them
>>>>>>> filled? Or are we leaking some data to userspace?
>>>>>> Yes, right. Planning clean-up in next patch.
>>>>>>
>>>>>> All of FASTRPC_MAX_CRCLIST is filled with crc data by DSP so copying should be fine.
>>>>> Huh? I definitely want to see documentation for function arguments.
>>>> Sure. I'll also modify the metadata layout doc here to add fdlist, CRC and other planned contents.
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n842
>>> This is not a documentation. E.g. I can not write code using that
>>> description. For example, it mentions neither FDLIST nor CRC.
>> I'm planning to add complete documentation for CRC and polling mode in user library project.
>> If I need to add documentation in driver, can you pls suggest what is the right place to add
>> the information?
> Library should be fine. We don't require documenting all hardware in the
> kernel. But the uAPI of the driver should be properly described.
Understood, thanks.

--ekansh
>
>> Thanks for your comments.
>>
>> --ekansh
>>>>>>>> +			return -EFAULT;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>  	return 0;
>>>>>>>>  }
>>>>>>>>