Re: [PATCH 1/2] remoteproc: Add userspace char device driver

From: rishabhb
Date: Fri Mar 27 2020 - 20:10:50 EST

On 2020-03-26 10:37, ClÃment Leger wrote:
Hi Rishabh,

While being interesting to have a such a userspace interface, I have
some remarks.

----- On 26 Mar, 2020, at 17:50, Rishabh Bhatnagar
rishabhb@xxxxxxxxxxxxxx wrote:

Add the driver for creating the character device interface for
userspace applications. The character device interface can be used
in order to boot up and shutdown the remote processor.
This might be helpful for remote processors that are booted by
userspace applications and need to shutdown when the application
crahes/shutsdown.

Change-Id: If23c8986272bb7c943eb76665f127ff706f12394
Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
---
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_internal.h | 6 +++
drivers/remoteproc/remoteproc_userspace.c | 90 +++++++++++++++++++++++++++++++
include/linux/remoteproc.h | 2 +
4 files changed, 99 insertions(+)
create mode 100644 drivers/remoteproc/remoteproc_userspace.c

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e30a1b1..facb3fa 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_REMOTEPROC) += remoteproc.o
remoteproc-y := remoteproc_core.o
remoteproc-y += remoteproc_debugfs.o
remoteproc-y += remoteproc_sysfs.o
+remoteproc-y += remoteproc_userspace.o
remoteproc-y += remoteproc_virtio.o
remoteproc-y += remoteproc_elf_loader.o
obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
diff --git a/drivers/remoteproc/remoteproc_internal.h
b/drivers/remoteproc/remoteproc_internal.h
index 493ef92..97513ba 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -47,6 +47,9 @@ struct dentry *rproc_create_trace_file(const char *name,
struct rproc *rproc,
int rproc_init_sysfs(void);
void rproc_exit_sysfs(void);

+void rproc_init_cdev(void);
+void rproc_exit_cdev(void);
+
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);

@@ -63,6 +66,9 @@ struct resource_table *rproc_elf_find_loaded_rsc_table(struct
rproc *rproc,
struct rproc_mem_entry *
rproc_find_carveout_by_name(struct rproc *rproc, const char *name, ...);

+/* from remoteproc_userspace.c */
+int rproc_char_device_add(struct rproc *rproc);
+
static inline
int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
{
diff --git a/drivers/remoteproc/remoteproc_userspace.c
b/drivers/remoteproc/remoteproc_userspace.c
new file mode 100644
index 0000000..2ef7679
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_userspace.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+#include <linux/mutex.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES 64
+static dev_t rproc_cdev;
+static DEFINE_IDA(cdev_minor_ida);
+
+static int rproc_open(struct inode *inode, struct file *file)
+{
+ struct rproc *rproc;
+
+ rproc = container_of(inode->i_cdev, struct rproc, char_dev);
+ if (!rproc)
+ return -EINVAL;
+
+ return rproc_boot(rproc);
+}

What happens if multiple user open this chardev ? Apparently,
rproc_boot returns 0 if already powered_up, so the next user won't know
what is the state of the rproc.
Exclusive access could probably be a good idea.
Since it is synchronized inside rproc_boot multiple users simultaneously
calling open shouldn't be a problem. If it is one after the other then
second caller will get result as 0 and assume that rproc booted successfully.
That is the expected flow right?

+
+static int rproc_release(struct inode *inode, struct file *file)
+{
+ struct rproc *rproc;
+
+ rproc = container_of(inode->i_cdev, struct rproc, char_dev);
+ if (!rproc)
+ return -EINVAL;
+
+ rproc_shutdown(rproc);
+
+ return 0;
+}
+
+static const struct file_operations rproc_fops = {
+ .open = rproc_open,
+ .release = rproc_release,
+};
+
+int rproc_char_device_add(struct rproc *rproc)
+{
+ int ret, minor;
+ dev_t cdevt;
+
+ minor = ida_simple_get(&cdev_minor_ida, 0, NUM_RPROC_DEVICES,
+ GFP_KERNEL);
+ if (minor < 0) {
+ pr_err("%s: No more minor numbers left! rc:%d\n", __func__,
+ minor);
+ return -ENODEV;
+ }

How can you make the link between the chardev and the device instance ?
I do this rproc->dev.devt = cdevt. Let me know of there is a better way
to do this?
In our case, we have several remoteproc instances and thus we will end
up having multiple chardev.

Regards,

ClÃment

rproc_char_device_add will be called for each remoteproc that is
added. So we will have one char dev per remoteproc.
+
+ cdev_init(&rproc->char_dev, &rproc_fops);
+ rproc->char_dev.owner = THIS_MODULE;
+
+ cdevt = MKDEV(MAJOR(rproc_cdev), minor);
+ ret = cdev_add(&rproc->char_dev, cdevt, 1);
+ if (ret < 0)
+ ida_simple_remove(&cdev_minor_ida, minor);
+
+ rproc->dev.devt = cdevt;
+
+ return ret;
+}
+
+void __init rproc_init_cdev(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&rproc_cdev, 0, NUM_RPROC_DEVICES, "rproc");
+ if (ret < 0) {
+ pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
+ return;
+ }
+}
+
+void __exit rproc_exit_cdev(void)
+{
+ unregister_chrdev_region(MKDEV(MAJOR(rproc_cdev), 0),
+ NUM_RPROC_DEVICES);
+}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad666..c4ca796 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -37,6 +37,7 @@

#include <linux/types.h>
#include <linux/mutex.h>
+#include <linux/cdev.h>
#include <linux/virtio.h>
#include <linux/completion.h>
#include <linux/idr.h>
@@ -514,6 +515,7 @@ struct rproc {
bool auto_boot;
struct list_head dump_segments;
int nb_vdev;
+ struct cdev char_dev;
};

/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project