This patch series adds a character device interface to remoteproc
framework. 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.
Changelog:
v3 -> v4:
- Addressed comments from Mathieu and Arnaud.
- Added locks while writing/reading from the automatic-shutdown-on-release bool.
- Changed return value when failing to copy to/from userspace.
- Changed logic for calling shutdown on release.
- Moved around code after the increase of max line length from 80 to 100.
- Moved the call adding character device before device_add in rproc_add to add
both sysfs and character device interface together.
v2 -> v3:
- Move booting of remoteproc from open to a write call.
- Add ioctl interface for future functionality extension.
- Add an ioctl call to default to rproc shutdown on release.
v1 -> v2:
- Fixed comments from Bjorn and Matthew.
Siddharth Gupta (2):
remoteproc: Add remoteproc character device interface
remoteproc: core: Register the character device interface
Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
drivers/remoteproc/Kconfig | 9 ++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
drivers/remoteproc/remoteproc_core.c | 10 ++
drivers/remoteproc/remoteproc_internal.h | 28 ++++
include/linux/remoteproc.h | 5 +
include/uapi/linux/remoteproc_cdev.h | 37 ++++++
8 files changed, 237 insertions(+)
create mode 100644 drivers/remoteproc/remoteproc_cdev.c
create mode 100644 include/uapi/linux/remoteproc_cdev.h
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Add the character device during rproc_add. This would create
a character device node at /dev/remoteproc<index>. Userspace
applications can interact with the remote processor using this
interface.
Signed-off-by: Rishabh Bhatnagar <[email protected]>
Signed-off-by: Siddharth Gupta <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0f95e02..ec7fb49 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1966,6 +1966,13 @@ int rproc_add(struct rproc *rproc)
struct device *dev = &rproc->dev;
int ret;
+ /* add char device for this remoteproc */
+ ret = rproc_char_device_add(rproc);
+ if (ret) {
+ dev_err(dev, "Failed to add char dev for %s\n", rproc->name);
+ return ret;
+ }
+
ret = device_add(dev);
if (ret < 0)
return ret;
@@ -2241,6 +2248,7 @@ int rproc_del(struct rproc *rproc)
mutex_unlock(&rproc->lock);
rproc_delete_debug_dir(rproc);
+ rproc_char_device_remove(rproc);
/* the rproc is downref'ed as soon as it's removed from the klist */
mutex_lock(&rproc_list_mutex);
@@ -2409,6 +2417,7 @@ static int __init remoteproc_init(void)
{
rproc_init_sysfs();
rproc_init_debugfs();
+ rproc_init_cdev();
rproc_init_panic();
return 0;
@@ -2420,6 +2429,7 @@ static void __exit remoteproc_exit(void)
ida_destroy(&rproc_dev_index);
rproc_exit_panic();
+ rproc_exit_cdev();
rproc_exit_debugfs();
rproc_exit_sysfs();
}
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or hangs.
Signed-off-by: Rishabh Bhatnagar <[email protected]>
Signed-off-by: Siddharth Gupta <[email protected]>
---
Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
drivers/remoteproc/Kconfig | 9 ++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 28 ++++
include/linux/remoteproc.h | 5 +
include/uapi/linux/remoteproc_cdev.h | 37 ++++++
7 files changed, 227 insertions(+)
create mode 100644 drivers/remoteproc/remoteproc_cdev.c
create mode 100644 include/uapi/linux/remoteproc_cdev.h
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 59472cd..2a19883 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -339,6 +339,7 @@ Code Seq# Include File Comments
0xB4 00-0F linux/gpio.h <mailto:[email protected]>
0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
0xB6 all linux/fpga-dfl.h
+0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
0xC0 00-0F linux/usb/iowarrior.h
0xCA 00-0F uapi/misc/cxl.h
0xCA 10-2F uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c4d1731..652060f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC
if REMOTEPROC
+config REMOTEPROC_CDEV
+ bool "Remoteproc character device interface"
+ help
+ Say y here to have a character device interface for the remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e8b886e..311ae3f 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
remoteproc-y += remoteproc_sysfs.o
remoteproc-y += remoteproc_virtio.o
remoteproc-y += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 0000000..8a0eb47
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,146 @@
+// 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/cdev.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/compat.h>
+#include <linux/remoteproc.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/remoteproc_cdev.h>
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES 64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
+{
+ struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
+ int ret = 0;
+ char cmd[10];
+
+ if (!len || len > sizeof(cmd))
+ return -EINVAL;
+
+ ret = copy_from_user(cmd, buf, sizeof(cmd));
+ if (ret)
+ return -EFAULT;
+
+ if (sysfs_streq(cmd, "start")) {
+ if (rproc->state == RPROC_RUNNING)
+ return -EBUSY;
+
+ ret = rproc_boot(rproc);
+ if (ret)
+ dev_err(&rproc->dev, "Boot failed:%d\n", ret);
+ } else if (sysfs_streq(cmd, "stop")) {
+ if (rproc->state != RPROC_RUNNING)
+ return -EINVAL;
+
+ rproc_shutdown(rproc);
+ } else {
+ dev_err(&rproc->dev, "Unrecognized option\n");
+ ret = -EINVAL;
+ }
+
+ return ret ? ret : len;
+}
+
+static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
+{
+ struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
+ void __user *argp = compat_ptr(arg);
+ int ret;
+ int32_t param;
+
+ switch (ioctl) {
+ case RPROC_SET_SHUTDOWN_ON_RELEASE:
+ ret = copy_from_user(¶m, argp, sizeof(int32_t));
+ if (ret) {
+ dev_err(&rproc->dev, "Data copy from userspace failed\n");
+ return -EFAULT;
+ }
+ mutex_lock(&rproc->lock);
+ rproc->cdev_put_on_release = param ? true : false;
+ mutex_unlock(&rproc->lock);
+ break;
+ case RPROC_GET_SHUTDOWN_ON_RELEASE:
+ mutex_lock(&rproc->lock);
+ ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
+ mutex_unlock(&rproc->lock);
+ if (ret) {
+ dev_err(&rproc->dev, "Data copy to userspace failed\n");
+ return -EFAULT;
+ }
+ break;
+ default:
+ dev_err(&rproc->dev, "Unsupported ioctl\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rproc_cdev_release(struct inode *inode, struct file *filp)
+{
+ struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
+ bool release;
+
+ mutex_lock(&rproc->lock);
+ release = rproc->cdev_put_on_release;
+ mutex_unlock(&rproc->lock);
+
+ if (release && rproc->state == RPROC_RUNNING)
+ rproc_shutdown(rproc);
+
+ return 0;
+}
+
+static const struct file_operations rproc_fops = {
+ .write = rproc_cdev_write,
+ .compat_ioctl = rproc_device_ioctl,
+ .release = rproc_cdev_release,
+};
+
+int rproc_char_device_add(struct rproc *rproc)
+{
+ int ret;
+ dev_t cdevt;
+
+ cdev_init(&rproc->char_dev, &rproc_fops);
+ rproc->char_dev.owner = THIS_MODULE;
+
+ cdevt = MKDEV(rproc_major, rproc->index);
+ ret = cdev_add(&rproc->char_dev, cdevt, 1);
+ if (ret < 0)
+ goto out;
+
+ rproc->dev.devt = cdevt;
+out:
+ return ret;
+}
+
+void rproc_char_device_remove(struct rproc *rproc)
+{
+ __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
+}
+
+void __init rproc_init_cdev(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
+ if (ret < 0)
+ pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
+}
+
+void __exit rproc_exit_cdev(void)
+{
+ unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
+}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 4ba7cb5..f091ddc 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -47,6 +47,34 @@ extern struct class rproc_class;
int rproc_init_sysfs(void);
void rproc_exit_sysfs(void);
+#ifdef CONFIG_REMOTEPROC_CDEV
+void rproc_init_cdev(void);
+void rproc_exit_cdev(void);
+int rproc_char_device_add(struct rproc *rproc);
+void rproc_char_device_remove(struct rproc *rproc);
+#else
+static inline void rproc_init_cdev(void)
+{
+}
+
+static inline void rproc_exit_cdev(void)
+{
+}
+
+/*
+ * The character device interface is an optional feature, if it is not enabled
+ * the function should not return an error.
+ */
+static inline int rproc_char_device_add(struct rproc *rproc)
+{
+ return 0;
+}
+
+static inline void rproc_char_device_remove(struct rproc *rproc)
+{
+}
+#endif
+
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e7b7bab..669cbfb 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -40,6 +40,7 @@
#include <linux/virtio.h>
#include <linux/completion.h>
#include <linux/idr.h>
+#include <linux/cdev.h>
#include <linux/of.h>
/**
@@ -488,6 +489,8 @@ struct rproc_dump_segment {
* @auto_boot: flag to indicate if remote processor should be auto-started
* @dump_segments: list of segments in the firmware
* @nb_vdev: number of vdev currently handled by rproc
+ * @char_dev: character device of the rproc
+ * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
*/
struct rproc {
struct list_head node;
@@ -523,6 +526,8 @@ struct rproc {
int nb_vdev;
u8 elf_class;
u16 elf_machine;
+ struct cdev char_dev;
+ bool cdev_put_on_release;
};
/**
diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
new file mode 100644
index 0000000..c43768e
--- /dev/null
+++ b/include/uapi/linux/remoteproc_cdev.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/*
+ * IOCTLs for Remoteproc's character device interface.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _UAPI_REMOTEPROC_CDEV_H_
+#define _UAPI_REMOTEPROC_CDEV_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define RPROC_MAGIC 0xB7
+
+/*
+ * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
+ * processor automatically when the controlling userpsace closes the char device interface.
+ *
+ * input parameter: integer
+ * 0 : disable automatic shutdown
+ * other : enable automatic shutdown
+ */
+#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
+
+/*
+ * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
+ * a remote processor is enabled or disabled when the controlling userspace closes the char device
+ * interface.
+ *
+ * output parameter: integer
+ * 0 : automatic shutdown disable
+ * other : automatic shutdown enable
+ */
+#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
+
+#endif
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> Add the character device interface into remoteproc framework.
> This interface can be used in order to boot/shutdown remote
> subsystems and provides a basic ioctl based interface to implement
> supplementary functionality. An ioctl call is implemented to enable
> the shutdown on release feature which will allow remote processors to
> be shutdown when the controlling userpsace application crashes or hangs.
>
> Signed-off-by: Rishabh Bhatnagar <[email protected]>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> drivers/remoteproc/Kconfig | 9 ++
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 28 ++++
> include/linux/remoteproc.h | 5 +
> include/uapi/linux/remoteproc_cdev.h | 37 ++++++
> 7 files changed, 227 insertions(+)
> create mode 100644 drivers/remoteproc/remoteproc_cdev.c
> create mode 100644 include/uapi/linux/remoteproc_cdev.h
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd..2a19883 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -339,6 +339,7 @@ Code Seq# Include File Comments
> 0xB4 00-0F linux/gpio.h <mailto:[email protected]>
> 0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
> 0xB6 all linux/fpga-dfl.h
> +0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
> 0xC0 00-0F linux/usb/iowarrior.h
> 0xCA 00-0F uapi/misc/cxl.h
> 0xCA 10-2F uapi/misc/ocxl.h
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c4d1731..652060f 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -14,6 +14,15 @@ config REMOTEPROC
>
> if REMOTEPROC
>
> +config REMOTEPROC_CDEV
> + bool "Remoteproc character device interface"
> + help
> + Say y here to have a character device interface for the remoteproc
> + framework. Userspace can boot/shutdown remote processors through
> + this interface.
> +
> + It's safe to say N if you don't want to use this interface.
> +
> config IMX_REMOTEPROC
> tristate "IMX6/7 remoteproc support"
> depends on ARCH_MXC
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index e8b886e..311ae3f 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> remoteproc-y += remoteproc_sysfs.o
> remoteproc-y += remoteproc_virtio.o
> remoteproc-y += remoteproc_elf_loader.o
> +obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> new file mode 100644
> index 0000000..8a0eb47
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -0,0 +1,146 @@
> +// 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/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/compat.h>
> +#include <linux/remoteproc.h>
> +#include <linux/uaccess.h>
> +#include <uapi/linux/remoteproc_cdev.h>
Alphabetical order please.
> +
> +#include "remoteproc_internal.h"
> +
> +#define NUM_RPROC_DEVICES 64
> +static dev_t rproc_major;
> +
> +static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
> +{
> + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> + int ret = 0;
> + char cmd[10];
> +
> + if (!len || len > sizeof(cmd))
> + return -EINVAL;
> +
> + ret = copy_from_user(cmd, buf, sizeof(cmd));
> + if (ret)
> + return -EFAULT;
> +
> + if (sysfs_streq(cmd, "start")) {
> + if (rproc->state == RPROC_RUNNING)
> + return -EBUSY;
> +
> + ret = rproc_boot(rproc);
> + if (ret)
> + dev_err(&rproc->dev, "Boot failed:%d\n", ret);
> + } else if (sysfs_streq(cmd, "stop")) {
> + if (rproc->state != RPROC_RUNNING)
> + return -EINVAL;
> +
> + rproc_shutdown(rproc);
> + } else {
> + dev_err(&rproc->dev, "Unrecognized option\n");
> + ret = -EINVAL;
> + }
> +
> + return ret ? ret : len;
> +}
> +
> +static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> +{
> + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> + void __user *argp = compat_ptr(arg);
> + int ret;
> + int32_t param;
> +
> + switch (ioctl) {
> + case RPROC_SET_SHUTDOWN_ON_RELEASE:
> + ret = copy_from_user(¶m, argp, sizeof(int32_t));
> + if (ret) {
> + dev_err(&rproc->dev, "Data copy from userspace failed\n");
> + return -EFAULT;
> + }
> + mutex_lock(&rproc->lock);
> + rproc->cdev_put_on_release = param ? true : false;
> + mutex_unlock(&rproc->lock);
> + break;
> + case RPROC_GET_SHUTDOWN_ON_RELEASE:
> + mutex_lock(&rproc->lock);
> + ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
> + mutex_unlock(&rproc->lock);
> + if (ret) {
> + dev_err(&rproc->dev, "Data copy to userspace failed\n");
> + return -EFAULT;
> + }
> + break;
> + default:
> + dev_err(&rproc->dev, "Unsupported ioctl\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rproc_cdev_release(struct inode *inode, struct file *filp)
> +{
> + struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> + bool release;
> +
> + mutex_lock(&rproc->lock);
> + release = rproc->cdev_put_on_release;
> + mutex_unlock(&rproc->lock);
> +
> + if (release && rproc->state == RPROC_RUNNING)
I think the state of the processor should also be acquired when the lock is
held. There is still a chance ->state can change between the time the lock is
released and rproc_shutdown() is called but that's a known problem for which
patches have been sent out.
> + rproc_shutdown(rproc);
> +
> + return 0;
> +}
> +
> +static const struct file_operations rproc_fops = {
> + .write = rproc_cdev_write,
> + .compat_ioctl = rproc_device_ioctl,
> + .release = rproc_cdev_release,
> +};
> +
> +int rproc_char_device_add(struct rproc *rproc)
> +{
> + int ret;
> + dev_t cdevt;
> +
> + cdev_init(&rproc->char_dev, &rproc_fops);
> + rproc->char_dev.owner = THIS_MODULE;
> +
> + cdevt = MKDEV(rproc_major, rproc->index);
> + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> + if (ret < 0)
> + goto out;
> +
> + rproc->dev.devt = cdevt;
> +out:
> + return ret;
> +}
> +
> +void rproc_char_device_remove(struct rproc *rproc)
> +{
> + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> +}
> +
> +void __init rproc_init_cdev(void)
> +{
> + int ret;
> +
> + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> + if (ret < 0)
> + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> +}
> +
> +void __exit rproc_exit_cdev(void)
> +{
> + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
Please go back to the comment I made on this during my last review and respin.
> +}
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ba7cb5..f091ddc 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,34 @@ extern struct class rproc_class;
> int rproc_init_sysfs(void);
> void rproc_exit_sysfs(void);
>
> +#ifdef CONFIG_REMOTEPROC_CDEV
> +void rproc_init_cdev(void);
> +void rproc_exit_cdev(void);
> +int rproc_char_device_add(struct rproc *rproc);
> +void rproc_char_device_remove(struct rproc *rproc);
> +#else
> +static inline void rproc_init_cdev(void)
> +{
> +}
> +
> +static inline void rproc_exit_cdev(void)
> +{
> +}
> +
> +/*
> + * The character device interface is an optional feature, if it is not enabled
> + * the function should not return an error.
> + */
> +static inline int rproc_char_device_add(struct rproc *rproc)
> +{
> + return 0;
> +}
> +
> +static inline void rproc_char_device_remove(struct rproc *rproc)
> +{
> +}
> +#endif
> +
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e7b7bab..669cbfb 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -40,6 +40,7 @@
> #include <linux/virtio.h>
> #include <linux/completion.h>
> #include <linux/idr.h>
> +#include <linux/cdev.h>
Move this above completion.h
With all of the above modifications:
Reviewed-by: Mathieu Poirier <[email protected]>
> #include <linux/of.h>
>
> /**
> @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> * @auto_boot: flag to indicate if remote processor should be auto-started
> * @dump_segments: list of segments in the firmware
> * @nb_vdev: number of vdev currently handled by rproc
> + * @char_dev: character device of the rproc
> + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> */
> struct rproc {
> struct list_head node;
> @@ -523,6 +526,8 @@ struct rproc {
> int nb_vdev;
> u8 elf_class;
> u16 elf_machine;
> + struct cdev char_dev;
> + bool cdev_put_on_release;
> };
>
> /**
> diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> new file mode 100644
> index 0000000..c43768e
> --- /dev/null
> +++ b/include/uapi/linux/remoteproc_cdev.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +/*
> + * IOCTLs for Remoteproc's character device interface.
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_REMOTEPROC_CDEV_H_
> +#define _UAPI_REMOTEPROC_CDEV_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#define RPROC_MAGIC 0xB7
> +
> +/*
> + * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
> + * processor automatically when the controlling userpsace closes the char device interface.
> + *
> + * input parameter: integer
> + * 0 : disable automatic shutdown
> + * other : enable automatic shutdown
> + */
> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
> +
> +/*
> + * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
> + * a remote processor is enabled or disabled when the controlling userspace closes the char device
> + * interface.
> + *
> + * output parameter: integer
> + * 0 : automatic shutdown disable
> + * other : automatic shutdown enable
> + */
> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
> +
> +#endif
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:
> On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> > Add the character device interface into remoteproc framework.
> > This interface can be used in order to boot/shutdown remote
> > subsystems and provides a basic ioctl based interface to implement
> > supplementary functionality. An ioctl call is implemented to enable
> > the shutdown on release feature which will allow remote processors to
> > be shutdown when the controlling userpsace application crashes or hangs.
> >
> > Signed-off-by: Rishabh Bhatnagar <[email protected]>
> > Signed-off-by: Siddharth Gupta <[email protected]>
> > ---
> > Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> > drivers/remoteproc/Kconfig | 9 ++
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
> > drivers/remoteproc/remoteproc_internal.h | 28 ++++
> > include/linux/remoteproc.h | 5 +
> > include/uapi/linux/remoteproc_cdev.h | 37 ++++++
> > 7 files changed, 227 insertions(+)
> > create mode 100644 drivers/remoteproc/remoteproc_cdev.c
> > create mode 100644 include/uapi/linux/remoteproc_cdev.h
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd..2a19883 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -339,6 +339,7 @@ Code Seq# Include File Comments
> > 0xB4 00-0F linux/gpio.h <mailto:[email protected]>
> > 0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
> > 0xB6 all linux/fpga-dfl.h
> > +0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
> > 0xC0 00-0F linux/usb/iowarrior.h
> > 0xCA 00-0F uapi/misc/cxl.h
> > 0xCA 10-2F uapi/misc/ocxl.h
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index c4d1731..652060f 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -14,6 +14,15 @@ config REMOTEPROC
> >
> > if REMOTEPROC
> >
> > +config REMOTEPROC_CDEV
> > + bool "Remoteproc character device interface"
> > + help
> > + Say y here to have a character device interface for the remoteproc
> > + framework. Userspace can boot/shutdown remote processors through
> > + this interface.
> > +
> > + It's safe to say N if you don't want to use this interface.
> > +
> > config IMX_REMOTEPROC
> > tristate "IMX6/7 remoteproc support"
> > depends on ARCH_MXC
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index e8b886e..311ae3f 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> > remoteproc-y += remoteproc_sysfs.o
> > remoteproc-y += remoteproc_virtio.o
> > remoteproc-y += remoteproc_elf_loader.o
> > +obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> > obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> > obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> > obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > new file mode 100644
> > index 0000000..8a0eb47
> > --- /dev/null
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -0,0 +1,146 @@
> > +// 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/cdev.h>
> > +#include <linux/fs.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/compat.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/uaccess.h>
> > +#include <uapi/linux/remoteproc_cdev.h>
>
> Alphabetical order please.
>
> > +
> > +#include "remoteproc_internal.h"
> > +
> > +#define NUM_RPROC_DEVICES 64
> > +static dev_t rproc_major;
> > +
> > +static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
> > +{
> > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > + int ret = 0;
> > + char cmd[10];
> > +
> > + if (!len || len > sizeof(cmd))
> > + return -EINVAL;
> > +
> > + ret = copy_from_user(cmd, buf, sizeof(cmd));
> > + if (ret)
> > + return -EFAULT;
> > +
> > + if (sysfs_streq(cmd, "start")) {
> > + if (rproc->state == RPROC_RUNNING)
> > + return -EBUSY;
> > +
> > + ret = rproc_boot(rproc);
> > + if (ret)
> > + dev_err(&rproc->dev, "Boot failed:%d\n", ret);
> > + } else if (sysfs_streq(cmd, "stop")) {
> > + if (rproc->state != RPROC_RUNNING)
> > + return -EINVAL;
> > +
> > + rproc_shutdown(rproc);
> > + } else {
> > + dev_err(&rproc->dev, "Unrecognized option\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret ? ret : len;
> > +}
> > +
> > +static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > +{
> > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > + void __user *argp = compat_ptr(arg);
> > + int ret;
> > + int32_t param;
> > +
> > + switch (ioctl) {
> > + case RPROC_SET_SHUTDOWN_ON_RELEASE:
> > + ret = copy_from_user(¶m, argp, sizeof(int32_t));
> > + if (ret) {
> > + dev_err(&rproc->dev, "Data copy from userspace failed\n");
> > + return -EFAULT;
> > + }
> > + mutex_lock(&rproc->lock);
> > + rproc->cdev_put_on_release = param ? true : false;
> > + mutex_unlock(&rproc->lock);
> > + break;
> > + case RPROC_GET_SHUTDOWN_ON_RELEASE:
> > + mutex_lock(&rproc->lock);
> > + ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
> > + mutex_unlock(&rproc->lock);
> > + if (ret) {
> > + dev_err(&rproc->dev, "Data copy to userspace failed\n");
> > + return -EFAULT;
> > + }
> > + break;
> > + default:
> > + dev_err(&rproc->dev, "Unsupported ioctl\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rproc_cdev_release(struct inode *inode, struct file *filp)
> > +{
> > + struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> > + bool release;
> > +
> > + mutex_lock(&rproc->lock);
> > + release = rproc->cdev_put_on_release;
> > + mutex_unlock(&rproc->lock);
> > +
> > + if (release && rproc->state == RPROC_RUNNING)
>
> I think the state of the processor should also be acquired when the lock is
> held. There is still a chance ->state can change between the time the lock is
> released and rproc_shutdown() is called but that's a known problem for which
> patches have been sent out.
>
> > + rproc_shutdown(rproc);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct file_operations rproc_fops = {
> > + .write = rproc_cdev_write,
> > + .compat_ioctl = rproc_device_ioctl,
> > + .release = rproc_cdev_release,
> > +};
> > +
> > +int rproc_char_device_add(struct rproc *rproc)
> > +{
> > + int ret;
> > + dev_t cdevt;
> > +
> > + cdev_init(&rproc->char_dev, &rproc_fops);
> > + rproc->char_dev.owner = THIS_MODULE;
> > +
> > + cdevt = MKDEV(rproc_major, rproc->index);
> > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
Trying this patchset on my side gave me the following splat[1]. After finding
the root case I can't understand how you haven't see it on your side when you
tested the feature.
[1]. https://pastebin.com/aYTUUCdQ
> > + if (ret < 0)
> > + goto out;
> > +
> > + rproc->dev.devt = cdevt;
> > +out:
> > + return ret;
> > +}
> > +
> > +void rproc_char_device_remove(struct rproc *rproc)
> > +{
> > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > +}
> > +
> > +void __init rproc_init_cdev(void)
> > +{
> > + int ret;
> > +
> > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > + if (ret < 0)
> > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > +}
> > +
> > +void __exit rproc_exit_cdev(void)
> > +{
> > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
>
> Please go back to the comment I made on this during my last review and respin.
After digging in the code while debugging the above problem, I don't see how
unregistering the chrdev region the way it is done here would have worked.
>
> > +}
> > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > index 4ba7cb5..f091ddc 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -47,6 +47,34 @@ extern struct class rproc_class;
> > int rproc_init_sysfs(void);
> > void rproc_exit_sysfs(void);
> >
> > +#ifdef CONFIG_REMOTEPROC_CDEV
> > +void rproc_init_cdev(void);
> > +void rproc_exit_cdev(void);
> > +int rproc_char_device_add(struct rproc *rproc);
> > +void rproc_char_device_remove(struct rproc *rproc);
> > +#else
> > +static inline void rproc_init_cdev(void)
> > +{
> > +}
> > +
> > +static inline void rproc_exit_cdev(void)
> > +{
> > +}
> > +
> > +/*
> > + * The character device interface is an optional feature, if it is not enabled
> > + * the function should not return an error.
> > + */
> > +static inline int rproc_char_device_add(struct rproc *rproc)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void rproc_char_device_remove(struct rproc *rproc)
> > +{
> > +}
> > +#endif
> > +
> > void rproc_free_vring(struct rproc_vring *rvring);
> > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e7b7bab..669cbfb 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -40,6 +40,7 @@
> > #include <linux/virtio.h>
> > #include <linux/completion.h>
> > #include <linux/idr.h>
> > +#include <linux/cdev.h>
>
> Move this above completion.h
>
> With all of the above modifications:
>
> Reviewed-by: Mathieu Poirier <[email protected]>
I'm taking that back - this patch needs some fixing...
>
> > #include <linux/of.h>
> >
> > /**
> > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > * @auto_boot: flag to indicate if remote processor should be auto-started
> > * @dump_segments: list of segments in the firmware
> > * @nb_vdev: number of vdev currently handled by rproc
> > + * @char_dev: character device of the rproc
> > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > */
> > struct rproc {
> > struct list_head node;
> > @@ -523,6 +526,8 @@ struct rproc {
> > int nb_vdev;
> > u8 elf_class;
> > u16 elf_machine;
> > + struct cdev char_dev;
> > + bool cdev_put_on_release;
> > };
> >
> > /**
> > diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> > new file mode 100644
> > index 0000000..c43768e
> > --- /dev/null
> > +++ b/include/uapi/linux/remoteproc_cdev.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> > +/*
> > + * IOCTLs for Remoteproc's character device interface.
> > + *
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#ifndef _UAPI_REMOTEPROC_CDEV_H_
> > +#define _UAPI_REMOTEPROC_CDEV_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +#define RPROC_MAGIC 0xB7
> > +
> > +/*
> > + * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
> > + * processor automatically when the controlling userpsace closes the char device interface.
> > + *
> > + * input parameter: integer
> > + * 0 : disable automatic shutdown
> > + * other : enable automatic shutdown
> > + */
> > +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
> > +
> > +/*
> > + * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
> > + * a remote processor is enabled or disabled when the controlling userspace closes the char device
> > + * interface.
> > + *
> > + * output parameter: integer
> > + * 0 : automatic shutdown disable
> > + * other : automatic shutdown enable
> > + */
> > +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
> > +
> > +#endif
> > --
> > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
On Tue 07 Jul 12:07 PDT 2020, Siddharth Gupta wrote:
> Add the character device interface into remoteproc framework.
> This interface can be used in order to boot/shutdown remote
> subsystems and provides a basic ioctl based interface to implement
> supplementary functionality. An ioctl call is implemented to enable
> the shutdown on release feature which will allow remote processors to
> be shutdown when the controlling userpsace application crashes or hangs.
>
> Signed-off-by: Rishabh Bhatnagar <[email protected]>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> drivers/remoteproc/Kconfig | 9 ++
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 28 ++++
> include/linux/remoteproc.h | 5 +
> include/uapi/linux/remoteproc_cdev.h | 37 ++++++
> 7 files changed, 227 insertions(+)
> create mode 100644 drivers/remoteproc/remoteproc_cdev.c
> create mode 100644 include/uapi/linux/remoteproc_cdev.h
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd..2a19883 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -339,6 +339,7 @@ Code Seq# Include File Comments
> 0xB4 00-0F linux/gpio.h <mailto:[email protected]>
> 0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
> 0xB6 all linux/fpga-dfl.h
> +0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
> 0xC0 00-0F linux/usb/iowarrior.h
> 0xCA 00-0F uapi/misc/cxl.h
> 0xCA 10-2F uapi/misc/ocxl.h
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c4d1731..652060f 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -14,6 +14,15 @@ config REMOTEPROC
>
> if REMOTEPROC
>
> +config REMOTEPROC_CDEV
> + bool "Remoteproc character device interface"
> + help
> + Say y here to have a character device interface for the remoteproc
> + framework. Userspace can boot/shutdown remote processors through
> + this interface.
> +
> + It's safe to say N if you don't want to use this interface.
> +
> config IMX_REMOTEPROC
> tristate "IMX6/7 remoteproc support"
> depends on ARCH_MXC
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index e8b886e..311ae3f 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> remoteproc-y += remoteproc_sysfs.o
> remoteproc-y += remoteproc_virtio.o
> remoteproc-y += remoteproc_elf_loader.o
> +obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> new file mode 100644
> index 0000000..8a0eb47
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -0,0 +1,146 @@
> +// 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/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/compat.h>
> +#include <linux/remoteproc.h>
> +#include <linux/uaccess.h>
> +#include <uapi/linux/remoteproc_cdev.h>
> +
> +#include "remoteproc_internal.h"
> +
> +#define NUM_RPROC_DEVICES 64
> +static dev_t rproc_major;
> +
> +static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
> +{
> + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> + int ret = 0;
> + char cmd[10];
> +
> + if (!len || len > sizeof(cmd))
> + return -EINVAL;
> +
> + ret = copy_from_user(cmd, buf, sizeof(cmd));
I believe you should copy "len" bytes here, instead of 10.
> + if (ret)
> + return -EFAULT;
> +
> + if (sysfs_streq(cmd, "start")) {
> + if (rproc->state == RPROC_RUNNING)
> + return -EBUSY;
> +
> + ret = rproc_boot(rproc);
> + if (ret)
> + dev_err(&rproc->dev, "Boot failed:%d\n", ret);
rproc_boot() of a child thereof has already told us why the boot failed,
so please omit this print.
> + } else if (sysfs_streq(cmd, "stop")) {
> + if (rproc->state != RPROC_RUNNING)
> + return -EINVAL;
> +
> + rproc_shutdown(rproc);
> + } else {
> + dev_err(&rproc->dev, "Unrecognized option\n");
> + ret = -EINVAL;
> + }
> +
> + return ret ? ret : len;
> +}
> +
> +static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> +{
> + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> + void __user *argp = compat_ptr(arg);
> + int ret;
> + int32_t param;
> +
> + switch (ioctl) {
> + case RPROC_SET_SHUTDOWN_ON_RELEASE:
> + ret = copy_from_user(¶m, argp, sizeof(int32_t));
> + if (ret) {
> + dev_err(&rproc->dev, "Data copy from userspace failed\n");
Please skip the error message and just return EFAULT.
> + return -EFAULT;
> + }
> + mutex_lock(&rproc->lock);
I don't see the reason for protecting this assignment. It can only race
with other assignments and given that it's a native type it will have
the last value written regardless.
> + rproc->cdev_put_on_release = param ? true : false;
The idiomatic way to "normalize" a bool is foo = !!param;
> + mutex_unlock(&rproc->lock);
> + break;
> + case RPROC_GET_SHUTDOWN_ON_RELEASE:
> + mutex_lock(&rproc->lock);
As above, I don't see that cdev_put_on_release will have an half-baked
value, so should be fine to omit the locking here.
> + ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
The documentation says that this is a signed 32-bit thing. So please do
sizeof(something-obviously-32-bits), e.g. sizeof(int32_t) as above.
> + mutex_unlock(&rproc->lock);
> + if (ret) {
> + dev_err(&rproc->dev, "Data copy to userspace failed\n");
As above, please don't print an error here, just return -EFAULT.
> + return -EFAULT;
> + }
> + break;
> + default:
> + dev_err(&rproc->dev, "Unsupported ioctl\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rproc_cdev_release(struct inode *inode, struct file *filp)
> +{
> + struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> + bool release;
> +
> + mutex_lock(&rproc->lock);
It's not possible to race with the ioctl, as this function is called as
the last open file descriptor is closed. So you can omit the lock here
as well and just check cdev_put_on_release in the conditional, without a
local variable.
Regards,
Bjorn
> + release = rproc->cdev_put_on_release;
> + mutex_unlock(&rproc->lock);
> +
> + if (release && rproc->state == RPROC_RUNNING)
> + rproc_shutdown(rproc);
> +
> + return 0;
> +}
> +
> +static const struct file_operations rproc_fops = {
> + .write = rproc_cdev_write,
> + .compat_ioctl = rproc_device_ioctl,
> + .release = rproc_cdev_release,
> +};
> +
> +int rproc_char_device_add(struct rproc *rproc)
> +{
> + int ret;
> + dev_t cdevt;
> +
> + cdev_init(&rproc->char_dev, &rproc_fops);
> + rproc->char_dev.owner = THIS_MODULE;
> +
> + cdevt = MKDEV(rproc_major, rproc->index);
> + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> + if (ret < 0)
> + goto out;
> +
> + rproc->dev.devt = cdevt;
> +out:
> + return ret;
> +}
> +
> +void rproc_char_device_remove(struct rproc *rproc)
> +{
> + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> +}
> +
> +void __init rproc_init_cdev(void)
> +{
> + int ret;
> +
> + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> + if (ret < 0)
> + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> +}
> +
> +void __exit rproc_exit_cdev(void)
> +{
> + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
> +}
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ba7cb5..f091ddc 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,34 @@ extern struct class rproc_class;
> int rproc_init_sysfs(void);
> void rproc_exit_sysfs(void);
>
> +#ifdef CONFIG_REMOTEPROC_CDEV
> +void rproc_init_cdev(void);
> +void rproc_exit_cdev(void);
> +int rproc_char_device_add(struct rproc *rproc);
> +void rproc_char_device_remove(struct rproc *rproc);
> +#else
> +static inline void rproc_init_cdev(void)
> +{
> +}
> +
> +static inline void rproc_exit_cdev(void)
> +{
> +}
> +
> +/*
> + * The character device interface is an optional feature, if it is not enabled
> + * the function should not return an error.
> + */
> +static inline int rproc_char_device_add(struct rproc *rproc)
> +{
> + return 0;
> +}
> +
> +static inline void rproc_char_device_remove(struct rproc *rproc)
> +{
> +}
> +#endif
> +
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e7b7bab..669cbfb 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -40,6 +40,7 @@
> #include <linux/virtio.h>
> #include <linux/completion.h>
> #include <linux/idr.h>
> +#include <linux/cdev.h>
> #include <linux/of.h>
>
> /**
> @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> * @auto_boot: flag to indicate if remote processor should be auto-started
> * @dump_segments: list of segments in the firmware
> * @nb_vdev: number of vdev currently handled by rproc
> + * @char_dev: character device of the rproc
> + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> */
> struct rproc {
> struct list_head node;
> @@ -523,6 +526,8 @@ struct rproc {
> int nb_vdev;
> u8 elf_class;
> u16 elf_machine;
> + struct cdev char_dev;
> + bool cdev_put_on_release;
> };
>
> /**
> diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> new file mode 100644
> index 0000000..c43768e
> --- /dev/null
> +++ b/include/uapi/linux/remoteproc_cdev.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +/*
> + * IOCTLs for Remoteproc's character device interface.
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_REMOTEPROC_CDEV_H_
> +#define _UAPI_REMOTEPROC_CDEV_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#define RPROC_MAGIC 0xB7
> +
> +/*
> + * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
> + * processor automatically when the controlling userpsace closes the char device interface.
> + *
> + * input parameter: integer
> + * 0 : disable automatic shutdown
> + * other : enable automatic shutdown
> + */
> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
> +
> +/*
> + * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
> + * a remote processor is enabled or disabled when the controlling userspace closes the char device
> + * interface.
> + *
> + * output parameter: integer
> + * 0 : automatic shutdown disable
> + * other : automatic shutdown enable
> + */
> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
> +
> +#endif
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Wed 15 Jul 13:18 PDT 2020, Mathieu Poirier wrote:
> On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> > Add the character device interface into remoteproc framework.
> > This interface can be used in order to boot/shutdown remote
> > subsystems and provides a basic ioctl based interface to implement
> > supplementary functionality. An ioctl call is implemented to enable
> > the shutdown on release feature which will allow remote processors to
> > be shutdown when the controlling userpsace application crashes or hangs.
> >
> > Signed-off-by: Rishabh Bhatnagar <[email protected]>
> > Signed-off-by: Siddharth Gupta <[email protected]>
> > ---
> > Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> > drivers/remoteproc/Kconfig | 9 ++
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
> > drivers/remoteproc/remoteproc_internal.h | 28 ++++
> > include/linux/remoteproc.h | 5 +
> > include/uapi/linux/remoteproc_cdev.h | 37 ++++++
> > 7 files changed, 227 insertions(+)
> > create mode 100644 drivers/remoteproc/remoteproc_cdev.c
> > create mode 100644 include/uapi/linux/remoteproc_cdev.h
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd..2a19883 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -339,6 +339,7 @@ Code Seq# Include File Comments
> > 0xB4 00-0F linux/gpio.h <mailto:[email protected]>
> > 0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
> > 0xB6 all linux/fpga-dfl.h
> > +0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
> > 0xC0 00-0F linux/usb/iowarrior.h
> > 0xCA 00-0F uapi/misc/cxl.h
> > 0xCA 10-2F uapi/misc/ocxl.h
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index c4d1731..652060f 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -14,6 +14,15 @@ config REMOTEPROC
> >
> > if REMOTEPROC
> >
> > +config REMOTEPROC_CDEV
> > + bool "Remoteproc character device interface"
> > + help
> > + Say y here to have a character device interface for the remoteproc
> > + framework. Userspace can boot/shutdown remote processors through
> > + this interface.
> > +
> > + It's safe to say N if you don't want to use this interface.
> > +
> > config IMX_REMOTEPROC
> > tristate "IMX6/7 remoteproc support"
> > depends on ARCH_MXC
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index e8b886e..311ae3f 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> > remoteproc-y += remoteproc_sysfs.o
> > remoteproc-y += remoteproc_virtio.o
> > remoteproc-y += remoteproc_elf_loader.o
> > +obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> > obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> > obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> > obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > new file mode 100644
> > index 0000000..8a0eb47
> > --- /dev/null
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -0,0 +1,146 @@
> > +// 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/cdev.h>
> > +#include <linux/fs.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/compat.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/uaccess.h>
> > +#include <uapi/linux/remoteproc_cdev.h>
>
> Alphabetical order please.
>
> > +
> > +#include "remoteproc_internal.h"
> > +
> > +#define NUM_RPROC_DEVICES 64
> > +static dev_t rproc_major;
> > +
> > +static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
> > +{
> > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > + int ret = 0;
> > + char cmd[10];
> > +
> > + if (!len || len > sizeof(cmd))
> > + return -EINVAL;
> > +
> > + ret = copy_from_user(cmd, buf, sizeof(cmd));
> > + if (ret)
> > + return -EFAULT;
> > +
> > + if (sysfs_streq(cmd, "start")) {
> > + if (rproc->state == RPROC_RUNNING)
> > + return -EBUSY;
> > +
> > + ret = rproc_boot(rproc);
> > + if (ret)
> > + dev_err(&rproc->dev, "Boot failed:%d\n", ret);
> > + } else if (sysfs_streq(cmd, "stop")) {
> > + if (rproc->state != RPROC_RUNNING)
> > + return -EINVAL;
> > +
> > + rproc_shutdown(rproc);
> > + } else {
> > + dev_err(&rproc->dev, "Unrecognized option\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret ? ret : len;
> > +}
> > +
> > +static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > +{
> > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > + void __user *argp = compat_ptr(arg);
> > + int ret;
> > + int32_t param;
> > +
> > + switch (ioctl) {
> > + case RPROC_SET_SHUTDOWN_ON_RELEASE:
> > + ret = copy_from_user(¶m, argp, sizeof(int32_t));
> > + if (ret) {
> > + dev_err(&rproc->dev, "Data copy from userspace failed\n");
> > + return -EFAULT;
> > + }
> > + mutex_lock(&rproc->lock);
> > + rproc->cdev_put_on_release = param ? true : false;
> > + mutex_unlock(&rproc->lock);
> > + break;
> > + case RPROC_GET_SHUTDOWN_ON_RELEASE:
> > + mutex_lock(&rproc->lock);
> > + ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
> > + mutex_unlock(&rproc->lock);
> > + if (ret) {
> > + dev_err(&rproc->dev, "Data copy to userspace failed\n");
> > + return -EFAULT;
> > + }
> > + break;
> > + default:
> > + dev_err(&rproc->dev, "Unsupported ioctl\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rproc_cdev_release(struct inode *inode, struct file *filp)
> > +{
> > + struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> > + bool release;
> > +
> > + mutex_lock(&rproc->lock);
> > + release = rproc->cdev_put_on_release;
> > + mutex_unlock(&rproc->lock);
> > +
> > + if (release && rproc->state == RPROC_RUNNING)
>
> I think the state of the processor should also be acquired when the lock is
> held. There is still a chance ->state can change between the time the lock is
> released and rproc_shutdown() is called but that's a known problem for which
> patches have been sent out.
>
There where patches for a similar bug in the debugfs interface, but I'm
not able to find anything for rproc_shutdown().
As I suggested in the previous version of this series I think it's ok
that we move forward with replicating the same faulty logic that we have
in the sysfs interface and then fix rproc_shutdown() so that it
internally checks the current state and return appropriately.
Regards,
Bjorn
> > + rproc_shutdown(rproc);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct file_operations rproc_fops = {
> > + .write = rproc_cdev_write,
> > + .compat_ioctl = rproc_device_ioctl,
> > + .release = rproc_cdev_release,
> > +};
> > +
> > +int rproc_char_device_add(struct rproc *rproc)
> > +{
> > + int ret;
> > + dev_t cdevt;
> > +
> > + cdev_init(&rproc->char_dev, &rproc_fops);
> > + rproc->char_dev.owner = THIS_MODULE;
> > +
> > + cdevt = MKDEV(rproc_major, rproc->index);
> > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> > + if (ret < 0)
> > + goto out;
> > +
> > + rproc->dev.devt = cdevt;
> > +out:
> > + return ret;
> > +}
> > +
> > +void rproc_char_device_remove(struct rproc *rproc)
> > +{
> > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > +}
> > +
> > +void __init rproc_init_cdev(void)
> > +{
> > + int ret;
> > +
> > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > + if (ret < 0)
> > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > +}
> > +
> > +void __exit rproc_exit_cdev(void)
> > +{
> > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
>
> Please go back to the comment I made on this during my last review and respin.
>
> > +}
> > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > index 4ba7cb5..f091ddc 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -47,6 +47,34 @@ extern struct class rproc_class;
> > int rproc_init_sysfs(void);
> > void rproc_exit_sysfs(void);
> >
> > +#ifdef CONFIG_REMOTEPROC_CDEV
> > +void rproc_init_cdev(void);
> > +void rproc_exit_cdev(void);
> > +int rproc_char_device_add(struct rproc *rproc);
> > +void rproc_char_device_remove(struct rproc *rproc);
> > +#else
> > +static inline void rproc_init_cdev(void)
> > +{
> > +}
> > +
> > +static inline void rproc_exit_cdev(void)
> > +{
> > +}
> > +
> > +/*
> > + * The character device interface is an optional feature, if it is not enabled
> > + * the function should not return an error.
> > + */
> > +static inline int rproc_char_device_add(struct rproc *rproc)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void rproc_char_device_remove(struct rproc *rproc)
> > +{
> > +}
> > +#endif
> > +
> > void rproc_free_vring(struct rproc_vring *rvring);
> > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e7b7bab..669cbfb 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -40,6 +40,7 @@
> > #include <linux/virtio.h>
> > #include <linux/completion.h>
> > #include <linux/idr.h>
> > +#include <linux/cdev.h>
>
> Move this above completion.h
>
> With all of the above modifications:
>
> Reviewed-by: Mathieu Poirier <[email protected]>
>
> > #include <linux/of.h>
> >
> > /**
> > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > * @auto_boot: flag to indicate if remote processor should be auto-started
> > * @dump_segments: list of segments in the firmware
> > * @nb_vdev: number of vdev currently handled by rproc
> > + * @char_dev: character device of the rproc
> > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > */
> > struct rproc {
> > struct list_head node;
> > @@ -523,6 +526,8 @@ struct rproc {
> > int nb_vdev;
> > u8 elf_class;
> > u16 elf_machine;
> > + struct cdev char_dev;
> > + bool cdev_put_on_release;
> > };
> >
> > /**
> > diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> > new file mode 100644
> > index 0000000..c43768e
> > --- /dev/null
> > +++ b/include/uapi/linux/remoteproc_cdev.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> > +/*
> > + * IOCTLs for Remoteproc's character device interface.
> > + *
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#ifndef _UAPI_REMOTEPROC_CDEV_H_
> > +#define _UAPI_REMOTEPROC_CDEV_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +#define RPROC_MAGIC 0xB7
> > +
> > +/*
> > + * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
> > + * processor automatically when the controlling userpsace closes the char device interface.
> > + *
> > + * input parameter: integer
> > + * 0 : disable automatic shutdown
> > + * other : enable automatic shutdown
> > + */
> > +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
> > +
> > +/*
> > + * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
> > + * a remote processor is enabled or disabled when the controlling userspace closes the char device
> > + * interface.
> > + *
> > + * output parameter: integer
> > + * 0 : automatic shutdown disable
> > + * other : automatic shutdown enable
> > + */
> > +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
> > +
> > +#endif
> > --
> > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
On Tue 07 Jul 12:07 PDT 2020, Siddharth Gupta wrote:
> Add the character device during rproc_add. This would create
> a character device node at /dev/remoteproc<index>. Userspace
> applications can interact with the remote processor using this
> interface.
>
> Signed-off-by: Rishabh Bhatnagar <[email protected]>
> Signed-off-by: Siddharth Gupta <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0f95e02..ec7fb49 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1966,6 +1966,13 @@ int rproc_add(struct rproc *rproc)
> struct device *dev = &rproc->dev;
> int ret;
>
> + /* add char device for this remoteproc */
> + ret = rproc_char_device_add(rproc);
> + if (ret) {
> + dev_err(dev, "Failed to add char dev for %s\n", rproc->name);
Please move this error message into rproc_char_device_add(), to make it
consistent with the other error handling in this function not printing.
Apart from that it looks good.
Regards,
Bjorn
> + return ret;
> + }
> +
> ret = device_add(dev);
> if (ret < 0)
> return ret;
> @@ -2241,6 +2248,7 @@ int rproc_del(struct rproc *rproc)
> mutex_unlock(&rproc->lock);
>
> rproc_delete_debug_dir(rproc);
> + rproc_char_device_remove(rproc);
>
> /* the rproc is downref'ed as soon as it's removed from the klist */
> mutex_lock(&rproc_list_mutex);
> @@ -2409,6 +2417,7 @@ static int __init remoteproc_init(void)
> {
> rproc_init_sysfs();
> rproc_init_debugfs();
> + rproc_init_cdev();
> rproc_init_panic();
>
> return 0;
> @@ -2420,6 +2429,7 @@ static void __exit remoteproc_exit(void)
> ida_destroy(&rproc_dev_index);
>
> rproc_exit_panic();
> + rproc_exit_cdev();
> rproc_exit_debugfs();
> rproc_exit_sysfs();
> }
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, 16 Jul 2020 at 23:48, Bjorn Andersson
<[email protected]> wrote:
>
> On Wed 15 Jul 13:18 PDT 2020, Mathieu Poirier wrote:
>
> > On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> > > Add the character device interface into remoteproc framework.
> > > This interface can be used in order to boot/shutdown remote
> > > subsystems and provides a basic ioctl based interface to implement
> > > supplementary functionality. An ioctl call is implemented to enable
> > > the shutdown on release feature which will allow remote processors to
> > > be shutdown when the controlling userpsace application crashes or hangs.
> > >
> > > Signed-off-by: Rishabh Bhatnagar <[email protected]>
> > > Signed-off-by: Siddharth Gupta <[email protected]>
> > > ---
> > > Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
> > > drivers/remoteproc/Kconfig | 9 ++
> > > drivers/remoteproc/Makefile | 1 +
> > > drivers/remoteproc/remoteproc_cdev.c | 146 +++++++++++++++++++++
> > > drivers/remoteproc/remoteproc_internal.h | 28 ++++
> > > include/linux/remoteproc.h | 5 +
> > > include/uapi/linux/remoteproc_cdev.h | 37 ++++++
> > > 7 files changed, 227 insertions(+)
> > > create mode 100644 drivers/remoteproc/remoteproc_cdev.c
> > > create mode 100644 include/uapi/linux/remoteproc_cdev.h
> > >
> > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > index 59472cd..2a19883 100644
> > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > @@ -339,6 +339,7 @@ Code Seq# Include File Comments
> > > 0xB4 00-0F linux/gpio.h <mailto:[email protected]>
> > > 0xB5 00-0F uapi/linux/rpmsg.h <mailto:[email protected]>
> > > 0xB6 all linux/fpga-dfl.h
> > > +0xB7 all uapi/linux/remoteproc_cdev.h <mailto:[email protected]>
> > > 0xC0 00-0F linux/usb/iowarrior.h
> > > 0xCA 00-0F uapi/misc/cxl.h
> > > 0xCA 10-2F uapi/misc/ocxl.h
> > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > > index c4d1731..652060f 100644
> > > --- a/drivers/remoteproc/Kconfig
> > > +++ b/drivers/remoteproc/Kconfig
> > > @@ -14,6 +14,15 @@ config REMOTEPROC
> > >
> > > if REMOTEPROC
> > >
> > > +config REMOTEPROC_CDEV
> > > + bool "Remoteproc character device interface"
> > > + help
> > > + Say y here to have a character device interface for the remoteproc
> > > + framework. Userspace can boot/shutdown remote processors through
> > > + this interface.
> > > +
> > > + It's safe to say N if you don't want to use this interface.
> > > +
> > > config IMX_REMOTEPROC
> > > tristate "IMX6/7 remoteproc support"
> > > depends on ARCH_MXC
> > > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > > index e8b886e..311ae3f 100644
> > > --- a/drivers/remoteproc/Makefile
> > > +++ b/drivers/remoteproc/Makefile
> > > @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> > > remoteproc-y += remoteproc_sysfs.o
> > > remoteproc-y += remoteproc_virtio.o
> > > remoteproc-y += remoteproc_elf_loader.o
> > > +obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> > > obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> > > obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> > > obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> > > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > > new file mode 100644
> > > index 0000000..8a0eb47
> > > --- /dev/null
> > > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > > @@ -0,0 +1,146 @@
> > > +// 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/cdev.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/compat.h>
> > > +#include <linux/remoteproc.h>
> > > +#include <linux/uaccess.h>
> > > +#include <uapi/linux/remoteproc_cdev.h>
> >
> > Alphabetical order please.
> >
> > > +
> > > +#include "remoteproc_internal.h"
> > > +
> > > +#define NUM_RPROC_DEVICES 64
> > > +static dev_t rproc_major;
> > > +
> > > +static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos)
> > > +{
> > > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > > + int ret = 0;
> > > + char cmd[10];
> > > +
> > > + if (!len || len > sizeof(cmd))
> > > + return -EINVAL;
> > > +
> > > + ret = copy_from_user(cmd, buf, sizeof(cmd));
> > > + if (ret)
> > > + return -EFAULT;
> > > +
> > > + if (sysfs_streq(cmd, "start")) {
> > > + if (rproc->state == RPROC_RUNNING)
> > > + return -EBUSY;
> > > +
> > > + ret = rproc_boot(rproc);
> > > + if (ret)
> > > + dev_err(&rproc->dev, "Boot failed:%d\n", ret);
> > > + } else if (sysfs_streq(cmd, "stop")) {
> > > + if (rproc->state != RPROC_RUNNING)
> > > + return -EINVAL;
> > > +
> > > + rproc_shutdown(rproc);
> > > + } else {
> > > + dev_err(&rproc->dev, "Unrecognized option\n");
> > > + ret = -EINVAL;
> > > + }
> > > +
> > > + return ret ? ret : len;
> > > +}
> > > +
> > > +static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > > +{
> > > + struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, char_dev);
> > > + void __user *argp = compat_ptr(arg);
> > > + int ret;
> > > + int32_t param;
> > > +
> > > + switch (ioctl) {
> > > + case RPROC_SET_SHUTDOWN_ON_RELEASE:
> > > + ret = copy_from_user(¶m, argp, sizeof(int32_t));
> > > + if (ret) {
> > > + dev_err(&rproc->dev, "Data copy from userspace failed\n");
> > > + return -EFAULT;
> > > + }
> > > + mutex_lock(&rproc->lock);
> > > + rproc->cdev_put_on_release = param ? true : false;
> > > + mutex_unlock(&rproc->lock);
> > > + break;
> > > + case RPROC_GET_SHUTDOWN_ON_RELEASE:
> > > + mutex_lock(&rproc->lock);
> > > + ret = copy_to_user(argp, &rproc->cdev_put_on_release, sizeof(bool));
> > > + mutex_unlock(&rproc->lock);
> > > + if (ret) {
> > > + dev_err(&rproc->dev, "Data copy to userspace failed\n");
> > > + return -EFAULT;
> > > + }
> > > + break;
> > > + default:
> > > + dev_err(&rproc->dev, "Unsupported ioctl\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int rproc_cdev_release(struct inode *inode, struct file *filp)
> > > +{
> > > + struct rproc *rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> > > + bool release;
> > > +
> > > + mutex_lock(&rproc->lock);
> > > + release = rproc->cdev_put_on_release;
> > > + mutex_unlock(&rproc->lock);
> > > +
> > > + if (release && rproc->state == RPROC_RUNNING)
> >
> > I think the state of the processor should also be acquired when the lock is
> > held. There is still a chance ->state can change between the time the lock is
> > released and rproc_shutdown() is called but that's a known problem for which
> > patches have been sent out.
> >
>
> There where patches for a similar bug in the debugfs interface, but I'm
> not able to find anything for rproc_shutdown().
You are correct - I was under the impression Alex's patch was for
rproc_shutdown() as well.
>
>
> As I suggested in the previous version of this series I think it's ok
> that we move forward with replicating the same faulty logic that we have
> in the sysfs interface and then fix rproc_shutdown() so that it
> internally checks the current state and return appropriately.
I also think this is the right way to proceed.
>
> Regards,
> Bjorn
>
> > > + rproc_shutdown(rproc);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct file_operations rproc_fops = {
> > > + .write = rproc_cdev_write,
> > > + .compat_ioctl = rproc_device_ioctl,
> > > + .release = rproc_cdev_release,
> > > +};
> > > +
> > > +int rproc_char_device_add(struct rproc *rproc)
> > > +{
> > > + int ret;
> > > + dev_t cdevt;
> > > +
> > > + cdev_init(&rproc->char_dev, &rproc_fops);
> > > + rproc->char_dev.owner = THIS_MODULE;
> > > +
> > > + cdevt = MKDEV(rproc_major, rproc->index);
> > > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> > > + if (ret < 0)
> > > + goto out;
> > > +
> > > + rproc->dev.devt = cdevt;
> > > +out:
> > > + return ret;
> > > +}
> > > +
> > > +void rproc_char_device_remove(struct rproc *rproc)
> > > +{
> > > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > > +}
> > > +
> > > +void __init rproc_init_cdev(void)
> > > +{
> > > + int ret;
> > > +
> > > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > > + if (ret < 0)
> > > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > > +}
> > > +
> > > +void __exit rproc_exit_cdev(void)
> > > +{
> > > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
> >
> > Please go back to the comment I made on this during my last review and respin.
> >
> > > +}
> > > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > > index 4ba7cb5..f091ddc 100644
> > > --- a/drivers/remoteproc/remoteproc_internal.h
> > > +++ b/drivers/remoteproc/remoteproc_internal.h
> > > @@ -47,6 +47,34 @@ extern struct class rproc_class;
> > > int rproc_init_sysfs(void);
> > > void rproc_exit_sysfs(void);
> > >
> > > +#ifdef CONFIG_REMOTEPROC_CDEV
> > > +void rproc_init_cdev(void);
> > > +void rproc_exit_cdev(void);
> > > +int rproc_char_device_add(struct rproc *rproc);
> > > +void rproc_char_device_remove(struct rproc *rproc);
> > > +#else
> > > +static inline void rproc_init_cdev(void)
> > > +{
> > > +}
> > > +
> > > +static inline void rproc_exit_cdev(void)
> > > +{
> > > +}
> > > +
> > > +/*
> > > + * The character device interface is an optional feature, if it is not enabled
> > > + * the function should not return an error.
> > > + */
> > > +static inline int rproc_char_device_add(struct rproc *rproc)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static inline void rproc_char_device_remove(struct rproc *rproc)
> > > +{
> > > +}
> > > +#endif
> > > +
> > > void rproc_free_vring(struct rproc_vring *rvring);
> > > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> > >
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index e7b7bab..669cbfb 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -40,6 +40,7 @@
> > > #include <linux/virtio.h>
> > > #include <linux/completion.h>
> > > #include <linux/idr.h>
> > > +#include <linux/cdev.h>
> >
> > Move this above completion.h
> >
> > With all of the above modifications:
> >
> > Reviewed-by: Mathieu Poirier <[email protected]>
> >
> > > #include <linux/of.h>
> > >
> > > /**
> > > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > > * @auto_boot: flag to indicate if remote processor should be auto-started
> > > * @dump_segments: list of segments in the firmware
> > > * @nb_vdev: number of vdev currently handled by rproc
> > > + * @char_dev: character device of the rproc
> > > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > > */
> > > struct rproc {
> > > struct list_head node;
> > > @@ -523,6 +526,8 @@ struct rproc {
> > > int nb_vdev;
> > > u8 elf_class;
> > > u16 elf_machine;
> > > + struct cdev char_dev;
> > > + bool cdev_put_on_release;
> > > };
> > >
> > > /**
> > > diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> > > new file mode 100644
> > > index 0000000..c43768e
> > > --- /dev/null
> > > +++ b/include/uapi/linux/remoteproc_cdev.h
> > > @@ -0,0 +1,37 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> > > +/*
> > > + * IOCTLs for Remoteproc's character device interface.
> > > + *
> > > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef _UAPI_REMOTEPROC_CDEV_H_
> > > +#define _UAPI_REMOTEPROC_CDEV_H_
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/types.h>
> > > +
> > > +#define RPROC_MAGIC 0xB7
> > > +
> > > +/*
> > > + * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the shutdown of a remote
> > > + * processor automatically when the controlling userpsace closes the char device interface.
> > > + *
> > > + * input parameter: integer
> > > + * 0 : disable automatic shutdown
> > > + * other : enable automatic shutdown
> > > + */
> > > +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, __s32)
> > > +
> > > +/*
> > > + * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about whether the automatic shutdown of
> > > + * a remote processor is enabled or disabled when the controlling userspace closes the char device
> > > + * interface.
> > > + *
> > > + * output parameter: integer
> > > + * 0 : automatic shutdown disable
> > > + * other : automatic shutdown enable
> > > + */
> > > +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, __s32)
> > > +
> > > +#endif
> > > --
> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > >
On Tue 21 Jul 12:16 PDT 2020, Siddharth Gupta wrote:
> On 7/15/2020 2:51 PM, Mathieu Poirier wrote:
> > On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:
> > > On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
[..]
> > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
[..]
> > > > +int rproc_char_device_add(struct rproc *rproc)
> > > > +{
> > > > + int ret;
> > > > + dev_t cdevt;
> > > > +
> > > > + cdev_init(&rproc->char_dev, &rproc_fops);
> > > > + rproc->char_dev.owner = THIS_MODULE;
> > > > +
> > > > + cdevt = MKDEV(rproc_major, rproc->index);
> > > > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> > Trying this patchset on my side gave me the following splat[1]. After finding
> > the root case I can't understand how you haven't see it on your side when you
> > tested the feature.
> >
> > [1]. https://pastebin.com/aYTUUCdQ
Mathieu, I've looked at this back and forth. Afaict this implies that
rproc_major is still 0. Could it be that either alloc_chrdev_region()
failed or somehow has yet to be called when you hit this point?
> Hey Mathieu,
>
> We aren't able to reproduce the error that you are seeing, the splat is
> coming
> from the check for whiteout device[1] - which shouldn't happen because of
> the
> find_dynamic_major call[2], right?
>
> We are successfully seeing all our character device files and able to
> successfully boot remoteprocs. From what I read and understood about
> whiteout
> devices they will be hidden in the fs.
>
> Could you provide more details about your configuration and testing?
>
> [1]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486
> <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123>
> [2]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123
>
> <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486>
> > > > + if (ret < 0)
> > > > + goto out;
> > > > +
> > > > + rproc->dev.devt = cdevt;
> > > > +out:
> > > > + return ret;
> > > > +}
> > > > +
> > > > +void rproc_char_device_remove(struct rproc *rproc)
> > > > +{
> > > > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > > > +}
> > > > +
> > > > +void __init rproc_init_cdev(void)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > > > + if (ret < 0)
> > > > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > > > +}
> > > > +
> > > > +void __exit rproc_exit_cdev(void)
> > > > +{
> > > > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
> > > Please go back to the comment I made on this during my last review and respin.
> > After digging in the code while debugging the above problem, I don't see how
> > unregistering the chrdev region the way it is done here would have worked.
> Since this is compiled statically and not built as a module, we will never
> exercise the code path, so I will remove it in the next patchset.
>
You're right Siddharth, since we changed CONFIG_REMOTEPROC to bool it's no longer
possible to hit remoteproc_exit(), so you can omit this function
entirely. (And we should clean up the rest of that as well)
[..]
> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
[..]
> > > > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > > > * @auto_boot: flag to indicate if remote processor should be auto-started
> > > > * @dump_segments: list of segments in the firmware
> > > > * @nb_vdev: number of vdev currently handled by rproc
> > > > + * @char_dev: character device of the rproc
> > > > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > > > */
> > > > struct rproc {
> > > > struct list_head node;
> > > > @@ -523,6 +526,8 @@ struct rproc {
> > > > int nb_vdev;
> > > > u8 elf_class;
> > > > u16 elf_machine;
> > > > + struct cdev char_dev;
As stated privately, I assumed based on this name that this is a struct
device related to that character device. So please rename this cdev to
save me from doing this mistake again.
Thanks,
Bjorn
On Tue, Jul 21, 2020 at 01:56:35PM -0700, Bjorn Andersson wrote:
> On Tue 21 Jul 12:16 PDT 2020, Siddharth Gupta wrote:
> > On 7/15/2020 2:51 PM, Mathieu Poirier wrote:
> > > On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:
> > > > On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> [..]
> > > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> [..]
> > > > > +int rproc_char_device_add(struct rproc *rproc)
> > > > > +{
> > > > > + int ret;
> > > > > + dev_t cdevt;
> > > > > +
> > > > > + cdev_init(&rproc->char_dev, &rproc_fops);
> > > > > + rproc->char_dev.owner = THIS_MODULE;
> > > > > +
> > > > > + cdevt = MKDEV(rproc_major, rproc->index);
> > > > > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> > > Trying this patchset on my side gave me the following splat[1]. After finding
> > > the root case I can't understand how you haven't see it on your side when you
> > > tested the feature.
> > >
> > > [1]. https://pastebin.com/aYTUUCdQ
>
> Mathieu, I've looked at this back and forth. Afaict this implies that
> rproc_major is still 0. Could it be that either alloc_chrdev_region()
> failed or somehow has yet to be called when you hit this point?
That is exacly what I thought when I first stumbled on this but instrumenting
the code showed otherwise.
After function rproc_init_cdev() has been called @rproc_major contains the
dynamically allocated major number in the upper 12 bits and the base minor
number in the lower 20 bits.
In rproc_char_device_add() we find this line:
cdevt = MKDEV(rproc_major, rproc->index);
Macro MKDEV() builds a device number by shifting @rproc_major by 20 bits to the
left and OR'ing that with @rproc->index. But the device's major number is
already occupying the upper 12bits, so shifthing another 20 bits to the left
makes the major portion of the device number '0'. That is causing cdev_add() to
complain bitterly.
The right way to do this is:
cdevt = MKDEV(MAJOR(rproc_major), rproc->index);
Once I found the problem I thought about 32/64 bit issues. Since Siddharth is
using a 64bit application processor shifting another 20 bits would still have
yielded a non-zero value. But that can't be since dev_t is a u32 in
linux/types.h.
As such I can't see how it is possible to not hit that problem on a 64bit
platform.
>
> > Hey Mathieu,
> >
> > We aren't able to reproduce the error that you are seeing, the splat is
> > coming
> > from the check for whiteout device[1] - which shouldn't happen because of
> > the
> > find_dynamic_major call[2], right?
> >
> > We are successfully seeing all our character device files and able to
> > successfully boot remoteprocs. From what I read and understood about
> > whiteout
> > devices they will be hidden in the fs.
> >
> > Could you provide more details about your configuration and testing?
> >
> > [1]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486
> > <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123>
> > [2]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123
> >
> > <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486>
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > +
> > > > > + rproc->dev.devt = cdevt;
> > > > > +out:
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +void rproc_char_device_remove(struct rproc *rproc)
> > > > > +{
> > > > > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > > > > +}
> > > > > +
> > > > > +void __init rproc_init_cdev(void)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > > > > + if (ret < 0)
> > > > > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > > > > +}
> > > > > +
> > > > > +void __exit rproc_exit_cdev(void)
> > > > > +{
> > > > > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
> > > > Please go back to the comment I made on this during my last review and respin.
> > > After digging in the code while debugging the above problem, I don't see how
> > > unregistering the chrdev region the way it is done here would have worked.
> > Since this is compiled statically and not built as a module, we will never
> > exercise the code path, so I will remove it in the next patchset.
> >
>
> You're right Siddharth, since we changed CONFIG_REMOTEPROC to bool it's no longer
> possible to hit remoteproc_exit(), so you can omit this function
> entirely. (And we should clean up the rest of that as well)
>
> [..]
> > > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> [..]
> > > > > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > > > > * @auto_boot: flag to indicate if remote processor should be auto-started
> > > > > * @dump_segments: list of segments in the firmware
> > > > > * @nb_vdev: number of vdev currently handled by rproc
> > > > > + * @char_dev: character device of the rproc
> > > > > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > > > > */
> > > > > struct rproc {
> > > > > struct list_head node;
> > > > > @@ -523,6 +526,8 @@ struct rproc {
> > > > > int nb_vdev;
> > > > > u8 elf_class;
> > > > > u16 elf_machine;
> > > > > + struct cdev char_dev;
>
> As stated privately, I assumed based on this name that this is a struct
> device related to that character device. So please rename this cdev to
> save me from doing this mistake again.
>
> Thanks,
> Bjorn
On 7/22/2020 10:18 AM, Mathieu Poirier wrote:
> On Tue, Jul 21, 2020 at 01:56:35PM -0700, Bjorn Andersson wrote:
>> On Tue 21 Jul 12:16 PDT 2020, Siddharth Gupta wrote:
>>> On 7/15/2020 2:51 PM, Mathieu Poirier wrote:
>>>> On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:
>>>>> On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
>> [..]
>>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>> [..]
>>>>>> +int rproc_char_device_add(struct rproc *rproc)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + dev_t cdevt;
>>>>>> +
>>>>>> + cdev_init(&rproc->char_dev, &rproc_fops);
>>>>>> + rproc->char_dev.owner = THIS_MODULE;
>>>>>> +
>>>>>> + cdevt = MKDEV(rproc_major, rproc->index);
>>>>>> + ret = cdev_add(&rproc->char_dev, cdevt, 1);
>>>> Trying this patchset on my side gave me the following splat[1]. After finding
>>>> the root case I can't understand how you haven't see it on your side when you
>>>> tested the feature.
>>>>
>>>> [1]. https://pastebin.com/aYTUUCdQ
>> Mathieu, I've looked at this back and forth. Afaict this implies that
>> rproc_major is still 0. Could it be that either alloc_chrdev_region()
>> failed or somehow has yet to be called when you hit this point?
> That is exacly what I thought when I first stumbled on this but instrumenting
> the code showed otherwise.
>
> After function rproc_init_cdev() has been called @rproc_major contains the
> dynamically allocated major number in the upper 12 bits and the base minor
> number in the lower 20 bits.
>
> In rproc_char_device_add() we find this line:
>
> cdevt = MKDEV(rproc_major, rproc->index);
>
> Macro MKDEV() builds a device number by shifting @rproc_major by 20 bits to the
> left and OR'ing that with @rproc->index. But the device's major number is
> already occupying the upper 12bits, so shifthing another 20 bits to the left
> makes the major portion of the device number '0'. That is causing cdev_add() to
> complain bitterly.
>
> The right way to do this is:
>
> cdevt = MKDEV(MAJOR(rproc_major), rproc->index);
>
> Once I found the problem I thought about 32/64 bit issues. Since Siddharth is
> using a 64bit application processor shifting another 20 bits would still have
> yielded a non-zero value. But that can't be since dev_t is a u32 in
> linux/types.h.
>
> As such I can't see how it is possible to not hit that problem on a 64bit
> platform.
Hey Mathieu,
I just checked my testing tree for our devices and realized that I have
an older version
of the patch. Hence I was unable to reproduce the error. I will fix this
problem, and
send out a new patchset today.
Sorry about this error!
Thanks,
Sid
>>> Hey Mathieu,
>>>
>>> We aren't able to reproduce the error that you are seeing, the splat is
>>> coming
>>> from the check for whiteout device[1] - which shouldn't happen because of
>>> the
>>> find_dynamic_major call[2], right?
>>>
>>> We are successfully seeing all our character device files and able to
>>> successfully boot remoteprocs. From what I read and understood about
>>> whiteout
>>> devices they will be hidden in the fs.
>>>
>>> Could you provide more details about your configuration and testing?
>>>
>>> [1]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486
>>> <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123>
>>> [2]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123
>>>
>>> <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486>
>>>>>> + if (ret < 0)
>>>>>> + goto out;
>>>>>> +
>>>>>> + rproc->dev.devt = cdevt;
>>>>>> +out:
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +void rproc_char_device_remove(struct rproc *rproc)
>>>>>> +{
>>>>>> + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
>>>>>> +}
>>>>>> +
>>>>>> +void __init rproc_init_cdev(void)
>>>>>> +{
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
>>>>>> + if (ret < 0)
>>>>>> + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
>>>>>> +}
>>>>>> +
>>>>>> +void __exit rproc_exit_cdev(void)
>>>>>> +{
>>>>>> + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
>>>>> Please go back to the comment I made on this during my last review and respin.
>>>> After digging in the code while debugging the above problem, I don't see how
>>>> unregistering the chrdev region the way it is done here would have worked.
>>> Since this is compiled statically and not built as a module, we will never
>>> exercise the code path, so I will remove it in the next patchset.
>>>
>> You're right Siddharth, since we changed CONFIG_REMOTEPROC to bool it's no longer
>> possible to hit remoteproc_exit(), so you can omit this function
>> entirely. (And we should clean up the rest of that as well)
>>
>> [..]
>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> [..]
>>>>>> @@ -488,6 +489,8 @@ struct rproc_dump_segment {
>>>>>> * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>>> * @dump_segments: list of segments in the firmware
>>>>>> * @nb_vdev: number of vdev currently handled by rproc
>>>>>> + * @char_dev: character device of the rproc
>>>>>> + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
>>>>>> */
>>>>>> struct rproc {
>>>>>> struct list_head node;
>>>>>> @@ -523,6 +526,8 @@ struct rproc {
>>>>>> int nb_vdev;
>>>>>> u8 elf_class;
>>>>>> u16 elf_machine;
>>>>>> + struct cdev char_dev;
>> As stated privately, I assumed based on this name that this is a struct
>> device related to that character device. So please rename this cdev to
>> save me from doing this mistake again.
>>
>> Thanks,
>> Bjorn
On Wed 22 Jul 10:18 PDT 2020, Mathieu Poirier wrote:
> On Tue, Jul 21, 2020 at 01:56:35PM -0700, Bjorn Andersson wrote:
> > On Tue 21 Jul 12:16 PDT 2020, Siddharth Gupta wrote:
> > > On 7/15/2020 2:51 PM, Mathieu Poirier wrote:
> > > > On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:
> > > > > On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:
> > [..]
> > > > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > [..]
> > > > > > +int rproc_char_device_add(struct rproc *rproc)
> > > > > > +{
> > > > > > + int ret;
> > > > > > + dev_t cdevt;
> > > > > > +
> > > > > > + cdev_init(&rproc->char_dev, &rproc_fops);
> > > > > > + rproc->char_dev.owner = THIS_MODULE;
> > > > > > +
> > > > > > + cdevt = MKDEV(rproc_major, rproc->index);
> > > > > > + ret = cdev_add(&rproc->char_dev, cdevt, 1);
> > > > Trying this patchset on my side gave me the following splat[1]. After finding
> > > > the root case I can't understand how you haven't see it on your side when you
> > > > tested the feature.
> > > >
> > > > [1]. https://pastebin.com/aYTUUCdQ
> >
> > Mathieu, I've looked at this back and forth. Afaict this implies that
> > rproc_major is still 0. Could it be that either alloc_chrdev_region()
> > failed or somehow has yet to be called when you hit this point?
>
> That is exacly what I thought when I first stumbled on this but instrumenting
> the code showed otherwise.
>
> After function rproc_init_cdev() has been called @rproc_major contains the
> dynamically allocated major number in the upper 12 bits and the base minor
> number in the lower 20 bits.
>
Ahh, alloc_chrdev_region() actually returns the dev_t, not the major.
Too bad that we all name this variable "major" to maximize the
confusion.
> In rproc_char_device_add() we find this line:
>
> cdevt = MKDEV(rproc_major, rproc->index);
>
> Macro MKDEV() builds a device number by shifting @rproc_major by 20 bits to the
> left and OR'ing that with @rproc->index. But the device's major number is
> already occupying the upper 12bits, so shifthing another 20 bits to the left
> makes the major portion of the device number '0'. That is causing cdev_add() to
> complain bitterly.
>
> The right way to do this is:
>
> cdevt = MKDEV(MAJOR(rproc_major), rproc->index);
>
Agreed (and let's continue naming it rproc_major, in line with all other
drivers - now I know better).
Thanks,
Bjorn
> Once I found the problem I thought about 32/64 bit issues. Since Siddharth is
> using a 64bit application processor shifting another 20 bits would still have
> yielded a non-zero value. But that can't be since dev_t is a u32 in
> linux/types.h.
>
> As such I can't see how it is possible to not hit that problem on a 64bit
> platform.
>
> >
> > > Hey Mathieu,
> > >
> > > We aren't able to reproduce the error that you are seeing, the splat is
> > > coming
> > > from the check for whiteout device[1] - which shouldn't happen because of
> > > the
> > > find_dynamic_major call[2], right?
> > >
> > > We are successfully seeing all our character device files and able to
> > > successfully boot remoteprocs. From what I read and understood about
> > > whiteout
> > > devices they will be hidden in the fs.
> > >
> > > Could you provide more details about your configuration and testing?
> > >
> > > [1]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486
> > > <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123>
> > > [2]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123
> > >
> > > <https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486>
> > > > > > + if (ret < 0)
> > > > > > + goto out;
> > > > > > +
> > > > > > + rproc->dev.devt = cdevt;
> > > > > > +out:
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +void rproc_char_device_remove(struct rproc *rproc)
> > > > > > +{
> > > > > > + __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
> > > > > > +}
> > > > > > +
> > > > > > +void __init rproc_init_cdev(void)
> > > > > > +{
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "remoteproc");
> > > > > > + if (ret < 0)
> > > > > > + pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
> > > > > > +}
> > > > > > +
> > > > > > +void __exit rproc_exit_cdev(void)
> > > > > > +{
> > > > > > + unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);
> > > > > Please go back to the comment I made on this during my last review and respin.
> > > > After digging in the code while debugging the above problem, I don't see how
> > > > unregistering the chrdev region the way it is done here would have worked.
> > > Since this is compiled statically and not built as a module, we will never
> > > exercise the code path, so I will remove it in the next patchset.
> > >
> >
> > You're right Siddharth, since we changed CONFIG_REMOTEPROC to bool it's no longer
> > possible to hit remoteproc_exit(), so you can omit this function
> > entirely. (And we should clean up the rest of that as well)
> >
> > [..]
> > > > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > [..]
> > > > > > @@ -488,6 +489,8 @@ struct rproc_dump_segment {
> > > > > > * @auto_boot: flag to indicate if remote processor should be auto-started
> > > > > > * @dump_segments: list of segments in the firmware
> > > > > > * @nb_vdev: number of vdev currently handled by rproc
> > > > > > + * @char_dev: character device of the rproc
> > > > > > + * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > > > > > */
> > > > > > struct rproc {
> > > > > > struct list_head node;
> > > > > > @@ -523,6 +526,8 @@ struct rproc {
> > > > > > int nb_vdev;
> > > > > > u8 elf_class;
> > > > > > u16 elf_machine;
> > > > > > + struct cdev char_dev;
> >
> > As stated privately, I assumed based on this name that this is a struct
> > device related to that character device. So please rename this cdev to
> > save me from doing this mistake again.
> >
> > Thanks,
> > Bjorn