| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200325041023.GE522435@yoga>
Date: Tue, 24 Mar 2020 21:10:23 -0700
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Rishabh Bhatnagar <rishabhb@...eaurora.org>
Cc: linux-remoteproc-owner@...r.kernel.org,
linux-kernel@...r.kernel.org, mathieu.poirier@...aro.org,
psodagud@...eaurora.org, tsoni@...eaurora.org,
sidgup@...eaurora.org
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@...eaurora.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
Powered by blists - more mailing lists