Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] Add character device interface to remoteproc
@ 2020-04-21 18:10 Rishabh Bhatnagar
  2020-04-21 18:10 ` Rishabh Bhatnagar
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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:

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.

v2 -> v1:
- Fixed comments from Bjorn and Matthew.

Rishabh Bhatnagar (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               | 142 +++++++++++++++++++++
 drivers/remoteproc/remoteproc_core.c               |  10 ++
 drivers/remoteproc/remoteproc_internal.h           |  21 +++
 include/linux/remoteproc.h                         |   3 +
 include/uapi/linux/remoteproc_cdev.h               |  21 +++
 8 files changed, 208 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 0/2] Add character device interface to remoteproc
  2020-04-21 18:10 [PATCH v3 0/2] Add character device interface to remoteproc Rishabh Bhatnagar
@ 2020-04-21 18:10 ` Rishabh Bhatnagar
  2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
  2020-04-21 18:10 ` [PATCH v3 2/2] remoteproc: core: Register the " Rishabh Bhatnagar
  2 siblings, 0 replies; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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:

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.

v2 -> v1:
- Fixed comments from Bjorn and Matthew.

Rishabh Bhatnagar (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               | 142 +++++++++++++++++++++
 drivers/remoteproc/remoteproc_core.c               |  10 ++
 drivers/remoteproc/remoteproc_internal.h           |  21 +++
 include/linux/remoteproc.h                         |   3 +
 include/uapi/linux/remoteproc_cdev.h               |  21 +++
 8 files changed, 208 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-21 18:10 [PATCH v3 0/2] Add character device interface to remoteproc Rishabh Bhatnagar
  2020-04-21 18:10 ` Rishabh Bhatnagar
@ 2020-04-21 18:10 ` Rishabh Bhatnagar
  2020-04-21 18:10   ` Rishabh Bhatnagar
                     ` (2 more replies)
  2020-04-21 18:10 ` [PATCH v3 2/2] remoteproc: core: Register the " Rishabh Bhatnagar
  2 siblings, 3 replies; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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 <rishabhb@codeaurora.org>
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig                         |   9 ++
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/remoteproc_cdev.c               | 143 +++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h           |  21 +++
 include/linux/remoteproc.h                         |   3 +
 include/uapi/linux/remoteproc_cdev.h               |  20 +++
 7 files changed, 198 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 2e91370..412b2a0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -337,6 +337,7 @@ Code  Seq#    Include File                                           Comments
 0xB4  00-0F  linux/gpio.h                                            <mailto:linux-gpio@vger.kernel.org>
 0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-remoteproc@vger.kernel.org>
 0xB6  all    linux/fpga-dfl.h
+0xB7  all    uapi/linux/remoteproc_cdev.h			     <mailto:linux-remoteproc@vger.kernel.org>
 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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 0000000..65142ec
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,143 @@
+// 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/remoteproc.h>
+#include <uapi/linux/remoteproc_cdev.h>
+#include <linux/uaccess.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_OFFLINE)
+			return -ENXIO;
+
+		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 = (void __user *)arg;
+	int ret;
+	bool param;
+
+	switch (ioctl) {
+	case RPROC_SET_SHUTDOWN_ON_RELEASE:
+		ret = copy_from_user(&param, argp, sizeof(bool));
+		if (ret) {
+			dev_err(&rproc->dev, "Data copy from userspace failed\n");
+			return -EINVAL;
+		}
+		rproc->cdev_put_on_release = param;
+		break;
+	case RPROC_GET_SHUTDOWN_ON_RELEASE:
+		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
+				   sizeof(bool));
+		if (ret) {
+			dev_err(&rproc->dev, "Data copy to userspace failed\n");
+			return -EINVAL;
+		}
+		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);
+
+	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
+		rproc_shutdown(rproc);
+
+	return 0;
+}
+
+
+static const struct file_operations rproc_fops = {
+	.write = rproc_cdev_write,
+	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, "rproc");
+}
+
+void __init rproc_init_cdev(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "rproc");
+	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(MAJOR(rproc_major), 0),
+				 NUM_RPROC_DEVICES);
+}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 493ef92..fb9d891 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
 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)
+{
+}
+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 16ad666..9bd2ff5 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>
 
 /**
@@ -514,6 +515,8 @@ struct rproc {
 	bool auto_boot;
 	struct list_head dump_segments;
 	int nb_vdev;
+	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..3975120
--- /dev/null
+++ b/include/uapi/linux/remoteproc_cdev.h
@@ -0,0 +1,20 @@
+/* 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
+
+#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
+#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
@ 2020-04-21 18:10   ` Rishabh Bhatnagar
  2020-04-29 23:09   ` Mathieu Poirier
  2020-04-30  8:30   ` Arnaud POULIQUEN
  2 siblings, 0 replies; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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 <rishabhb@codeaurora.org>
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig                         |   9 ++
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/remoteproc_cdev.c               | 143 +++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h           |  21 +++
 include/linux/remoteproc.h                         |   3 +
 include/uapi/linux/remoteproc_cdev.h               |  20 +++
 7 files changed, 198 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 2e91370..412b2a0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -337,6 +337,7 @@ Code  Seq#    Include File                                           Comments
 0xB4  00-0F  linux/gpio.h                                            <mailto:linux-gpio@vger.kernel.org>
 0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-remoteproc@vger.kernel.org>
 0xB6  all    linux/fpga-dfl.h
+0xB7  all    uapi/linux/remoteproc_cdev.h			     <mailto:linux-remoteproc@vger.kernel.org>
 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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 0000000..65142ec
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,143 @@
+// 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/remoteproc.h>
+#include <uapi/linux/remoteproc_cdev.h>
+#include <linux/uaccess.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_OFFLINE)
+			return -ENXIO;
+
+		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 = (void __user *)arg;
+	int ret;
+	bool param;
+
+	switch (ioctl) {
+	case RPROC_SET_SHUTDOWN_ON_RELEASE:
+		ret = copy_from_user(&param, argp, sizeof(bool));
+		if (ret) {
+			dev_err(&rproc->dev, "Data copy from userspace failed\n");
+			return -EINVAL;
+		}
+		rproc->cdev_put_on_release = param;
+		break;
+	case RPROC_GET_SHUTDOWN_ON_RELEASE:
+		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
+				   sizeof(bool));
+		if (ret) {
+			dev_err(&rproc->dev, "Data copy to userspace failed\n");
+			return -EINVAL;
+		}
+		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);
+
+	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
+		rproc_shutdown(rproc);
+
+	return 0;
+}
+
+
+static const struct file_operations rproc_fops = {
+	.write = rproc_cdev_write,
+	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, "rproc");
+}
+
+void __init rproc_init_cdev(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "rproc");
+	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(MAJOR(rproc_major), 0),
+				 NUM_RPROC_DEVICES);
+}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 493ef92..fb9d891 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
 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)
+{
+}
+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 16ad666..9bd2ff5 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>
 
 /**
@@ -514,6 +515,8 @@ struct rproc {
 	bool auto_boot;
 	struct list_head dump_segments;
 	int nb_vdev;
+	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..3975120
--- /dev/null
+++ b/include/uapi/linux/remoteproc_cdev.h
@@ -0,0 +1,20 @@
+/* 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
+
+#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
+#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] remoteproc: core: Register the character device interface
  2020-04-21 18:10 [PATCH v3 0/2] Add character device interface to remoteproc Rishabh Bhatnagar
  2020-04-21 18:10 ` Rishabh Bhatnagar
  2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
@ 2020-04-21 18:10 ` Rishabh Bhatnagar
  2020-04-21 18:10   ` Rishabh Bhatnagar
  2 siblings, 1 reply; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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 <rishabhb@codeaurora.org>
---
 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 097f33e..76ba171 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1916,6 +1916,13 @@ int rproc_add(struct rproc *rproc)
 	/* create debugfs entries */
 	rproc_create_debug_dir(rproc);
 
+	/* 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;
+	}
+
 	/* if rproc is marked always-on, request it to boot */
 	if (rproc->auto_boot) {
 		ret = rproc_trigger_auto_boot(rproc);
@@ -2137,6 +2144,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);
@@ -2220,6 +2228,7 @@ static int __init remoteproc_init(void)
 {
 	rproc_init_sysfs();
 	rproc_init_debugfs();
+	rproc_init_cdev();
 
 	return 0;
 }
@@ -2231,6 +2240,7 @@ static void __exit remoteproc_exit(void)
 
 	rproc_exit_debugfs();
 	rproc_exit_sysfs();
+	rproc_exit_cdev();
 }
 module_exit(remoteproc_exit);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] remoteproc: core: Register the character device interface
  2020-04-21 18:10 ` [PATCH v3 2/2] remoteproc: core: Register the " Rishabh Bhatnagar
@ 2020-04-21 18:10   ` Rishabh Bhatnagar
  0 siblings, 0 replies; 17+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-21 18:10 UTC (permalink / raw)
  To: linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup,
	Rishabh Bhatnagar

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 <rishabhb@codeaurora.org>
---
 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 097f33e..76ba171 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1916,6 +1916,13 @@ int rproc_add(struct rproc *rproc)
 	/* create debugfs entries */
 	rproc_create_debug_dir(rproc);
 
+	/* 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;
+	}
+
 	/* if rproc is marked always-on, request it to boot */
 	if (rproc->auto_boot) {
 		ret = rproc_trigger_auto_boot(rproc);
@@ -2137,6 +2144,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);
@@ -2220,6 +2228,7 @@ static int __init remoteproc_init(void)
 {
 	rproc_init_sysfs();
 	rproc_init_debugfs();
+	rproc_init_cdev();
 
 	return 0;
 }
@@ -2231,6 +2240,7 @@ static void __exit remoteproc_exit(void)
 
 	rproc_exit_debugfs();
 	rproc_exit_sysfs();
+	rproc_exit_cdev();
 }
 module_exit(remoteproc_exit);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
  2020-04-21 18:10   ` Rishabh Bhatnagar
@ 2020-04-29 23:09   ` Mathieu Poirier
  2020-04-30  8:30   ` Arnaud POULIQUEN
  2 siblings, 0 replies; 17+ messages in thread
From: Mathieu Poirier @ 2020-04-29 23:09 UTC (permalink / raw)
  To: Rishabh Bhatnagar
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad, tsoni,
	psodagud, sidgup

On Tue, Apr 21, 2020 at 11:10:17AM -0700, Rishabh Bhatnagar 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 <rishabhb@codeaurora.org>
> ---
>  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>  drivers/remoteproc/Kconfig                         |   9 ++
>  drivers/remoteproc/Makefile                        |   1 +
>  drivers/remoteproc/remoteproc_cdev.c               | 143 +++++++++++++++++++++
>  drivers/remoteproc/remoteproc_internal.h           |  21 +++
>  include/linux/remoteproc.h                         |   3 +
>  include/uapi/linux/remoteproc_cdev.h               |  20 +++
>  7 files changed, 198 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 2e91370..412b2a0 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -337,6 +337,7 @@ Code  Seq#    Include File                                           Comments
>  0xB4  00-0F  linux/gpio.h                                            <mailto:linux-gpio@vger.kernel.org>
>  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-remoteproc@vger.kernel.org>
>  0xB6  all    linux/fpga-dfl.h
> +0xB7  all    uapi/linux/remoteproc_cdev.h			     <mailto:linux-remoteproc@vger.kernel.org>
>  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 de3862c..6374b79 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 Remoteproc

s/Remoteproc/"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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> new file mode 100644
> index 0000000..65142ec
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -0,0 +1,143 @@
> +// 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/remoteproc.h>
> +#include <uapi/linux/remoteproc_cdev.h>
> +#include <linux/uaccess.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_OFFLINE)
> +			return -ENXIO;
> +
> +		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 = (void __user *)arg;
> +	int ret;
> +	bool param;
> +
> +	switch (ioctl) {
> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
> +		ret = copy_from_user(&param, argp, sizeof(bool));
> +		if (ret) {
> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
> +			return -EINVAL;
> +		}
> +		rproc->cdev_put_on_release = param;
> +		break;
> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
> +				   sizeof(bool));
> +		if (ret) {
> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
> +			return -EINVAL;
> +		}
> +		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);
> +
> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
> +		rproc_shutdown(rproc);
> +
> +	return 0;
> +}
> +
> +
> +static const struct file_operations rproc_fops = {
> +	.write = rproc_cdev_write,
> +	.unlocked_ioctl = rproc_device_ioctl,

Because this is available on any kind of system it is probably best to provide
an compat_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, "rproc");
> +}
> +
> +void __init rproc_init_cdev(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "rproc");
> +	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(MAJOR(rproc_major), 0),
> +				 NUM_RPROC_DEVICES);

Why not simply using rproc_major here?

> +}
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 493ef92..fb9d891 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>  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)
> +{
> +}
> +static inline int rproc_char_device_add(struct rproc *rproc)
> +{
> +	return 0;

Add a comment that mentions the support of the character interface is optional
and as such and error is not generated if not available on the system.

> +}
> +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 16ad666..9bd2ff5 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>
>  
>  /**
> @@ -514,6 +515,8 @@ struct rproc {
>  	bool auto_boot;
>  	struct list_head dump_segments;
>  	int nb_vdev;
> +	struct cdev char_dev;
> +	bool cdev_put_on_release;

Please add documentation for these.  Also note you will have to rebase your work
on the latest rproc-next.

Thanks,
Mathieu

>  };
>  
>  /**
> diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> new file mode 100644
> index 0000000..3975120
> --- /dev/null
> +++ b/include/uapi/linux/remoteproc_cdev.h
> @@ -0,0 +1,20 @@
> +/* 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
> +
> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
> +
> +#endif
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
  2020-04-21 18:10   ` Rishabh Bhatnagar
  2020-04-29 23:09   ` Mathieu Poirier
@ 2020-04-30  8:30   ` Arnaud POULIQUEN
  2020-04-30 16:44     ` Mathieu Poirier
  2020-06-16 19:56     ` rishabhb
  2 siblings, 2 replies; 17+ messages in thread
From: Arnaud POULIQUEN @ 2020-04-30  8:30 UTC (permalink / raw)
  To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
  Cc: bjorn.andersson, mathieu.poirier, ohad, tsoni, psodagud, sidgup

Hi Rishabh,


On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
> 

Thanks for intruducing Ioctl, this will help for future evolutions.

> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>  drivers/remoteproc/Kconfig                         |   9 ++
>  drivers/remoteproc/Makefile                        |   1 +
>  drivers/remoteproc/remoteproc_cdev.c               | 143 +++++++++++++++++++++
>  drivers/remoteproc/remoteproc_internal.h           |  21 +++
>  include/linux/remoteproc.h                         |   3 +
>  include/uapi/linux/remoteproc_cdev.h               |  20 +++
>  7 files changed, 198 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 2e91370..412b2a0 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -337,6 +337,7 @@ Code  Seq#    Include File                                           Comments
>  0xB4  00-0F  linux/gpio.h                                            <mailto:linux-gpio@vger.kernel.org>
>  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-remoteproc@vger.kernel.org>
>  0xB6  all    linux/fpga-dfl.h
> +0xB7  all    uapi/linux/remoteproc_cdev.h			     <mailto:linux-remoteproc@vger.kernel.org>
>  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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> new file mode 100644
> index 0000000..65142ec
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -0,0 +1,143 @@
> +// 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/remoteproc.h>
> +#include <uapi/linux/remoteproc_cdev.h>
> +#include <linux/uaccess.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_OFFLINE)
> +			return -ENXIO;

returning ENXIO in this case seems to me no appropriate , what about EPERM or
EINVAL (rproc_sysfs) ?

> +
> +		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 = (void __user *)arg;
> +	int ret;
> +	bool param;
> +
> +	switch (ioctl) {
> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
> +		ret = copy_from_user(&param, argp, sizeof(bool));
> +		if (ret) {
> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
> +			return -EINVAL;
> +		}
> +		rproc->cdev_put_on_release = param;

argp is an void value, where cdev_put_on_release is a bool a check or a conversion  seems
missing

> +		break;
> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
> +				   sizeof(bool));
> +		if (ret) {
> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
> +			return -EINVAL;
> +		}
> +		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);
> +
> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
> +		rproc_shutdown(rproc);
> +
> +	return 0;
> +}
> +
> +
> +static const struct file_operations rproc_fops = {
> +	.write = rproc_cdev_write,
> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, "rproc");
> +}
> +
> +void __init rproc_init_cdev(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "rproc");

"remoteproc"instead of "rproc" (in line with sysfs and debugfs naming) .

> +	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(MAJOR(rproc_major), 0),
> +				 NUM_RPROC_DEVICES);
> +}
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 493ef92..fb9d891 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>  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)
> +{
> +}
> +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 16ad666..9bd2ff5 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>
>  
>  /**
> @@ -514,6 +515,8 @@ struct rproc {
>  	bool auto_boot;
>  	struct list_head dump_segments;
>  	int nb_vdev;
> +	struct cdev char_dev;
> +	bool cdev_put_on_release;
>  };

These parameters are local variables of rproc_cdev. Could be defined in a separate structure.
with a pointer in rproc to this structure.

>  
>  /**
> diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> new file mode 100644
> index 0000000..3975120
> --- /dev/null
> +++ b/include/uapi/linux/remoteproc_cdev.h
> @@ -0,0 +1,20 @@
> +/* 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
> +
> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
> +
> +#endif
> 
IOCTLs should probaly be documented.

Thanks,
Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-30  8:30   ` Arnaud POULIQUEN
@ 2020-04-30 16:44     ` Mathieu Poirier
  2020-06-16 19:56     ` rishabhb
  1 sibling, 0 replies; 17+ messages in thread
From: Mathieu Poirier @ 2020-04-30 16:44 UTC (permalink / raw)
  To: Arnaud POULIQUEN
  Cc: Rishabh Bhatnagar, linux-remoteproc, Linux Kernel Mailing List,
	Bjorn Andersson, Ohad Ben-Cohen, tsoni, psodagud,
	Siddharth Gupta

On Thu, 30 Apr 2020 at 02:30, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Rishabh,
>
>
> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
> >
>
> Thanks for intruducing Ioctl, this will help for future evolutions.
>
> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > ---
> >  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
> >  drivers/remoteproc/Kconfig                         |   9 ++
> >  drivers/remoteproc/Makefile                        |   1 +
> >  drivers/remoteproc/remoteproc_cdev.c               | 143 +++++++++++++++++++++
> >  drivers/remoteproc/remoteproc_internal.h           |  21 +++
> >  include/linux/remoteproc.h                         |   3 +
> >  include/uapi/linux/remoteproc_cdev.h               |  20 +++
> >  7 files changed, 198 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 2e91370..412b2a0 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -337,6 +337,7 @@ Code  Seq#    Include File                                           Comments
> >  0xB4  00-0F  linux/gpio.h                                            <mailto:linux-gpio@vger.kernel.org>
> >  0xB5  00-0F  uapi/linux/rpmsg.h                                      <mailto:linux-remoteproc@vger.kernel.org>
> >  0xB6  all    linux/fpga-dfl.h
> > +0xB7  all    uapi/linux/remoteproc_cdev.h                         <mailto:linux-remoteproc@vger.kernel.org>
> >  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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)                        += mtk_scp.o mtk_scp_ipi.o
> >  obj-$(CONFIG_OMAP_REMOTEPROC)                += omap_remoteproc.o
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > new file mode 100644
> > index 0000000..65142ec
> > --- /dev/null
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -0,0 +1,143 @@
> > +// 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/remoteproc.h>
> > +#include <uapi/linux/remoteproc_cdev.h>
> > +#include <linux/uaccess.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_OFFLINE)
> > +                     return -ENXIO;
>
> returning ENXIO in this case seems to me no appropriate , what about EPERM or
> EINVAL (rproc_sysfs) ?
>
> > +
> > +             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 = (void __user *)arg;
> > +     int ret;
> > +     bool param;
> > +
> > +     switch (ioctl) {
> > +     case RPROC_SET_SHUTDOWN_ON_RELEASE:
> > +             ret = copy_from_user(&param, argp, sizeof(bool));
> > +             if (ret) {
> > +                     dev_err(&rproc->dev, "Data copy from userspace failed\n");
> > +                     return -EINVAL;
> > +             }
> > +             rproc->cdev_put_on_release = param;
>
> argp is an void value, where cdev_put_on_release is a bool a check or a conversion  seems
> missing
>
> > +             break;
> > +     case RPROC_GET_SHUTDOWN_ON_RELEASE:
> > +             ret = copy_to_user(argp, &rproc->cdev_put_on_release,
> > +                                sizeof(bool));
> > +             if (ret) {
> > +                     dev_err(&rproc->dev, "Data copy to userspace failed\n");
> > +                     return -EINVAL;
> > +             }
> > +             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);
> > +
> > +     if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
> > +             rproc_shutdown(rproc);
> > +
> > +     return 0;
> > +}
> > +
> > +
> > +static const struct file_operations rproc_fops = {
> > +     .write = rproc_cdev_write,
> > +     .unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, "rproc");
> > +}
> > +
> > +void __init rproc_init_cdev(void)
> > +{
> > +     int ret;
> > +
> > +     ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, "rproc");
>
> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming) .
>
> > +     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(MAJOR(rproc_major), 0),
> > +                              NUM_RPROC_DEVICES);
> > +}
> > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > index 493ef92..fb9d891 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> >  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)
> > +{
> > +}
> > +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 16ad666..9bd2ff5 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>
> >
> >  /**
> > @@ -514,6 +515,8 @@ struct rproc {
> >       bool auto_boot;
> >       struct list_head dump_segments;
> >       int nb_vdev;
> > +     struct cdev char_dev;
> > +     bool cdev_put_on_release;
> >  };
>
> These parameters are local variables of rproc_cdev. Could be defined in a separate structure.
> with a pointer in rproc to this structure.

I think structure @char_dev is just one more core member of rproc - to
me the real problem is with @cdev_put_on_release.  It is yet another
option that has an effect on the processing done on the remote
processor.  Other 'bool' types like has_iommu, auto_boot and
recovery_disabled are additional examples.  No matter how much we are
careful at limiting their proliferation it is clear to me the current
way of declaring bool types every time we need a new option does not
scale.

I have been thinking about introducing a new u32 field called
"options" and assigning each of the current (and future) boolean flags
a bitfield.  Individual options would be set with an exported
function, something like rproc_set_option(RPROC_OPTION_AUTO_BOOT).
That way users never directly manipulate the bitfields and it gives us
the opportunity to change how options are managed in the future if we
need to.

I will be sending a patchset for this feature in the coming days so
that we can discuss the best way to deal with flags.

Thanks,
Mathieu

>
> >
> >  /**
> > diff --git a/include/uapi/linux/remoteproc_cdev.h b/include/uapi/linux/remoteproc_cdev.h
> > new file mode 100644
> > index 0000000..3975120
> > --- /dev/null
> > +++ b/include/uapi/linux/remoteproc_cdev.h
> > @@ -0,0 +1,20 @@
> > +/* 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
> > +
> > +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
> > +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
> > +
> > +#endif
> >
> IOCTLs should probaly be documented.
>
> Thanks,
> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-04-30  8:30   ` Arnaud POULIQUEN
  2020-04-30 16:44     ` Mathieu Poirier
@ 2020-06-16 19:56     ` rishabhb
  2020-06-17  8:44       ` Arnaud POULIQUEN
  1 sibling, 1 reply; 17+ messages in thread
From: rishabhb @ 2020-06-16 19:56 UTC (permalink / raw)
  To: Arnaud POULIQUEN
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, sidgup, linux-remoteproc-owner

On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
> Hi Rishabh,
> 
> 
> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>> 
> 
> Thanks for intruducing Ioctl, this will help for future evolutions.
> 
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>>  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>  drivers/remoteproc/Kconfig                         |   9 ++
>>  drivers/remoteproc/Makefile                        |   1 +
>>  drivers/remoteproc/remoteproc_cdev.c               | 143 
>> +++++++++++++++++++++
>>  drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>  include/linux/remoteproc.h                         |   3 +
>>  include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>  7 files changed, 198 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 2e91370..412b2a0 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -337,6 +337,7 @@ Code  Seq#    Include File                         
>>                   Comments
>>  0xB4  00-0F  linux/gpio.h                                            
>> <mailto:linux-gpio@vger.kernel.org>
>>  0xB5  00-0F  uapi/linux/rpmsg.h                                      
>> <mailto:linux-remoteproc@vger.kernel.org>
>>  0xB6  all    linux/fpga-dfl.h
>> +0xB7  all    uapi/linux/remoteproc_cdev.h			     
>> <mailto:linux-remoteproc@vger.kernel.org>
>>  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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>> diff --git a/drivers/remoteproc/remoteproc_cdev.c 
>> b/drivers/remoteproc/remoteproc_cdev.c
>> new file mode 100644
>> index 0000000..65142ec
>> --- /dev/null
>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>> @@ -0,0 +1,143 @@
>> +// 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/remoteproc.h>
>> +#include <uapi/linux/remoteproc_cdev.h>
>> +#include <linux/uaccess.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_OFFLINE)
>> +			return -ENXIO;
> 
> returning ENXIO in this case seems to me no appropriate , what about 
> EPERM or
> EINVAL (rproc_sysfs) ?
> 
>> +
>> +		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 = (void __user *)arg;
>> +	int ret;
>> +	bool param;
>> +
>> +	switch (ioctl) {
>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>> +		if (ret) {
>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>> +			return -EINVAL;
>> +		}
>> +		rproc->cdev_put_on_release = param;
> 
> argp is an void value, where cdev_put_on_release is a bool a check or
> a conversion  seems
> missing
> 
>> +		break;
>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>> +				   sizeof(bool));
>> +		if (ret) {
>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>> +			return -EINVAL;
>> +		}
>> +		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);
>> +
>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>> +		rproc_shutdown(rproc);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static const struct file_operations rproc_fops = {
>> +	.write = rproc_cdev_write,
>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, 
>> "rproc");
>> +}
>> +
>> +void __init rproc_init_cdev(void)
>> +{
>> +	int ret;
>> +
>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, 
>> "rproc");
> 
> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming) 
> .
> 
>> +	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(MAJOR(rproc_major), 0),
>> +				 NUM_RPROC_DEVICES);
>> +}
>> diff --git a/drivers/remoteproc/remoteproc_internal.h 
>> b/drivers/remoteproc/remoteproc_internal.h
>> index 493ef92..fb9d891 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char 
>> *name, struct rproc *rproc,
>>  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)
>> +{
>> +}
>> +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 16ad666..9bd2ff5 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>
>> 
>>  /**
>> @@ -514,6 +515,8 @@ struct rproc {
>>  	bool auto_boot;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>> +	struct cdev char_dev;
>> +	bool cdev_put_on_release;
>>  };
> 
> These parameters are local variables of rproc_cdev. Could be defined
> in a separate structure.
> with a pointer in rproc to this structure.
> 
>> 
>>  /**
>> diff --git a/include/uapi/linux/remoteproc_cdev.h 
>> b/include/uapi/linux/remoteproc_cdev.h
>> new file mode 100644
>> index 0000000..3975120
>> --- /dev/null
>> +++ b/include/uapi/linux/remoteproc_cdev.h
>> @@ -0,0 +1,20 @@
>> +/* 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
>> +
>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>> +
>> +#endif
>> 
> IOCTLs should probaly be documented.
I have added documentation to 
Documentation/userspace-api/ioctl/ioctl-number.rst
Is there another place where I need to add documentation for this?
> 
> Thanks,
> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-16 19:56     ` rishabhb
@ 2020-06-17  8:44       ` Arnaud POULIQUEN
  2020-06-30  5:38         ` Siddharth Gupta
  0 siblings, 1 reply; 17+ messages in thread
From: Arnaud POULIQUEN @ 2020-06-17  8:44 UTC (permalink / raw)
  To: rishabhb
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, sidgup, linux-remoteproc-owner



On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
>> Hi Rishabh,
>>
>>
>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>>>
>>
>> Thanks for intruducing Ioctl, this will help for future evolutions.
>>
>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>> ---
>>>  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>>  drivers/remoteproc/Kconfig                         |   9 ++
>>>  drivers/remoteproc/Makefile                        |   1 +
>>>  drivers/remoteproc/remoteproc_cdev.c               | 143 
>>> +++++++++++++++++++++
>>>  drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>>  include/linux/remoteproc.h                         |   3 +
>>>  include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>>  7 files changed, 198 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 2e91370..412b2a0 100644
>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File                         
>>>                   Comments
>>>  0xB4  00-0F  linux/gpio.h                                            
>>> <mailto:linux-gpio@vger.kernel.org>
>>>  0xB5  00-0F  uapi/linux/rpmsg.h                                      
>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>  0xB6  all    linux/fpga-dfl.h
>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			     
>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>  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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c 
>>> b/drivers/remoteproc/remoteproc_cdev.c
>>> new file mode 100644
>>> index 0000000..65142ec
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>> @@ -0,0 +1,143 @@
>>> +// 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/remoteproc.h>
>>> +#include <uapi/linux/remoteproc_cdev.h>
>>> +#include <linux/uaccess.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_OFFLINE)
>>> +			return -ENXIO;
>>
>> returning ENXIO in this case seems to me no appropriate , what about 
>> EPERM or
>> EINVAL (rproc_sysfs) ?
>>
>>> +
>>> +		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 = (void __user *)arg;
>>> +	int ret;
>>> +	bool param;
>>> +
>>> +	switch (ioctl) {
>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>>> +		if (ret) {
>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>>> +			return -EINVAL;
>>> +		}
>>> +		rproc->cdev_put_on_release = param;
>>
>> argp is an void value, where cdev_put_on_release is a bool a check or
>> a conversion  seems
>> missing
>>
>>> +		break;
>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>>> +				   sizeof(bool));
>>> +		if (ret) {
>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>>> +			return -EINVAL;
>>> +		}
>>> +		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);
>>> +
>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>>> +		rproc_shutdown(rproc);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +
>>> +static const struct file_operations rproc_fops = {
>>> +	.write = rproc_cdev_write,
>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1, 
>>> "rproc");
>>> +}
>>> +
>>> +void __init rproc_init_cdev(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES, 
>>> "rproc");
>>
>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming) 
>> .
>>
>>> +	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(MAJOR(rproc_major), 0),
>>> +				 NUM_RPROC_DEVICES);
>>> +}
>>> diff --git a/drivers/remoteproc/remoteproc_internal.h 
>>> b/drivers/remoteproc/remoteproc_internal.h
>>> index 493ef92..fb9d891 100644
>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char 
>>> *name, struct rproc *rproc,
>>>  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)
>>> +{
>>> +}
>>> +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 16ad666..9bd2ff5 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>
>>>
>>>  /**
>>> @@ -514,6 +515,8 @@ struct rproc {
>>>  	bool auto_boot;
>>>  	struct list_head dump_segments;
>>>  	int nb_vdev;
>>> +	struct cdev char_dev;
>>> +	bool cdev_put_on_release;
>>>  };
>>
>> These parameters are local variables of rproc_cdev. Could be defined
>> in a separate structure.
>> with a pointer in rproc to this structure.
>>
>>>
>>>  /**
>>> diff --git a/include/uapi/linux/remoteproc_cdev.h 
>>> b/include/uapi/linux/remoteproc_cdev.h
>>> new file mode 100644
>>> index 0000000..3975120
>>> --- /dev/null
>>> +++ b/include/uapi/linux/remoteproc_cdev.h
>>> @@ -0,0 +1,20 @@
>>> +/* 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
>>> +
>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>>> +
>>> +#endif
>>>
>> IOCTLs should probaly be documented.
> I have added documentation to 
> Documentation/userspace-api/ioctl/ioctl-number.rst
> Is there another place where I need to add documentation for this?

Could you add in this file comments that describe
the IOCTL usage and associated parameter?

Regards,
Arnaud

>>
>> Thanks,
>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-17  8:44       ` Arnaud POULIQUEN
@ 2020-06-30  5:38         ` Siddharth Gupta
  2020-06-30  7:43           ` Arnaud POULIQUEN
  0 siblings, 1 reply; 17+ messages in thread
From: Siddharth Gupta @ 2020-06-30  5:38 UTC (permalink / raw)
  To: Arnaud POULIQUEN, rishabhb
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, linux-remoteproc-owner


On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
>
> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
>>> Hi Rishabh,
>>>
>>>
>>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>>>>
>>> Thanks for intruducing Ioctl, this will help for future evolutions.
>>>
>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>>> ---
>>>>   Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>>>   drivers/remoteproc/Kconfig                         |   9 ++
>>>>   drivers/remoteproc/Makefile                        |   1 +
>>>>   drivers/remoteproc/remoteproc_cdev.c               | 143
>>>> +++++++++++++++++++++
>>>>   drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>>>   include/linux/remoteproc.h                         |   3 +
>>>>   include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>>>   7 files changed, 198 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 2e91370..412b2a0 100644
>>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File
>>>>                    Comments
>>>>   0xB4  00-0F  linux/gpio.h
>>>> <mailto:linux-gpio@vger.kernel.org>
>>>>   0xB5  00-0F  uapi/linux/rpmsg.h
>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>   0xB6  all    linux/fpga-dfl.h
>>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			
>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>   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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c
>>>> b/drivers/remoteproc/remoteproc_cdev.c
>>>> new file mode 100644
>>>> index 0000000..65142ec
>>>> --- /dev/null
>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>> @@ -0,0 +1,143 @@
>>>> +// 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/remoteproc.h>
>>>> +#include <uapi/linux/remoteproc_cdev.h>
>>>> +#include <linux/uaccess.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_OFFLINE)
>>>> +			return -ENXIO;
>>> returning ENXIO in this case seems to me no appropriate , what about
>>> EPERM or
>>> EINVAL (rproc_sysfs) ?

I think EPERM would indicate the operation is not permitted on the 
device, and
EINVAL would indicate that the command/string they wrote to the char device
is not a valid command/string. I guess even ENXIO may not be appropriate?

In that case we could use EHOSTDOWN or ESHUTDOWN, thoughts?

Thanks,
Sid

>>>
>>>> +
>>>> +		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 = (void __user *)arg;
>>>> +	int ret;
>>>> +	bool param;
>>>> +
>>>> +	switch (ioctl) {
>>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>>>> +		if (ret) {
>>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +		rproc->cdev_put_on_release = param;
>>> argp is an void value, where cdev_put_on_release is a bool a check or
>>> a conversion  seems
>>> missing
>>>
>>>> +		break;
>>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>>>> +				   sizeof(bool));
>>>> +		if (ret) {
>>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +		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);
>>>> +
>>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>>>> +		rproc_shutdown(rproc);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +
>>>> +static const struct file_operations rproc_fops = {
>>>> +	.write = rproc_cdev_write,
>>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1,
>>>> "rproc");
>>>> +}
>>>> +
>>>> +void __init rproc_init_cdev(void)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES,
>>>> "rproc");
>>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming)
>>> .
>>>
>>>> +	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(MAJOR(rproc_major), 0),
>>>> +				 NUM_RPROC_DEVICES);
>>>> +}
>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>> index 493ef92..fb9d891 100644
>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char
>>>> *name, struct rproc *rproc,
>>>>   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)
>>>> +{
>>>> +}
>>>> +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 16ad666..9bd2ff5 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>
>>>>
>>>>   /**
>>>> @@ -514,6 +515,8 @@ struct rproc {
>>>>   	bool auto_boot;
>>>>   	struct list_head dump_segments;
>>>>   	int nb_vdev;
>>>> +	struct cdev char_dev;
>>>> +	bool cdev_put_on_release;
>>>>   };
>>> These parameters are local variables of rproc_cdev. Could be defined
>>> in a separate structure.
>>> with a pointer in rproc to this structure.
>>>
>>>>   /**
>>>> diff --git a/include/uapi/linux/remoteproc_cdev.h
>>>> b/include/uapi/linux/remoteproc_cdev.h
>>>> new file mode 100644
>>>> index 0000000..3975120
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/remoteproc_cdev.h
>>>> @@ -0,0 +1,20 @@
>>>> +/* 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
>>>> +
>>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>>>> +
>>>> +#endif
>>>>
>>> IOCTLs should probaly be documented.
>> I have added documentation to
>> Documentation/userspace-api/ioctl/ioctl-number.rst
>> Is there another place where I need to add documentation for this?
> Could you add in this file comments that describe
> the IOCTL usage and associated parameter?
>
> Regards,
> Arnaud
>
>>> Thanks,
>>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-30  5:38         ` Siddharth Gupta
@ 2020-06-30  7:43           ` Arnaud POULIQUEN
  2020-06-30 22:02             ` Siddharth Gupta
  2020-07-01  4:38             ` Bjorn Andersson
  0 siblings, 2 replies; 17+ messages in thread
From: Arnaud POULIQUEN @ 2020-06-30  7:43 UTC (permalink / raw)
  To: Siddharth Gupta, rishabhb
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, linux-remoteproc-owner



On 6/30/20 7:38 AM, Siddharth Gupta wrote:
> 
> On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
>>
>> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
>>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
>>>> Hi Rishabh,
>>>>
>>>>
>>>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>>>>>
>>>> Thanks for intruducing Ioctl, this will help for future evolutions.
>>>>
>>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>>>> ---
>>>>>   Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>>>>   drivers/remoteproc/Kconfig                         |   9 ++
>>>>>   drivers/remoteproc/Makefile                        |   1 +
>>>>>   drivers/remoteproc/remoteproc_cdev.c               | 143
>>>>> +++++++++++++++++++++
>>>>>   drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>>>>   include/linux/remoteproc.h                         |   3 +
>>>>>   include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>>>>   7 files changed, 198 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 2e91370..412b2a0 100644
>>>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File
>>>>>                    Comments
>>>>>   0xB4  00-0F  linux/gpio.h
>>>>> <mailto:linux-gpio@vger.kernel.org>
>>>>>   0xB5  00-0F  uapi/linux/rpmsg.h
>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>   0xB6  all    linux/fpga-dfl.h
>>>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			
>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>   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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c
>>>>> b/drivers/remoteproc/remoteproc_cdev.c
>>>>> new file mode 100644
>>>>> index 0000000..65142ec
>>>>> --- /dev/null
>>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>>> @@ -0,0 +1,143 @@
>>>>> +// 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/remoteproc.h>
>>>>> +#include <uapi/linux/remoteproc_cdev.h>
>>>>> +#include <linux/uaccess.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_OFFLINE)
>>>>> +			return -ENXIO;
>>>> returning ENXIO in this case seems to me no appropriate , what about
>>>> EPERM or
>>>> EINVAL (rproc_sysfs) ?
> 
> I think EPERM would indicate the operation is not permitted on the 
> device, and
> EINVAL would indicate that the command/string they wrote to the char device
> is not a valid command/string. I guess even ENXIO may not be appropriate?
> 
> In that case we could use EHOSTDOWN or ESHUTDOWN, thoughts?

Regarding remoteproc_sysfs.c[1], seems that the -EINVAL return would be coherent.
It would probably also be better to replace your condition with 
	if (rproc->state != RPROC_RUNNING)
 

[1] https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_sysfs.c#L104

Regards,
Arnaud

> 
> Thanks,
> Sid
> 
>>>>
>>>>> +
>>>>> +		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 = (void __user *)arg;
>>>>> +	int ret;
>>>>> +	bool param;
>>>>> +
>>>>> +	switch (ioctl) {
>>>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>>>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>>>>> +		if (ret) {
>>>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +		rproc->cdev_put_on_release = param;
>>>> argp is an void value, where cdev_put_on_release is a bool a check or
>>>> a conversion  seems
>>>> missing
>>>>
>>>>> +		break;
>>>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>>>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>>>>> +				   sizeof(bool));
>>>>> +		if (ret) {
>>>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +		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);
>>>>> +
>>>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>>>>> +		rproc_shutdown(rproc);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +
>>>>> +static const struct file_operations rproc_fops = {
>>>>> +	.write = rproc_cdev_write,
>>>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1,
>>>>> "rproc");
>>>>> +}
>>>>> +
>>>>> +void __init rproc_init_cdev(void)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES,
>>>>> "rproc");
>>>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming)
>>>> .
>>>>
>>>>> +	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(MAJOR(rproc_major), 0),
>>>>> +				 NUM_RPROC_DEVICES);
>>>>> +}
>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>> index 493ef92..fb9d891 100644
>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char
>>>>> *name, struct rproc *rproc,
>>>>>   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)
>>>>> +{
>>>>> +}
>>>>> +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 16ad666..9bd2ff5 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>
>>>>>
>>>>>   /**
>>>>> @@ -514,6 +515,8 @@ struct rproc {
>>>>>   	bool auto_boot;
>>>>>   	struct list_head dump_segments;
>>>>>   	int nb_vdev;
>>>>> +	struct cdev char_dev;
>>>>> +	bool cdev_put_on_release;
>>>>>   };
>>>> These parameters are local variables of rproc_cdev. Could be defined
>>>> in a separate structure.
>>>> with a pointer in rproc to this structure.
>>>>
>>>>>   /**
>>>>> diff --git a/include/uapi/linux/remoteproc_cdev.h
>>>>> b/include/uapi/linux/remoteproc_cdev.h
>>>>> new file mode 100644
>>>>> index 0000000..3975120
>>>>> --- /dev/null
>>>>> +++ b/include/uapi/linux/remoteproc_cdev.h
>>>>> @@ -0,0 +1,20 @@
>>>>> +/* 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
>>>>> +
>>>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>>>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>>>>> +
>>>>> +#endif
>>>>>
>>>> IOCTLs should probaly be documented.
>>> I have added documentation to
>>> Documentation/userspace-api/ioctl/ioctl-number.rst
>>> Is there another place where I need to add documentation for this?
>> Could you add in this file comments that describe
>> the IOCTL usage and associated parameter?
>>
>> Regards,
>> Arnaud
>>
>>>> Thanks,
>>>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-30  7:43           ` Arnaud POULIQUEN
@ 2020-06-30 22:02             ` Siddharth Gupta
  2020-07-01 12:13               ` Arnaud POULIQUEN
  2020-07-01  4:38             ` Bjorn Andersson
  1 sibling, 1 reply; 17+ messages in thread
From: Siddharth Gupta @ 2020-06-30 22:02 UTC (permalink / raw)
  To: Arnaud POULIQUEN, rishabhb
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, linux-remoteproc-owner


On 6/30/2020 12:43 AM, Arnaud POULIQUEN wrote:
>
> On 6/30/20 7:38 AM, Siddharth Gupta wrote:
>> On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
>>> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
>>>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
>>>>> Hi Rishabh,
>>>>>
>>>>>
>>>>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>>>>>>
>>>>> Thanks for intruducing Ioctl, this will help for future evolutions.
>>>>>
>>>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>>>>> ---
>>>>>>    Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>>>>>    drivers/remoteproc/Kconfig                         |   9 ++
>>>>>>    drivers/remoteproc/Makefile                        |   1 +
>>>>>>    drivers/remoteproc/remoteproc_cdev.c               | 143
>>>>>> +++++++++++++++++++++
>>>>>>    drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>>>>>    include/linux/remoteproc.h                         |   3 +
>>>>>>    include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>>>>>    7 files changed, 198 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 2e91370..412b2a0 100644
>>>>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File
>>>>>>                     Comments
>>>>>>    0xB4  00-0F  linux/gpio.h
>>>>>> <mailto:linux-gpio@vger.kernel.org>
>>>>>>    0xB5  00-0F  uapi/linux/rpmsg.h
>>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>>    0xB6  all    linux/fpga-dfl.h
>>>>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			
>>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>>    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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>>>>>    obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c
>>>>>> b/drivers/remoteproc/remoteproc_cdev.c
>>>>>> new file mode 100644
>>>>>> index 0000000..65142ec
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>>>> @@ -0,0 +1,143 @@
>>>>>> +// 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/remoteproc.h>
>>>>>> +#include <uapi/linux/remoteproc_cdev.h>
>>>>>> +#include <linux/uaccess.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_OFFLINE)
>>>>>> +			return -ENXIO;
>>>>> returning ENXIO in this case seems to me no appropriate , what about
>>>>> EPERM or
>>>>> EINVAL (rproc_sysfs) ?
>> I think EPERM would indicate the operation is not permitted on the
>> device, and
>> EINVAL would indicate that the command/string they wrote to the char device
>> is not a valid command/string. I guess even ENXIO may not be appropriate?
>>
>> In that case we could use EHOSTDOWN or ESHUTDOWN, thoughts?
> Regarding remoteproc_sysfs.c[1], seems that the -EINVAL return would be coherent.
Okay sure.
> It would probably also be better to replace your condition with
> 	if (rproc->state != RPROC_RUNNING)
Will do.
>   
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_sysfs.c#L104
>
> Regards,
> Arnaud
>
>> Thanks,
>> Sid
>>
>>>>>> +
>>>>>> +		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 = (void __user *)arg;
>>>>>> +	int ret;
>>>>>> +	bool param;
>>>>>> +
>>>>>> +	switch (ioctl) {
>>>>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>>>>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>>>>>> +		if (ret) {
>>>>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +		rproc->cdev_put_on_release = param;
>>>>> argp is an void value, where cdev_put_on_release is a bool a check or
>>>>> a conversion  seems
>>>>> missing
>>>>>
>>>>>> +		break;
>>>>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>>>>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>>>>>> +				   sizeof(bool));
>>>>>> +		if (ret) {
>>>>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +		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);
>>>>>> +
>>>>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>>>>>> +		rproc_shutdown(rproc);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +static const struct file_operations rproc_fops = {
>>>>>> +	.write = rproc_cdev_write,
>>>>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1,
>>>>>> "rproc");
>>>>>> +}
>>>>>> +
>>>>>> +void __init rproc_init_cdev(void)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES,
>>>>>> "rproc");
>>>>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming)
>>>>> .
>>>>>
>>>>>> +	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(MAJOR(rproc_major), 0),
>>>>>> +				 NUM_RPROC_DEVICES);
>>>>>> +}
>>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>>> index 493ef92..fb9d891 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char
>>>>>> *name, struct rproc *rproc,
>>>>>>    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)
>>>>>> +{
>>>>>> +}
>>>>>> +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 16ad666..9bd2ff5 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>
>>>>>>
>>>>>>    /**
>>>>>> @@ -514,6 +515,8 @@ struct rproc {
>>>>>>    	bool auto_boot;
>>>>>>    	struct list_head dump_segments;
>>>>>>    	int nb_vdev;
>>>>>> +	struct cdev char_dev;
>>>>>> +	bool cdev_put_on_release;
>>>>>>    };
>>>>> These parameters are local variables of rproc_cdev. Could be defined
>>>>> in a separate structure.
>>>>> with a pointer in rproc to this structure.
>>>>>
>>>>>>    /**
>>>>>> diff --git a/include/uapi/linux/remoteproc_cdev.h
>>>>>> b/include/uapi/linux/remoteproc_cdev.h
>>>>>> new file mode 100644
>>>>>> index 0000000..3975120
>>>>>> --- /dev/null
>>>>>> +++ b/include/uapi/linux/remoteproc_cdev.h
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +/* 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
>>>>>> +
>>>>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>>>>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>>>>>> +
>>>>>> +#endif
>>>>>>
>>>>> IOCTLs should probaly be documented.
>>>> I have added documentation to
>>>> Documentation/userspace-api/ioctl/ioctl-number.rst
>>>> Is there another place where I need to add documentation for this?
>>> Could you add in this file comments that describe
>>> the IOCTL usage and associated parameter?
>>>
>>> Regards,
>>> Arnaud

I don't see any for of comments that describe in detail the IOCTL usage, 
i.e.,
input parameters, etc. Could you provide an example so I can follow it?

If not, should I instead put them in Documentation/remoteproc.txt?

Thanks,
Sid

>>>
>>>>> Thanks,
>>>>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-30  7:43           ` Arnaud POULIQUEN
  2020-06-30 22:02             ` Siddharth Gupta
@ 2020-07-01  4:38             ` Bjorn Andersson
  1 sibling, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2020-07-01  4:38 UTC (permalink / raw)
  To: Arnaud POULIQUEN
  Cc: Siddharth Gupta, rishabhb, linux-remoteproc, linux-kernel,
	mathieu.poirier, ohad, tsoni, psodagud, linux-remoteproc-owner

On Tue 30 Jun 00:43 PDT 2020, Arnaud POULIQUEN wrote:

> 
> 
> On 6/30/20 7:38 AM, Siddharth Gupta wrote:
> > 
> > On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
> >>
> >> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
> >>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
> >>>> Hi Rishabh,
> >>>>
> >>>>
> >>>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
> >>>>>
> >>>> Thanks for intruducing Ioctl, this will help for future evolutions.
> >>>>
> >>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> >>>>> ---
> >>>>>   Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
> >>>>>   drivers/remoteproc/Kconfig                         |   9 ++
> >>>>>   drivers/remoteproc/Makefile                        |   1 +
> >>>>>   drivers/remoteproc/remoteproc_cdev.c               | 143
> >>>>> +++++++++++++++++++++
> >>>>>   drivers/remoteproc/remoteproc_internal.h           |  21 +++
> >>>>>   include/linux/remoteproc.h                         |   3 +
> >>>>>   include/uapi/linux/remoteproc_cdev.h               |  20 +++
> >>>>>   7 files changed, 198 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 2e91370..412b2a0 100644
> >>>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> >>>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> >>>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File
> >>>>>                    Comments
> >>>>>   0xB4  00-0F  linux/gpio.h
> >>>>> <mailto:linux-gpio@vger.kernel.org>
> >>>>>   0xB5  00-0F  uapi/linux/rpmsg.h
> >>>>> <mailto:linux-remoteproc@vger.kernel.org>
> >>>>>   0xB6  all    linux/fpga-dfl.h
> >>>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			
> >>>>> <mailto:linux-remoteproc@vger.kernel.org>
> >>>>>   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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
> >>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
> >>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c
> >>>>> b/drivers/remoteproc/remoteproc_cdev.c
> >>>>> new file mode 100644
> >>>>> index 0000000..65142ec
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >>>>> @@ -0,0 +1,143 @@
> >>>>> +// 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/remoteproc.h>
> >>>>> +#include <uapi/linux/remoteproc_cdev.h>
> >>>>> +#include <linux/uaccess.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_OFFLINE)
> >>>>> +			return -ENXIO;
> >>>> returning ENXIO in this case seems to me no appropriate , what about
> >>>> EPERM or
> >>>> EINVAL (rproc_sysfs) ?
> > 
> > I think EPERM would indicate the operation is not permitted on the 
> > device, and
> > EINVAL would indicate that the command/string they wrote to the char device
> > is not a valid command/string. I guess even ENXIO may not be appropriate?
> > 
> > In that case we could use EHOSTDOWN or ESHUTDOWN, thoughts?
> 
> Regarding remoteproc_sysfs.c[1], seems that the -EINVAL return would be coherent.
> It would probably also be better to replace your condition with 
> 	if (rproc->state != RPROC_RUNNING)
>  

I agree to both points.


Separate of this, and presumably as part of the work to support
"detaching" (e.g. to restart Linux with the remoteproc running), we
should move the conditional into rproc_shutdown(), under the lock.

But for now, let's mimic what we have in the sysfs interface.

Regards,
Bjorn

> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_sysfs.c#L104
> 
> Regards,
> Arnaud
> 
> > 
> > Thanks,
> > Sid
> > 
> >>>>
> >>>>> +
> >>>>> +		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 = (void __user *)arg;
> >>>>> +	int ret;
> >>>>> +	bool param;
> >>>>> +
> >>>>> +	switch (ioctl) {
> >>>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
> >>>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
> >>>>> +		if (ret) {
> >>>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +		rproc->cdev_put_on_release = param;
> >>>> argp is an void value, where cdev_put_on_release is a bool a check or
> >>>> a conversion  seems
> >>>> missing
> >>>>
> >>>>> +		break;
> >>>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
> >>>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
> >>>>> +				   sizeof(bool));
> >>>>> +		if (ret) {
> >>>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +		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);
> >>>>> +
> >>>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
> >>>>> +		rproc_shutdown(rproc);
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>> +
> >>>>> +static const struct file_operations rproc_fops = {
> >>>>> +	.write = rproc_cdev_write,
> >>>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1,
> >>>>> "rproc");
> >>>>> +}
> >>>>> +
> >>>>> +void __init rproc_init_cdev(void)
> >>>>> +{
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES,
> >>>>> "rproc");
> >>>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming)
> >>>> .
> >>>>
> >>>>> +	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(MAJOR(rproc_major), 0),
> >>>>> +				 NUM_RPROC_DEVICES);
> >>>>> +}
> >>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
> >>>>> b/drivers/remoteproc/remoteproc_internal.h
> >>>>> index 493ef92..fb9d891 100644
> >>>>> --- a/drivers/remoteproc/remoteproc_internal.h
> >>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
> >>>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char
> >>>>> *name, struct rproc *rproc,
> >>>>>   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)
> >>>>> +{
> >>>>> +}
> >>>>> +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 16ad666..9bd2ff5 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>
> >>>>>
> >>>>>   /**
> >>>>> @@ -514,6 +515,8 @@ struct rproc {
> >>>>>   	bool auto_boot;
> >>>>>   	struct list_head dump_segments;
> >>>>>   	int nb_vdev;
> >>>>> +	struct cdev char_dev;
> >>>>> +	bool cdev_put_on_release;
> >>>>>   };
> >>>> These parameters are local variables of rproc_cdev. Could be defined
> >>>> in a separate structure.
> >>>> with a pointer in rproc to this structure.
> >>>>
> >>>>>   /**
> >>>>> diff --git a/include/uapi/linux/remoteproc_cdev.h
> >>>>> b/include/uapi/linux/remoteproc_cdev.h
> >>>>> new file mode 100644
> >>>>> index 0000000..3975120
> >>>>> --- /dev/null
> >>>>> +++ b/include/uapi/linux/remoteproc_cdev.h
> >>>>> @@ -0,0 +1,20 @@
> >>>>> +/* 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
> >>>>> +
> >>>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
> >>>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
> >>>>> +
> >>>>> +#endif
> >>>>>
> >>>> IOCTLs should probaly be documented.
> >>> I have added documentation to
> >>> Documentation/userspace-api/ioctl/ioctl-number.rst
> >>> Is there another place where I need to add documentation for this?
> >> Could you add in this file comments that describe
> >> the IOCTL usage and associated parameter?
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>> Thanks,
> >>>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-06-30 22:02             ` Siddharth Gupta
@ 2020-07-01 12:13               ` Arnaud POULIQUEN
  2020-07-01 18:53                 ` Bjorn Andersson
  0 siblings, 1 reply; 17+ messages in thread
From: Arnaud POULIQUEN @ 2020-07-01 12:13 UTC (permalink / raw)
  To: Siddharth Gupta, rishabhb
  Cc: linux-remoteproc, linux-kernel, bjorn.andersson, mathieu.poirier,
	ohad, tsoni, psodagud, linux-remoteproc-owner



On 7/1/20 12:02 AM, Siddharth Gupta wrote:
> 
> On 6/30/2020 12:43 AM, Arnaud POULIQUEN wrote:
>>
>> On 6/30/20 7:38 AM, Siddharth Gupta wrote:
>>> On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
>>>> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
>>>>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
>>>>>> Hi Rishabh,
>>>>>>
>>>>>>
>>>>>> On 4/21/20 8:10 PM, Rishabh Bhatnagar 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.
>>>>>>>
>>>>>> Thanks for intruducing Ioctl, this will help for future evolutions.
>>>>>>
>>>>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>>>>>> ---
>>>>>>>    Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
>>>>>>>    drivers/remoteproc/Kconfig                         |   9 ++
>>>>>>>    drivers/remoteproc/Makefile                        |   1 +
>>>>>>>    drivers/remoteproc/remoteproc_cdev.c               | 143
>>>>>>> +++++++++++++++++++++
>>>>>>>    drivers/remoteproc/remoteproc_internal.h           |  21 +++
>>>>>>>    include/linux/remoteproc.h                         |   3 +
>>>>>>>    include/uapi/linux/remoteproc_cdev.h               |  20 +++
>>>>>>>    7 files changed, 198 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 2e91370..412b2a0 100644
>>>>>>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>>>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>>>> @@ -337,6 +337,7 @@ Code  Seq#    Include File
>>>>>>>                     Comments
>>>>>>>    0xB4  00-0F  linux/gpio.h
>>>>>>> <mailto:linux-gpio@vger.kernel.org>
>>>>>>>    0xB5  00-0F  uapi/linux/rpmsg.h
>>>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>>>    0xB6  all    linux/fpga-dfl.h
>>>>>>> +0xB7  all    uapi/linux/remoteproc_cdev.h			
>>>>>>> <mailto:linux-remoteproc@vger.kernel.org>
>>>>>>>    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 de3862c..6374b79 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 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 e30a1b1..b7d4f77 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>>>>>>    obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c
>>>>>>> b/drivers/remoteproc/remoteproc_cdev.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..65142ec
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>>>>> @@ -0,0 +1,143 @@
>>>>>>> +// 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/remoteproc.h>
>>>>>>> +#include <uapi/linux/remoteproc_cdev.h>
>>>>>>> +#include <linux/uaccess.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_OFFLINE)
>>>>>>> +			return -ENXIO;
>>>>>> returning ENXIO in this case seems to me no appropriate , what about
>>>>>> EPERM or
>>>>>> EINVAL (rproc_sysfs) ?
>>> I think EPERM would indicate the operation is not permitted on the
>>> device, and
>>> EINVAL would indicate that the command/string they wrote to the char device
>>> is not a valid command/string. I guess even ENXIO may not be appropriate?
>>>
>>> In that case we could use EHOSTDOWN or ESHUTDOWN, thoughts?
>> Regarding remoteproc_sysfs.c[1], seems that the -EINVAL return would be coherent.
> Okay sure.
>> It would probably also be better to replace your condition with
>> 	if (rproc->state != RPROC_RUNNING)
> Will do.
>>   
>>
>> [1] https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_sysfs.c#L104
>>
>> Regards,
>> Arnaud
>>
>>> Thanks,
>>> Sid
>>>
>>>>>>> +
>>>>>>> +		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 = (void __user *)arg;
>>>>>>> +	int ret;
>>>>>>> +	bool param;
>>>>>>> +
>>>>>>> +	switch (ioctl) {
>>>>>>> +	case RPROC_SET_SHUTDOWN_ON_RELEASE:
>>>>>>> +		ret = copy_from_user(&param, argp, sizeof(bool));
>>>>>>> +		if (ret) {
>>>>>>> +			dev_err(&rproc->dev, "Data copy from userspace failed\n");
>>>>>>> +			return -EINVAL;
>>>>>>> +		}
>>>>>>> +		rproc->cdev_put_on_release = param;
>>>>>> argp is an void value, where cdev_put_on_release is a bool a check or
>>>>>> a conversion  seems
>>>>>> missing
>>>>>>
>>>>>>> +		break;
>>>>>>> +	case RPROC_GET_SHUTDOWN_ON_RELEASE:
>>>>>>> +		ret = copy_to_user(argp, &rproc->cdev_put_on_release,
>>>>>>> +				   sizeof(bool));
>>>>>>> +		if (ret) {
>>>>>>> +			dev_err(&rproc->dev, "Data copy to userspace failed\n");
>>>>>>> +			return -EINVAL;
>>>>>>> +		}
>>>>>>> +		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);
>>>>>>> +
>>>>>>> +	if (rproc->cdev_put_on_release && rproc->state != RPROC_OFFLINE)
>>>>>>> +		rproc_shutdown(rproc);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +static const struct file_operations rproc_fops = {
>>>>>>> +	.write = rproc_cdev_write,
>>>>>>> +	.unlocked_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(MAJOR(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(MAJOR(rproc->dev.devt), rproc->index, 1,
>>>>>>> "rproc");
>>>>>>> +}
>>>>>>> +
>>>>>>> +void __init rproc_init_cdev(void)
>>>>>>> +{
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	ret = alloc_chrdev_region(&rproc_major, 0, NUM_RPROC_DEVICES,
>>>>>>> "rproc");
>>>>>> "remoteproc"instead of "rproc" (in line with sysfs and debugfs naming)
>>>>>> .
>>>>>>
>>>>>>> +	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(MAJOR(rproc_major), 0),
>>>>>>> +				 NUM_RPROC_DEVICES);
>>>>>>> +}
>>>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>>>> index 493ef92..fb9d891 100644
>>>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>>>> @@ -47,6 +47,27 @@ struct dentry *rproc_create_trace_file(const char
>>>>>>> *name, struct rproc *rproc,
>>>>>>>    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)
>>>>>>> +{
>>>>>>> +}
>>>>>>> +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 16ad666..9bd2ff5 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>
>>>>>>>
>>>>>>>    /**
>>>>>>> @@ -514,6 +515,8 @@ struct rproc {
>>>>>>>    	bool auto_boot;
>>>>>>>    	struct list_head dump_segments;
>>>>>>>    	int nb_vdev;
>>>>>>> +	struct cdev char_dev;
>>>>>>> +	bool cdev_put_on_release;
>>>>>>>    };
>>>>>> These parameters are local variables of rproc_cdev. Could be defined
>>>>>> in a separate structure.
>>>>>> with a pointer in rproc to this structure.
>>>>>>
>>>>>>>    /**
>>>>>>> diff --git a/include/uapi/linux/remoteproc_cdev.h
>>>>>>> b/include/uapi/linux/remoteproc_cdev.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3975120
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/uapi/linux/remoteproc_cdev.h
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +/* 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
>>>>>>> +
>>>>>>> +#define RPROC_SET_SHUTDOWN_ON_RELEASE _IOW(RPROC_MAGIC, 1, int)
>>>>>>> +#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
>>>>>>> +
>>>>>>> +#endif
>>>>>>>
>>>>>> IOCTLs should probaly be documented.
>>>>> I have added documentation to
>>>>> Documentation/userspace-api/ioctl/ioctl-number.rst
>>>>> Is there another place where I need to add documentation for this?
>>>> Could you add in this file comments that describe
>>>> the IOCTL usage and associated parameter?
>>>>
>>>> Regards,
>>>> Arnaud
> 
> I don't see any for of comments that describe in detail the IOCTL usage, 
> i.e.,
> input parameters, etc. Could you provide an example so I can follow it?
> 
> If not, should I instead put them in Documentation/remoteproc.txt?

Bjorn and Mathieu are the right persons to decide on the best place.

Here are 2 exemples of "inline" documented IOCTRLs.

https://elixir.bootlin.com/linux/latest/source/arch/s390/include/uapi/asm/tape390.h#L15
https://elixir.bootlin.com/linux/latest/source/drivers/net/wan/cosa.h#L63 

Could be something like that:

/*
 * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the 
 * automatic remote processors shutdown 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, int)

/*
 * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about the 
 * automatic remote processors shutdown when the controlling userpsace
 * closes the char device interface.
 *
 * output parameter: integer
 *   0: automatic shutdown disable
 *   1: automatic shutdown enable
 */ 
#define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)


Regards,
Arnaud
> 
> Thanks,
> Sid
> 
>>>>
>>>>>> Thanks,
>>>>>> Arnaud

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface
  2020-07-01 12:13               ` Arnaud POULIQUEN
@ 2020-07-01 18:53                 ` Bjorn Andersson
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2020-07-01 18:53 UTC (permalink / raw)
  To: Arnaud POULIQUEN
  Cc: Siddharth Gupta, rishabhb, linux-remoteproc, linux-kernel,
	mathieu.poirier, ohad, tsoni, psodagud, linux-remoteproc-owner

On Wed 01 Jul 05:13 PDT 2020, Arnaud POULIQUEN wrote:

> 
> 
> On 7/1/20 12:02 AM, Siddharth Gupta wrote:
> > 
> > On 6/30/2020 12:43 AM, Arnaud POULIQUEN wrote:
> >>
> >> On 6/30/20 7:38 AM, Siddharth Gupta wrote:
> >>> On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:
> >>>> On 6/16/20 9:56 PM, rishabhb@codeaurora.org wrote:
> >>>>> On 2020-04-30 01:30, Arnaud POULIQUEN wrote:
[..]
> >>>>>> IOCTLs should probaly be documented.
> >>>>> I have added documentation to
> >>>>> Documentation/userspace-api/ioctl/ioctl-number.rst
> >>>>> Is there another place where I need to add documentation for this?
> >>>> Could you add in this file comments that describe
> >>>> the IOCTL usage and associated parameter?
> >>>>
> >>>> Regards,
> >>>> Arnaud
> > 
> > I don't see any for of comments that describe in detail the IOCTL usage, 
> > i.e.,
> > input parameters, etc. Could you provide an example so I can follow it?
> > 
> > If not, should I instead put them in Documentation/remoteproc.txt?
> 
> Bjorn and Mathieu are the right persons to decide on the best place.
> 
> Here are 2 exemples of "inline" documented IOCTRLs.
> 
> https://elixir.bootlin.com/linux/latest/source/arch/s390/include/uapi/asm/tape390.h#L15
> https://elixir.bootlin.com/linux/latest/source/drivers/net/wan/cosa.h#L63 
> 
> Could be something like that:
> 
> /*
>  * The RPROC_SET_SHUTDOWN_ON_RELEASE ioctl allows to enable/disable the 
>  * automatic remote processors shutdown 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, int)
> 
> /*
>  * The RPROC_GET_SHUTDOWN_ON_RELEASE ioctl gets information about the 
>  * automatic remote processors shutdown when the controlling userpsace
>  * closes the char device interface.
>  *
>  * output parameter: integer
>  *   0: automatic shutdown disable
>  *   1: automatic shutdown enable
>  */ 
> #define RPROC_GET_SHUTDOWN_ON_RELEASE _IOR(RPROC_MAGIC, 2, int)
> 

I like this. Thanks for digging up an example Arnaud!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 18:10 [PATCH v3 0/2] Add character device interface to remoteproc Rishabh Bhatnagar
2020-04-21 18:10 ` Rishabh Bhatnagar
2020-04-21 18:10 ` [PATCH v3 1/2] remoteproc: Add remoteproc character device interface Rishabh Bhatnagar
2020-04-21 18:10   ` Rishabh Bhatnagar
2020-04-29 23:09   ` Mathieu Poirier
2020-04-30  8:30   ` Arnaud POULIQUEN
2020-04-30 16:44     ` Mathieu Poirier
2020-06-16 19:56     ` rishabhb
2020-06-17  8:44       ` Arnaud POULIQUEN
2020-06-30  5:38         ` Siddharth Gupta
2020-06-30  7:43           ` Arnaud POULIQUEN
2020-06-30 22:02             ` Siddharth Gupta
2020-07-01 12:13               ` Arnaud POULIQUEN
2020-07-01 18:53                 ` Bjorn Andersson
2020-07-01  4:38             ` Bjorn Andersson
2020-04-21 18:10 ` [PATCH v3 2/2] remoteproc: core: Register the " Rishabh Bhatnagar
2020-04-21 18:10   ` Rishabh Bhatnagar

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git