This patch series adds the listed features that have been missing
in upstream fastRPC driver.
- Add missing bug fixes.
- Add static PD restart support for audio and sensors PD using
PDR framework.
- Redesign and improve remote heap management.
- Add fixes for unsigned PD. Unsigned PD can be enabled
using userspace API:
https://git.codelinaro.org/linaro/qcomlt/fastrpc/-/blob/master/src/fastrpc_apps_user.c?ref_type=heads#L1173
Changes in v2:
- Added separate patch to add newlines in dev_err.
- Added a bug fix in fastrpc capability function.
- Added a new patch to save and restore interrupted context.
- Fixed config dependency for PDR support.
Changes in v3:
- Dropped interrupted context patch.
- Splitted few of the bug fix patches.
- Added Fixes tag wherever applicable.
- Updated proper commit message for few of the patches.
Changes in v4:
- Dropped untrusted process and system unsigned PD patches.
- Updated proper commit message for few of the patches.
- Splitted patches in more meaningful way.
- Added helped functions for fastrpc_req_mmap.
Ekansh Gupta (11):
misc: fastrpc: Add missing dev_err newlines
misc: fastrpc: Fix DSP capabilities request
misc: fastrpc: Copy the complete capability structure to user
misc: fastrpc: Avoid updating PD type for capability request
misc: fastrpc: Add static PD restart support
misc: fastrpc: Fix memory leak in audio daemon attach operation
misc: fastrpc: Redesign remote heap management
misc: fastrpc: Fix ownership reassignment of remote heap
misc: fastrpc: Fix remote heap alloc and free user request
misc: fastrpc: Fix unsigned PD support
misc: fastrpc: Restrict untrusted app to attach to privileged PD
drivers/misc/Kconfig | 2 +
drivers/misc/fastrpc.c | 635 +++++++++++++++++++++++++++++-------
include/uapi/misc/fastrpc.h | 3 +
3 files changed, 527 insertions(+), 113 deletions(-)
--
2.43.0
Few dev_err calls are missing newlines. This can result in unrelated
lines getting appended which might make logs difficult to understand.
Add trailing newlines to avoid this.
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4c67e2c5a82e..4028cb96bcf2 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -325,7 +325,7 @@ static void fastrpc_free_map(struct kref *ref)
err = qcom_scm_assign_mem(map->phys, map->size,
&src_perms, &perm, 1);
if (err) {
- dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+ dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
map->phys, map->size, err);
return;
}
@@ -816,7 +816,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
map->attr = attr;
err = qcom_scm_assign_mem(map->phys, (u64)map->size, &src_perms, dst_perms, 2);
if (err) {
- dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+ dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
map->phys, map->size, err);
goto map_err;
}
@@ -1222,7 +1222,7 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
* that does not support unsigned PD offload
*/
if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
- dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD");
+ dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
return true;
}
}
@@ -1285,7 +1285,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
&src_perms,
fl->cctx->vmperms, fl->cctx->vmcount);
if (err) {
- dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+ dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
goto err_map;
}
@@ -1337,7 +1337,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
(u64)fl->cctx->remote_heap->size,
&src_perms, &dst_perms, 1);
if (err)
- dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+ dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
}
err_map:
--
2.43.0
User is passing capability ioctl structure(argp) to get DSP
capabilities. This argp is copied to a local structure to get domain
and attribute_id information. After getting the capability, only
capability value is getting copied to user argp which will not be
useful if the use is trying to get the capability by checking the
capability member of fastrpc_ioctl_capability structure. Add changes
to copy the complete capability structure so that user can get the
capability value from the expected member of the structure.
Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index abf7df7c0c85..f64781c3012f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1784,7 +1784,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
if (err)
return err;
- if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
+ if (copy_to_user(argp, &cap, sizeof(cap)))
return -EFAULT;
return 0;
--
2.43.0
Audio PD daemon will allocate memory for audio PD dynamic loading
usage when it is attaching for the first time to audio PD. As
part of this, the memory ownership is moved to the VM where
audio PD can use it. In case daemon process is killed without any
impact to DSP audio PD, the daemon process will retry to attach to
audio PD and in this case memory won't be reallocated. If the invoke
fails due to any reason, as part of err_invoke, the memory ownership
is getting reassigned to HLOS even when the memory was not allocated.
At this time the audio PD might still be using the memory and an
attemp of ownership reassignment would result in memory issue.
Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 3686b2d34741..68c1595446d5 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1334,6 +1334,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
u64 phys = 0, size = 0;
char *name;
int err;
+ bool scm_done = false;
struct {
int pgid;
u32 namelen;
@@ -1398,6 +1399,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
phys, size, err);
goto err_map;
}
+ scm_done = true;
}
}
@@ -1439,7 +1441,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
return 0;
err_invoke:
- if (fl->cctx->vmcount) {
+ if (fl->cctx->vmcount && scm_done) {
u64 src_perms = 0;
struct qcom_scm_vmperm dst_perms;
u32 i;
--
2.43.0
Unsigned PDs are sandboxed DSP processes used to offload computation
workloads to the DSP. Unsigned PD have less privileges in terms of
DSP resource access as compared to Signed PD.
Unsigned PD requires more initial memory to spawn. Also most of the
memory request are allocated from user space. Current initial memory
size is not sufficient and mapping request for user space allocated
buffer is not supported. This results in failure of unsigned PD offload
request. Add changes to fix initial memory size and user space allocated
buffer mapping support.
Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 180 ++++++++++++++++++++++++++++++-----------
1 file changed, 133 insertions(+), 47 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 32f2e6f625ed..5ffb6098ac38 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_CTXID_MASK (0xFF0)
-#define INIT_FILELEN_MAX (2 * 1024 * 1024)
+#define INIT_FILELEN_MAX (5 * 1024 * 1024)
#define INIT_FILE_NAMELEN_MAX (128)
#define FASTRPC_DEVICE_NAME "fastrpc"
@@ -327,6 +327,7 @@ struct fastrpc_user {
int tgid;
int pd;
bool is_secure_dev;
+ bool is_unsigned_pd;
char *servloc_name;
/* Lock for lists */
spinlock_t lock;
@@ -1488,7 +1489,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
u32 siglen;
} inbuf;
u32 sc;
- bool unsigned_module = false;
args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
if (!args)
@@ -1500,9 +1500,9 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
}
if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
- unsigned_module = true;
+ fl->is_unsigned_pd = true;
- if (is_session_rejected(fl, unsigned_module)) {
+ if (is_session_rejected(fl, fl->is_unsigned_pd)) {
err = -ECONNREFUSED;
goto err;
}
@@ -1985,6 +1985,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
{
struct fastrpc_buf *buf = NULL, *iter, *b;
struct fastrpc_req_munmap req;
+ struct fastrpc_map *map = NULL, *iterm, *m;
struct device *dev = fl->sctx->dev;
int err = 0;
@@ -2031,34 +2032,75 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
}
return err;
}
- dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
+ spin_lock(&fl->lock);
+ list_for_each_entry_safe(iterm, m, &fl->maps, node) {
+ if (iterm->raddr == req.vaddrout) {
+ map = iterm;
+ break;
+ }
+ }
+ spin_unlock(&fl->lock);
+ if (!map) {
+ dev_dbg(dev, "buffer not found addr 0x%09llx, len 0x%08llx\n",
req.vaddrout, req.size);
- return -EINVAL;
+ return -EINVAL;
+ }
+
+ err = fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
+ if (err)
+ dev_dbg(dev, "unmmap\tpt fd = %d, 0x%09llx error\n", map->fd, map->raddr);
+ else
+ fastrpc_map_put(map);
+
+ return err;
}
-static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
+static int fastrpc_req_map_dsp(struct fastrpc_user *fl, u64 phys,
+ u64 size, u32 flag, uintptr_t vaddrin, u64 *raddr)
{
struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
- struct fastrpc_buf *buf = NULL;
struct fastrpc_mmap_req_msg req_msg;
struct fastrpc_mmap_rsp_msg rsp_msg;
struct fastrpc_phy_page pages;
- 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;
+ req_msg.pgid = fl->tgid;
+ req_msg.flags = flag;
+ req_msg.vaddr = vaddrin;
+ req_msg.num = sizeof(pages);
- if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) {
- dev_err(dev, "flag not supported 0x%x\n", req.flags);
+ args[0].ptr = (u64) (uintptr_t) &req_msg;
+ args[0].length = sizeof(req_msg);
- return -EINVAL;
+ pages.addr = phys;
+ pages.size = size;
+
+ args[1].ptr = (u64) (uintptr_t) &pages;
+ args[1].length = sizeof(pages);
+ sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
+ err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
+ &args[0]);
+
+ if (err) {
+ dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size);
+ return err;
}
+ *raddr = rsp_msg.vaddr;
+
+ return err;
+}
+
+static int fastrpc_req_buf_alloc(struct fastrpc_user *fl,
+ struct fastrpc_req_mmap req, char __user *argp)
+{
+ struct device *dev = fl->sctx->dev;
+ struct fastrpc_buf *buf = NULL;
+ u64 raddr = 0;
+ int err;
if (req.vaddrin) {
- dev_err(dev, "adding user allocated pages is not supported\n");
+ dev_err(dev, "adding user allocated pages is not supported for signed PD\n");
return -EINVAL;
}
@@ -2091,36 +2133,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
}
}
- req_msg.pgid = fl->tgid;
- req_msg.flags = req.flags;
- req_msg.vaddr = req.vaddrin;
- req_msg.num = sizeof(pages);
-
- args[0].ptr = (u64) (uintptr_t) &req_msg;
- args[0].length = sizeof(req_msg);
-
- pages.addr = buf->phys;
- pages.size = buf->size;
-
- args[1].ptr = (u64) (uintptr_t) &pages;
- args[1].length = sizeof(pages);
-
- 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]);
- if (err) {
- dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
+ err = fastrpc_req_map_dsp(fl, buf->phys, buf->size, buf->flag,
+ req.vaddrin, &raddr);
+ if (err)
goto err_invoke;
- }
/* update the buffer to be able to deallocate the memory on the DSP */
- buf->raddr = (uintptr_t) rsp_msg.vaddr;
+ buf->raddr = (uintptr_t) raddr;
/* let the client know the address to use */
- req.vaddrout = rsp_msg.vaddr;
+ req.vaddrout = raddr;
spin_lock(&fl->lock);
if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
@@ -2129,16 +2151,14 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
list_add_tail(&buf->node, &fl->mmaps);
spin_unlock(&fl->lock);
+ dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
+ buf->raddr, buf->size);
+
if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
err = -EFAULT;
goto err_copy;
}
-
- dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
- buf->raddr, buf->size);
-
return 0;
-
err_copy:
spin_lock(&fl->lock);
list_del(&buf->node);
@@ -2146,11 +2166,77 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
fastrpc_req_munmap_impl(fl, buf);
buf = NULL;
err_invoke:
- fastrpc_buf_free(buf);
+ if (buf)
+ fastrpc_buf_free(buf);
+
+ return err;
+}
+
+static int fastrpc_req_map_create(struct fastrpc_user *fl,
+ struct fastrpc_req_mmap req, char __user *argp)
+{
+ struct fastrpc_map *map = NULL;
+ struct device *dev = fl->sctx->dev;
+ u64 raddr = 0;
+ int err;
+
+ if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->is_unsigned_pd) {
+ dev_err(dev, "secure memory allocation is not supported in unsigned PD\n");
+ return -EINVAL;
+ }
+ err = fastrpc_map_create(fl, req.fd, req.size, 0, &map);
+ if (err) {
+ dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
+ return err;
+ }
+
+ err = fastrpc_req_map_dsp(fl, map->phys, map->size, req.flags,
+ req.vaddrin, &raddr);
+ if (err)
+ goto err_invoke;
+
+ /* update the buffer to be able to deallocate the memory on the DSP */
+ map->raddr = (uintptr_t) raddr;
+
+ /* let the client know the address to use */
+ req.vaddrout = raddr;
+ dev_dbg(dev, "mmap\t\tpt 0x%09llx OK [len 0x%08llx]\n",
+ map->raddr, map->size);
+
+ if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
+ err = -EFAULT;
+ goto err_copy;
+ }
+ return 0;
+err_copy:
+ fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
+err_invoke:
+ fastrpc_map_put(map);
return err;
}
+static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
+{
+ struct fastrpc_req_mmap req;
+ int err;
+
+ if (copy_from_user(&req, argp, sizeof(req)))
+ return -EFAULT;
+
+ if ((req.flags == ADSP_MMAP_ADD_PAGES ||
+ req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
+ err = fastrpc_req_buf_alloc(fl, req, argp);
+ if (err)
+ return err;
+ } else {
+ err = fastrpc_req_map_create(fl, req, argp);
+ if (err)
+ return err;
+ }
+ return 0;
+}
+
static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
{
struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
--
2.43.0
Untrusted application with access to only non-secure fastrpc device
node can attach to root_pd or static PDs if it can make the respective
init request. This can cause problems as the untrusted application
can send bad requests to root_pd or static PDs. Add changes to reject
attach to privileged PDs if the request is being made using non-secure
fastrpc device node.
Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 22 +++++++++++++++++++---
include/uapi/misc/fastrpc.h | 3 +++
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 5ffb6098ac38..bd6ceb39ded1 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2368,6 +2368,16 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
return err;
}
+static int is_attach_rejected(struct fastrpc_user *fl)
+{
+ /* Check if the device node is non-secure */
+ if (!fl->is_secure_dev) {
+ dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+ return -EACCES;
+ }
+ return 0;
+}
+
static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -2380,13 +2390,19 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
err = fastrpc_invoke(fl, argp);
break;
case FASTRPC_IOCTL_INIT_ATTACH:
- err = fastrpc_init_attach(fl, ROOT_PD);
+ err = is_attach_rejected(fl);
+ if (!err)
+ err = fastrpc_init_attach(fl, ROOT_PD);
break;
case FASTRPC_IOCTL_INIT_ATTACH_SNS:
- err = fastrpc_init_attach(fl, SENSORS_PD);
+ err = is_attach_rejected(fl);
+ if (!err)
+ err = fastrpc_init_attach(fl, SENSORS_PD);
break;
case FASTRPC_IOCTL_INIT_CREATE_STATIC:
- err = fastrpc_init_create_static_process(fl, argp);
+ err = is_attach_rejected(fl);
+ if (!err)
+ err = fastrpc_init_create_static_process(fl, argp);
break;
case FASTRPC_IOCTL_INIT_CREATE:
err = fastrpc_init_create_process(fl, argp);
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index f33d914d8f46..91583690bddc 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -8,11 +8,14 @@
#define FASTRPC_IOCTL_ALLOC_DMA_BUFF _IOWR('R', 1, struct fastrpc_alloc_dma_buf)
#define FASTRPC_IOCTL_FREE_DMA_BUFF _IOWR('R', 2, __u32)
#define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_invoke)
+/* This ioctl is only supported with secure device nodes */
#define FASTRPC_IOCTL_INIT_ATTACH _IO('R', 4)
#define FASTRPC_IOCTL_INIT_CREATE _IOWR('R', 5, struct fastrpc_init_create)
#define FASTRPC_IOCTL_MMAP _IOWR('R', 6, struct fastrpc_req_mmap)
#define FASTRPC_IOCTL_MUNMAP _IOWR('R', 7, struct fastrpc_req_munmap)
+/* This ioctl is only supported with secure device nodes */
#define FASTRPC_IOCTL_INIT_ATTACH_SNS _IO('R', 8)
+/* This ioctl is only supported with secure device nodes */
#define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static)
#define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map)
#define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap)
--
2.43.0
A static PD daemon process can request for remote heap allocation
which will allocate memory and change the ownership if the VMID
information were defined. This allocations are currently getting
added to fl mmaps list which could get freed when there is any
daemon kill. There is no way to request for freeing of this memory
also. Add changes to maintain the remote heap allocation in the
static PD specific structure and add method to free the memory
on user request.
Fixes: 532ad70c6d44 ("misc: fastrpc: Add mmap request assigning for static PD pool")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 129 +++++++++++++++++++++++++++++++----------
1 file changed, 98 insertions(+), 31 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 68c1595446d5..32f2e6f625ed 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -210,6 +210,7 @@ struct fastrpc_buf {
struct dma_buf *dmabuf;
struct device *dev;
void *virt;
+ u32 flag;
u64 phys;
u64 size;
/* Lock for dma buf attachments */
@@ -1924,29 +1925,54 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
return 0;
}
-static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
+static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size)
{
struct fastrpc_invoke_args args[1] = { [0] = { 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;
- req_msg.vaddr = buf->raddr;
+ req_msg.size = size;
+ req_msg.vaddr = raddr;
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]);
+
+ return err;
+}
+
+static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
+{
+ struct device *dev = fl->sctx->dev;
+ int err;
+
+ err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size);
if (!err) {
+ if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+ if (fl->cctx->vmcount) {
+ u64 src_perms = 0;
+ struct qcom_scm_vmperm dst_perms;
+ u32 i;
+
+ for (i = 0; i < fl->cctx->vmcount; i++)
+ src_perms |= BIT(fl->cctx->vmperms[i].vmid);
+
+ dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+ dst_perms.perm = QCOM_SCM_PERM_RWX;
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, &dst_perms, 1);
+ if (err) {
+ dev_err(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ buf->phys, buf->size, err);
+ return err;
+ }
+ }
+ }
dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
- spin_lock(&fl->lock);
- list_del(&buf->node);
- spin_unlock(&fl->lock);
fastrpc_buf_free(buf);
} else {
dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
@@ -1960,6 +1986,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
struct fastrpc_buf *buf = NULL, *iter, *b;
struct fastrpc_req_munmap req;
struct device *dev = fl->sctx->dev;
+ int err = 0;
if (copy_from_user(&req, argp, sizeof(req)))
return -EFAULT;
@@ -1968,18 +1995,45 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
buf = iter;
+ list_del(&buf->node);
break;
}
}
spin_unlock(&fl->lock);
- if (!buf) {
- dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
- req.vaddrout, req.size);
- return -EINVAL;
+ if (buf) {
+ err = fastrpc_req_munmap_impl(fl, buf);
+ if (err) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &fl->mmaps);
+ spin_unlock(&fl->lock);
+ }
+ return err;
}
- return fastrpc_req_munmap_impl(fl, buf);
+ spin_lock(&fl->lock);
+ if (fl->spd) {
+ list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) {
+ if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
+ buf = iter;
+ list_del(&buf->node);
+ break;
+ }
+ }
+ }
+ spin_unlock(&fl->lock);
+ if (buf) {
+ err = fastrpc_req_munmap_impl(fl, buf);
+ if (err) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &fl->spd->rmaps);
+ spin_unlock(&fl->lock);
+ }
+ return err;
+ }
+ dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
+ req.vaddrout, req.size);
+ return -EINVAL;
}
static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
@@ -2008,15 +2062,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
return -EINVAL;
}
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
+ if (!fl->spd || !fl->spd->ispdup) {
+ dev_err(dev, "remote heap request supported only for active static PD\n");
+ return -EINVAL;
+ }
err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
- else
+ } else {
err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
+ }
if (err) {
dev_err(dev, "failed to allocate buffer\n");
return err;
}
+ buf->flag = req.flags;
+
+ /* Add memory to static PD pool, protection through hypervisor */
+ if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
+ u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
+ if (err) {
+ dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ buf->phys, buf->size, err);
+ goto err_invoke;
+ }
+ }
req_msg.pgid = fl->tgid;
req_msg.flags = req.flags;
@@ -2049,26 +2122,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
/* let the client know the address to use */
req.vaddrout = rsp_msg.vaddr;
- /* Add memory to static PD pool, protection thru hypervisor */
- if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
- u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
-
- err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
- &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
- if (err) {
- dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
- buf->phys, buf->size, err);
- goto err_assign;
- }
- }
-
spin_lock(&fl->lock);
- list_add_tail(&buf->node, &fl->mmaps);
+ if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
+ list_add_tail(&buf->node, &fl->spd->rmaps);
+ else
+ list_add_tail(&buf->node, &fl->mmaps);
spin_unlock(&fl->lock);
if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
err = -EFAULT;
- goto err_assign;
+ goto err_copy;
}
dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
@@ -2076,8 +2139,12 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
return 0;
-err_assign:
+err_copy:
+ spin_lock(&fl->lock);
+ list_del(&buf->node);
+ spin_unlock(&fl->lock);
fastrpc_req_munmap_impl(fl, buf);
+ buf = NULL;
err_invoke:
fastrpc_buf_free(buf);
--
2.43.0
Static PDs are created on DSPs to support specific use cases like Audio
and Sensors. The static PDs uses any CPU requirements like file
operations or memory need with the help of a daemon running on the CPU.
Audio and sensors daemons attaches to audio PD and sensors PD on DSP.
Audio PD expects some CMA memory for dynamic loading purpose which is
allocated and sent to DSP in fastrpc_init_create_static_process call.
For sensor daemon, the expectation is just to attach to sensors PD and
take up any requests made by the PD(like file operations etc.).
Static PDs run on the audio and sensor supporting subsystem which can
be ADSP or SDSP. They are expected to support PD restart. There are some
CPU resources like buffers etc. for static PDs which are expected to be
cleaned up by fastrpc driver during PDR scenario. For this, there is a
requirement of PD service locator to get the event notifications for
static PD services. Also when events are received, the driver needs to
handle based on PD states.
PDR handling is required for static PD only. There are no static PD
supported on MDSP or CDSP hence no PDR handling is required. PDR is also
not required for root_pd as if root_pd is shutting down, that basically
suggests that the remoteproc itself is shutting down which is handled
with rpmsg functionalities(probe and remove).
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/Kconfig | 2 +
drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 195 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index faf983680040..e2d83cd085b5 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -280,8 +280,10 @@ config QCOM_FASTRPC
tristate "Qualcomm FastRPC"
depends on ARCH_QCOM || COMPILE_TEST
depends on RPMSG
+ depends on NET
select DMA_SHARED_BUFFER
select QCOM_SCM
+ select QCOM_PDR_HELPERS
help
Provides a communication mechanism that allows for clients to
make remote method invocations across processor boundary to
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index abdd35b7c3ad..13e368279765 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -22,6 +22,7 @@
#include <linux/firmware/qcom/qcom_scm.h>
#include <uapi/misc/fastrpc.h>
#include <linux/of_reserved_mem.h>
+#include <linux/soc/qcom/pdr.h>
#define ADSP_DOMAIN_ID (0)
#define MDSP_DOMAIN_ID (1)
@@ -29,6 +30,7 @@
#define CDSP_DOMAIN_ID (3)
#define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
#define FASTRPC_MAX_SESSIONS 14
+#define FASTRPC_MAX_SPD 4
#define FASTRPC_MAX_VMIDS 16
#define FASTRPC_ALIGN 128
#define FASTRPC_MAX_FDLIST 16
@@ -105,6 +107,18 @@
#define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
+#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
+#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
+#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
+
+#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
+#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
+#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
+
+#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
+#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
+#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
+
static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
"sdsp", "cdsp"};
struct fastrpc_phy_page {
@@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
bool valid;
};
+struct fastrpc_static_pd {
+ char *servloc_name;
+ char *spdname;
+ void *pdrhandle;
+ struct fastrpc_channel_ctx *cctx;
+ struct fastrpc_user *fl;
+ bool ispdup;
+};
+
struct fastrpc_channel_ctx {
int domain_id;
int sesscount;
@@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
struct rpmsg_device *rpdev;
struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
+ struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
spinlock_t lock;
struct idr ctx_idr;
struct list_head users;
@@ -297,10 +321,12 @@ struct fastrpc_user {
struct fastrpc_channel_ctx *cctx;
struct fastrpc_session_ctx *sctx;
struct fastrpc_buf *init_mem;
+ struct fastrpc_static_pd *spd;
int tgid;
int pd;
bool is_secure_dev;
+ char *servloc_name;
/* Lock for lists */
spinlock_t lock;
/* lock for allocations */
@@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
return false;
}
+static struct fastrpc_static_pd *fastrpc_get_spd_session(
+ struct fastrpc_user *fl)
+{
+ int i;
+ struct fastrpc_static_pd *spd = NULL;
+ struct fastrpc_channel_ctx *cctx = fl->cctx;
+
+ for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
+ if (!cctx->spd[i].servloc_name)
+ continue;
+ if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
+ spd = &cctx->spd[i];
+ spd->fl = fl;
+ break;
+ }
+ }
+
+ return spd;
+}
+
static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
char __user *argp)
{
struct fastrpc_init_create_static init;
struct fastrpc_invoke_args *args;
struct fastrpc_phy_page pages[1];
+ struct fastrpc_static_pd *spd = NULL;
char *name;
int err;
struct {
@@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_name;
}
+ fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
+
+ spd = fastrpc_get_spd_session(fl);
+ if (!spd) {
+ err = -EUSERS;
+ goto err_name;
+ }
+
+ if (!spd->ispdup) {
+ err = -ENOTCONN;
+ goto err_name;
+ }
+ fl->spd = spd;
if (!fl->cctx->remote_heap) {
err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
&fl->cctx->remote_heap);
@@ -1645,6 +1705,7 @@ 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_static_pd *spd = NULL;
int tgid = fl->tgid;
u32 sc;
@@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
fl->pd = pd;
+ if (pd == SENSORS_PD) {
+ if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
+ fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
+ else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
+ fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
+
+ spd = fastrpc_get_spd_session(fl);
+ if (!spd)
+ return -EUSERS;
+
+ if (!spd->ispdup)
+ return -ENOTCONN;
+
+ fl->spd = spd;
+ }
+
return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
sc, &args[0]);
}
@@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
return err;
}
+static void fastrpc_notify_users(struct fastrpc_user *user)
+{
+ struct fastrpc_invoke_ctx *ctx;
+
+ spin_lock(&user->lock);
+ list_for_each_entry(ctx, &user->pending, node) {
+ ctx->retval = -EPIPE;
+ complete(&ctx->work);
+ }
+ spin_unlock(&user->lock);
+}
+
+static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
+ char *servloc_name)
+{
+ struct fastrpc_user *fl;
+ unsigned long flags;
+
+ spin_lock_irqsave(&cctx->lock, flags);
+ list_for_each_entry(fl, &cctx->users, user) {
+ if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
+ fastrpc_notify_users(fl);
+ }
+ spin_unlock_irqrestore(&cctx->lock, flags);
+}
+
+static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
+{
+ struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
+ struct fastrpc_channel_ctx *cctx;
+
+ if (!spd)
+ return;
+
+ cctx = spd->cctx;
+ switch (state) {
+ case SERVREG_SERVICE_STATE_DOWN:
+ dev_info(&cctx->rpdev->dev,
+ "%s: %s (%s) is down for PDR on %s\n",
+ __func__, spd->spdname,
+ spd->servloc_name,
+ domains[cctx->domain_id]);
+ spd->ispdup = false;
+ fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
+ break;
+ case SERVREG_SERVICE_STATE_UP:
+ dev_info(&cctx->rpdev->dev,
+ "%s: %s (%s) is up for PDR on %s\n",
+ __func__, spd->spdname,
+ spd->servloc_name,
+ domains[cctx->domain_id]);
+ spd->ispdup = true;
+ break;
+ default:
+ break;
+ }
+}
+
static const struct file_operations fastrpc_fops = {
.open = fastrpc_device_open,
.release = fastrpc_device_release,
@@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
return err;
}
+static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
+ char *service_name, char *service_path, int domain, int spd_session)
+{
+ int err = 0;
+ struct pdr_handle *handle = NULL;
+ struct pdr_service *service = NULL;
+
+ /* Register the service locator's callback function */
+ handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
+ if (IS_ERR(handle)) {
+ err = PTR_ERR(handle);
+ goto bail;
+ }
+ cctx->spd[spd_session].pdrhandle = handle;
+ cctx->spd[spd_session].servloc_name = client_name;
+ cctx->spd[spd_session].spdname = service_path;
+ cctx->spd[spd_session].cctx = cctx;
+ service = pdr_add_lookup(handle, service_name, service_path);
+ if (IS_ERR(service)) {
+ err = PTR_ERR(service);
+ goto bail;
+ }
+ pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
+ __func__, service_name, client_name, service_path);
+
+bail:
+ if (err) {
+ pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
+ __func__, service_name, client_name, service_path, err);
+ }
+ return err;
+}
+
static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
{
struct device *rdev = &rpdev->dev;
@@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
goto fdev_error;
}
+ if (domain_id == ADSP_DOMAIN_ID) {
+ err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
+ AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
+ if (err)
+ goto populate_error;
+
+ err = fastrpc_setup_service_locator(data,
+ SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
+ SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
+ if (err)
+ goto populate_error;
+ } else if (domain_id == SDSP_DOMAIN_ID) {
+ err = fastrpc_setup_service_locator(data,
+ SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
+ SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
+ if (err)
+ goto populate_error;
+ }
+
kref_init(&data->refcount);
dev_set_drvdata(&rpdev->dev, data);
@@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
return err;
}
-static void fastrpc_notify_users(struct fastrpc_user *user)
-{
- struct fastrpc_invoke_ctx *ctx;
-
- spin_lock(&user->lock);
- list_for_each_entry(ctx, &user->pending, node) {
- ctx->retval = -EPIPE;
- complete(&ctx->work);
- }
- spin_unlock(&user->lock);
-}
-
static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
{
struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
struct fastrpc_buf *buf, *b;
struct fastrpc_user *user;
unsigned long flags;
+ int i;
/* No invocations past this point */
spin_lock_irqsave(&cctx->lock, flags);
@@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
if (cctx->remote_heap)
fastrpc_buf_free(cctx->remote_heap);
+ for (i = 0; i < FASTRPC_MAX_SPD; i++) {
+ if (cctx->spd[i].pdrhandle)
+ pdr_handle_release(cctx->spd[i].pdrhandle);
+ }
+
of_platform_depopulate(&rpdev->dev);
fastrpc_channel_ctx_put(cctx);
--
2.43.0
Audio PD daemon send the name as part of the init IOCTL call. This
mane needs to be copied to kernel for which memory is allocated.
This memory is never freed which might result in memory leak. Add
changes to free the memory when it is not needed.
Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 13e368279765..7ee8bb3a9a6f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1380,6 +1380,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_invoke;
kfree(args);
+ kfree(name);
return 0;
err_invoke:
--
2.43.0
When user is requesting for DSP capability, the process pd type is
getting updated to USER_PD which is incorrect as DSP will assume the
process which is making the request is a user PD and this will never
get updated back to the original value. The actual PD type should not
be updated for capability request and it should be serviced by the
respective PD on DSP side. Adding changes to avoid process PD type
modification.
Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f64781c3012f..abdd35b7c3ad 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1703,7 +1703,6 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
args[1].length = dsp_attr_buf_len * sizeof(u32);
args[1].fd = -1;
- fl->pd = USER_PD;
return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
FASTRPC_SCALARS(0, 1, 1), args);
--
2.43.0
Current remote heap design is adding the memory to channel context
but here is also a possibility of audio daemon requesting new remote
heap memory using map ioctl. For this purpose, it's much easier to
maintain these types of static PD specific remote heap allocations
as a list. This remote heap memory is only getting freed during
rpmsg remove but it is also needed to be freed when static PD is
shutting down as this memory will no longed be used by static PDs.
For daemon kill cases where audio PD is still alive, the init IOCTL
will again take the same address and size to DSP which DSP would try
to map again which would result in mapping failure the PD might
already be using the memory. In Daemon kill cases, the address and
size is needed to be sent as zero and DSP will skip mapping in this
case. Add changes to manage remote heap in a way that it can be added
and removed as per needed and the information is sent as zero in daemon
kill cases.
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 94 +++++++++++++++++++++++++++++++++---------
1 file changed, 75 insertions(+), 19 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7ee8bb3a9a6f..3686b2d34741 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -274,6 +274,7 @@ struct fastrpc_session_ctx {
};
struct fastrpc_static_pd {
+ struct list_head rmaps;
char *servloc_name;
char *spdname;
void *pdrhandle;
@@ -299,7 +300,6 @@ struct fastrpc_channel_ctx {
u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
struct fastrpc_device *secure_fdevice;
struct fastrpc_device *fdevice;
- struct fastrpc_buf *remote_heap;
struct list_head invoke_interrupted_mmaps;
bool secure;
bool unsigned_support;
@@ -1256,6 +1256,53 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
return false;
}
+static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
+{
+ struct fastrpc_buf *buf, *b;
+ struct fastrpc_channel_ctx *cctx;
+ int err;
+
+ if (!spd || !spd->fl)
+ return;
+
+ cctx = spd->cctx;
+
+ spin_lock(&spd->fl->lock);
+ list_for_each_entry_safe(buf, b, &spd->rmaps, node) {
+ list_del(&buf->node);
+ spin_unlock(&spd->fl->lock);
+ if (cctx->vmcount) {
+ u64 src_perms = 0;
+ struct qcom_scm_vmperm dst_perms;
+ u32 i;
+
+ for (i = 0; i < cctx->vmcount; i++)
+ src_perms |= BIT(cctx->vmperms[i].vmid);
+
+ dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+ dst_perms.perm = QCOM_SCM_PERM_RWX;
+ err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
+ &src_perms, &dst_perms, 1);
+ if (err) {
+ pr_err("%s: Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
+ __func__, buf->phys, buf->size, err);
+ return;
+ }
+ }
+ fastrpc_buf_free(buf);
+ spin_lock(&spd->fl->lock);
+ }
+ spin_unlock(&spd->fl->lock);
+}
+
+static void fastrpc_mmap_remove_ssr(struct fastrpc_channel_ctx *cctx)
+{
+ int i;
+
+ for (i = 0; i < FASTRPC_MAX_SPD; i++)
+ fastrpc_mmap_remove_pdr(&cctx->spd[i]);
+}
+
static struct fastrpc_static_pd *fastrpc_get_spd_session(
struct fastrpc_user *fl)
{
@@ -1282,7 +1329,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
struct fastrpc_init_create_static init;
struct fastrpc_invoke_args *args;
struct fastrpc_phy_page pages[1];
+ struct fastrpc_buf *buf = NULL;
struct fastrpc_static_pd *spd = NULL;
+ u64 phys = 0, size = 0;
char *name;
int err;
struct {
@@ -1330,23 +1379,23 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
goto err_name;
}
fl->spd = spd;
- if (!fl->cctx->remote_heap) {
- err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
- &fl->cctx->remote_heap);
+ if (list_empty(&spd->rmaps)) {
+ err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf);
if (err)
goto err_name;
+ phys = buf->phys;
+ size = buf->size;
/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
if (fl->cctx->vmcount) {
u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
- err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
- (u64)fl->cctx->remote_heap->size,
- &src_perms,
- fl->cctx->vmperms, fl->cctx->vmcount);
+ err = qcom_scm_assign_mem(phys, (u64)size,
+ &src_perms,
+ fl->cctx->vmperms, fl->cctx->vmcount);
if (err) {
dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
- fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+ phys, size, err);
goto err_map;
}
}
@@ -1365,8 +1414,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
args[1].length = inbuf.namelen;
args[1].fd = -1;
- pages[0].addr = fl->cctx->remote_heap->phys;
- pages[0].size = fl->cctx->remote_heap->size;
+ pages[0].addr = phys;
+ pages[0].size = size;
args[2].ptr = (u64)(uintptr_t) pages;
args[2].length = sizeof(*pages);
@@ -1382,6 +1431,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
kfree(args);
kfree(name);
+ if (buf) {
+ spin_lock(&fl->lock);
+ list_add_tail(&buf->node, &spd->rmaps);
+ spin_unlock(&fl->lock);
+ }
+
return 0;
err_invoke:
if (fl->cctx->vmcount) {
@@ -1394,15 +1449,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
dst_perms.vmid = QCOM_SCM_VMID_HLOS;
dst_perms.perm = QCOM_SCM_PERM_RWX;
- err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
- (u64)fl->cctx->remote_heap->size,
- &src_perms, &dst_perms, 1);
+ err = qcom_scm_assign_mem(phys, (u64)size,
+ &src_perms, &dst_perms, 1);
if (err)
dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
- fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
+ phys, size, err);
}
err_map:
- fastrpc_buf_free(fl->cctx->remote_heap);
+ if (buf)
+ fastrpc_buf_free(buf);
err_name:
kfree(name);
err:
@@ -2250,6 +2305,7 @@ static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
spd->servloc_name,
domains[cctx->domain_id]);
spd->ispdup = false;
+ fastrpc_mmap_remove_pdr(spd);
fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
break;
case SERVREG_SERVICE_STATE_UP:
@@ -2401,6 +2457,7 @@ static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char
cctx->spd[spd_session].servloc_name = client_name;
cctx->spd[spd_session].spdname = service_path;
cctx->spd[spd_session].cctx = cctx;
+ INIT_LIST_HEAD(&cctx->spd[spd_session].rmaps);
service = pdr_add_lookup(handle, service_name, service_path);
if (IS_ERR(service)) {
err = PTR_ERR(service);
@@ -2567,9 +2624,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
list_del(&buf->node);
- if (cctx->remote_heap)
- fastrpc_buf_free(cctx->remote_heap);
-
for (i = 0; i < FASTRPC_MAX_SPD; i++) {
if (cctx->spd[i].pdrhandle)
pdr_handle_release(cctx->spd[i].pdrhandle);
@@ -2577,6 +2631,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
of_platform_depopulate(&rpdev->dev);
+ fastrpc_mmap_remove_ssr(cctx);
+
fastrpc_channel_ctx_put(cctx);
}
--
2.43.0
The DSP capability request call expects 2 arguments. First is the
information about the total number of attributes to be copied from
DSP and second is the information about the buffer where the DSP
needs to copy the information. The current design is passing the
information about te size to be copied from DSP which would be
considered as a bad argument to the call by DSP causing a failure
suggesting the same. The second argument carries the information
about the buffer where the DSP needs to copy the capability
information and the size to be copied. As the first entry of
capability attribute is getting skipped, same should also be
considered while sending the information to DSP. Add changes to
pass proper arguments to DSP.
Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
drivers/misc/fastrpc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4028cb96bcf2..abf7df7c0c85 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1695,12 +1695,13 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
/* Capability filled in userspace */
dsp_attr_buf[0] = 0;
+ dsp_attr_buf_len -= 1;
args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
args[0].length = sizeof(dsp_attr_buf_len);
args[0].fd = -1;
args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
- args[1].length = dsp_attr_buf_len;
+ args[1].length = dsp_attr_buf_len * sizeof(u32);
args[1].fd = -1;
fl->pd = USER_PD;
@@ -1730,7 +1731,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
if (!dsp_attributes)
return -ENOMEM;
- err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
+ err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
if (err == DSP_UNSUPPORTED_API) {
dev_info(&cctx->rpdev->dev,
"Warning: DSP capabilities not supported on domain: %d\n", domain);
--
2.43.0
On Thu, Jun 06, 2024 at 10:29:22PM +0530, Ekansh Gupta wrote:
> The DSP capability request call expects 2 arguments. First is the
> information about the total number of attributes to be copied from
> DSP and second is the information about the buffer where the DSP
> needs to copy the information. The current design is passing the
> information about te size to be copied from DSP which would be
> considered as a bad argument to the call by DSP causing a failure
> suggesting the same. The second argument carries the information
> about the buffer where the DSP needs to copy the capability
> information and the size to be copied. As the first entry of
> capability attribute is getting skipped, same should also be
> considered while sending the information to DSP. Add changes to
> pass proper arguments to DSP.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:23PM +0530, Ekansh Gupta wrote:
> User is passing capability ioctl structure(argp) to get DSP
> capabilities. This argp is copied to a local structure to get domain
> and attribute_id information. After getting the capability, only
> capability value is getting copied to user argp which will not be
> useful if the use is trying to get the capability by checking the
> capability member of fastrpc_ioctl_capability structure. Add changes
> to copy the complete capability structure so that user can get the
> capability value from the expected member of the structure.
Nit: s/Add changes to copy/Copy/
Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:24PM +0530, Ekansh Gupta wrote:
> When user is requesting for DSP capability, the process pd type is
> getting updated to USER_PD which is incorrect as DSP will assume the
> process which is making the request is a user PD and this will never
> get updated back to the original value. The actual PD type should not
> be updated for capability request and it should be serviced by the
> respective PD on DSP side. Adding changes to avoid process PD type
> modification.
Nit: Imperative style, please.
Something like: 'Don't change process's PD type for this request'
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f64781c3012f..abdd35b7c3ad 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1703,7 +1703,6 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> args[1].length = dsp_attr_buf_len * sizeof(u32);
> args[1].fd = -1;
> - fl->pd = USER_PD;
>
> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> FASTRPC_SCALARS(0, 1, 1), args);
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote:
> Static PDs are created on DSPs to support specific use cases like Audio
> and Sensors. The static PDs uses any CPU requirements like file
> operations or memory need with the help of a daemon running on the CPU.
What do you mean by 'CPU requirements' here?
> Audio and sensors daemons attaches to audio PD and sensors PD on DSP.
attach
> Audio PD expects some CMA memory for dynamic loading purpose which is
> allocated and sent to DSP in fastrpc_init_create_static_process call.
A pointer to audio daemon would be helpful. We don't have such a thing
in the normal Linux operation cycle.
> For sensor daemon, the expectation is just to attach to sensors PD and
> take up any requests made by the PD(like file operations etc.).
>
> Static PDs run on the audio and sensor supporting subsystem which can
> be ADSP or SDSP. They are expected to support PD restart. There are some
> CPU resources like buffers etc. for static PDs which are expected to be
> cleaned up by fastrpc driver during PDR scenario. For this, there is a
Why are they cleaned by fastrpc driver and not by the userspace daemon
itself? Userspace can also subscribe to PDR notifications and handle
restarts on its own.
> requirement of PD service locator to get the event notifications for
> static PD services. Also when events are received, the driver needs to
> handle based on PD states.
To handle what? And why?
>
> PDR handling is required for static PD only. There are no static PD
> supported on MDSP or CDSP hence no PDR handling is required. PDR is also
> not required for root_pd as if root_pd is shutting down, that basically
> suggests that the remoteproc itself is shutting down which is handled
> with rpmsg functionalities(probe and remove).
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/Kconfig | 2 +
> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 195 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index faf983680040..e2d83cd085b5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
> tristate "Qualcomm FastRPC"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on RPMSG
> + depends on NET
> select DMA_SHARED_BUFFER
> select QCOM_SCM
> + select QCOM_PDR_HELPERS
> help
> Provides a communication mechanism that allows for clients to
> make remote method invocations across processor boundary to
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index abdd35b7c3ad..13e368279765 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -22,6 +22,7 @@
> #include <linux/firmware/qcom/qcom_scm.h>
> #include <uapi/misc/fastrpc.h>
> #include <linux/of_reserved_mem.h>
> +#include <linux/soc/qcom/pdr.h>
>
> #define ADSP_DOMAIN_ID (0)
> #define MDSP_DOMAIN_ID (1)
> @@ -29,6 +30,7 @@
> #define CDSP_DOMAIN_ID (3)
> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
> #define FASTRPC_MAX_SESSIONS 14
> +#define FASTRPC_MAX_SPD 4
> #define FASTRPC_MAX_VMIDS 16
> #define FASTRPC_ALIGN 128
> #define FASTRPC_MAX_FDLIST 16
> @@ -105,6 +107,18 @@
>
> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>
> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
> +
> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
> +
> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
> +
> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> "sdsp", "cdsp"};
> struct fastrpc_phy_page {
> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
> bool valid;
> };
>
> +struct fastrpc_static_pd {
> + char *servloc_name;
> + char *spdname;
> + void *pdrhandle;
> + struct fastrpc_channel_ctx *cctx;
> + struct fastrpc_user *fl;
> + bool ispdup;
is_pd_up
> +};
> +
> struct fastrpc_channel_ctx {
> int domain_id;
> int sesscount;
> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
> struct rpmsg_device *rpdev;
> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
> spinlock_t lock;
> struct idr ctx_idr;
> struct list_head users;
> @@ -297,10 +321,12 @@ struct fastrpc_user {
> struct fastrpc_channel_ctx *cctx;
> struct fastrpc_session_ctx *sctx;
> struct fastrpc_buf *init_mem;
> + struct fastrpc_static_pd *spd;
>
> int tgid;
> int pd;
> bool is_secure_dev;
> + char *servloc_name;
> /* Lock for lists */
> spinlock_t lock;
> /* lock for allocations */
> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
> + struct fastrpc_user *fl)
Single line, please
> +{
> + int i;
> + struct fastrpc_static_pd *spd = NULL;
> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
> + if (!cctx->spd[i].servloc_name)
> + continue;
> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
> + spd = &cctx->spd[i];
> + spd->fl = fl;
> + break;
> + }
Should there be a protection for spd->fl being overwritten / overtaken?
How should it be handled by the driver?
> + }
> +
> + return spd;
> +}
> +
> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> char __user *argp)
> {
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_static_pd *spd = NULL;
> char *name;
> int err;
> struct {
> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
>
> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd) {
> + err = -EUSERS;
"Too many users" ?
> + goto err_name;
> + }
> +
> + if (!spd->ispdup) {
This is racy. What if the domain restarts right after this line?
> + err = -ENOTCONN;
> + goto err_name;
> + }
> + fl->spd = spd;
> if (!fl->cctx->remote_heap) {
> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> &fl->cctx->remote_heap);
> @@ -1645,6 +1705,7 @@ 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_static_pd *spd = NULL;
> int tgid = fl->tgid;
> u32 sc;
>
> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> fl->pd = pd;
>
> + if (pd == SENSORS_PD) {
> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd)
> + return -EUSERS;
> +
> + if (!spd->ispdup)
> + return -ENOTCONN;
> +
> + fl->spd = spd;
> + }
> +
> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> sc, &args[0]);
> }
> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> return err;
> }
>
> +static void fastrpc_notify_users(struct fastrpc_user *user)
> +{
> + struct fastrpc_invoke_ctx *ctx;
> +
> + spin_lock(&user->lock);
> + list_for_each_entry(ctx, &user->pending, node) {
> + ctx->retval = -EPIPE;
> + complete(&ctx->work);
> + }
> + spin_unlock(&user->lock);
> +}
> +
> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
> + char *servloc_name)
> +{
> + struct fastrpc_user *fl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cctx->lock, flags);
> + list_for_each_entry(fl, &cctx->users, user) {
> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
> + fastrpc_notify_users(fl);
> + }
> + spin_unlock_irqrestore(&cctx->lock, flags);
> +}
> +
> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> +{
> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
> + struct fastrpc_channel_ctx *cctx;
> +
> + if (!spd)
> + return;
> +
> + cctx = spd->cctx;
> + switch (state) {
> + case SERVREG_SERVICE_STATE_DOWN:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is down for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
Too noisy. Is it really dev_info()? Or dev_warn? There should be no need
for __func__.
> + spd->ispdup = false;
> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> + break;
> + case SERVREG_SERVICE_STATE_UP:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is up for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
Same comment here.
> + spd->ispdup = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> static const struct file_operations fastrpc_fops = {
> .open = fastrpc_device_open,
> .release = fastrpc_device_release,
> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> return err;
> }
>
> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
> + char *service_name, char *service_path, int domain, int spd_session)
> +{
> + int err = 0;
> + struct pdr_handle *handle = NULL;
> + struct pdr_service *service = NULL;
> +
> + /* Register the service locator's callback function */
> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto bail;
> + }
> + cctx->spd[spd_session].pdrhandle = handle;
> + cctx->spd[spd_session].servloc_name = client_name;
> + cctx->spd[spd_session].spdname = service_path;
> + cctx->spd[spd_session].cctx = cctx;
> + service = pdr_add_lookup(handle, service_name, service_path);
> + if (IS_ERR(service)) {
> + err = PTR_ERR(service);
> + goto bail;
> + }
> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
> + __func__, service_name, client_name, service_path);
Definitely too noisy. Drop __func__ everywhere. Use dev_dbg instead.
> +
> +bail:
> + if (err) {
> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
> + __func__, service_name, client_name, service_path, err);
dev_warn, drop brackets, align properly. Did you run this through checkpatch.pl --strict?
> + }
> + return err;
> +}
> +
> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> {
> struct device *rdev = &rpdev->dev;
> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> goto fdev_error;
> }
>
> + if (domain_id == ADSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> +
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
> + if (err)
> + goto populate_error;
> + } else if (domain_id == SDSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> + }
> +
> kref_init(&data->refcount);
>
> dev_set_drvdata(&rpdev->dev, data);
> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> return err;
> }
>
> -static void fastrpc_notify_users(struct fastrpc_user *user)
> -{
> - struct fastrpc_invoke_ctx *ctx;
> -
> - spin_lock(&user->lock);
> - list_for_each_entry(ctx, &user->pending, node) {
> - ctx->retval = -EPIPE;
> - complete(&ctx->work);
> - }
> - spin_unlock(&user->lock);
> -}
> -
> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> {
> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> struct fastrpc_buf *buf, *b;
> struct fastrpc_user *user;
> unsigned long flags;
> + int i;
>
> /* No invocations past this point */
> spin_lock_irqsave(&cctx->lock, flags);
> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->remote_heap)
> fastrpc_buf_free(cctx->remote_heap);
>
> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> + if (cctx->spd[i].pdrhandle)
> + pdr_handle_release(cctx->spd[i].pdrhandle);
> + }
> +
> of_platform_depopulate(&rpdev->dev);
>
> fastrpc_channel_ctx_put(cctx);
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:26PM +0530, Ekansh Gupta wrote:
> Audio PD daemon send the name as part of the init IOCTL call. This
> mane needs to be copied to kernel for which memory is allocated.
> This memory is never freed which might result in memory leak. Add
> changes to free the memory when it is not needed.
>
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
Fixes go before the non-fixes patches.
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/misc/fastrpc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 13e368279765..7ee8bb3a9a6f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1380,6 +1380,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_invoke;
>
A comment that the remote_heap persists would be helpful.
> kfree(args);
> + kfree(name);
>
> return 0;
> err_invoke:
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:27PM +0530, Ekansh Gupta wrote:
> Current remote heap design is adding the memory to channel context
> but here is also a possibility of audio daemon requesting new remote
> heap memory using map ioctl. For this purpose, it's much easier to
> maintain these types of static PD specific remote heap allocations
> as a list. This remote heap memory is only getting freed during
> rpmsg remove but it is also needed to be freed when static PD is
> shutting down as this memory will no longed be used by static PDs.
> For daemon kill cases where audio PD is still alive, the init IOCTL
> will again take the same address and size to DSP which DSP would try
> to map again which would result in mapping failure the PD might
> already be using the memory. In Daemon kill cases, the address and
> size is needed to be sent as zero and DSP will skip mapping in this
> case.
Are these two sentences describing the same usecase or two different
cases?
> Add changes to manage remote heap in a way that it can be added
> and removed as per needed and the information is sent as zero in daemon
> kill cases.
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 94 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 75 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7ee8bb3a9a6f..3686b2d34741 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -274,6 +274,7 @@ struct fastrpc_session_ctx {
> };
>
> struct fastrpc_static_pd {
> + struct list_head rmaps;
> char *servloc_name;
> char *spdname;
> void *pdrhandle;
> @@ -299,7 +300,6 @@ struct fastrpc_channel_ctx {
> u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
> struct fastrpc_device *secure_fdevice;
> struct fastrpc_device *fdevice;
> - struct fastrpc_buf *remote_heap;
> struct list_head invoke_interrupted_mmaps;
> bool secure;
> bool unsigned_support;
> @@ -1256,6 +1256,53 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
This isn't removing pdr.
> +{
> + struct fastrpc_buf *buf, *b;
> + struct fastrpc_channel_ctx *cctx;
> + int err;
> +
> + if (!spd || !spd->fl)
> + return;
Any protection against concurrent spd->fl modification?
> +
> + cctx = spd->cctx;
> +
> + spin_lock(&spd->fl->lock);
> + list_for_each_entry_safe(buf, b, &spd->rmaps, node) {
> + list_del(&buf->node);
> + spin_unlock(&spd->fl->lock);
> + if (cctx->vmcount) {
> + u64 src_perms = 0;
> + struct qcom_scm_vmperm dst_perms;
> + u32 i;
> +
> + for (i = 0; i < cctx->vmcount; i++)
> + src_perms |= BIT(cctx->vmperms[i].vmid);
> +
> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> + dst_perms.perm = QCOM_SCM_PERM_RWX;
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, &dst_perms, 1);
> + if (err) {
> + pr_err("%s: Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + __func__, buf->phys, buf->size, err);
dev_err, no __func__
> + return;
> + }
> + }
> + fastrpc_buf_free(buf);
> + spin_lock(&spd->fl->lock);
> + }
> + spin_unlock(&spd->fl->lock);
> +}
> +
> +static void fastrpc_mmap_remove_ssr(struct fastrpc_channel_ctx *cctx)
SSR?
> +{
> + int i;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD; i++)
> + fastrpc_mmap_remove_pdr(&cctx->spd[i]);
> +}
> +
> static struct fastrpc_static_pd *fastrpc_get_spd_session(
> struct fastrpc_user *fl)
> {
> @@ -1282,7 +1329,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_buf *buf = NULL;
> struct fastrpc_static_pd *spd = NULL;
> + u64 phys = 0, size = 0;
> char *name;
> int err;
> struct {
> @@ -1330,23 +1379,23 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
> fl->spd = spd;
> - if (!fl->cctx->remote_heap) {
> - err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> - &fl->cctx->remote_heap);
> + if (list_empty(&spd->rmaps)) {
> + err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf);
> if (err)
> goto err_name;
>
> + phys = buf->phys;
> + size = buf->size;
> /* Map if we have any heap VMIDs associated with this ADSP Static Process. */
> if (fl->cctx->vmcount) {
> u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>
> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> - (u64)fl->cctx->remote_heap->size,
> - &src_perms,
> - fl->cctx->vmperms, fl->cctx->vmcount);
> + err = qcom_scm_assign_mem(phys, (u64)size,
> + &src_perms,
> + fl->cctx->vmperms, fl->cctx->vmcount);
> if (err) {
> dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> + phys, size, err);
> goto err_map;
> }
> }
> @@ -1365,8 +1414,8 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> args[1].length = inbuf.namelen;
> args[1].fd = -1;
>
> - pages[0].addr = fl->cctx->remote_heap->phys;
> - pages[0].size = fl->cctx->remote_heap->size;
> + pages[0].addr = phys;
> + pages[0].size = size;
>
> args[2].ptr = (u64)(uintptr_t) pages;
> args[2].length = sizeof(*pages);
> @@ -1382,6 +1431,12 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> kfree(args);
> kfree(name);
>
> + if (buf) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &spd->rmaps);
> + spin_unlock(&fl->lock);
> + }
> +
> return 0;
> err_invoke:
> if (fl->cctx->vmcount) {
> @@ -1394,15 +1449,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>
> dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> dst_perms.perm = QCOM_SCM_PERM_RWX;
> - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
> - (u64)fl->cctx->remote_heap->size,
> - &src_perms, &dst_perms, 1);
> + err = qcom_scm_assign_mem(phys, (u64)size,
Why do you need to convert to u64?
> + &src_perms, &dst_perms, 1);
Maybe it can fit into a single line?
> if (err)
> dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> + phys, size, err);
> }
> err_map:
> - fastrpc_buf_free(fl->cctx->remote_heap);
> + if (buf)
> + fastrpc_buf_free(buf);
> err_name:
> kfree(name);
> err:
> @@ -2250,6 +2305,7 @@ static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> spd->servloc_name,
> domains[cctx->domain_id]);
> spd->ispdup = false;
> + fastrpc_mmap_remove_pdr(spd);
> fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> break;
> case SERVREG_SERVICE_STATE_UP:
> @@ -2401,6 +2457,7 @@ static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char
> cctx->spd[spd_session].servloc_name = client_name;
> cctx->spd[spd_session].spdname = service_path;
> cctx->spd[spd_session].cctx = cctx;
> + INIT_LIST_HEAD(&cctx->spd[spd_session].rmaps);
> service = pdr_add_lookup(handle, service_name, service_path);
> if (IS_ERR(service)) {
> err = PTR_ERR(service);
> @@ -2567,9 +2624,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
> list_del(&buf->node);
>
> - if (cctx->remote_heap)
> - fastrpc_buf_free(cctx->remote_heap);
> -
> for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> if (cctx->spd[i].pdrhandle)
> pdr_handle_release(cctx->spd[i].pdrhandle);
> @@ -2577,6 +2631,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>
> of_platform_depopulate(&rpdev->dev);
>
> + fastrpc_mmap_remove_ssr(cctx);
> +
> fastrpc_channel_ctx_put(cctx);
> }
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:28PM +0530, Ekansh Gupta wrote:
> Audio PD daemon will allocate memory for audio PD dynamic loading
> usage when it is attaching for the first time to audio PD. As
> part of this, the memory ownership is moved to the VM where
> audio PD can use it. In case daemon process is killed without any
> impact to DSP audio PD, the daemon process will retry to attach to
> audio PD and in this case memory won't be reallocated. If the invoke
> fails due to any reason, as part of err_invoke, the memory ownership
> is getting reassigned to HLOS even when the memory was not allocated.
> At this time the audio PD might still be using the memory and an
> attemp of ownership reassignment would result in memory issue.
>
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Fixes before functional changes.
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 3686b2d34741..68c1595446d5 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1334,6 +1334,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> u64 phys = 0, size = 0;
> char *name;
> int err;
> + bool scm_done = false;
> struct {
> int pgid;
> u32 namelen;
> @@ -1398,6 +1399,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> phys, size, err);
> goto err_map;
> }
> + scm_done = true;
> }
> }
>
> @@ -1439,7 +1441,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>
> return 0;
> err_invoke:
> - if (fl->cctx->vmcount) {
> + if (fl->cctx->vmcount && scm_done) {
> u64 src_perms = 0;
> struct qcom_scm_vmperm dst_perms;
> u32 i;
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:29PM +0530, Ekansh Gupta wrote:
> A static PD daemon process can request for remote heap allocation
> which will allocate memory and change the ownership if the VMID
> information were defined. This allocations are currently getting
> added to fl mmaps list which could get freed when there is any
> daemon kill. There is no way to request for freeing of this memory
> also. Add changes to maintain the remote heap allocation in the
Simpler. 'Maintain foo'
> static PD specific structure and add method to free the memory
> on user request.
Should this be split into two patches? 'foo and bar' suggests that.
>
> Fixes: 532ad70c6d44 ("misc: fastrpc: Add mmap request assigning for static PD pool")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 129 +++++++++++++++++++++++++++++++----------
> 1 file changed, 98 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 68c1595446d5..32f2e6f625ed 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -210,6 +210,7 @@ struct fastrpc_buf {
> struct dma_buf *dmabuf;
> struct device *dev;
> void *virt;
> + u32 flag;
> u64 phys;
> u64 size;
> /* Lock for dma buf attachments */
> @@ -1924,29 +1925,54 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> return 0;
> }
>
> -static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
> +static int fastrpc_req_munmap_dsp(struct fastrpc_user *fl, uintptr_t raddr, u64 size)
> {
> struct fastrpc_invoke_args args[1] = { [0] = { 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;
> - req_msg.vaddr = buf->raddr;
> + req_msg.size = size;
> + req_msg.vaddr = raddr;
>
> 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]);
> +
> + return err;
> +}
> +
> +static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *buf)
> +{
> + struct device *dev = fl->sctx->dev;
> + int err;
> +
> + err = fastrpc_req_munmap_dsp(fl, buf->raddr, buf->size);
> if (!err) {
> + if (buf->flag == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> + if (fl->cctx->vmcount) {
> + u64 src_perms = 0;
> + struct qcom_scm_vmperm dst_perms;
> + u32 i;
> +
> + for (i = 0; i < fl->cctx->vmcount; i++)
> + src_perms |= BIT(fl->cctx->vmperms[i].vmid);
> +
> + dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> + dst_perms.perm = QCOM_SCM_PERM_RWX;
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, &dst_perms, 1);
> + if (err) {
> + dev_err(dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + buf->phys, buf->size, err);
> + return err;
> + }
It looks like this is too nested. Consider refactoring this code. For
example, extract the function that you have c&p'ed.
> + }
> + }
> dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
> - spin_lock(&fl->lock);
> - list_del(&buf->node);
> - spin_unlock(&fl->lock);
> fastrpc_buf_free(buf);
> } else {
> dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
> @@ -1960,6 +1986,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
> struct fastrpc_buf *buf = NULL, *iter, *b;
> struct fastrpc_req_munmap req;
> struct device *dev = fl->sctx->dev;
> + int err = 0;
Why =0 is necessary?
>
> if (copy_from_user(&req, argp, sizeof(req)))
> return -EFAULT;
> @@ -1968,18 +1995,45 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
> list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
> if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
> buf = iter;
> + list_del(&buf->node);
> break;
> }
> }
> spin_unlock(&fl->lock);
>
> - if (!buf) {
> - dev_err(dev, "mmap\t\tpt 0x%09llx [len 0x%08llx] not in list\n",
> - req.vaddrout, req.size);
> - return -EINVAL;
> + if (buf) {
> + err = fastrpc_req_munmap_impl(fl, buf);
> + if (err) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &fl->mmaps);
> + spin_unlock(&fl->lock);
> + }
> + return err;
> }
>
> - return fastrpc_req_munmap_impl(fl, buf);
> + spin_lock(&fl->lock);
> + if (fl->spd) {
> + list_for_each_entry_safe(iter, b, &fl->spd->rmaps, node) {
> + if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
> + buf = iter;
> + list_del(&buf->node);
> + break;
> + }
> + }
> + }
> + spin_unlock(&fl->lock);
> + if (buf) {
> + err = fastrpc_req_munmap_impl(fl, buf);
> + if (err) {
> + spin_lock(&fl->lock);
> + list_add_tail(&buf->node, &fl->spd->rmaps);
> + spin_unlock(&fl->lock);
> + }
> + return err;
> + }
> + dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
> + req.vaddrout, req.size);
Can this be triggered by the user? If so, it's dev_dbg() at best.
> + return -EINVAL;
> }
>
> static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> @@ -2008,15 +2062,34 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> return -EINVAL;
> }
>
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) {
> + if (!fl->spd || !fl->spd->ispdup) {
> + dev_err(dev, "remote heap request supported only for active static PD\n");
> + return -EINVAL;
> + }
> err = fastrpc_remote_heap_alloc(fl, dev, req.size, &buf);
> - else
> + } else {
> err = fastrpc_buf_alloc(fl, dev, req.size, &buf);
> + }
>
> if (err) {
> dev_err(dev, "failed to allocate buffer\n");
> return err;
> }
> + buf->flag = req.flags;
> +
> + /* Add memory to static PD pool, protection through hypervisor */
> + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->cctx->vmcount) {
> + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
> + err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> + &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> + if (err) {
> + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> + buf->phys, buf->size, err);
misaligned
> + goto err_invoke;
> + }
> + }
>
> req_msg.pgid = fl->tgid;
> req_msg.flags = req.flags;
> @@ -2049,26 +2122,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> /* let the client know the address to use */
> req.vaddrout = rsp_msg.vaddr;
>
> - /* Add memory to static PD pool, protection thru hypervisor */
> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
> - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> -
> - err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
> - if (err) {
> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
> - buf->phys, buf->size, err);
> - goto err_assign;
> - }
> - }
> -
> spin_lock(&fl->lock);
> - list_add_tail(&buf->node, &fl->mmaps);
> + if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> + list_add_tail(&buf->node, &fl->spd->rmaps);
> + else
> + list_add_tail(&buf->node, &fl->mmaps);
> spin_unlock(&fl->lock);
>
> if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> err = -EFAULT;
> - goto err_assign;
> + goto err_copy;
> }
>
> dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> @@ -2076,8 +2139,12 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>
> return 0;
>
> -err_assign:
> +err_copy:
> + spin_lock(&fl->lock);
> + list_del(&buf->node);
> + spin_unlock(&fl->lock);
> fastrpc_req_munmap_impl(fl, buf);
> + buf = NULL;
> err_invoke:
> fastrpc_buf_free(buf);
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:20PM +0530, Ekansh Gupta wrote:
> This patch series adds the listed features that have been missing
> in upstream fastRPC driver.
> - Add missing bug fixes.
> - Add static PD restart support for audio and sensors PD using
> PDR framework.
> - Redesign and improve remote heap management.
> - Add fixes for unsigned PD. Unsigned PD can be enabled
> using userspace API:
> https://git.codelinaro.org/linaro/qcomlt/fastrpc/-/blob/master/src/fastrpc_apps_user.c?ref_type=heads#L1173
>
> Changes in v2:
> - Added separate patch to add newlines in dev_err.
> - Added a bug fix in fastrpc capability function.
> - Added a new patch to save and restore interrupted context.
> - Fixed config dependency for PDR support.
>
> Changes in v3:
> - Dropped interrupted context patch.
> - Splitted few of the bug fix patches.
> - Added Fixes tag wherever applicable.
> - Updated proper commit message for few of the patches.
>
> Changes in v4:
> - Dropped untrusted process and system unsigned PD patches.
> - Updated proper commit message for few of the patches.
> - Splitted patches in more meaningful way.
> - Added helped functions for fastrpc_req_mmap.
>
I'd suggest to land patches 1-4, they seem to be fine.
The rest of the series needs more rework. Please start by reordering the
patches, so that fixes come first. Think about the people who will
backport them to earlier kernels.
--
With best wishes
Dmitry
On Thu, Jun 06, 2024 at 10:29:30PM +0530, Ekansh Gupta wrote:
> Unsigned PDs are sandboxed DSP processes used to offload computation
> workloads to the DSP. Unsigned PD have less privileges in terms of
> DSP resource access as compared to Signed PD.
>
> Unsigned PD requires more initial memory to spawn. Also most of the
> memory request are allocated from user space. Current initial memory
> size is not sufficient and mapping request for user space allocated
> buffer is not supported. This results in failure of unsigned PD offload
> request. Add changes to fix initial memory size and user space allocated
> buffer mapping support.
You can guess my comment here.
>
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
And here.
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 180 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 133 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 32f2e6f625ed..5ffb6098ac38 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_CTXID_MASK (0xFF0)
> -#define INIT_FILELEN_MAX (2 * 1024 * 1024)
> +#define INIT_FILELEN_MAX (5 * 1024 * 1024)
So, there are two things being fixed here. One is the insufficient
memory size, another one is the mmap request. Separate commits, please.
> #define INIT_FILE_NAMELEN_MAX (128)
> #define FASTRPC_DEVICE_NAME "fastrpc"
>
> @@ -327,6 +327,7 @@ struct fastrpc_user {
> int tgid;
> int pd;
> bool is_secure_dev;
> + bool is_unsigned_pd;
> char *servloc_name;
> /* Lock for lists */
> spinlock_t lock;
> @@ -1488,7 +1489,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> u32 siglen;
> } inbuf;
> u32 sc;
> - bool unsigned_module = false;
>
> args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
> if (!args)
> @@ -1500,9 +1500,9 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> }
>
> if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
> - unsigned_module = true;
> + fl->is_unsigned_pd = true;
>
> - if (is_session_rejected(fl, unsigned_module)) {
> + if (is_session_rejected(fl, fl->is_unsigned_pd)) {
> err = -ECONNREFUSED;
> goto err;
> }
> @@ -1985,6 +1985,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
> {
> struct fastrpc_buf *buf = NULL, *iter, *b;
> struct fastrpc_req_munmap req;
> + struct fastrpc_map *map = NULL, *iterm, *m;
> struct device *dev = fl->sctx->dev;
> int err = 0;
>
> @@ -2031,34 +2032,75 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
> }
> return err;
> }
> - dev_err(dev, "buffer not found addr 0x%09lx, len 0x%08llx\n",
> + spin_lock(&fl->lock);
> + list_for_each_entry_safe(iterm, m, &fl->maps, node) {
> + if (iterm->raddr == req.vaddrout) {
> + map = iterm;
> + break;
> + }
> + }
> + spin_unlock(&fl->lock);
> + if (!map) {
> + dev_dbg(dev, "buffer not found addr 0x%09llx, len 0x%08llx\n",
> req.vaddrout, req.size);
> - return -EINVAL;
> + return -EINVAL;
> + }
> +
> + err = fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
> + if (err)
> + dev_dbg(dev, "unmmap\tpt fd = %d, 0x%09llx error\n", map->fd, map->raddr);
Which error? The message is useless.
> + else
> + fastrpc_map_put(map);
Should the fl->lock be still held here? Can the map be modified
concurrently?
> +
> + return err;
> }
>
> -static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> +static int fastrpc_req_map_dsp(struct fastrpc_user *fl, u64 phys,
> + u64 size, u32 flag, uintptr_t vaddrin, u64 *raddr)
> {
> struct fastrpc_invoke_args args[3] = { [0 ... 2] = { 0 } };
> - struct fastrpc_buf *buf = NULL;
> struct fastrpc_mmap_req_msg req_msg;
> struct fastrpc_mmap_rsp_msg rsp_msg;
> struct fastrpc_phy_page pages;
> - 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;
> + req_msg.pgid = fl->tgid;
> + req_msg.flags = flag;
> + req_msg.vaddr = vaddrin;
> + req_msg.num = sizeof(pages);
>
> - if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) {
> - dev_err(dev, "flag not supported 0x%x\n", req.flags);
> + args[0].ptr = (u64) (uintptr_t) &req_msg;
> + args[0].length = sizeof(req_msg);
>
> - return -EINVAL;
> + pages.addr = phys;
> + pages.size = size;
> +
> + args[1].ptr = (u64) (uintptr_t) &pages;
> + args[1].length = sizeof(pages);
> + sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MMAP, 2, 1);
> + err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
> + &args[0]);
> +
> + if (err) {
> + dev_err(fl->sctx->dev, "mmap error (len 0x%08llx)\n", size);
> + return err;
> }
> + *raddr = rsp_msg.vaddr;
> +
> + return err;
> +}
> +
> +static int fastrpc_req_buf_alloc(struct fastrpc_user *fl,
> + struct fastrpc_req_mmap req, char __user *argp)
> +{
> + struct device *dev = fl->sctx->dev;
> + struct fastrpc_buf *buf = NULL;
> + u64 raddr = 0;
> + int err;
>
> if (req.vaddrin) {
> - dev_err(dev, "adding user allocated pages is not supported\n");
> + dev_err(dev, "adding user allocated pages is not supported for signed PD\n");
Drop, less chance of users spamming the dmesg.
> return -EINVAL;
> }
>
> @@ -2091,36 +2133,16 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> }
> }
>
> - req_msg.pgid = fl->tgid;
> - req_msg.flags = req.flags;
> - req_msg.vaddr = req.vaddrin;
> - req_msg.num = sizeof(pages);
> -
> - args[0].ptr = (u64) (uintptr_t) &req_msg;
> - args[0].length = sizeof(req_msg);
> -
> - pages.addr = buf->phys;
> - pages.size = buf->size;
> -
> - args[1].ptr = (u64) (uintptr_t) &pages;
> - args[1].length = sizeof(pages);
> -
> - 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]);
> - if (err) {
> - dev_err(dev, "mmap error (len 0x%08llx)\n", buf->size);
> + err = fastrpc_req_map_dsp(fl, buf->phys, buf->size, buf->flag,
> + req.vaddrin, &raddr);
> + if (err)
> goto err_invoke;
> - }
>
> /* update the buffer to be able to deallocate the memory on the DSP */
> - buf->raddr = (uintptr_t) rsp_msg.vaddr;
> + buf->raddr = (uintptr_t) raddr;
>
> /* let the client know the address to use */
> - req.vaddrout = rsp_msg.vaddr;
> + req.vaddrout = raddr;
>
> spin_lock(&fl->lock);
> if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR)
> @@ -2129,16 +2151,14 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> list_add_tail(&buf->node, &fl->mmaps);
> spin_unlock(&fl->lock);
>
> + dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> + buf->raddr, buf->size);
> +
> if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> err = -EFAULT;
> goto err_copy;
> }
> -
> - dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> - buf->raddr, buf->size);
> -
> return 0;
> -
> err_copy:
> spin_lock(&fl->lock);
> list_del(&buf->node);
> @@ -2146,11 +2166,77 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> fastrpc_req_munmap_impl(fl, buf);
> buf = NULL;
> err_invoke:
> - fastrpc_buf_free(buf);
> + if (buf)
> + fastrpc_buf_free(buf);
> +
> + return err;
> +}
> +
> +static int fastrpc_req_map_create(struct fastrpc_user *fl,
> + struct fastrpc_req_mmap req, char __user *argp)
> +{
> + struct fastrpc_map *map = NULL;
> + struct device *dev = fl->sctx->dev;
> + u64 raddr = 0;
> + int err;
> +
> + if ((req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && fl->is_unsigned_pd) {
> + dev_err(dev, "secure memory allocation is not supported in unsigned PD\n");
> + return -EINVAL;
> + }
> + err = fastrpc_map_create(fl, req.fd, req.size, 0, &map);
> + if (err) {
> + dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
> + return err;
> + }
> +
> + err = fastrpc_req_map_dsp(fl, map->phys, map->size, req.flags,
> + req.vaddrin, &raddr);
> + if (err)
> + goto err_invoke;
> +
> + /* update the buffer to be able to deallocate the memory on the DSP */
> + map->raddr = (uintptr_t) raddr;
> +
> + /* let the client know the address to use */
> + req.vaddrout = raddr;
> + dev_dbg(dev, "mmap\t\tpt 0x%09llx OK [len 0x%08llx]\n",
> + map->raddr, map->size);
> +
> + if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> + err = -EFAULT;
> + goto err_copy;
> + }
> + return 0;
> +err_copy:
> + fastrpc_req_munmap_dsp(fl, map->raddr, map->size);
> +err_invoke:
> + fastrpc_map_put(map);
>
> return err;
> }
>
> +static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> +{
> + struct fastrpc_req_mmap req;
> + int err;
> +
> + if (copy_from_user(&req, argp, sizeof(req)))
> + return -EFAULT;
> +
> + if ((req.flags == ADSP_MMAP_ADD_PAGES ||
> + req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) && !fl->is_unsigned_pd) {
> + err = fastrpc_req_buf_alloc(fl, req, argp);
> + if (err)
> + return err;
> + } else {
> + err = fastrpc_req_map_create(fl, req, argp);
> + if (err)
> + return err;
> + }
> + return 0;
> +}
> +
> static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
> {
> struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
> --
> 2.43.0
>
--
With best wishes
Dmitry
On 06/06/2024 18:59, Ekansh Gupta wrote:
> The DSP capability request call expects 2 arguments. First is the
> information about the total number of attributes to be copied from
> DSP and second is the information about the buffer where the DSP
> needs to copy the information. The current design is passing the
> information about te size to be copied from DSP which would be
*the size
> considered as a bad argument to the call by DSP causing a failure
> suggesting the same. The second argument carries the information
> about the buffer where the DSP needs to copy the capability
> information and the size to be copied. As the first entry of
> capability attribute is getting skipped, same should also be
> considered while sending the information to DSP. Add changes to
> pass proper arguments to DSP.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4028cb96bcf2..abf7df7c0c85 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1695,12 +1695,13 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
>
> /* Capability filled in userspace */
> dsp_attr_buf[0] = 0;
> + dsp_attr_buf_len -= 1;
The commit message states "As the first entry of the capability
attribute is getting skipped, same should also be considered while
sending the information to the DSP.". But it doesn't explain *why* this
is the case. Please add a comment here explaining why you subtract 1.
With that change:
Reviewed-by: Caleb Connolly <[email protected]>
>
> args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
> args[0].length = sizeof(dsp_attr_buf_len);
> args[0].fd = -1;
> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> - args[1].length = dsp_attr_buf_len;
> + args[1].length = dsp_attr_buf_len * sizeof(u32); > args[1].fd = -1;
> fl->pd = USER_PD;
>
> @@ -1730,7 +1731,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> if (!dsp_attributes)
> return -ENOMEM;
>
> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
> if (err == DSP_UNSUPPORTED_API) {
> dev_info(&cctx->rpdev->dev,
> "Warning: DSP capabilities not supported on domain: %d\n", domain);
--
// Caleb (they/them)
On 06/06/2024 18:59, Ekansh Gupta wrote:
> Few dev_err calls are missing newlines. This can result in unrelated
> lines getting appended which might make logs difficult to understand.
> Add trailing newlines to avoid this.
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> drivers/misc/fastrpc.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4c67e2c5a82e..4028cb96bcf2 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -325,7 +325,7 @@ static void fastrpc_free_map(struct kref *ref)
> err = qcom_scm_assign_mem(map->phys, map->size,
> &src_perms, &perm, 1);
> if (err) {
> - dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
> + dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> map->phys, map->size, err);
> return;
> }
> @@ -816,7 +816,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> map->attr = attr;
> err = qcom_scm_assign_mem(map->phys, (u64)map->size, &src_perms, dst_perms, 2);
> if (err) {
> - dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
> + dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
> map->phys, map->size, err);
> goto map_err;
> }
> @@ -1222,7 +1222,7 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> * that does not support unsigned PD offload
> */
> if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
> - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD");
> + dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
> return true;
> }
> }
> @@ -1285,7 +1285,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> &src_perms,
> fl->cctx->vmperms, fl->cctx->vmcount);
> if (err) {
> - dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
> + dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
> fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> goto err_map;
> }
> @@ -1337,7 +1337,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> (u64)fl->cctx->remote_heap->size,
> &src_perms, &dst_perms, 1);
> if (err)
> - dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
> + dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
> fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> }
> err_map:
--
// Caleb (they/them)
On 06/06/2024 18:59, Ekansh Gupta wrote:
> User is passing capability ioctl structure(argp) to get DSP
> capabilities. This argp is copied to a local structure to get domain
> and attribute_id information. After getting the capability, only
> capability value is getting copied to user argp which will not be
> useful if the use is trying to get the capability by checking the
> capability member of fastrpc_ioctl_capability structure. Add changes
> to copy the complete capability structure so that user can get the
> capability value from the expected member of the structure.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> drivers/misc/fastrpc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index abf7df7c0c85..f64781c3012f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1784,7 +1784,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> if (err)
> return err;
>
> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> + if (copy_to_user(argp, &cap, sizeof(cap)))
> return -EFAULT;
>
> return 0;
--
// Caleb (they/them)
On 06/06/2024 18:59, Ekansh Gupta wrote:
> When user is requesting for DSP capability, the process pd type is
> getting updated to USER_PD which is incorrect as DSP will assume the
> process which is making the request is a user PD and this will never
> get updated back to the original value. The actual PD type should not
> be updated for capability request and it should be serviced by the
> respective PD on DSP side. Adding changes to avoid process PD type
> modification.
>
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> drivers/misc/fastrpc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f64781c3012f..abdd35b7c3ad 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1703,7 +1703,6 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> args[1].length = dsp_attr_buf_len * sizeof(u32);
> args[1].fd = -1;
> - fl->pd = USER_PD;
>
> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> FASTRPC_SCALARS(0, 1, 1), args);
--
// Caleb (they/them)
On 06/06/2024 18:59, Ekansh Gupta wrote:
> Static PDs are created on DSPs to support specific use cases like Audio
> and Sensors. The static PDs uses any CPU requirements like file
> operations or memory need with the help of a daemon running on the CPU.
> Audio and sensors daemons attaches to audio PD and sensors PD on DSP.
> Audio PD expects some CMA memory for dynamic loading purpose which is
> allocated and sent to DSP in fastrpc_init_create_static_process call.
> For sensor daemon, the expectation is just to attach to sensors PD and
> take up any requests made by the PD(like file operations etc.).
>
> Static PDs run on the audio and sensor supporting subsystem which can
> be ADSP or SDSP. They are expected to support PD restart. There are some
> CPU resources like buffers etc. for static PDs which are expected to be
> cleaned up by fastrpc driver during PDR scenario. For this, there is a
> requirement of PD service locator to get the event notifications for
> static PD services. Also when events are received, the driver needs to
> handle based on PD states.
>
> PDR handling is required for static PD only. There are no static PD
> supported on MDSP or CDSP hence no PDR handling is required. PDR is also
> not required for root_pd as if root_pd is shutting down, that basically
> suggests that the remoteproc itself is shutting down which is handled
> with rpmsg functionalities(probe and remove).
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/Kconfig | 2 +
> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
I think this justifies introducing a new C file. The functionality added
here should be quite easily abstracted behind a sensible interface.
> 2 files changed, 195 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index faf983680040..e2d83cd085b5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
> tristate "Qualcomm FastRPC"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on RPMSG
> + depends on NET
> select DMA_SHARED_BUFFER
> select QCOM_SCM
> + select QCOM_PDR_HELPERS
> help
> Provides a communication mechanism that allows for clients to
> make remote method invocations across processor boundary to
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index abdd35b7c3ad..13e368279765 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -22,6 +22,7 @@
> #include <linux/firmware/qcom/qcom_scm.h>
> #include <uapi/misc/fastrpc.h>
> #include <linux/of_reserved_mem.h>
> +#include <linux/soc/qcom/pdr.h>
>
> #define ADSP_DOMAIN_ID (0)
> #define MDSP_DOMAIN_ID (1)
> @@ -29,6 +30,7 @@
> #define CDSP_DOMAIN_ID (3)
> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
> #define FASTRPC_MAX_SESSIONS 14
> +#define FASTRPC_MAX_SPD 4
Why 4? This patch only makes use of two.
> #define FASTRPC_MAX_VMIDS 16
> #define FASTRPC_ALIGN 128
> #define FASTRPC_MAX_FDLIST 16
> @@ -105,6 +107,18 @@
>
> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>
> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
> +
> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
> +
> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
This data should be defined in some const static data struct, not as a
bunch of macros, then you can actually describe the relationship between
a domain_id and the fact that the ADSP registers two PDR lookups. See my
comments in fastrpc_setup_service_locator() and where it's called in probe.
struct fastrpc_pdr_domain {
const char *servloc_client_name;
const char *service_name;
const char *pd_name;
};
static const struct fastrpc_pdr_domain adsp_pdr_services[] = {
{
.servloc_client_name = "audio_pdr_adsp";
.service_name = "avs/audio";
.pd_name = "msm/adsp/audio_pd";
},
{
.servloc_client_name = "sensors_pdr_adsp";
...
};
> +
> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> "sdsp", "cdsp"};
> struct fastrpc_phy_page {
> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
> bool valid;
> };
>
> +struct fastrpc_static_pd {
> + char *servloc_name;
> + char *spdname;
> + void *pdrhandle;
> + struct fastrpc_channel_ctx *cctx;
> + struct fastrpc_user *fl;
> + bool ispdup;
> +};
> +
> struct fastrpc_channel_ctx {
> int domain_id;
> int sesscount;
> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
> struct rpmsg_device *rpdev;
> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
> spinlock_t lock;
> struct idr ctx_idr;
> struct list_head users;
> @@ -297,10 +321,12 @@ struct fastrpc_user {
> struct fastrpc_channel_ctx *cctx;
> struct fastrpc_session_ctx *sctx;
> struct fastrpc_buf *init_mem;
> + struct fastrpc_static_pd *spd;
>
> int tgid;
> int pd;
> bool is_secure_dev;
> + char *servloc_name;
This is duplicated from spd->servloc_name
> /* Lock for lists */
> spinlock_t lock;
> /* lock for allocations */
> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
> return false;
> }
>
> +static struct fastrpc_static_pd *fastrpc_get_spd_session( > + struct fastrpc_user *fl)
Every caller of this function has the same handling (checking !spd and
then !spd->ispdup and returning different errors in each case). Please
lift this common error handling into this function. While at it, I'd
propose dropping the opaque *fl pointer and instead passing in the data
which is actually used. The servloc_name could be looked up from the
fastrpc_pdr_domain data.
static int fastrpc_pdr_is_up(int pd, int domain_id, struct
fastrpc_static_pd *spds,
struct fastrpc_static_pd *spd)
> +{
> + int i;
> + struct fastrpc_static_pd *spd = NULL;
> + struct fastrpc_channel_ctx *cctx = fl->cctx;
> +
> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
> + if (!cctx->spd[i].servloc_name)
> + continue;
> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
> + spd = &cctx->spd[i];
> + spd->fl = fl;
> + break;
> + }
> + }
> +
> + return spd;
> +}
> +
> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> char __user *argp)
> {
> struct fastrpc_init_create_static init;
> struct fastrpc_invoke_args *args;
> struct fastrpc_phy_page pages[1];
> + struct fastrpc_static_pd *spd = NULL;
> char *name;
> int err;
> struct {
> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> goto err_name;
> }
>
> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd) {
> + err = -EUSERS;
> + goto err_name;
> + }
> +
> + if (!spd->ispdup) {
> + err = -ENOTCONN;
> + goto err_name;
> + }
> + fl->spd = spd;
> if (!fl->cctx->remote_heap) {
> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> &fl->cctx->remote_heap);
> @@ -1645,6 +1705,7 @@ 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_static_pd *spd = NULL;
> int tgid = fl->tgid;
> u32 sc;
>
> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> fl->pd = pd;
>
> + if (pd == SENSORS_PD) {
Why is this only relevant for the sensors pd?
> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
What if domain_id isn't either of these? Should this be checked? If not,
why?
This whole block could be replaced with a call to the
fastrpc_pdr_is_up() function I proposed.
> +
> + spd = fastrpc_get_spd_session(fl);
> + if (!spd)
> + return -EUSERS;
> +
> + if (!spd->ispdup)
> + return -ENOTCONN;
> +
> + fl->spd = spd;
> + }
> +
> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
> sc, &args[0]);
> }
> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> return err;
> }
>
> +static void fastrpc_notify_users(struct fastrpc_user *user)
> +{
> + struct fastrpc_invoke_ctx *ctx;
> +
> + spin_lock(&user->lock);
> + list_for_each_entry(ctx, &user->pending, node) {
> + ctx->retval = -EPIPE;
> + complete(&ctx->work);
> + }
> + spin_unlock(&user->lock);
> +}
> +
> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
> + char *servloc_name)
> +{
> + struct fastrpc_user *fl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cctx->lock, flags);
> + list_for_each_entry(fl, &cctx->users, user) {
> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
> + fastrpc_notify_users(fl);
> + }
> + spin_unlock_irqrestore(&cctx->lock, flags);
> +}
> +
> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
> +{
> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
> + struct fastrpc_channel_ctx *cctx;
> +
> + if (!spd)
> + return;
> +
> + cctx = spd->cctx;
> + switch (state) {
> + case SERVREG_SERVICE_STATE_DOWN:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is down for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
dev_dbg
> + spd->ispdup = false;
> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
> + break;
> + case SERVREG_SERVICE_STATE_UP:
> + dev_info(&cctx->rpdev->dev,
> + "%s: %s (%s) is up for PDR on %s\n",
> + __func__, spd->spdname,
> + spd->servloc_name,
> + domains[cctx->domain_id]);
> + spd->ispdup = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> static const struct file_operations fastrpc_fops = {
> .open = fastrpc_device_open,
> .release = fastrpc_device_release,
> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> return err;
> }
>
> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
> + char *service_name, char *service_path, int domain, int spd_session)
This function should instead take a struct fastrpc_pdr_domain *services
array.
> +{
> + int err = 0;
> + struct pdr_handle *handle = NULL;
> + struct pdr_service *service = NULL;
> +
> + /* Register the service locator's callback function */
> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto bail;
> + }
> + cctx->spd[spd_session].pdrhandle = handle;
> + cctx->spd[spd_session].servloc_name = client_name;
> + cctx->spd[spd_session].spdname = service_path;
> + cctx->spd[spd_session].cctx = cctx;
> + service = pdr_add_lookup(handle, service_name, service_path);
> + if (IS_ERR(service)) {
> + err = PTR_ERR(service);
> + goto bail;
> + }
> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
dev_dbg
> + __func__, service_name, client_name, service_path);
> +
> +bail:
> + if (err) {
> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
dev_err
> + __func__, service_name, client_name, service_path, err);
> + }
> + return err;
> +}
> +
> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> {
> struct device *rdev = &rpdev->dev;
> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> goto fdev_error;
> }
>
> + if (domain_id == ADSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> +
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
I assume this is basically a nop on platforms where this service doesn't
exist? A comment mentioning this would be nice.
> + if (err)
> + goto populate_error;
> + } else if (domain_id == SDSP_DOMAIN_ID) {
> + err = fastrpc_setup_service_locator(data,
> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
> + if (err)
> + goto populate_error;
> + }
This block should be moved into the domain_id switch/case which is
directly above it.
> +
> kref_init(&data->refcount);
>
> dev_set_drvdata(&rpdev->dev, data);
> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> return err;
> }
>
> -static void fastrpc_notify_users(struct fastrpc_user *user)
> -{
> - struct fastrpc_invoke_ctx *ctx;
> -
> - spin_lock(&user->lock);
> - list_for_each_entry(ctx, &user->pending, node) {
> - ctx->retval = -EPIPE;
> - complete(&ctx->work);
> - }
> - spin_unlock(&user->lock);
> -}
> -
> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> {
> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> struct fastrpc_buf *buf, *b;
> struct fastrpc_user *user;
> unsigned long flags;
> + int i;
>
> /* No invocations past this point */
> spin_lock_irqsave(&cctx->lock, flags);
> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->remote_heap)
> fastrpc_buf_free(cctx->remote_heap);
>
> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
> + if (cctx->spd[i].pdrhandle)
> + pdr_handle_release(cctx->spd[i].pdrhandle);
> + }
> +
> of_platform_depopulate(&rpdev->dev);
>
> fastrpc_channel_ctx_put(cctx);
--
// Caleb (they/them)
On 6/7/2024 4:55 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote:
>> Static PDs are created on DSPs to support specific use cases like Audio
>> and Sensors. The static PDs uses any CPU requirements like file
>> operations or memory need with the help of a daemon running on the CPU.
> What do you mean by 'CPU requirements' here?
Something like file system request or any memory requirement. For these types of requirements
PD will rely on CPU process.
Planning to drop this patch from the series and move the pd notification specific implementations
to a different .c file as per Caleb's suggestion.
I'll address all other comments in the new patch series.
>
>> Audio and sensors daemons attaches to audio PD and sensors PD on DSP.
> attach
>
>> Audio PD expects some CMA memory for dynamic loading purpose which is
>> allocated and sent to DSP in fastrpc_init_create_static_process call.
> A pointer to audio daemon would be helpful. We don't have such a thing
> in the normal Linux operation cycle.
>
>> For sensor daemon, the expectation is just to attach to sensors PD and
>> take up any requests made by the PD(like file operations etc.).
>>
>> Static PDs run on the audio and sensor supporting subsystem which can
>> be ADSP or SDSP. They are expected to support PD restart. There are some
>> CPU resources like buffers etc. for static PDs which are expected to be
>> cleaned up by fastrpc driver during PDR scenario. For this, there is a
> Why are they cleaned by fastrpc driver and not by the userspace daemon
> itself? Userspace can also subscribe to PDR notifications and handle
> restarts on its own.
>
>> requirement of PD service locator to get the event notifications for
>> static PD services. Also when events are received, the driver needs to
>> handle based on PD states.
> To handle what? And why?
>
>> PDR handling is required for static PD only. There are no static PD
>> supported on MDSP or CDSP hence no PDR handling is required. PDR is also
>> not required for root_pd as if root_pd is shutting down, that basically
>> suggests that the remoteproc itself is shutting down which is handled
>> with rpmsg functionalities(probe and remove).
>>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/Kconfig | 2 +
>> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 195 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index faf983680040..e2d83cd085b5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
>> tristate "Qualcomm FastRPC"
>> depends on ARCH_QCOM || COMPILE_TEST
>> depends on RPMSG
>> + depends on NET
>> select DMA_SHARED_BUFFER
>> select QCOM_SCM
>> + select QCOM_PDR_HELPERS
>> help
>> Provides a communication mechanism that allows for clients to
>> make remote method invocations across processor boundary to
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index abdd35b7c3ad..13e368279765 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -22,6 +22,7 @@
>> #include <linux/firmware/qcom/qcom_scm.h>
>> #include <uapi/misc/fastrpc.h>
>> #include <linux/of_reserved_mem.h>
>> +#include <linux/soc/qcom/pdr.h>
>>
>> #define ADSP_DOMAIN_ID (0)
>> #define MDSP_DOMAIN_ID (1)
>> @@ -29,6 +30,7 @@
>> #define CDSP_DOMAIN_ID (3)
>> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
>> #define FASTRPC_MAX_SESSIONS 14
>> +#define FASTRPC_MAX_SPD 4
>> #define FASTRPC_MAX_VMIDS 16
>> #define FASTRPC_ALIGN 128
>> #define FASTRPC_MAX_FDLIST 16
>> @@ -105,6 +107,18 @@
>>
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>
>> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
>> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
>> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
>> +
>> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
>> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
>> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
>> +
>> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
>> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
>> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
>> +
>> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> "sdsp", "cdsp"};
>> struct fastrpc_phy_page {
>> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
>> bool valid;
>> };
>>
>> +struct fastrpc_static_pd {
>> + char *servloc_name;
>> + char *spdname;
>> + void *pdrhandle;
>> + struct fastrpc_channel_ctx *cctx;
>> + struct fastrpc_user *fl;
>> + bool ispdup;
> is_pd_up
>
>> +};
>> +
>> struct fastrpc_channel_ctx {
>> int domain_id;
>> int sesscount;
>> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
>> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>> struct rpmsg_device *rpdev;
>> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
>> spinlock_t lock;
>> struct idr ctx_idr;
>> struct list_head users;
>> @@ -297,10 +321,12 @@ struct fastrpc_user {
>> struct fastrpc_channel_ctx *cctx;
>> struct fastrpc_session_ctx *sctx;
>> struct fastrpc_buf *init_mem;
>> + struct fastrpc_static_pd *spd;
>>
>> int tgid;
>> int pd;
>> bool is_secure_dev;
>> + char *servloc_name;
>> /* Lock for lists */
>> spinlock_t lock;
>> /* lock for allocations */
>> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>> return false;
>> }
>>
>> +static struct fastrpc_static_pd *fastrpc_get_spd_session(
>> + struct fastrpc_user *fl)
> Single line, please
>
>> +{
>> + int i;
>> + struct fastrpc_static_pd *spd = NULL;
>> + struct fastrpc_channel_ctx *cctx = fl->cctx;
>> +
>> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
>> + if (!cctx->spd[i].servloc_name)
>> + continue;
>> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
>> + spd = &cctx->spd[i];
>> + spd->fl = fl;
>> + break;
>> + }
> Should there be a protection for spd->fl being overwritten / overtaken?
> How should it be handled by the driver?
>
>> + }
>> +
>> + return spd;
>> +}
>> +
>> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> char __user *argp)
>> {
>> struct fastrpc_init_create_static init;
>> struct fastrpc_invoke_args *args;
>> struct fastrpc_phy_page pages[1];
>> + struct fastrpc_static_pd *spd = NULL;
>> char *name;
>> int err;
>> struct {
>> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_name;
>> }
>>
>> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd) {
>> + err = -EUSERS;
> "Too many users" ?
>
>> + goto err_name;
>> + }
>> +
>> + if (!spd->ispdup) {
> This is racy. What if the domain restarts right after this line?
>
>> + err = -ENOTCONN;
>> + goto err_name;
>> + }
>> + fl->spd = spd;
>> if (!fl->cctx->remote_heap) {
>> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>> &fl->cctx->remote_heap);
>> @@ -1645,6 +1705,7 @@ 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_static_pd *spd = NULL;
>> int tgid = fl->tgid;
>> u32 sc;
>>
>> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>> fl->pd = pd;
>>
>> + if (pd == SENSORS_PD) {
>> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
>> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
>> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
>> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd)
>> + return -EUSERS;
>> +
>> + if (!spd->ispdup)
>> + return -ENOTCONN;
>> +
>> + fl->spd = spd;
>> + }
>> +
>> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> sc, &args[0]);
>> }
>> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>> return err;
>> }
>>
>> +static void fastrpc_notify_users(struct fastrpc_user *user)
>> +{
>> + struct fastrpc_invoke_ctx *ctx;
>> +
>> + spin_lock(&user->lock);
>> + list_for_each_entry(ctx, &user->pending, node) {
>> + ctx->retval = -EPIPE;
>> + complete(&ctx->work);
>> + }
>> + spin_unlock(&user->lock);
>> +}
>> +
>> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
>> + char *servloc_name)
>> +{
>> + struct fastrpc_user *fl;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&cctx->lock, flags);
>> + list_for_each_entry(fl, &cctx->users, user) {
>> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
>> + fastrpc_notify_users(fl);
>> + }
>> + spin_unlock_irqrestore(&cctx->lock, flags);
>> +}
>> +
>> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
>> +{
>> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
>> + struct fastrpc_channel_ctx *cctx;
>> +
>> + if (!spd)
>> + return;
>> +
>> + cctx = spd->cctx;
>> + switch (state) {
>> + case SERVREG_SERVICE_STATE_DOWN:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is down for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
> Too noisy. Is it really dev_info()? Or dev_warn? There should be no need
> for __func__.
>
>> + spd->ispdup = false;
>> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
>> + break;
>> + case SERVREG_SERVICE_STATE_UP:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is up for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
> Same comment here.
>
>> + spd->ispdup = true;
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> static const struct file_operations fastrpc_fops = {
>> .open = fastrpc_device_open,
>> .release = fastrpc_device_release,
>> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>> return err;
>> }
>>
>> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
>> + char *service_name, char *service_path, int domain, int spd_session)
>> +{
>> + int err = 0;
>> + struct pdr_handle *handle = NULL;
>> + struct pdr_service *service = NULL;
>> +
>> + /* Register the service locator's callback function */
>> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
>> + if (IS_ERR(handle)) {
>> + err = PTR_ERR(handle);
>> + goto bail;
>> + }
>> + cctx->spd[spd_session].pdrhandle = handle;
>> + cctx->spd[spd_session].servloc_name = client_name;
>> + cctx->spd[spd_session].spdname = service_path;
>> + cctx->spd[spd_session].cctx = cctx;
>> + service = pdr_add_lookup(handle, service_name, service_path);
>> + if (IS_ERR(service)) {
>> + err = PTR_ERR(service);
>> + goto bail;
>> + }
>> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
>> + __func__, service_name, client_name, service_path);
> Definitely too noisy. Drop __func__ everywhere. Use dev_dbg instead.
>
>> +
>> +bail:
>> + if (err) {
>> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
>> + __func__, service_name, client_name, service_path, err);
> dev_warn, drop brackets, align properly. Did you run this through checkpatch.pl --strict?
>
>> + }
>> + return err;
>> +}
>> +
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> goto fdev_error;
>> }
>>
>> + if (domain_id == ADSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
>> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> +
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
>> + if (err)
>> + goto populate_error;
>> + } else if (domain_id == SDSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> + }
>> +
>> kref_init(&data->refcount);
>>
>> dev_set_drvdata(&rpdev->dev, data);
>> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> return err;
>> }
>>
>> -static void fastrpc_notify_users(struct fastrpc_user *user)
>> -{
>> - struct fastrpc_invoke_ctx *ctx;
>> -
>> - spin_lock(&user->lock);
>> - list_for_each_entry(ctx, &user->pending, node) {
>> - ctx->retval = -EPIPE;
>> - complete(&ctx->work);
>> - }
>> - spin_unlock(&user->lock);
>> -}
>> -
>> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> {
>> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
>> struct fastrpc_buf *buf, *b;
>> struct fastrpc_user *user;
>> unsigned long flags;
>> + int i;
>>
>> /* No invocations past this point */
>> spin_lock_irqsave(&cctx->lock, flags);
>> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> if (cctx->remote_heap)
>> fastrpc_buf_free(cctx->remote_heap);
>>
>> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
>> + if (cctx->spd[i].pdrhandle)
>> + pdr_handle_release(cctx->spd[i].pdrhandle);
>> + }
>> +
>> of_platform_depopulate(&rpdev->dev);
>>
>> fastrpc_channel_ctx_put(cctx);
>> --
>> 2.43.0
>>
On 6/7/2024 7:15 PM, Caleb Connolly wrote:
>
>
> On 06/06/2024 18:59, Ekansh Gupta wrote:
>> Static PDs are created on DSPs to support specific use cases like Audio
>> and Sensors. The static PDs uses any CPU requirements like file
>> operations or memory need with the help of a daemon running on the CPU.
>> Audio and sensors daemons attaches to audio PD and sensors PD on DSP.
>> Audio PD expects some CMA memory for dynamic loading purpose which is
>> allocated and sent to DSP in fastrpc_init_create_static_process call.
>> For sensor daemon, the expectation is just to attach to sensors PD and
>> take up any requests made by the PD(like file operations etc.).
>>
>> Static PDs run on the audio and sensor supporting subsystem which can
>> be ADSP or SDSP. They are expected to support PD restart. There are some
>> CPU resources like buffers etc. for static PDs which are expected to be
>> cleaned up by fastrpc driver during PDR scenario. For this, there is a
>> requirement of PD service locator to get the event notifications for
>> static PD services. Also when events are received, the driver needs to
>> handle based on PD states.
>>
>> PDR handling is required for static PD only. There are no static PD
>> supported on MDSP or CDSP hence no PDR handling is required. PDR is also
>> not required for root_pd as if root_pd is shutting down, that basically
>> suggests that the remoteproc itself is shutting down which is handled
>> with rpmsg functionalities(probe and remove).
>>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> drivers/misc/Kconfig | 2 +
>> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++---
>
> I think this justifies introducing a new C file. The functionality added here should be quite easily abstracted behind a sensible interface.
Thanks for the suggestion, Caleb. I'm dropping this patch from the series for now. I'll move PD notification specific implementations
to a new C file and send the patches separately. I'll also address all your comments there.
--ekansh
>> 2 files changed, 195 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index faf983680040..e2d83cd085b5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -280,8 +280,10 @@ config QCOM_FASTRPC
>> tristate "Qualcomm FastRPC"
>> depends on ARCH_QCOM || COMPILE_TEST
>> depends on RPMSG
>> + depends on NET
>> select DMA_SHARED_BUFFER
>> select QCOM_SCM
>> + select QCOM_PDR_HELPERS
>> help
>> Provides a communication mechanism that allows for clients to
>> make remote method invocations across processor boundary to
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index abdd35b7c3ad..13e368279765 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -22,6 +22,7 @@
>> #include <linux/firmware/qcom/qcom_scm.h>
>> #include <uapi/misc/fastrpc.h>
>> #include <linux/of_reserved_mem.h>
>> +#include <linux/soc/qcom/pdr.h>
>> #define ADSP_DOMAIN_ID (0)
>> #define MDSP_DOMAIN_ID (1)
>> @@ -29,6 +30,7 @@
>> #define CDSP_DOMAIN_ID (3)
>> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/
>> #define FASTRPC_MAX_SESSIONS 14
>> +#define FASTRPC_MAX_SPD 4
>
> Why 4? This patch only makes use of two.
>> #define FASTRPC_MAX_VMIDS 16
>> #define FASTRPC_ALIGN 128
>> #define FASTRPC_MAX_FDLIST 16
>> @@ -105,6 +107,18 @@
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp"
>> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio"
>> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd"
>> +
>> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp"
>> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg"
>> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd"
>> +
>> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi"
>> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME
>> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd"
>
> This data should be defined in some const static data struct, not as a bunch of macros, then you can actually describe the relationship between a domain_id and the fact that the ADSP registers two PDR lookups. See my comments in fastrpc_setup_service_locator() and where it's called in probe.
>
> struct fastrpc_pdr_domain {
> const char *servloc_client_name;
> const char *service_name;
> const char *pd_name;
> };
>
> static const struct fastrpc_pdr_domain adsp_pdr_services[] = {
> {
> .servloc_client_name = "audio_pdr_adsp";
> .service_name = "avs/audio";
> .pd_name = "msm/adsp/audio_pd";
> },
> {
> .servloc_client_name = "sensors_pdr_adsp";
> ...
> };
>> +
>> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> "sdsp", "cdsp"};
>> struct fastrpc_phy_page {
>> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx {
>> bool valid;
>> };
>> +struct fastrpc_static_pd {
>> + char *servloc_name;
>> + char *spdname;
>> + void *pdrhandle;
>> + struct fastrpc_channel_ctx *cctx;
>> + struct fastrpc_user *fl;
>> + bool ispdup;
>> +};
>> +
>> struct fastrpc_channel_ctx {
>> int domain_id;
>> int sesscount;
>> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx {
>> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>> struct rpmsg_device *rpdev;
>> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD];
>> spinlock_t lock;
>> struct idr ctx_idr;
>> struct list_head users;
>> @@ -297,10 +321,12 @@ struct fastrpc_user {
>> struct fastrpc_channel_ctx *cctx;
>> struct fastrpc_session_ctx *sctx;
>> struct fastrpc_buf *init_mem;
>> + struct fastrpc_static_pd *spd;
>> int tgid;
>> int pd;
>> bool is_secure_dev;
>> + char *servloc_name;
>
> This is duplicated from spd->servloc_name
>> /* Lock for lists */
>> spinlock_t lock;
>> /* lock for allocations */
>> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
>> return false;
>> }
>> +static struct fastrpc_static_pd *fastrpc_get_spd_session( > + struct fastrpc_user *fl)
>
> Every caller of this function has the same handling (checking !spd and then !spd->ispdup and returning different errors in each case). Please lift this common error handling into this function. While at it, I'd propose dropping the opaque *fl pointer and instead passing in the data which is actually used. The servloc_name could be looked up from the fastrpc_pdr_domain data.
>
> static int fastrpc_pdr_is_up(int pd, int domain_id, struct
> fastrpc_static_pd *spds,
> struct fastrpc_static_pd *spd)
>> +{
>> + int i;
>> + struct fastrpc_static_pd *spd = NULL;
>> + struct fastrpc_channel_ctx *cctx = fl->cctx;
>> +
>> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) {
>> + if (!cctx->spd[i].servloc_name)
>> + continue;
>> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) {
>> + spd = &cctx->spd[i];
>> + spd->fl = fl;
>> + break;
>> + }
>> + }
>> +
>> + return spd;
>> +}
>> +
>> static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> char __user *argp)
>> {
>> struct fastrpc_init_create_static init;
>> struct fastrpc_invoke_args *args;
>> struct fastrpc_phy_page pages[1];
>> + struct fastrpc_static_pd *spd = NULL;
>> char *name;
>> int err;
>> struct {
>> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_name;
>> }
>> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME;
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd) {
>> + err = -EUSERS;
>> + goto err_name;
>> + }
>> +
>> + if (!spd->ispdup) {
>> + err = -ENOTCONN;
>> + goto err_name;
>> + }
>> + fl->spd = spd;
>> if (!fl->cctx->remote_heap) {
>> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
>> &fl->cctx->remote_heap);
>> @@ -1645,6 +1705,7 @@ 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_static_pd *spd = NULL;
>> int tgid = fl->tgid;
>> u32 sc;
>> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>> fl->pd = pd;
>> + if (pd == SENSORS_PD) {
>
> Why is this only relevant for the sensors pd?
>> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID)
>> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME;
>> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID)
>> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME;
> What if domain_id isn't either of these? Should this be checked? If not, why?
>
> This whole block could be replaced with a call to the fastrpc_pdr_is_up() function I proposed.
>> +
>> + spd = fastrpc_get_spd_session(fl);
>> + if (!spd)
>> + return -EUSERS;
>> +
>> + if (!spd->ispdup)
>> + return -ENOTCONN;
>> +
>> + fl->spd = spd;
>> + }
>> +
>> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>> sc, &args[0]);
>> }
>> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>> return err;
>> }
>> +static void fastrpc_notify_users(struct fastrpc_user *user)
>> +{
>> + struct fastrpc_invoke_ctx *ctx;
>> +
>> + spin_lock(&user->lock);
>> + list_for_each_entry(ctx, &user->pending, node) {
>> + ctx->retval = -EPIPE;
>> + complete(&ctx->work);
>> + }
>> + spin_unlock(&user->lock);
>> +}
>> +
>> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx,
>> + char *servloc_name)
>> +{
>> + struct fastrpc_user *fl;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&cctx->lock, flags);
>> + list_for_each_entry(fl, &cctx->users, user) {
>> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name))
>> + fastrpc_notify_users(fl);
>> + }
>> + spin_unlock_irqrestore(&cctx->lock, flags);
>> +}
>> +
>> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv)
>> +{
>> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv;
>> + struct fastrpc_channel_ctx *cctx;
>> +
>> + if (!spd)
>> + return;
>> +
>> + cctx = spd->cctx;
>> + switch (state) {
>> + case SERVREG_SERVICE_STATE_DOWN:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is down for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
>
> dev_dbg
>> + spd->ispdup = false;
>> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name);
>> + break;
>> + case SERVREG_SERVICE_STATE_UP:
>> + dev_info(&cctx->rpdev->dev,
>> + "%s: %s (%s) is up for PDR on %s\n",
>> + __func__, spd->spdname,
>> + spd->servloc_name,
>> + domains[cctx->domain_id]);
>> + spd->ispdup = true;
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> static const struct file_operations fastrpc_fops = {
>> .open = fastrpc_device_open,
>> .release = fastrpc_device_release,
>> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>> return err;
>> }
>> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name,
>> + char *service_name, char *service_path, int domain, int spd_session)
>
> This function should instead take a struct fastrpc_pdr_domain *services array.
>> +{
>> + int err = 0;
>> + struct pdr_handle *handle = NULL;
>> + struct pdr_service *service = NULL;
>> +
>> + /* Register the service locator's callback function */
>> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]);
>> + if (IS_ERR(handle)) {
>> + err = PTR_ERR(handle);
>> + goto bail;
>> + }
>> + cctx->spd[spd_session].pdrhandle = handle;
>> + cctx->spd[spd_session].servloc_name = client_name;
>> + cctx->spd[spd_session].spdname = service_path;
>> + cctx->spd[spd_session].cctx = cctx;
>> + service = pdr_add_lookup(handle, service_name, service_path);
>> + if (IS_ERR(service)) {
>> + err = PTR_ERR(service);
>> + goto bail;
>> + }
>> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n",
> dev_dbg
>> + __func__, service_name, client_name, service_path);
>> +
>> +bail:
>> + if (err) {
>> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n",
> dev_err
>> + __func__, service_name, client_name, service_path, err);
>> + }
>> + return err;
>> +}
>> +
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> goto fdev_error;
>> }
>> + if (domain_id == ADSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME,
>> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> +
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1);
>
> I assume this is basically a nop on platforms where this service doesn't exist? A comment mentioning this would be nice.
>> + if (err)
>> + goto populate_error;
>> + } else if (domain_id == SDSP_DOMAIN_ID) {
>> + err = fastrpc_setup_service_locator(data,
>> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME,
>> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0);
>> + if (err)
>> + goto populate_error;
>> + }
>
> This block should be moved into the domain_id switch/case which is directly above it.
>
>> +
>> kref_init(&data->refcount);
>> dev_set_drvdata(&rpdev->dev, data);
>> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> return err;
>> }
>> -static void fastrpc_notify_users(struct fastrpc_user *user)
>> -{
>> - struct fastrpc_invoke_ctx *ctx;
>> -
>> - spin_lock(&user->lock);
>> - list_for_each_entry(ctx, &user->pending, node) {
>> - ctx->retval = -EPIPE;
>> - complete(&ctx->work);
>> - }
>> - spin_unlock(&user->lock);
>> -}
>> -
>> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> {
>> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
>> struct fastrpc_buf *buf, *b;
>> struct fastrpc_user *user;
>> unsigned long flags;
>> + int i;
>> /* No invocations past this point */
>> spin_lock_irqsave(&cctx->lock, flags);
>> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> if (cctx->remote_heap)
>> fastrpc_buf_free(cctx->remote_heap);
>> + for (i = 0; i < FASTRPC_MAX_SPD; i++) {
>> + if (cctx->spd[i].pdrhandle)
>> + pdr_handle_release(cctx->spd[i].pdrhandle);
>> + }
>> +
>> of_platform_depopulate(&rpdev->dev);
>> fastrpc_channel_ctx_put(cctx);
>
On 6/7/2024 4:58 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 06, 2024 at 10:29:26PM +0530, Ekansh Gupta wrote:
>> Audio PD daemon send the name as part of the init IOCTL call. This
>> mane needs to be copied to kernel for which memory is allocated.
>> This memory is never freed which might result in memory leak. Add
>> changes to free the memory when it is not needed.
>>
>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
> Fixes go before the non-fixes patches.
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
>
>> ---
>> drivers/misc/fastrpc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 13e368279765..7ee8bb3a9a6f 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1380,6 +1380,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>> goto err_invoke;
>>
> A comment that the remote_heap persists would be helpful.
I'll add this information in remote heap redesign patch. Thanks.
--ekansh
>
>> kfree(args);
>> + kfree(name);
>>
>> return 0;
>> err_invoke:
>> --
>> 2.43.0
>>
On Mon, Jun 10, 2024 at 02:35:48PM +0530, Ekansh Gupta wrote:
>
>
> On 6/7/2024 4:55 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote:
> >> Static PDs are created on DSPs to support specific use cases like Audio
> >> and Sensors. The static PDs uses any CPU requirements like file
> >> operations or memory need with the help of a daemon running on the CPU.
> > What do you mean by 'CPU requirements' here?
> Something like file system request or any memory requirement. For these types of requirements
> PD will rely on CPU process.
So, this is not 'CPU requirements'. It should be something like
'requests to the HLOS'. CPU requirements would usually mean something
requiring CPU cycles or min/max freq, etc.
>
> Planning to drop this patch from the series and move the pd notification specific implementations
> to a different .c file as per Caleb's suggestion.
>
> I'll address all other comments in the new patch series.
Ack
--
With best wishes
Dmitry