* [PATCH 0/2] Add character device interface to remoteproc @ 2020-03-26 16:50 Rishabh Bhatnagar 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar 2020-03-26 16:50 ` [PATCH 2/2] remoteproc: core: Register the character device interface Rishabh Bhatnagar 0 siblings, 2 replies; 16+ messages in thread From: Rishabh Bhatnagar @ 2020-03-26 16:50 UTC (permalink / raw) To: linux-kernel, linux-remoteproc, bjorn.andersson, ohad Cc: psodagud, tsoni, sidgup, Rishabh Bhatnagar This patch series adds a character device interface to remoteproc framework. This interface can be used by userspace clients in order to boot and shutdown the remote processor. Currently there is only a sysfs interface which the userspace clients can use. If a usersapce application crashes after booting the remote processor through the sysfs interface the remote processor does not get any indication about the crash. It might still assume that the application is running. For example modem uses remotefs service to data from disk/flash memory. If the remotefs service crashes, modem still keeps on requesting data which might lead to crash on modem. Even if the service is restarted the file handles modem requested previously would become stale. Adding a character device interface makes the remote processor tightly coupled with the user space application. A crash of the application leads to a close on the file descriptors therefore shutting down the remoteproc. Rishabh Bhatnagar (2): remoteproc: Add userspace char device driver remoteproc: core: Register the character device interface drivers/remoteproc/Makefile | 1 + drivers/remoteproc/remoteproc_core.c | 8 +- drivers/remoteproc/remoteproc_internal.h | 3 + drivers/remoteproc/remoteproc_userspace.c | 126 ++++++++++++++++++++++++++++++ include/linux/remoteproc.h | 2 + 5 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 drivers/remoteproc/remoteproc_userspace.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-26 16:50 [PATCH 0/2] Add character device interface to remoteproc Rishabh Bhatnagar @ 2020-03-26 16:50 ` Rishabh Bhatnagar 2020-03-26 17:37 ` Clément Leger ` (2 more replies) 2020-03-26 16:50 ` [PATCH 2/2] remoteproc: core: Register the character device interface Rishabh Bhatnagar 1 sibling, 3 replies; 16+ messages in thread From: Rishabh Bhatnagar @ 2020-03-26 16:50 UTC (permalink / raw) To: linux-kernel, linux-remoteproc, bjorn.andersson, ohad Cc: psodagud, tsoni, sidgup, Rishabh Bhatnagar 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); +} + +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; + } + + 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar @ 2020-03-26 17:37 ` Clément Leger 2020-03-28 0:09 ` rishabhb 2020-03-27 4:00 ` kbuild test robot 2020-03-30 22:12 ` Mathieu Poirier 2 siblings, 1 reply; 16+ messages in thread From: Clément Leger @ 2020-03-26 17:37 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-kernel, linux-remoteproc, Bjorn Andersson, Ohad Ben-Cohen, psodagud, tsoni, sidgup 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. > + > +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 ? In our case, we have several remoteproc instances and thus we will end up having multiple chardev. Regards, Clément > + > + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-26 17:37 ` Clément Leger @ 2020-03-28 0:09 ` rishabhb 2020-03-30 10:42 ` Clément Leger 0 siblings, 1 reply; 16+ messages in thread From: rishabhb @ 2020-03-28 0:09 UTC (permalink / raw) To: Clément Leger Cc: linux-kernel, linux-remoteproc, Bjorn Andersson, Ohad Ben-Cohen, psodagud, tsoni, sidgup 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-28 0:09 ` rishabhb @ 2020-03-30 10:42 ` Clément Leger 0 siblings, 0 replies; 16+ messages in thread From: Clément Leger @ 2020-03-30 10:42 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-kernel, linux-remoteproc, Bjorn Andersson, Ohad Ben-Cohen, psodagud, tsoni, sidgup Hi Rishabh, ----- On 28 Mar, 2020, at 01:09, Rishabh Bhatnagar rishabhb@codeaurora.org wrote: > 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. It will be the same for close, it will assume the rproc has been stopped ? But in fact it will still be running until the refcount is 0. > That is the expected flow right? I would expect only one caller to be successful, others should probably receive a EBUSY errno IMHO. >> >>> + >>> +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? If this is sufficient to create a link in the sysfs, then it's ok but I'm no expert here. Clément >> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar @ 2020-03-27 4:00 ` kbuild test robot 2020-03-27 4:00 ` kbuild test robot 2020-03-30 22:12 ` Mathieu Poirier 2 siblings, 0 replies; 16+ messages in thread From: kbuild test robot @ 2020-03-27 4:00 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: kbuild-all, linux-kernel, linux-remoteproc, bjorn.andersson, ohad [-- Attachment #1: Type: text/plain, Size: 2113 bytes --] Hi Rishabh, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on linux/master v5.6-rc7] [cannot apply to next-20200326] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Add-character-device-interface-to-remoteproc/20200327-062958 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9420e8ade4353a6710908ffafa23ecaf1caa0123 config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.2.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/remoteproc/remoteproc_userspace.c:31:12: error: conflicting types for 'rproc_release' 31 | static int rproc_release(struct inode *inode, struct file *file) | ^~~~~~~~~~~~~ In file included from drivers/remoteproc/remoteproc_userspace.c:14: drivers/remoteproc/remoteproc_internal.h:28:6: note: previous declaration of 'rproc_release' was here 28 | void rproc_release(struct kref *kref); | ^~~~~~~~~~~~~ vim +/rproc_release +31 drivers/remoteproc/remoteproc_userspace.c 30 > 31 static int rproc_release(struct inode *inode, struct file *file) 32 { 33 struct rproc *rproc; 34 35 rproc = container_of(inode->i_cdev, struct rproc, char_dev); 36 if (!rproc) 37 return -EINVAL; 38 39 rproc_shutdown(rproc); 40 41 return 0; 42 } 43 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 52754 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver @ 2020-03-27 4:00 ` kbuild test robot 0 siblings, 0 replies; 16+ messages in thread From: kbuild test robot @ 2020-03-27 4:00 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2169 bytes --] Hi Rishabh, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on linux/master v5.6-rc7] [cannot apply to next-20200326] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/Add-character-device-interface-to-remoteproc/20200327-062958 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9420e8ade4353a6710908ffafa23ecaf1caa0123 config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.2.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/remoteproc/remoteproc_userspace.c:31:12: error: conflicting types for 'rproc_release' 31 | static int rproc_release(struct inode *inode, struct file *file) | ^~~~~~~~~~~~~ In file included from drivers/remoteproc/remoteproc_userspace.c:14: drivers/remoteproc/remoteproc_internal.h:28:6: note: previous declaration of 'rproc_release' was here 28 | void rproc_release(struct kref *kref); | ^~~~~~~~~~~~~ vim +/rproc_release +31 drivers/remoteproc/remoteproc_userspace.c 30 > 31 static int rproc_release(struct inode *inode, struct file *file) 32 { 33 struct rproc *rproc; 34 35 rproc = container_of(inode->i_cdev, struct rproc, char_dev); 36 if (!rproc) 37 return -EINVAL; 38 39 rproc_shutdown(rproc); 40 41 return 0; 42 } 43 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 52754 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar 2020-03-26 17:37 ` Clément Leger 2020-03-27 4:00 ` kbuild test robot @ 2020-03-30 22:12 ` Mathieu Poirier 2020-03-30 22:45 ` Bjorn Andersson 2 siblings, 1 reply; 16+ messages in thread From: Mathieu Poirier @ 2020-03-30 22:12 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-kernel, linux-remoteproc, bjorn.andersson, ohad, psodagud, tsoni, sidgup On Thu, Mar 26, 2020 at 09:50:39AM -0700, Rishabh Bhatnagar 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 On my side checkpatch is complaining loudly that Change-Id tags should be removed... I wonder how you didn't get it on your side. > 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> Alphabetical oder. > + > +#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); > +} > + > +static int rproc_release(struct inode *inode, struct file *file) > +{ This function declaration conflicts with the one already defined in remoteproc_internal.h. Again, I wonder how you didn't hit that problem when you compiled this patch. > + struct rproc *rproc; > + > + rproc = container_of(inode->i_cdev, struct rproc, char_dev); > + if (!rproc) > + return -EINVAL; > + > + rproc_shutdown(rproc); The scenario I see here is that a userspace program will call open(/dev/rproc_xyz, SOME_OPTION) when it is launched. The file stays open until either the application shuts down, in which case it calls close() or it crashes. In that case the system will automatically close all file descriptors that were open by the application, which will also call rproc_shutdown(). To me the same functionality can be achieved with the current functionality provided by sysfs. When the application starts it needs to read "/sys/class/remoteproc/remoteprocX/state". If the state is "offline" then "start" should be written to "/sys/.../state". If the state is "running" the application just crashed and got restarted. In which case it needs to stop the remote processor and start it again. > + > + 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); Indentation issue. > + if (minor < 0) { > + pr_err("%s: No more minor numbers left! rc:%d\n", __func__, Here to... > + minor); And here. > + return -ENODEV; > + } > + > + 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); If the module is removed unregister_chrdev_region() is called but I don't see anywhere in there that cdev_del() is called. > + > + 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); Indentation problem. > +} > 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 next time you send a patchset, please compile it and run checkpatch on it. Thanks, Mathieu > }; > > /** > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-30 22:12 ` Mathieu Poirier @ 2020-03-30 22:45 ` Bjorn Andersson 2020-03-31 16:47 ` Mathieu Poirier 0 siblings, 1 reply; 16+ messages in thread From: Bjorn Andersson @ 2020-03-30 22:45 UTC (permalink / raw) To: Mathieu Poirier Cc: Rishabh Bhatnagar, linux-kernel, linux-remoteproc, ohad, psodagud, tsoni, sidgup On Mon 30 Mar 15:12 PDT 2020, Mathieu Poirier wrote: [..] > > + struct rproc *rproc; > > + > > + rproc = container_of(inode->i_cdev, struct rproc, char_dev); > > + if (!rproc) > > + return -EINVAL; > > + > > + rproc_shutdown(rproc); > > The scenario I see here is that a userspace program will call > open(/dev/rproc_xyz, SOME_OPTION) when it is launched. The file stays open > until either the application shuts down, in which case it calls close() or it > crashes. In that case the system will automatically close all file descriptors > that were open by the application, which will also call rproc_shutdown(). > > To me the same functionality can be achieved with the current functionality > provided by sysfs. > > When the application starts it needs to read > "/sys/class/remoteproc/remoteprocX/state". If the state is "offline" then > "start" should be written to "/sys/.../state". If the state is "running" the > application just crashed and got restarted. In which case it needs to stop the > remote processor and start it again. > A case when this would be useful is the Qualcomm modem, which relies on disk access through a "remote file system service" [1]. Today we register the service (a few layers ontop of rpmsg/GLINK) then find the modem remoteproc and write "start" into the state sysfs file. When we get a signal for termination we write "stop" into state to stop the remoteproc before exiting. There is however no way for us to indicate to the modem that rmtfs just died, e.g. a kill -9 on the process will result in the modem continue and the next IO request will fail which in most cases will be fatal. So instead having rmtfs holding /dev/rproc_foo open would upon its termination cause the modem to be stopped automatically, and as the system respawns rmtfs the modem would be started anew and the two sides would be synced up again. [1] https://github.com/andersson/rmtfs Regards, Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-30 22:45 ` Bjorn Andersson @ 2020-03-31 16:47 ` Mathieu Poirier 2020-03-31 17:38 ` rishabhb 2020-03-31 17:55 ` Bjorn Andersson 0 siblings, 2 replies; 16+ messages in thread From: Mathieu Poirier @ 2020-03-31 16:47 UTC (permalink / raw) To: Bjorn Andersson Cc: Rishabh Bhatnagar, Linux Kernel Mailing List, linux-remoteproc, Ohad Ben-Cohen, psodagud, tsoni, Siddharth Gupta On Mon, 30 Mar 2020 at 16:45, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Mon 30 Mar 15:12 PDT 2020, Mathieu Poirier wrote: > [..] > > > + struct rproc *rproc; > > > + > > > + rproc = container_of(inode->i_cdev, struct rproc, char_dev); > > > + if (!rproc) > > > + return -EINVAL; > > > + > > > + rproc_shutdown(rproc); > > > > The scenario I see here is that a userspace program will call > > open(/dev/rproc_xyz, SOME_OPTION) when it is launched. The file stays open > > until either the application shuts down, in which case it calls close() or it > > crashes. In that case the system will automatically close all file descriptors > > that were open by the application, which will also call rproc_shutdown(). > > > > To me the same functionality can be achieved with the current functionality > > provided by sysfs. > > > > When the application starts it needs to read > > "/sys/class/remoteproc/remoteprocX/state". If the state is "offline" then > > "start" should be written to "/sys/.../state". If the state is "running" the > > application just crashed and got restarted. In which case it needs to stop the > > remote processor and start it again. > > > > A case when this would be useful is the Qualcomm modem, which relies on > disk access through a "remote file system service" [1]. > > Today we register the service (a few layers ontop of rpmsg/GLINK) then > find the modem remoteproc and write "start" into the state sysfs file. > > When we get a signal for termination we write "stop" into state to stop > the remoteproc before exiting. > > There is however no way for us to indicate to the modem that rmtfs just > died, e.g. a kill -9 on the process will result in the modem continue > and the next IO request will fail which in most cases will be fatal. The modem will crash when attempting an IO while rmtfs is down? > > So instead having rmtfs holding /dev/rproc_foo open would upon its > termination cause the modem to be stopped automatically, and as the > system respawns rmtfs the modem would be started anew and the two sides > would be synced up again. I have a better idea of what is going on now - thanks for writing this up. I would make this feature a kernel configurable option as some people may not want it. I also think having "/dev/remoteprocX" is fine, so no need to change anything currently visible in sysfs. Mathieu > > [1] https://github.com/andersson/rmtfs > > Regards, > Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-31 16:47 ` Mathieu Poirier @ 2020-03-31 17:38 ` rishabhb 2020-03-31 17:55 ` Bjorn Andersson 1 sibling, 0 replies; 16+ messages in thread From: rishabhb @ 2020-03-31 17:38 UTC (permalink / raw) To: Mathieu Poirier Cc: Bjorn Andersson, Linux Kernel Mailing List, linux-remoteproc, Ohad Ben-Cohen, psodagud, tsoni, Siddharth Gupta, linux-remoteproc-owner On 2020-03-31 09:47, Mathieu Poirier wrote: > On Mon, 30 Mar 2020 at 16:45, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: >> >> On Mon 30 Mar 15:12 PDT 2020, Mathieu Poirier wrote: >> [..] >> > > + struct rproc *rproc; >> > > + >> > > + rproc = container_of(inode->i_cdev, struct rproc, char_dev); >> > > + if (!rproc) >> > > + return -EINVAL; >> > > + >> > > + rproc_shutdown(rproc); >> > >> > The scenario I see here is that a userspace program will call >> > open(/dev/rproc_xyz, SOME_OPTION) when it is launched. The file stays open >> > until either the application shuts down, in which case it calls close() or it >> > crashes. In that case the system will automatically close all file descriptors >> > that were open by the application, which will also call rproc_shutdown(). >> > >> > To me the same functionality can be achieved with the current functionality >> > provided by sysfs. >> > >> > When the application starts it needs to read >> > "/sys/class/remoteproc/remoteprocX/state". If the state is "offline" then >> > "start" should be written to "/sys/.../state". If the state is "running" the >> > application just crashed and got restarted. In which case it needs to stop the >> > remote processor and start it again. >> > >> >> A case when this would be useful is the Qualcomm modem, which relies >> on >> disk access through a "remote file system service" [1]. >> >> Today we register the service (a few layers ontop of rpmsg/GLINK) then >> find the modem remoteproc and write "start" into the state sysfs file. >> >> When we get a signal for termination we write "stop" into state to >> stop >> the remoteproc before exiting. >> >> There is however no way for us to indicate to the modem that rmtfs >> just >> died, e.g. a kill -9 on the process will result in the modem continue >> and the next IO request will fail which in most cases will be fatal. I have this scenario written down in the cover letter for this patchset "[PATCH 0/2] Add character device interface to remoteproc" I'll add it to the commit text as well. > > The modem will crash when attempting an IO while rmtfs is down? > >> >> So instead having rmtfs holding /dev/rproc_foo open would upon its >> termination cause the modem to be stopped automatically, and as the >> system respawns rmtfs the modem would be started anew and the two >> sides >> would be synced up again. > > I have a better idea of what is going on now - thanks for writing this > up. > > I would make this feature a kernel configurable option as some people > may not want it. I also think having "/dev/remoteprocX" is fine, so > no need to change anything currently visible in sysfs. Ok. Makes sense. > > Mathieu > >> >> [1] https://github.com/andersson/rmtfs >> >> Regards, >> Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-31 16:47 ` Mathieu Poirier 2020-03-31 17:38 ` rishabhb @ 2020-03-31 17:55 ` Bjorn Andersson 1 sibling, 0 replies; 16+ messages in thread From: Bjorn Andersson @ 2020-03-31 17:55 UTC (permalink / raw) To: Mathieu Poirier Cc: Rishabh Bhatnagar, Linux Kernel Mailing List, linux-remoteproc, Ohad Ben-Cohen, psodagud, tsoni, Siddharth Gupta On Tue 31 Mar 09:47 PDT 2020, Mathieu Poirier wrote: > On Mon, 30 Mar 2020 at 16:45, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Mon 30 Mar 15:12 PDT 2020, Mathieu Poirier wrote: > > [..] > > > > + struct rproc *rproc; > > > > + > > > > + rproc = container_of(inode->i_cdev, struct rproc, char_dev); > > > > + if (!rproc) > > > > + return -EINVAL; > > > > + > > > > + rproc_shutdown(rproc); > > > > > > The scenario I see here is that a userspace program will call > > > open(/dev/rproc_xyz, SOME_OPTION) when it is launched. The file stays open > > > until either the application shuts down, in which case it calls close() or it > > > crashes. In that case the system will automatically close all file descriptors > > > that were open by the application, which will also call rproc_shutdown(). > > > > > > To me the same functionality can be achieved with the current functionality > > > provided by sysfs. > > > > > > When the application starts it needs to read > > > "/sys/class/remoteproc/remoteprocX/state". If the state is "offline" then > > > "start" should be written to "/sys/.../state". If the state is "running" the > > > application just crashed and got restarted. In which case it needs to stop the > > > remote processor and start it again. > > > > > > > A case when this would be useful is the Qualcomm modem, which relies on > > disk access through a "remote file system service" [1]. > > > > Today we register the service (a few layers ontop of rpmsg/GLINK) then > > find the modem remoteproc and write "start" into the state sysfs file. > > > > When we get a signal for termination we write "stop" into state to stop > > the remoteproc before exiting. > > > > There is however no way for us to indicate to the modem that rmtfs just > > died, e.g. a kill -9 on the process will result in the modem continue > > and the next IO request will fail which in most cases will be fatal. > > The modem will crash when attempting an IO while rmtfs is down? > In certain cases there's nothing else to do. > > > > So instead having rmtfs holding /dev/rproc_foo open would upon its > > termination cause the modem to be stopped automatically, and as the > > system respawns rmtfs the modem would be started anew and the two sides > > would be synced up again. > > I have a better idea of what is going on now - thanks for writing this up. > > I would make this feature a kernel configurable option as some people > may not want it. Sounds reasonable. > I also think having "/dev/remoteprocX" is fine, so > no need to change anything currently visible in sysfs. > I agree, it sure is annoying that remoteproc%d isn't stable, but it's what we have and consistency is important. Regards, Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] remoteproc: core: Register the character device interface 2020-03-26 16:50 [PATCH 0/2] Add character device interface to remoteproc Rishabh Bhatnagar 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar @ 2020-03-26 16:50 ` Rishabh Bhatnagar 2020-03-30 22:13 ` Mathieu Poirier 1 sibling, 1 reply; 16+ messages in thread From: Rishabh Bhatnagar @ 2020-03-26 16:50 UTC (permalink / raw) To: linux-kernel, linux-remoteproc, bjorn.andersson, ohad Cc: psodagud, tsoni, sidgup, Rishabh Bhatnagar Add the character device during rproc_add. This would create a character device node at /dev/subsys_<rproc_name>. Userspace applications can interact with the remote processor using this interface rather than using sysfs node. To distinguish between different remote processor nodes the device name has been changed to include the rproc name appended to "rproc_" string. Change-Id: I2114f77f8d2b5fd97e281021ec9905ef5c2fb54c Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> --- drivers/remoteproc/remoteproc_core.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 097f33e..f657983 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1907,6 +1907,12 @@ int rproc_add(struct rproc *rproc) struct device *dev = &rproc->dev; int ret; + ret = rproc_char_device_add(rproc); + if (ret) { + pr_err("error while adding character device\n"); + return ret; + } + ret = device_add(dev); if (ret < 0) return ret; @@ -2044,7 +2050,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, return NULL; } - dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); + dev_set_name(&rproc->dev, "rproc_%s", rproc->name); atomic_set(&rproc->power, 0); @@ -2220,6 +2226,7 @@ static int __init remoteproc_init(void) { rproc_init_sysfs(); rproc_init_debugfs(); + rproc_init_cdev(); return 0; } @@ -2231,6 +2238,7 @@ static void __exit remoteproc_exit(void) rproc_exit_debugfs(); rproc_exit_sysfs(); + rproc_exit_cdev(); } module_exit(remoteproc_exit); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] remoteproc: core: Register the character device interface 2020-03-26 16:50 ` [PATCH 2/2] remoteproc: core: Register the character device interface Rishabh Bhatnagar @ 2020-03-30 22:13 ` Mathieu Poirier 0 siblings, 0 replies; 16+ messages in thread From: Mathieu Poirier @ 2020-03-30 22:13 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-kernel, linux-remoteproc, bjorn.andersson, ohad, psodagud, tsoni, sidgup On Thu, Mar 26, 2020 at 09:50:40AM -0700, Rishabh Bhatnagar wrote: > Add the character device during rproc_add. This would create > a character device node at /dev/subsys_<rproc_name>. Userspace > applications can interact with the remote processor using this > interface rather than using sysfs node. To distinguish between > different remote processor nodes the device name has been changed > to include the rproc name appended to "rproc_" string. > > Change-Id: I2114f77f8d2b5fd97e281021ec9905ef5c2fb54c > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> > --- > drivers/remoteproc/remoteproc_core.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 097f33e..f657983 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1907,6 +1907,12 @@ int rproc_add(struct rproc *rproc) > struct device *dev = &rproc->dev; > int ret; > > + ret = rproc_char_device_add(rproc); > + if (ret) { > + pr_err("error while adding character device\n"); > + return ret; > + } > + > ret = device_add(dev); > if (ret < 0) > return ret; > @@ -2044,7 +2050,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > return NULL; > } > > - dev_set_name(&rproc->dev, "remoteproc%d", rproc->index); > + dev_set_name(&rproc->dev, "rproc_%s", rproc->name); Unfortunately you can't do that. The name of the remoteproc in sysfs is visible by users and a lot of scripts will fail because of this change. > > atomic_set(&rproc->power, 0); > > @@ -2220,6 +2226,7 @@ static int __init remoteproc_init(void) > { > rproc_init_sysfs(); > rproc_init_debugfs(); > + rproc_init_cdev(); > > return 0; > } > @@ -2231,6 +2238,7 @@ static void __exit remoteproc_exit(void) > > rproc_exit_debugfs(); > rproc_exit_sysfs(); > + rproc_exit_cdev(); > } > module_exit(remoteproc_exit); > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/2] Add character device interface to remoteproc @ 2020-03-20 23:36 Rishabh Bhatnagar 2020-03-20 23:36 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar 0 siblings, 1 reply; 16+ messages in thread From: Rishabh Bhatnagar @ 2020-03-20 23:36 UTC (permalink / raw) To: linux-remoteproc-owner, linux-kernel, bjorn.andersson, mathieu.poirier Cc: psodagud, tsoni, sidgup, Rishabh Bhatnagar This patch series adds a character device interface to remoteproc framework. This interface can be used by userspace clients in order to boot and shutdown the remote processor. Currently there is only a sysfs interface which the userspace clients can use. If a usersapce application crashes after booting the remote processor through the sysfs interface the remote processor does not get any indication about the crash. It might still assume that the application is running. For example modem uses remotefs service to data from disk/flash memory. If the remotefs service crashes, modem still keeps on requesting data which might lead to crash on modem. Even if the service is restarted the file handles modem requested previously would become stale. Adding a character device interface makes the remote processor tightly coupled with the user space application. A crash of the application leads to a close on the file descriptors therefore shutting down the remoteproc. Rishabh Bhatnagar (2): remoteproc: Add userspace char device driver remoteproc: core: Register the character device interface drivers/remoteproc/Makefile | 1 + drivers/remoteproc/remoteproc_core.c | 8 +- drivers/remoteproc/remoteproc_internal.h | 3 + drivers/remoteproc/remoteproc_userspace.c | 126 ++++++++++++++++++++++++++++++ include/linux/remoteproc.h | 2 + 5 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 drivers/remoteproc/remoteproc_userspace.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-20 23:36 [PATCH 0/2] Add character device interface to remoteproc Rishabh Bhatnagar @ 2020-03-20 23:36 ` Rishabh Bhatnagar 2020-03-25 4:10 ` Bjorn Andersson 0 siblings, 1 reply; 16+ messages in thread From: Rishabh Bhatnagar @ 2020-03-20 23:36 UTC (permalink / raw) To: linux-remoteproc-owner, linux-kernel, bjorn.andersson, mathieu.poirier Cc: psodagud, tsoni, sidgup, Rishabh Bhatnagar 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. Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> --- drivers/remoteproc/Makefile | 1 + drivers/remoteproc/remoteproc_internal.h | 3 + drivers/remoteproc/remoteproc_userspace.c | 126 ++++++++++++++++++++++++++++++ include/linux/remoteproc.h | 2 + 4 files changed, 132 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..bafaa12 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -63,6 +63,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 */ +extern 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..e3017e7 --- /dev/null +++ b/drivers/remoteproc/remoteproc_userspace.c @@ -0,0 +1,126 @@ +// 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" + +static LIST_HEAD(rproc_chrdev_list); + +struct rproc_char_dev { + struct list_head node; + dev_t dev_no; + struct rproc *rproc; +}; + +static DEFINE_MUTEX(rproc_chrdev_lock); + +static struct rproc *rproc_get_by_dev_no(int minor) +{ + struct rproc_char_dev *r; + + mutex_lock(&rproc_chrdev_lock); + list_for_each_entry(r, &rproc_chrdev_list, node) { + if (MINOR(r->dev_no) == minor) + break; + } + mutex_unlock(&rproc_chrdev_lock); + + return r->rproc; +} + +static int rproc_open(struct inode *inode, struct file *file) +{ + struct rproc *rproc; + int retval; + + rproc = rproc_get_by_dev_no(iminor(inode)); + if (!rproc) + return -EINVAL; + + if (!try_module_get(rproc->dev.parent->driver->owner)) { + dev_err(&rproc->dev, "can't get owner\n"); + return -EINVAL; + } + + get_device(&rproc->dev); + retval = rproc_boot(rproc); + + return retval; +} + +static int rproc_close(struct inode *inode, struct file *file) +{ + struct rproc *rproc; + + rproc = rproc_get_by_dev_no(iminor(inode)); + if (!rproc) + return -EINVAL; + + rproc_shutdown(rproc); + rproc_put(rproc); + + return 0; +} + +static const struct file_operations rproc_fops = { + .open = rproc_open, + .release = rproc_close, +}; + +int rproc_char_device_add(struct rproc *rproc) +{ + int ret = 0; + static int major, minor; + dev_t dev_no; + struct rproc_char_dev *chrdev; + + mutex_lock(&rproc_chrdev_lock); + if (!major) { + ret = alloc_chrdev_region(&dev_no, 0, 4, "subsys"); + if (ret < 0) { + pr_err("Failed to alloc subsys_dev region, err %d\n", + ret); + goto fail; + } + major = MAJOR(dev_no); + minor = MINOR(dev_no); + } else + dev_no = MKDEV(major, minor); + + cdev_init(&rproc->char_dev, &rproc_fops); + rproc->char_dev.owner = THIS_MODULE; + ret = cdev_add(&rproc->char_dev, dev_no, 1); + if (ret < 0) + goto fail_unregister_cdev_region; + + rproc->dev.devt = dev_no; + + chrdev = kzalloc(sizeof(struct rproc_char_dev), GFP_KERNEL); + if (!chrdev) { + ret = -ENOMEM; + goto fail_unregister_cdev_region; + } + + chrdev->rproc = rproc; + chrdev->dev_no = dev_no; + list_add(&chrdev->node, &rproc_chrdev_list); + ++minor; + mutex_unlock(&rproc_chrdev_lock); + + return 0; + +fail_unregister_cdev_region: + unregister_chrdev_region(dev_no, 1); +fail: + mutex_unlock(&rproc_chrdev_lock); + return ret; +} 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] remoteproc: Add userspace char device driver 2020-03-20 23:36 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar @ 2020-03-25 4:10 ` Bjorn Andersson 0 siblings, 0 replies; 16+ messages in thread From: Bjorn Andersson @ 2020-03-25 4:10 UTC (permalink / raw) To: Rishabh Bhatnagar Cc: linux-remoteproc-owner, linux-kernel, mathieu.poirier, psodagud, tsoni, sidgup On Fri 20 Mar 16:36 PDT 2020, Rishabh Bhatnagar 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. > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org> Please use linux-remoteproc@ instead of linux-remoteproc-owner@, to ensure you're reaching the whole community. > --- > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/remoteproc_internal.h | 3 + > drivers/remoteproc/remoteproc_userspace.c | 126 ++++++++++++++++++++++++++++++ > include/linux/remoteproc.h | 2 + > 4 files changed, 132 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..bafaa12 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -63,6 +63,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 */ > +extern int rproc_char_device_add(struct rproc *rproc); Please omit "external" from this. > + > 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..e3017e7 > --- /dev/null > +++ b/drivers/remoteproc/remoteproc_userspace.c > @@ -0,0 +1,126 @@ > +// 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" > + > +static LIST_HEAD(rproc_chrdev_list); > + > +struct rproc_char_dev { > + struct list_head node; > + dev_t dev_no; > + struct rproc *rproc; > +}; Per below suggestions you don't need this struct (or list). > + > +static DEFINE_MUTEX(rproc_chrdev_lock); > + > +static struct rproc *rproc_get_by_dev_no(int minor) > +{ > + struct rproc_char_dev *r; > + > + mutex_lock(&rproc_chrdev_lock); > + list_for_each_entry(r, &rproc_chrdev_list, node) { > + if (MINOR(r->dev_no) == minor) > + break; > + } > + mutex_unlock(&rproc_chrdev_lock); > + > + return r->rproc; > +} > + > +static int rproc_open(struct inode *inode, struct file *file) > +{ > + struct rproc *rproc; > + int retval; > + rproc can be found with: container_of(inode->i_cdev, struct rproc, char_dev); so you don't need rproc_get_by_dev_no() and hence not rproc_chrdev_list. > + rproc = rproc_get_by_dev_no(iminor(inode)); > + if (!rproc) > + return -EINVAL; > + > + if (!try_module_get(rproc->dev.parent->driver->owner)) { > + dev_err(&rproc->dev, "can't get owner\n"); > + return -EINVAL; > + } > + > + get_device(&rproc->dev); > + retval = rproc_boot(rproc); return rproc_boot(); and drop "retval". > + > + return retval; > +} > + > +static int rproc_close(struct inode *inode, struct file *file) s/close/release/ > +{ > + struct rproc *rproc; > + > + rproc = rproc_get_by_dev_no(iminor(inode)); > + if (!rproc) > + return -EINVAL; > + > + rproc_shutdown(rproc); > + rproc_put(rproc); > + > + return 0; > +} > + > +static const struct file_operations rproc_fops = { > + .open = rproc_open, > + .release = rproc_close, > +}; > + > +int rproc_char_device_add(struct rproc *rproc) > +{ > + int ret = 0; No need to initialize ret, its first use is an assignment. > + static int major, minor; Move these to the top of the file, and use an ida for allocating the minor. > + dev_t dev_no; > + struct rproc_char_dev *chrdev; > + > + mutex_lock(&rproc_chrdev_lock); > + if (!major) { > + ret = alloc_chrdev_region(&dev_no, 0, 4, "subsys"); Please do this during remoteproc_init() Regards, Bjorn > + if (ret < 0) { > + pr_err("Failed to alloc subsys_dev region, err %d\n", > + ret); > + goto fail; > + } > + major = MAJOR(dev_no); > + minor = MINOR(dev_no); > + } else > + dev_no = MKDEV(major, minor); > + > + cdev_init(&rproc->char_dev, &rproc_fops); > + rproc->char_dev.owner = THIS_MODULE; > + ret = cdev_add(&rproc->char_dev, dev_no, 1); > + if (ret < 0) > + goto fail_unregister_cdev_region; > + > + rproc->dev.devt = dev_no; > + > + chrdev = kzalloc(sizeof(struct rproc_char_dev), GFP_KERNEL); > + if (!chrdev) { > + ret = -ENOMEM; > + goto fail_unregister_cdev_region; > + } > + > + chrdev->rproc = rproc; > + chrdev->dev_no = dev_no; > + list_add(&chrdev->node, &rproc_chrdev_list); > + ++minor; > + mutex_unlock(&rproc_chrdev_lock); > + > + return 0; > + > +fail_unregister_cdev_region: > + unregister_chrdev_region(dev_no, 1); > +fail: > + mutex_unlock(&rproc_chrdev_lock); > + return ret; > +} > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-03-31 17:55 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-26 16:50 [PATCH 0/2] Add character device interface to remoteproc Rishabh Bhatnagar 2020-03-26 16:50 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar 2020-03-26 17:37 ` Clément Leger 2020-03-28 0:09 ` rishabhb 2020-03-30 10:42 ` Clément Leger 2020-03-27 4:00 ` kbuild test robot 2020-03-27 4:00 ` kbuild test robot 2020-03-30 22:12 ` Mathieu Poirier 2020-03-30 22:45 ` Bjorn Andersson 2020-03-31 16:47 ` Mathieu Poirier 2020-03-31 17:38 ` rishabhb 2020-03-31 17:55 ` Bjorn Andersson 2020-03-26 16:50 ` [PATCH 2/2] remoteproc: core: Register the character device interface Rishabh Bhatnagar 2020-03-30 22:13 ` Mathieu Poirier -- strict thread matches above, loose matches on Subject: below -- 2020-03-20 23:36 [PATCH 0/2] Add character device interface to remoteproc Rishabh Bhatnagar 2020-03-20 23:36 ` [PATCH 1/2] remoteproc: Add userspace char device driver Rishabh Bhatnagar 2020-03-25 4:10 ` Bjorn Andersson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.