Messages in this thread |  | | Date | Fri, 27 Mar 2020 17:09:41 -0700 | From | rishabhb@codeauro ... | Subject | Re: [PATCH 1/2] remoteproc: Add userspace char device driver |
| |
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@codeaurora.org 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@codeaurora.org> >> --- >> 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
|  |