Messages in this thread | | | Date | Tue, 24 Mar 2020 21:10:23 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH 1/2] remoteproc: Add userspace char device driver |
| |
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
| |