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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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