[PATCH v3 0/6] Restructure the rpmsg char and introduce the rpmsg-raw channel

[PATCH v3 0/6] Restructure the rpmsg char and introduce the rpmsg-raw channel

[Arnaud Pouliquen]

Main update from V2 [1]
 - Management of the endpoint creation/release to handle it for all the use cases
   in the rpmsg_eptdev_open and rpmsg_eptdev_release function

Other patch updates are described at the end of their commit message.


This series can be applied on git/andersson/remoteproc.git 
for-next branch (dc0e14fa833b) + "rpmsg: char: Remove useless includes" patch[3].

Series description:
This series is the second step in the division of the series [2]: 
"Introducing a Generic IOCTL Interface for RPMsg Channel Management".

The purpose of this patchset is to:
- split the control code related to the control
  and the endpoint. 
- define the rpmsg-raw channel, associated with the rpmsg char device to
  allow it to be instantiated using a name service announcement.
    
An important point to keep in mind for this patchset is that the concept of
channel is associated with a default endpoint. To facilitate communication
with the remote side, this default endpoint must have a fixed address.

Therefore, for /dev/rpmsgX device created during the instantiation of he "rpmsg-raw"
device, the endpoint creation/release is not dynamically managed on each FS open/close.

This is only applicable for channels probed by the rpmsg bus. The behavior,
using the RPMSG_CREATE_EPT_IOCTL and RPMSG_DESTROY_EPT_IOCTL controls, is preserved.
  
The next step should be:
Introduce the IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL
to instantiate the rpmsg devices

[1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=466357
[2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
[3]: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210429080639.6379-1-arnaud.pouliquen@foss.st.com/


Arnaud Pouliquen (6):
  rpmsg: char: Export eptdev create an destroy functions
  rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl
  rpmsg: Update rpmsg_chrdev_register_device function
  rpmsg: char: Add possibility to create and reuse default endpoint
  rpmsg: char: Introduce a rpmsg driver for the rpmsg char device
  rpmsg: char: Return error if user tries to destroy a default endpoint.

 drivers/rpmsg/Kconfig             |   9 ++
 drivers/rpmsg/Makefile            |   1 +
 drivers/rpmsg/qcom_glink_native.c |   2 +-
 drivers/rpmsg/qcom_smd.c          |   2 +-
 drivers/rpmsg/rpmsg_char.c        | 223 ++++++++++--------------------
 drivers/rpmsg/rpmsg_char.h        |  51 +++++++
 drivers/rpmsg/rpmsg_ctrl.c        | 209 ++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |   8 +-
 drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
 9 files changed, 350 insertions(+), 157 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

[PATCH v3 1/6] rpmsg: char: Export eptdev create an destroy functions

[Arnaud Pouliquen]

To prepare the split of the code related to the control (ctrldev)
and the endpoint (eptdev) devices in 2 separate files:

- Rename and export the functions in rpmsg_char.h.

- Suppress the dependency with the rpmsg_ctrldev struct in the
  rpmsg_eptdev_create function.

- The rpmsg class is provided as parameter in rpmsg_chrdev_create_eptdev,
  because the class is associated to the control part.

Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

---
Update from v2:

- rename functions from rpmsg_chrdev_create/destroy_eptdev to
  rpmsg_chrdev_eptdev_create/destroy
---
 drivers/rpmsg/rpmsg_char.c | 18 ++++++++------
 drivers/rpmsg/rpmsg_char.h | 49 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 7 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index e4e54f515af6..3c53ece557a9 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
+ * Copyright (C) 2021, STMicroelectronics
  * Copyright (c) 2016, Linaro Ltd.
  * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
  * Copyright (c) 2012, PetaLogix
@@ -15,6 +16,8 @@
 #include <linux/rpmsg.h>
 #include <linux/skbuff.h>
 
+#include "rpmsg_char.h"
+
 #define RPMSG_DEV_MAX	(MINORMASK + 1)
 
 static dev_t rpmsg_major;
@@ -69,7 +72,7 @@ struct rpmsg_eptdev {
 	wait_queue_head_t readq;
 };
 
-static int rpmsg_eptdev_destroy(struct device *dev, void *data)
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
 {
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
@@ -88,6 +91,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
 
 	return 0;
 }
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
 
 static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
 			void *priv, u32 addr)
@@ -271,7 +275,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
 		return -EINVAL;
 
-	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+	return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
 }
 
 static const struct file_operations rpmsg_eptdev_fops = {
@@ -330,10 +334,9 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 	kfree(eptdev);
 }
 
-static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
 			       struct rpmsg_channel_info chinfo)
 {
-	struct rpmsg_device *rpdev = ctrldev->rpdev;
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
 	int ret;
@@ -353,7 +356,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 
 	device_initialize(dev);
 	dev->class = rpmsg_class;
-	dev->parent = &ctrldev->dev;
+	dev->parent = parent;
 	dev->groups = rpmsg_eptdev_groups;
 	dev_set_drvdata(dev, eptdev);
 
@@ -396,6 +399,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 
 	return ret;
 }
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
 static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
 {
@@ -435,7 +439,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	chinfo.src = eptinfo.src;
 	chinfo.dst = eptinfo.dst;
 
-	return rpmsg_eptdev_create(ctrldev, chinfo);
+	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
 };
 
 static const struct file_operations rpmsg_ctrldev_fops = {
@@ -521,7 +525,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
 	int ret;
 
 	/* Destroy all endpoints */
-	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
+	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
 	if (ret)
 		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
 
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
new file mode 100644
index 000000000000..facd324290a4
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) STMicroelectronics 2021.
+ */
+
+#ifndef __RPMSG_CHRDEV_H__
+#define __RPMSG_CHRDEV_H__
+
+#if IS_REACHABLE(CONFIG_RPMSG_CHAR)
+/**
+ * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
+ * @rpdev:  prepared rpdev to be used for creating endpoints
+ * @parent: parent device
+ * @chinfo: assiated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+			       struct rpmsg_channel_info chinfo);
+
+/**
+ * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
+ * @data: private data associated to the endpoint device
+ *
+ * This function destroys a rpmsg char endpoint device created by the RPMSG_DESTROY_EPT_IOCTL
+ * control.
+ */
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
+
+#else  /*IS_REACHABLE(CONFIG_RPMSG_CHAR) */
+
+static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+					     struct rpmsg_channel_info chinfo)
+{
+	return -EINVAL;
+}
+
+static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return 0;
+}
+
+#endif /*IS_REACHABLE(CONFIG_RPMSG_CHAR) */
+
+#endif /*__RPMSG_CHRDEV_H__ */
-- 
2.17.1

[PATCH v3 2/6] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud Pouliquen]

Create the rpmsg_ctrl.c module and move the code related to the
rpmsg_ctrldev device in this new module.

Add the dependency between rpmsg_char and rpmsg_ctrl in the
kconfig file.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

---
Update from V2:

- supress the "rpmsg" class in the rpmsg_ctrl module
- cleanup useless includes
---
 drivers/rpmsg/Kconfig      |   9 ++
 drivers/rpmsg/Makefile     |   1 +
 drivers/rpmsg/rpmsg_char.c | 170 +-----------------------------
 drivers/rpmsg/rpmsg_char.h |   2 +
 drivers/rpmsg/rpmsg_ctrl.c | 209 +++++++++++++++++++++++++++++++++++++
 5 files changed, 223 insertions(+), 168 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0b4407abdf13..d822ec9ec692 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -10,11 +10,20 @@ config RPMSG_CHAR
 	tristate "RPMSG device interface"
 	depends on RPMSG
 	depends on NET
+	select RPMSG_CTRL
 	help
 	  Say Y here to export rpmsg endpoints as device files, usually found
 	  in /dev. They make it possible for user-space programs to send and
 	  receive rpmsg packets.
 
+config RPMSG_CTRL
+	tristate "RPMSG control interface"
+	depends on RPMSG
+	help
+	  Say Y here to enable the support of the /dev/rpmsg_ctrlX API. This API
+	  allows user-space programs to create endpoints with specific service name,
+	  source and destination addresses.
+
 config RPMSG_NS
 	tristate "RPMSG name service announcement"
 	depends on RPMSG
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..58e3b382e316 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
+obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
 obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
 obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
 qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3c53ece557a9..3b07a050b75e 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -18,33 +18,15 @@
 
 #include "rpmsg_char.h"
 
-#define RPMSG_DEV_MAX	(MINORMASK + 1)
-
 static dev_t rpmsg_major;
 static struct class *rpmsg_class;
 
-static DEFINE_IDA(rpmsg_ctrl_ida);
 static DEFINE_IDA(rpmsg_ept_ida);
 static DEFINE_IDA(rpmsg_minor_ida);
 
 #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
 #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
 
-#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
-#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
-
-/**
- * struct rpmsg_ctrldev - control device for instantiating endpoint devices
- * @rpdev:	underlaying rpmsg device
- * @cdev:	cdev for the ctrl device
- * @dev:	device for the ctrl device
- */
-struct rpmsg_ctrldev {
-	struct rpmsg_device *rpdev;
-	struct cdev cdev;
-	struct device dev;
-};
-
 /**
  * struct rpmsg_eptdev - endpoint device context
  * @dev:	endpoint device
@@ -401,151 +383,11 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
-static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
-{
-	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
-	get_device(&ctrldev->dev);
-	filp->private_data = ctrldev;
-
-	return 0;
-}
-
-static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
-{
-	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
-	put_device(&ctrldev->dev);
-
-	return 0;
-}
-
-static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
-				unsigned long arg)
-{
-	struct rpmsg_ctrldev *ctrldev = fp->private_data;
-	void __user *argp = (void __user *)arg;
-	struct rpmsg_endpoint_info eptinfo;
-	struct rpmsg_channel_info chinfo;
-
-	if (cmd != RPMSG_CREATE_EPT_IOCTL)
-		return -EINVAL;
-
-	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
-		return -EFAULT;
-
-	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
-	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
-	chinfo.src = eptinfo.src;
-	chinfo.dst = eptinfo.dst;
-
-	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
-};
-
-static const struct file_operations rpmsg_ctrldev_fops = {
-	.owner = THIS_MODULE,
-	.open = rpmsg_ctrldev_open,
-	.release = rpmsg_ctrldev_release,
-	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-};
-
-static void rpmsg_ctrldev_release_device(struct device *dev)
-{
-	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
-
-	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
-	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
-	cdev_del(&ctrldev->cdev);
-	kfree(ctrldev);
-}
-
-static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
-{
-	struct rpmsg_ctrldev *ctrldev;
-	struct device *dev;
-	int ret;
-
-	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
-	if (!ctrldev)
-		return -ENOMEM;
-
-	ctrldev->rpdev = rpdev;
-
-	dev = &ctrldev->dev;
-	device_initialize(dev);
-	dev->parent = &rpdev->dev;
-	dev->class = rpmsg_class;
-
-	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
-	ctrldev->cdev.owner = THIS_MODULE;
-
-	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
-	if (ret < 0)
-		goto free_ctrldev;
-	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-
-	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
-	if (ret < 0)
-		goto free_minor_ida;
-	dev->id = ret;
-	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
-
-	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
-	if (ret)
-		goto free_ctrl_ida;
-
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_ctrldev_release_device;
-
-	ret = device_add(dev);
-	if (ret) {
-		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
-		put_device(dev);
-	}
-
-	dev_set_drvdata(&rpdev->dev, ctrldev);
-
-	return ret;
-
-free_ctrl_ida:
-	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
-free_minor_ida:
-	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
-free_ctrldev:
-	put_device(dev);
-	kfree(ctrldev);
-
-	return ret;
-}
-
-static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
-{
-	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
-	int ret;
-
-	/* Destroy all endpoints */
-	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
-	if (ret)
-		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
-
-	device_del(&ctrldev->dev);
-	put_device(&ctrldev->dev);
-}
-
-static struct rpmsg_driver rpmsg_chrdev_driver = {
-	.probe = rpmsg_chrdev_probe,
-	.remove = rpmsg_chrdev_remove,
-	.drv = {
-		.name = "rpmsg_chrdev",
-	},
-};
-
 static int rpmsg_chrdev_init(void)
 {
 	int ret;
 
-	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
+	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char");
 	if (ret < 0) {
 		pr_err("rpmsg: failed to allocate char dev region\n");
 		return ret;
@@ -558,20 +400,12 @@ static int rpmsg_chrdev_init(void)
 		return PTR_ERR(rpmsg_class);
 	}
 
-	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
-	if (ret < 0) {
-		pr_err("rpmsgchr: failed to register rpmsg driver\n");
-		class_destroy(rpmsg_class);
-		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
-	}
-
-	return ret;
+	return 0;
 }
 postcore_initcall(rpmsg_chrdev_init);
 
 static void rpmsg_chrdev_exit(void)
 {
-	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
 	class_destroy(rpmsg_class);
 	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 }
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
index facd324290a4..311f5a7690f5 100644
--- a/drivers/rpmsg/rpmsg_char.h
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -6,6 +6,8 @@
 #ifndef __RPMSG_CHRDEV_H__
 #define __RPMSG_CHRDEV_H__
 
+#define RPMSG_DEV_MAX	(MINORMASK + 1)
+
 #if IS_REACHABLE(CONFIG_RPMSG_CHAR)
 /**
  * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
new file mode 100644
index 000000000000..6b4d705b2dfd
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021, STMicroelectronics
+ * Copyright (c) 2016, Linaro Ltd.
+ * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
+ * Copyright (c) 2012, PetaLogix
+ * Copyright (c) 2011, Texas Instruments, Inc.
+ * Copyright (c) 2011, Google, Inc.
+ *
+ * Based on rpmsg performance statistics driver by Michal Simek, which in turn
+ * was based on TI & Google OMX rpmsg driver.
+ */
+#include <linux/cdev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+#include <linux/slab.h>
+
+#include "rpmsg_char.h"
+
+static dev_t rpmsg_major;
+
+static DEFINE_IDA(rpmsg_ctrl_ida);
+static DEFINE_IDA(rpmsg_minor_ida);
+
+#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
+#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
+
+/**
+ * struct rpmsg_ctrldev - control device for instantiating endpoint devices
+ * @rpdev:	underlaying rpmsg device
+ * @cdev:	cdev for the ctrl device
+ * @dev:	device for the ctrl device
+ */
+struct rpmsg_ctrldev {
+	struct rpmsg_device *rpdev;
+	struct cdev cdev;
+	struct device dev;
+};
+
+static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	get_device(&ctrldev->dev);
+	filp->private_data = ctrldev;
+
+	return 0;
+}
+
+static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	put_device(&ctrldev->dev);
+
+	return 0;
+}
+
+static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
+				unsigned long arg)
+{
+	struct rpmsg_ctrldev *ctrldev = fp->private_data;
+	void __user *argp = (void __user *)arg;
+	struct rpmsg_endpoint_info eptinfo;
+	struct rpmsg_channel_info chinfo;
+
+	if (cmd != RPMSG_CREATE_EPT_IOCTL)
+		return -EINVAL;
+
+	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+		return -EFAULT;
+
+	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+	chinfo.src = eptinfo.src;
+	chinfo.dst = eptinfo.dst;
+
+	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
+};
+
+static const struct file_operations rpmsg_ctrldev_fops = {
+	.owner = THIS_MODULE,
+	.open = rpmsg_ctrldev_open,
+	.release = rpmsg_ctrldev_release,
+	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+static void rpmsg_ctrldev_release_device(struct device *dev)
+{
+	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
+
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+	cdev_del(&ctrldev->cdev);
+	kfree(ctrldev);
+}
+
+static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrldev *ctrldev;
+	struct device *dev;
+	int ret;
+
+	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
+	if (!ctrldev)
+		return -ENOMEM;
+
+	ctrldev->rpdev = rpdev;
+
+	dev = &ctrldev->dev;
+	device_initialize(dev);
+	dev->parent = &rpdev->dev;
+
+	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
+	ctrldev->cdev.owner = THIS_MODULE;
+
+	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
+	if (ret < 0)
+		goto free_ctrldev;
+	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
+	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto free_minor_ida;
+	dev->id = ret;
+	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
+
+	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
+	if (ret)
+		goto free_ctrl_ida;
+
+	/* We can now rely on the release function for cleanup */
+	dev->release = rpmsg_ctrldev_release_device;
+
+	ret = device_add(dev);
+	if (ret) {
+		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
+		put_device(dev);
+	}
+
+	dev_set_drvdata(&rpdev->dev, ctrldev);
+
+	return ret;
+
+free_ctrl_ida:
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+free_minor_ida:
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+free_ctrldev:
+	put_device(dev);
+	kfree(ctrldev);
+
+	return ret;
+}
+
+static void rpmsg_ctrldev_remove(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
+	int ret;
+
+	/* Destroy all endpoints */
+	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
+	if (ret)
+		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
+
+	device_del(&ctrldev->dev);
+	put_device(&ctrldev->dev);
+}
+
+static struct rpmsg_driver rpmsg_ctrldev_driver = {
+	.probe = rpmsg_ctrldev_probe,
+	.remove = rpmsg_ctrldev_remove,
+	.drv = {
+		.name = "rpmsg_chrdev",
+	},
+};
+
+static int rpmsg_ctrldev_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_ctrl");
+	if (ret < 0) {
+		pr_err("rpmsg: failed to allocate char dev region\n");
+		return ret;
+	}
+
+	ret = register_rpmsg_driver(&rpmsg_ctrldev_driver);
+	if (ret < 0) {
+		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
+		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+	}
+
+	return ret;
+}
+postcore_initcall(rpmsg_ctrldev_init);
+
+static void rpmsg_ctrldev_exit(void)
+{
+	unregister_rpmsg_driver(&rpmsg_ctrldev_driver);
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+}
+module_exit(rpmsg_ctrldev_exit);
+
+MODULE_DESCRIPTION("rpmsg control interface");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v3 3/6] rpmsg: Update rpmsg_chrdev_register_device function

[Arnaud Pouliquen]

The rpmsg_chrdev driver has been replaced by the rpmsg_ctrl driver
for the /dev/rpmsg_ctrlX devices management. The reference for the
driver override is now the rpmsg_ctrl.

Update the rpmsg_chrdev_register_device function to reflect the update,
and rename the function to use the rpmsg_ctrldev prefix.

The platform drivers are updated accordingly.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/qcom_glink_native.c | 2 +-
 drivers/rpmsg/qcom_smd.c          | 2 +-
 drivers/rpmsg/rpmsg_ctrl.c        | 2 +-
 drivers/rpmsg/rpmsg_internal.h    | 8 ++++----
 drivers/rpmsg/virtio_rpmsg_bus.c  | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 05533c71b10e..7d7e809800ec 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1642,7 +1642,7 @@ static int qcom_glink_create_chrdev(struct qcom_glink *glink)
 	rpdev->dev.parent = glink->dev;
 	rpdev->dev.release = qcom_glink_device_release;
 
-	return rpmsg_chrdev_register_device(rpdev);
+	return rpmsg_ctrldev_register_device(rpdev);
 }
 
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 8da1b5cb31b3..d223e438d17c 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1113,7 +1113,7 @@ static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)
 	qsdev->rpdev.dev.parent = &edge->dev;
 	qsdev->rpdev.dev.release = qcom_smd_release_device;
 
-	return rpmsg_chrdev_register_device(&qsdev->rpdev);
+	return rpmsg_ctrldev_register_device(&qsdev->rpdev);
 }
 
 /*
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 6b4d705b2dfd..6749e26b68fc 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -173,7 +173,7 @@ static struct rpmsg_driver rpmsg_ctrldev_driver = {
 	.probe = rpmsg_ctrldev_probe,
 	.remove = rpmsg_ctrldev_remove,
 	.drv = {
-		.name = "rpmsg_chrdev",
+		.name = "rpmsg_ctrl",
 	},
 };
 
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index a76c344253bf..8c500a8f29aa 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -82,16 +82,16 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
 int rpmsg_release_channel(struct rpmsg_device *rpdev,
 			  struct rpmsg_channel_info *chinfo);
 /**
- * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
+ * rpmsg_ctrl_register_device() - register a char device for control based on rpdev
  * @rpdev:	prepared rpdev to be used for creating endpoints
  *
  * This function wraps rpmsg_register_device() preparing the rpdev for use as
  * basis for the rpmsg chrdev.
  */
-static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
+static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
 {
-	strcpy(rpdev->id.name, "rpmsg_chrdev");
-	rpdev->driver_override = "rpmsg_chrdev";
+	strcpy(rpdev->id.name, "rpmsg_ctrl");
+	rpdev->driver_override = "rpmsg_ctrl";
 
 	return rpmsg_register_device(rpdev);
 }
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 8e49a3bacfc7..e42234a3e2ab 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -840,7 +840,7 @@ static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
 	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
 	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
 
-	err = rpmsg_chrdev_register_device(rpdev_ctrl);
+	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
 	if (err) {
 		kfree(vch);
 		return ERR_PTR(err);
-- 
2.17.1

[PATCH v3 4/6] rpmsg: char: Add possibility to create and reuse default endpoint

[Arnaud Pouliquen]

This patch prepares the introduction of a rpmsg channel device for the
char device. The rpmsg channel device will require a default endpoint to
communicate to the remote processor.

Add the use_default_ept field in rpmsg_eptdev structure. This boolean
determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.

- If use_default_ept == false:
  Use the legacy behavior by creating a new endpoint each time
  rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release
  is called on /dev/rpmsgX device open/close.

- if use_default_ept == true:
  create a endpoint only on first rpmsg_eptdev_open call (if no default
  endpoint already exists) and associate it to the default endpoint.
  The endpoint is released when the rpmsg device is removed.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

---
Update from V2

This patch is a merge of two patches proposed in V2
 rpmsg: char: Introduce __rpmsg_chrdev_create_eptdev function
 rpmsg: char: No dynamic endpoint management for the default one

With the introduction of the use_default_ept to determine the behaviour
on  rpmsg_eptdev_open/release.
---
 drivers/rpmsg/rpmsg_char.c | 46 +++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3b07a050b75e..5c6a7da6e4d7 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -38,6 +38,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
  * @queue_lock:	synchronization of @queue operations
  * @queue:	incoming message queue
  * @readq:	wait object for incoming queue
+ * @use_default_ept: specify if the endpoint has to be created at each device opening or
+ *                   if the default endpoint should be reused.
  */
 struct rpmsg_eptdev {
 	struct device dev;
@@ -52,6 +54,8 @@ struct rpmsg_eptdev {
 	spinlock_t queue_lock;
 	struct sk_buff_head queue;
 	wait_queue_head_t readq;
+
+	bool use_default_ept;
 };
 
 int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
@@ -109,11 +113,29 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 
 	get_device(dev);
 
-	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
-	if (!ept) {
-		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
-		put_device(dev);
-		return -EINVAL;
+	/*
+	 * If the rpmsg device default endpoint is used, create it only the first time then reuse
+	 * it. Else a new endpoint is created on open that will be destroyed on release.
+	 */
+	if (eptdev->use_default_ept && rpdev->ept) {
+		ept = rpdev->ept;
+	} else {
+		ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+		if (!ept) {
+			dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
+			put_device(dev);
+			return -EINVAL;
+		}
+
+		if (eptdev->use_default_ept) {
+			/*
+			 * Assign the new endpoint as default endpoint and update the channel
+			 * info source to reflect the allocation in the rpmsg class.
+			 */
+			rpdev->ept = ept;
+			rpdev->src = ept->addr;
+			eptdev->chinfo.src = rpdev->src;
+		}
 	}
 
 	eptdev->ept = ept;
@@ -130,7 +152,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
 	/* Close the endpoint, if it's not already destroyed by the parent */
 	mutex_lock(&eptdev->ept_lock);
 	if (eptdev->ept) {
-		rpmsg_destroy_ept(eptdev->ept);
+		if (!eptdev->use_default_ept)
+			rpmsg_destroy_ept(eptdev->ept);
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
@@ -316,8 +339,8 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 	kfree(eptdev);
 }
 
-int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
-			       struct rpmsg_channel_info chinfo)
+static int __rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+					struct rpmsg_channel_info chinfo, bool use_default_ept)
 {
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
@@ -330,6 +353,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 	dev = &eptdev->dev;
 	eptdev->rpdev = rpdev;
 	eptdev->chinfo = chinfo;
+	eptdev->use_default_ept = use_default_ept;
 
 	mutex_init(&eptdev->ept_lock);
 	spin_lock_init(&eptdev->queue_lock);
@@ -381,6 +405,12 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 
 	return ret;
 }
+
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+			       struct rpmsg_channel_info chinfo)
+{
+	return __rpmsg_chrdev_eptdev_create(rpdev, parent, chinfo, false);
+}
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
 static int rpmsg_chrdev_init(void)
-- 
2.17.1

[PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Arnaud Pouliquen]

A rpmsg char device allows to probe the endpoint device on a remote name
service announcement.

With this patch the /dev/rpmsgX interface is created either by a user
application or by the remote firmware.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

---
update from V1:

 - add missing unregister_rpmsg_driver call on module exit.
---
 drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 5c6a7da6e4d7..9166454c1310 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -18,6 +18,8 @@
 
 #include "rpmsg_char.h"
 
+#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
+
 static dev_t rpmsg_major;
 static struct class *rpmsg_class;
 
@@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
+static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_channel_info chinfo;
+
+	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
+	chinfo.src = rpdev->src;
+	chinfo.dst = rpdev->dst;
+
+	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
+}
+
+static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
+{
+	int ret;
+
+	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
+	if (ret)
+		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
+}
+
+static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
+	{ .name	= RPMSG_CHAR_DEVNAME },
+	{ },
+};
+
+static struct rpmsg_driver rpmsg_chrdev_driver = {
+	.probe = rpmsg_chrdev_probe,
+	.remove = rpmsg_chrdev_remove,
+	.id_table = rpmsg_chrdev_id_table,
+	.drv = {
+		.name = "rpmsg_chrdev",
+	},
+};
+
 static int rpmsg_chrdev_init(void)
 {
 	int ret;
@@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
 	if (IS_ERR(rpmsg_class)) {
 		pr_err("failed to create rpmsg class\n");
 		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
-		return PTR_ERR(rpmsg_class);
+		ret = PTR_ERR(rpmsg_class);
+		goto free_region;
+	}
+
+	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+	if (ret < 0) {
+		pr_err("rpmsg: failed to register rpmsg raw driver\n");
+		goto free_class;
 	}
 
 	return 0;
+
+free_class:
+	class_destroy(rpmsg_class);
+free_region:
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+
+	return ret;
 }
 postcore_initcall(rpmsg_chrdev_init);
 
 static void rpmsg_chrdev_exit(void)
 {
+	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
 	class_destroy(rpmsg_class);
 	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 }
-- 
2.17.1

[PATCH v3 6/6] rpmsg: char: Return error if user tries to destroy a default endpoint.

[Arnaud Pouliquen]

Using the RPMSG_DESTROY_EPT_IOCTL control, user application can
destroy an endpoint. This patch prevents to destroy a default endpoint
associated to a channel.

This update is needed to manage the "rpmsg-raw" channel. In this
case a default endpoint is used and destroying it without the
channel does not make sense.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 9166454c1310..f1cb352a59ed 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -282,6 +282,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
 		return -EINVAL;
 
+	/* Don't allow to destroy a default endpoint. */
+	if (!eptdev->rpdev || eptdev->ept == eptdev->rpdev->ept)
+		return -EPERM;
+
 	return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
 }
 
-- 
2.17.1

Re: [PATCH v3 1/6] rpmsg: char: Export eptdev create an destroy functions

[Mathieu Poirier]

On Thu, Apr 29, 2021 at 03:55:02PM +0200, Arnaud Pouliquen wrote:
> To prepare the split of the code related to the control (ctrldev)
> and the endpoint (eptdev) devices in 2 separate files:
> 
> - Rename and export the functions in rpmsg_char.h.
> 
> - Suppress the dependency with the rpmsg_ctrldev struct in the
>   rpmsg_eptdev_create function.
> 
> - The rpmsg class is provided as parameter in rpmsg_chrdev_create_eptdev,
>   because the class is associated to the control part.
> 
> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> ---
> Update from v2:
> 
> - rename functions from rpmsg_chrdev_create/destroy_eptdev to
>   rpmsg_chrdev_eptdev_create/destroy
> ---
>  drivers/rpmsg/rpmsg_char.c | 18 ++++++++------
>  drivers/rpmsg/rpmsg_char.h | 49 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_char.h
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index e4e54f515af6..3c53ece557a9 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> + * Copyright (C) 2021, STMicroelectronics
>   * Copyright (c) 2016, Linaro Ltd.
>   * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
>   * Copyright (c) 2012, PetaLogix
> @@ -15,6 +16,8 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  
> +#include "rpmsg_char.h"
> +
>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
>  
>  static dev_t rpmsg_major;
> @@ -69,7 +72,7 @@ struct rpmsg_eptdev {
>  	wait_queue_head_t readq;
>  };
>  
> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>  {
>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>  
> @@ -88,6 +91,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>  
>  static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>  			void *priv, u32 addr)
> @@ -271,7 +275,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>  		return -EINVAL;
>  
> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +	return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>  }
>  
>  static const struct file_operations rpmsg_eptdev_fops = {
> @@ -330,10 +334,9 @@ static void rpmsg_eptdev_release_device(struct device *dev)
>  	kfree(eptdev);
>  }
>  
> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
>  			       struct rpmsg_channel_info chinfo)
>  {
> -	struct rpmsg_device *rpdev = ctrldev->rpdev;
>  	struct rpmsg_eptdev *eptdev;
>  	struct device *dev;
>  	int ret;
> @@ -353,7 +356,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  
>  	device_initialize(dev);
>  	dev->class = rpmsg_class;
> -	dev->parent = &ctrldev->dev;
> +	dev->parent = parent;
>  	dev->groups = rpmsg_eptdev_groups;
>  	dev_set_drvdata(dev, eptdev);
>  
> @@ -396,6 +399,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>  
>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>  {
> @@ -435,7 +439,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	chinfo.src = eptinfo.src;
>  	chinfo.dst = eptinfo.dst;
>  
> -	return rpmsg_eptdev_create(ctrldev, chinfo);
> +	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>  };
>  
>  static const struct file_operations rpmsg_ctrldev_fops = {
> @@ -521,7 +525,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>  	int ret;
>  
>  	/* Destroy all endpoints */
> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
> +	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>  	if (ret)
>  		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>  
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> new file mode 100644
> index 000000000000..facd324290a4
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) STMicroelectronics 2021.
> + */
> +
> +#ifndef __RPMSG_CHRDEV_H__
> +#define __RPMSG_CHRDEV_H__
> +
> +#if IS_REACHABLE(CONFIG_RPMSG_CHAR)
> +/**
> + * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
> + * @rpdev:  prepared rpdev to be used for creating endpoints
> + * @parent: parent device
> + * @chinfo: assiated endpoint channel information.

s/assiated/associated

> + *
> + * This function create a new rpmsg char endpoint device to instantiate a new
> + * endpoint based on chinfo information.
> + */
> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> +			       struct rpmsg_channel_info chinfo);
> +
> +/**
> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
> + * @data: private data associated to the endpoint device
> + *
> + * This function destroys a rpmsg char endpoint device created by the RPMSG_DESTROY_EPT_IOCTL
> + * control.
> + */
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
> +
> +#else  /*IS_REACHABLE(CONFIG_RPMSG_CHAR) */
> +
> +static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> +					     struct rpmsg_channel_info chinfo)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return 0;
> +}
> +
> +#endif /*IS_REACHABLE(CONFIG_RPMSG_CHAR) */
> +
> +#endif /*__RPMSG_CHRDEV_H__ */
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/6] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Mathieu Poirier]

On Thu, Apr 29, 2021 at 03:55:03PM +0200, Arnaud Pouliquen wrote:
> Create the rpmsg_ctrl.c module and move the code related to the
> rpmsg_ctrldev device in this new module.
> 
> Add the dependency between rpmsg_char and rpmsg_ctrl in the
> kconfig file.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> ---
> Update from V2:
> 
> - supress the "rpmsg" class in the rpmsg_ctrl module
> - cleanup useless includes

The includes need fixing in rpmsg_ctrl.c.  Otherwise this looks good now.

> ---
>  drivers/rpmsg/Kconfig      |   9 ++
>  drivers/rpmsg/Makefile     |   1 +
>  drivers/rpmsg/rpmsg_char.c | 170 +-----------------------------
>  drivers/rpmsg/rpmsg_char.h |   2 +
>  drivers/rpmsg/rpmsg_ctrl.c | 209 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 223 insertions(+), 168 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..d822ec9ec692 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -10,11 +10,20 @@ config RPMSG_CHAR
>  	tristate "RPMSG device interface"
>  	depends on RPMSG
>  	depends on NET
> +	select RPMSG_CTRL
>  	help
>  	  Say Y here to export rpmsg endpoints as device files, usually found
>  	  in /dev. They make it possible for user-space programs to send and
>  	  receive rpmsg packets.
>  
> +config RPMSG_CTRL
> +	tristate "RPMSG control interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the /dev/rpmsg_ctrlX API. This API
> +	  allows user-space programs to create endpoints with specific service name,
> +	  source and destination addresses.
> +
>  config RPMSG_NS
>  	tristate "RPMSG name service announcement"
>  	depends on RPMSG
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..58e3b382e316 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 3c53ece557a9..3b07a050b75e 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -18,33 +18,15 @@
>  
>  #include "rpmsg_char.h"
>  
> -#define RPMSG_DEV_MAX	(MINORMASK + 1)
> -
>  static dev_t rpmsg_major;
>  static struct class *rpmsg_class;
>  
> -static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_ept_ida);
>  static DEFINE_IDA(rpmsg_minor_ida);
>  
>  #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
>  #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
>  
> -#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
> -#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
> -
> -/**
> - * struct rpmsg_ctrldev - control device for instantiating endpoint devices
> - * @rpdev:	underlaying rpmsg device
> - * @cdev:	cdev for the ctrl device
> - * @dev:	device for the ctrl device
> - */
> -struct rpmsg_ctrldev {
> -	struct rpmsg_device *rpdev;
> -	struct cdev cdev;
> -	struct device dev;
> -};
> -
>  /**
>   * struct rpmsg_eptdev - endpoint device context
>   * @dev:	endpoint device
> @@ -401,151 +383,11 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>  
> -static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> -{
> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> -
> -	get_device(&ctrldev->dev);
> -	filp->private_data = ctrldev;
> -
> -	return 0;
> -}
> -
> -static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
> -{
> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> -
> -	put_device(&ctrldev->dev);
> -
> -	return 0;
> -}
> -
> -static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> -				unsigned long arg)
> -{
> -	struct rpmsg_ctrldev *ctrldev = fp->private_data;
> -	void __user *argp = (void __user *)arg;
> -	struct rpmsg_endpoint_info eptinfo;
> -	struct rpmsg_channel_info chinfo;
> -
> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> -		return -EINVAL;
> -
> -	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> -		return -EFAULT;
> -
> -	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> -	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
> -	chinfo.src = eptinfo.src;
> -	chinfo.dst = eptinfo.dst;
> -
> -	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
> -};
> -
> -static const struct file_operations rpmsg_ctrldev_fops = {
> -	.owner = THIS_MODULE,
> -	.open = rpmsg_ctrldev_open,
> -	.release = rpmsg_ctrldev_release,
> -	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
> -	.compat_ioctl = compat_ptr_ioctl,
> -};
> -
> -static void rpmsg_ctrldev_release_device(struct device *dev)
> -{
> -	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
> -
> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> -	cdev_del(&ctrldev->cdev);
> -	kfree(ctrldev);
> -}
> -
> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> -{
> -	struct rpmsg_ctrldev *ctrldev;
> -	struct device *dev;
> -	int ret;
> -
> -	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> -	if (!ctrldev)
> -		return -ENOMEM;
> -
> -	ctrldev->rpdev = rpdev;
> -
> -	dev = &ctrldev->dev;
> -	device_initialize(dev);
> -	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;
> -
> -	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> -	ctrldev->cdev.owner = THIS_MODULE;
> -
> -	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> -	if (ret < 0)
> -		goto free_ctrldev;
> -	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> -
> -	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> -	if (ret < 0)
> -		goto free_minor_ida;
> -	dev->id = ret;
> -	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> -
> -	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> -	if (ret)
> -		goto free_ctrl_ida;
> -
> -	/* We can now rely on the release function for cleanup */
> -	dev->release = rpmsg_ctrldev_release_device;
> -
> -	ret = device_add(dev);
> -	if (ret) {
> -		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> -		put_device(dev);
> -	}
> -
> -	dev_set_drvdata(&rpdev->dev, ctrldev);
> -
> -	return ret;
> -
> -free_ctrl_ida:
> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> -free_minor_ida:
> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> -free_ctrldev:
> -	put_device(dev);
> -	kfree(ctrldev);
> -
> -	return ret;
> -}
> -
> -static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> -{
> -	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
> -	int ret;
> -
> -	/* Destroy all endpoints */
> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> -	if (ret)
> -		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> -
> -	device_del(&ctrldev->dev);
> -	put_device(&ctrldev->dev);
> -}
> -
> -static struct rpmsg_driver rpmsg_chrdev_driver = {
> -	.probe = rpmsg_chrdev_probe,
> -	.remove = rpmsg_chrdev_remove,
> -	.drv = {
> -		.name = "rpmsg_chrdev",
> -	},
> -};
> -
>  static int rpmsg_chrdev_init(void)
>  {
>  	int ret;
>  
> -	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char");
>  	if (ret < 0) {
>  		pr_err("rpmsg: failed to allocate char dev region\n");
>  		return ret;
> @@ -558,20 +400,12 @@ static int rpmsg_chrdev_init(void)
>  		return PTR_ERR(rpmsg_class);
>  	}
>  
> -	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> -	if (ret < 0) {
> -		pr_err("rpmsgchr: failed to register rpmsg driver\n");
> -		class_destroy(rpmsg_class);
> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> -	}
> -
> -	return ret;
> +	return 0;
>  }
>  postcore_initcall(rpmsg_chrdev_init);
>  
>  static void rpmsg_chrdev_exit(void)
>  {
> -	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>  	class_destroy(rpmsg_class);
>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>  }
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> index facd324290a4..311f5a7690f5 100644
> --- a/drivers/rpmsg/rpmsg_char.h
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -6,6 +6,8 @@
>  #ifndef __RPMSG_CHRDEV_H__
>  #define __RPMSG_CHRDEV_H__
>  
> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> +
>  #if IS_REACHABLE(CONFIG_RPMSG_CHAR)
>  /**
>   * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> new file mode 100644
> index 000000000000..6b4d705b2dfd
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021, STMicroelectronics
> + * Copyright (c) 2016, Linaro Ltd.
> + * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
> + * Copyright (c) 2012, PetaLogix
> + * Copyright (c) 2011, Texas Instruments, Inc.
> + * Copyright (c) 2011, Google, Inc.
> + *
> + * Based on rpmsg performance statistics driver by Michal Simek, which in turn
> + * was based on TI & Google OMX rpmsg driver.
> + */
> +#include <linux/cdev.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/rpmsg.h>
> +#include <linux/slab.h>
> +
> +#include "rpmsg_char.h"
> +
> +static dev_t rpmsg_major;
> +
> +static DEFINE_IDA(rpmsg_ctrl_ida);
> +static DEFINE_IDA(rpmsg_minor_ida);
> +
> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
> +
> +/**
> + * struct rpmsg_ctrldev - control device for instantiating endpoint devices
> + * @rpdev:	underlaying rpmsg device
> + * @cdev:	cdev for the ctrl device
> + * @dev:	device for the ctrl device
> + */
> +struct rpmsg_ctrldev {
> +	struct rpmsg_device *rpdev;
> +	struct cdev cdev;
> +	struct device dev;
> +};
> +
> +static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	get_device(&ctrldev->dev);
> +	filp->private_data = ctrldev;
> +
> +	return 0;
> +}
> +
> +static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	put_device(&ctrldev->dev);
> +
> +	return 0;
> +}
> +
> +static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> +				unsigned long arg)
> +{
> +	struct rpmsg_ctrldev *ctrldev = fp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_channel_info chinfo;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
> +};
> +
> +static const struct file_operations rpmsg_ctrldev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = rpmsg_ctrldev_open,
> +	.release = rpmsg_ctrldev_release,
> +	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +static void rpmsg_ctrldev_release_device(struct device *dev)
> +{
> +	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
> +
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +	cdev_del(&ctrldev->cdev);
> +	kfree(ctrldev);
> +}
> +
> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrldev *ctrldev;
> +	struct device *dev;
> +	int ret;
> +
> +	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> +	if (!ctrldev)
> +		return -ENOMEM;
> +
> +	ctrldev->rpdev = rpdev;
> +
> +	dev = &ctrldev->dev;
> +	device_initialize(dev);
> +	dev->parent = &rpdev->dev;
> +
> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> +	ctrldev->cdev.owner = THIS_MODULE;
> +
> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_ctrldev;
> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_minor_ida;
> +	dev->id = ret;
> +	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> +
> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> +	if (ret)
> +		goto free_ctrl_ida;
> +
> +	/* We can now rely on the release function for cleanup */
> +	dev->release = rpmsg_ctrldev_release_device;
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> +		put_device(dev);
> +	}
> +
> +	dev_set_drvdata(&rpdev->dev, ctrldev);
> +
> +	return ret;
> +
> +free_ctrl_ida:
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +free_minor_ida:
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +free_ctrldev:
> +	put_device(dev);
> +	kfree(ctrldev);
> +
> +	return ret;
> +}
> +
> +static void rpmsg_ctrldev_remove(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
> +	int ret;
> +
> +	/* Destroy all endpoints */
> +	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> +	if (ret)
> +		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> +
> +	device_del(&ctrldev->dev);
> +	put_device(&ctrldev->dev);
> +}
> +
> +static struct rpmsg_driver rpmsg_ctrldev_driver = {
> +	.probe = rpmsg_ctrldev_probe,
> +	.remove = rpmsg_ctrldev_remove,
> +	.drv = {
> +		.name = "rpmsg_chrdev",
> +	},
> +};
> +
> +static int rpmsg_ctrldev_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_ctrl");
> +	if (ret < 0) {
> +		pr_err("rpmsg: failed to allocate char dev region\n");
> +		return ret;
> +	}
> +
> +	ret = register_rpmsg_driver(&rpmsg_ctrldev_driver);
> +	if (ret < 0) {
> +		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
> +		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +	}
> +
> +	return ret;
> +}
> +postcore_initcall(rpmsg_ctrldev_init);
> +
> +static void rpmsg_ctrldev_exit(void)
> +{
> +	unregister_rpmsg_driver(&rpmsg_ctrldev_driver);
> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +}
> +module_exit(rpmsg_ctrldev_exit);
> +
> +MODULE_DESCRIPTION("rpmsg control interface");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH v3 3/6] rpmsg: Update rpmsg_chrdev_register_device function

[Mathieu Poirier]

On Thu, Apr 29, 2021 at 03:55:04PM +0200, Arnaud Pouliquen wrote:
> The rpmsg_chrdev driver has been replaced by the rpmsg_ctrl driver
> for the /dev/rpmsg_ctrlX devices management. The reference for the
> driver override is now the rpmsg_ctrl.
> 
> Update the rpmsg_chrdev_register_device function to reflect the update,
> and rename the function to use the rpmsg_ctrldev prefix.
> 
> The platform drivers are updated accordingly.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 2 +-
>  drivers/rpmsg/qcom_smd.c          | 2 +-
>  drivers/rpmsg/rpmsg_ctrl.c        | 2 +-
>  drivers/rpmsg/rpmsg_internal.h    | 8 ++++----
>  drivers/rpmsg/virtio_rpmsg_bus.c  | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 05533c71b10e..7d7e809800ec 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1642,7 +1642,7 @@ static int qcom_glink_create_chrdev(struct qcom_glink *glink)
>  	rpdev->dev.parent = glink->dev;
>  	rpdev->dev.release = qcom_glink_device_release;
>  
> -	return rpmsg_chrdev_register_device(rpdev);
> +	return rpmsg_ctrldev_register_device(rpdev);
>  }
>  
>  struct qcom_glink *qcom_glink_native_probe(struct device *dev,
> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> index 8da1b5cb31b3..d223e438d17c 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -1113,7 +1113,7 @@ static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)
>  	qsdev->rpdev.dev.parent = &edge->dev;
>  	qsdev->rpdev.dev.release = qcom_smd_release_device;
>  
> -	return rpmsg_chrdev_register_device(&qsdev->rpdev);
> +	return rpmsg_ctrldev_register_device(&qsdev->rpdev);
>  }
>  
>  /*
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 6b4d705b2dfd..6749e26b68fc 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -173,7 +173,7 @@ static struct rpmsg_driver rpmsg_ctrldev_driver = {
>  	.probe = rpmsg_ctrldev_probe,
>  	.remove = rpmsg_ctrldev_remove,
>  	.drv = {
> -		.name = "rpmsg_chrdev",
> +		.name = "rpmsg_ctrl",
>  	},
>  };
>  
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index a76c344253bf..8c500a8f29aa 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -82,16 +82,16 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
>  int rpmsg_release_channel(struct rpmsg_device *rpdev,
>  			  struct rpmsg_channel_info *chinfo);
>  /**
> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> + * rpmsg_ctrl_register_device() - register a char device for control based on rpdev

rpmsg_ctrldev_register_device()

>   * @rpdev:	prepared rpdev to be used for creating endpoints
>   *
>   * This function wraps rpmsg_register_device() preparing the rpdev for use as
>   * basis for the rpmsg chrdev.
>   */
> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> +static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
>  {
> -	strcpy(rpdev->id.name, "rpmsg_chrdev");
> -	rpdev->driver_override = "rpmsg_chrdev";
> +	strcpy(rpdev->id.name, "rpmsg_ctrl");
> +	rpdev->driver_override = "rpmsg_ctrl";
>  
>  	return rpmsg_register_device(rpdev);
>  }
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 8e49a3bacfc7..e42234a3e2ab 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -840,7 +840,7 @@ static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
>  	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
>  	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
>  
> -	err = rpmsg_chrdev_register_device(rpdev_ctrl);
> +	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
>  	if (err) {
>  		kfree(vch);
>  		return ERR_PTR(err);
> -- 
> 2.17.1
> 

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

Hi Arnaud,

On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> A rpmsg char device allows to probe the endpoint device on a remote name
> service announcement.
> 
> With this patch the /dev/rpmsgX interface is created either by a user
> application or by the remote firmware.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> ---
> update from V1:
> 
>  - add missing unregister_rpmsg_driver call on module exit.
> ---
>  drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 5c6a7da6e4d7..9166454c1310 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -18,6 +18,8 @@
>  
>  #include "rpmsg_char.h"
>  
> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> +
>  static dev_t rpmsg_major;
>  static struct class *rpmsg_class;
>  
> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>  
> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_channel_info chinfo;
> +
> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> +	chinfo.src = rpdev->src;
> +	chinfo.dst = rpdev->dst;
> +
> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> +}
> +
> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> +{
> +	int ret;
> +
> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> +	if (ret)
> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
> +}
> +
> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
> +	{ .name	= RPMSG_CHAR_DEVNAME },
> +	{ },
> +};
> +
> +static struct rpmsg_driver rpmsg_chrdev_driver = {
> +	.probe = rpmsg_chrdev_probe,
> +	.remove = rpmsg_chrdev_remove,
> +	.id_table = rpmsg_chrdev_id_table,
> +	.drv = {
> +		.name = "rpmsg_chrdev",
> +	},
> +};

The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
name service - but is it really needed?  Up to now and aside from GLINK and SMD,
there asn't been other users of it so I'm wondering if it is worth going through
all this trouble.

As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
service.  That way patches 4, 5 and 6 of this set can be dropped.

Thanks,
Mathieu

> +
>  static int rpmsg_chrdev_init(void)
>  {
>  	int ret;
> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
>  	if (IS_ERR(rpmsg_class)) {
>  		pr_err("failed to create rpmsg class\n");
>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> -		return PTR_ERR(rpmsg_class);
> +		ret = PTR_ERR(rpmsg_class);
> +		goto free_region;
> +	}
> +
> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> +	if (ret < 0) {
> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
> +		goto free_class;
>  	}
>  
>  	return 0;
> +
> +free_class:
> +	class_destroy(rpmsg_class);
> +free_region:
> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +
> +	return ret;
>  }
>  postcore_initcall(rpmsg_chrdev_init);
>  
>  static void rpmsg_chrdev_exit(void)
>  {
> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>  	class_destroy(rpmsg_class);
>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Arnaud POULIQUEN]

Hi Mathieu,

On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> Hi Arnaud,
> 
> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
>> A rpmsg char device allows to probe the endpoint device on a remote name
>> service announcement.
>>
>> With this patch the /dev/rpmsgX interface is created either by a user
>> application or by the remote firmware.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>
>> ---
>> update from V1:
>>
>>  - add missing unregister_rpmsg_driver call on module exit.
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 5c6a7da6e4d7..9166454c1310 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -18,6 +18,8 @@
>>  
>>  #include "rpmsg_char.h"
>>  
>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
>> +
>>  static dev_t rpmsg_major;
>>  static struct class *rpmsg_class;
>>  
>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>>  
>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_channel_info chinfo;
>> +
>> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>> +	chinfo.src = rpdev->src;
>> +	chinfo.dst = rpdev->dst;
>> +
>> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>> +}
>> +
>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>> +{
>> +	int ret;
>> +
>> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>> +	if (ret)
>> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
>> +}
>> +
>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
>> +	{ .name	= RPMSG_CHAR_DEVNAME },
>> +	{ },
>> +};
>> +
>> +static struct rpmsg_driver rpmsg_chrdev_driver = {
>> +	.probe = rpmsg_chrdev_probe,
>> +	.remove = rpmsg_chrdev_remove,
>> +	.id_table = rpmsg_chrdev_id_table,
>> +	.drv = {
>> +		.name = "rpmsg_chrdev",
>> +	},
>> +};
> 
> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> there asn't been other users of it so I'm wondering if it is worth going through
> all this trouble.

It is a good point.

Just as a reminder, the need of ST and, I assume, some other companies, is to
have a basic/generic communication channel to control a remote processor
application.

Nothing generic exists today for a virtio transport based implementation.
Companies have to create their own driver.

The purpose of my work is to allow our customer to use RPMsg without developing
a specific driver to control remote applications.

The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
inter-processor link to send and receive data. The rpmsg_tty is another one.

Focusing on the rpmsg_chrdev:
We did a part of the work with the first patch set that would be in 5.13.
But is it simple to use it for virtio transport based platforms?
If we don't implement the NS announcement support in rpmsg_chrdev, using
rpmsg_chrdev for a user application seems rather tricky.
How to instantiate the communication?
The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
determine the services and associated remote address.

I don't think the QCOM drivers have the same problem because they seems to
initiate the communication and work directly with the RPMsg endpoints ( new
channel creation on endpoint creation) while Virtio works with the RPMsg channel.

By introducing the ability to instantiate rpmsg_chrdevs through the NS
announcement, we make this easy for applications to use.

And without rpmsg_chrdevs instantiation, It also means that we can't create an
RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
right?

That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
I'm not against leaving the rpmsg_char in this state and switching to the
rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
channels.

We could come back on this if requested by someone else.

Thanks,
Arnaud

> 
> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
> service.  That way patches 4, 5 and 6 of this set can be dropped.
> 
> Thanks,
> Mathieu
> 
>> +
>>  static int rpmsg_chrdev_init(void)
>>  {
>>  	int ret;
>> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
>>  	if (IS_ERR(rpmsg_class)) {
>>  		pr_err("failed to create rpmsg class\n");
>>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>> -		return PTR_ERR(rpmsg_class);
>> +		ret = PTR_ERR(rpmsg_class);
>> +		goto free_region;
>> +	}
>> +
>> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>> +	if (ret < 0) {
>> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
>> +		goto free_class;
>>  	}
>>  
>>  	return 0;
>> +
>> +free_class:
>> +	class_destroy(rpmsg_class);
>> +free_region:
>> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>> +
>> +	return ret;
>>  }
>>  postcore_initcall(rpmsg_chrdev_init);
>>  
>>  static void rpmsg_chrdev_exit(void)
>>  {
>> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>>  	class_destroy(rpmsg_class);
>>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>  }
>> -- 
>> 2.17.1
>>

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

Good day,

On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> > Hi Arnaud,
> > 
> > On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> >> A rpmsg char device allows to probe the endpoint device on a remote name
> >> service announcement.
> >>
> >> With this patch the /dev/rpmsgX interface is created either by a user
> >> application or by the remote firmware.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>
> >> ---
> >> update from V1:
> >>
> >>  - add missing unregister_rpmsg_driver call on module exit.
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 52 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 5c6a7da6e4d7..9166454c1310 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -18,6 +18,8 @@
> >>  
> >>  #include "rpmsg_char.h"
> >>  
> >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> >> +
> >>  static dev_t rpmsg_major;
> >>  static struct class *rpmsg_class;
> >>  
> >> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
> >>  
> >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >> +{
> >> +	struct rpmsg_channel_info chinfo;
> >> +
> >> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >> +	chinfo.src = rpdev->src;
> >> +	chinfo.dst = rpdev->dst;
> >> +
> >> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> >> +}
> >> +
> >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> >> +	if (ret)
> >> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
> >> +}
> >> +
> >> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
> >> +	{ .name	= RPMSG_CHAR_DEVNAME },
> >> +	{ },
> >> +};
> >> +
> >> +static struct rpmsg_driver rpmsg_chrdev_driver = {
> >> +	.probe = rpmsg_chrdev_probe,
> >> +	.remove = rpmsg_chrdev_remove,
> >> +	.id_table = rpmsg_chrdev_id_table,
> >> +	.drv = {
> >> +		.name = "rpmsg_chrdev",
> >> +	},
> >> +};
> > 
> > The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> > name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> > there asn't been other users of it so I'm wondering if it is worth going through
> > all this trouble.
> 
> It is a good point.
> 
> Just as a reminder, the need of ST and, I assume, some other companies, is to
> have a basic/generic communication channel to control a remote processor
> application.
> 
> Nothing generic exists today for a virtio transport based implementation.
> Companies have to create their own driver.
> 
> The purpose of my work is to allow our customer to use RPMsg without developing
> a specific driver to control remote applications.
> 
> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> inter-processor link to send and receive data. The rpmsg_tty is another one.
> 
> Focusing on the rpmsg_chrdev:
> We did a part of the work with the first patch set that would be in 5.13.
> But is it simple to use it for virtio transport based platforms?
> If we don't implement the NS announcement support in rpmsg_chrdev, using
> rpmsg_chrdev for a user application seems rather tricky.
> How to instantiate the communication?

Since we already have /dev/rpmsg_ctrlX user space can instantiate an rpmsg_chrdev
using that interface, which is how things are done in the GLINK/SMD world.

Wouldn't that cover the usecases you had in mind?

As you pointed out above rpmsg_chrdev should be light and simple - eliminating
patches 4, 5 and 6 would yield that.

> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> determine the services and associated remote address.
> 
> I don't think the QCOM drivers have the same problem because they seems to
> initiate the communication and work directly with the RPMsg endpoints ( new
> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> 
> By introducing the ability to instantiate rpmsg_chrdevs through the NS
> announcement, we make this easy for applications to use.
> 
> And without rpmsg_chrdevs instantiation, It also means that we can't create an
> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> right?
> 
> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> I'm not against leaving the rpmsg_char in this state and switching to the
> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> channels.
> 
> We could come back on this if requested by someone else.
> 
> Thanks,
> Arnaud
> 
> > 
> > As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
> > service.  That way patches 4, 5 and 6 of this set can be dropped.
> > 
> > Thanks,
> > Mathieu
> > 
> >> +
> >>  static int rpmsg_chrdev_init(void)
> >>  {
> >>  	int ret;
> >> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
> >>  	if (IS_ERR(rpmsg_class)) {
> >>  		pr_err("failed to create rpmsg class\n");
> >>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> -		return PTR_ERR(rpmsg_class);
> >> +		ret = PTR_ERR(rpmsg_class);
> >> +		goto free_region;
> >> +	}
> >> +
> >> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> >> +	if (ret < 0) {
> >> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
> >> +		goto free_class;
> >>  	}
> >>  
> >>  	return 0;
> >> +
> >> +free_class:
> >> +	class_destroy(rpmsg_class);
> >> +free_region:
> >> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +
> >> +	return ret;
> >>  }
> >>  postcore_initcall(rpmsg_chrdev_init);
> >>  
> >>  static void rpmsg_chrdev_exit(void)
> >>  {
> >> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
> >>  	class_destroy(rpmsg_class);
> >>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >>  }
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Julien Massot]

Hi Mathieu, Arnaud,

On 5/5/21 8:25 PM, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
>> Hi Arnaud,
>>
>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
>>> A rpmsg char device allows to probe the endpoint device on a remote name
>>> service announcement.
>>>
>>> With this patch the /dev/rpmsgX interface is created either by a user
>>> application or by the remote firmware.
>>>
>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>
>>> ---
>>> update from V1:
>>>
>>>   - add missing unregister_rpmsg_driver call on module exit.
>>> ---
>>>   drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index 5c6a7da6e4d7..9166454c1310 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -18,6 +18,8 @@
>>>   
>>>   #include "rpmsg_char.h"
>>>   
>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
>>> +
>>>   static dev_t rpmsg_major;
>>>   static struct class *rpmsg_class;
>>>   
>>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>>   }
>>>   EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>>>   
>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>> +{
>>> +	struct rpmsg_channel_info chinfo;
>>> +
>>> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>>> +	chinfo.src = rpdev->src;
>>> +	chinfo.dst = rpdev->dst;
>>> +
>>> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>>> +}
>>> +
>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>>> +	if (ret)
>>> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
>>> +}
>>> +
>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
>>> +	{ .name	= RPMSG_CHAR_DEVNAME },
>>> +	{ },
>>> +};
>>> +
>>> +static struct rpmsg_driver rpmsg_chrdev_driver = {
>>> +	.probe = rpmsg_chrdev_probe,
>>> +	.remove = rpmsg_chrdev_remove,
>>> +	.id_table = rpmsg_chrdev_id_table,
>>> +	.drv = {
>>> +		.name = "rpmsg_chrdev",
>>> +	},
>>> +};
>>
>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
>> there asn't been other users of it so I'm wondering if it is worth going through
>> all this trouble.
> 
> It is a good point.
> 
> Just as a reminder, the need of ST and, I assume, some other companies, is to
> have a basic/generic communication channel to control a remote processor
> application.
> 
> Nothing generic exists today for a virtio transport based implementation.
> Companies have to create their own driver.
> 
> The purpose of my work is to allow our customer to use RPMsg without developing
> a specific driver to control remote applications.
> 
> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> inter-processor link to send and receive data. The rpmsg_tty is another one.
> 
> Focusing on the rpmsg_chrdev:
> We did a part of the work with the first patch set that would be in 5.13.
> But is it simple to use it for virtio transport based platforms?
> If we don't implement the NS announcement support in rpmsg_chrdev, using
> rpmsg_chrdev for a user application seems rather tricky.
> How to instantiate the communication?
> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> determine the services and associated remote address.
> 
> I don't think the QCOM drivers have the same problem because they seems to
> initiate the communication and work directly with the RPMsg endpoints ( new
> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> 
> By introducing the ability to instantiate rpmsg_chrdevs through the NS
> announcement, we make this easy for applications to use.
> 
> And without rpmsg_chrdevs instantiation, It also means that we can't create an
> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> right?
> 
> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> I'm not against leaving the rpmsg_char in this state and switching to the
> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> channels.
> 
> We could come back on this if requested by someone else.

I'm personnaly following this thread, our project is to be able to do RPC call
from Linux to an RTOS (Zephyr). Our plan is to do that in userspace using the nameservice
announcement from virtio/rpmsg.

We did an hackish patch to do that internally:
https://github.com/iotbzh/meta-rcar-zephyr/blob/master/recipes-kernel/linux/linux-renesas/0001-Add-device-driver-for-rcar-r7-
rpmsg.patch

That we will be really happy to drop by any cleaner solution.

Thanks for your work !
Julien

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Arnaud POULIQUEN]

Hi Mathieu,

On 5/6/21 6:11 PM, Mathieu Poirier wrote:
> Good day,
> 
> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
>>> Hi Arnaud,
>>>
>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
>>>> A rpmsg char device allows to probe the endpoint device on a remote name
>>>> service announcement.
>>>>
>>>> With this patch the /dev/rpmsgX interface is created either by a user
>>>> application or by the remote firmware.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>>
>>>> ---
>>>> update from V1:
>>>>
>>>>  - add missing unregister_rpmsg_driver call on module exit.
>>>> ---
>>>>  drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index 5c6a7da6e4d7..9166454c1310 100644
>>>> --- a/drivers/rpmsg/rpmsg_char.c
>>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>>> @@ -18,6 +18,8 @@
>>>>  
>>>>  #include "rpmsg_char.h"
>>>>  
>>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
>>>> +
>>>>  static dev_t rpmsg_major;
>>>>  static struct class *rpmsg_class;
>>>>  
>>>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>>>  }
>>>>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>>>>  
>>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>> +{
>>>> +	struct rpmsg_channel_info chinfo;
>>>> +
>>>> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>>>> +	chinfo.src = rpdev->src;
>>>> +	chinfo.dst = rpdev->dst;
>>>> +
>>>> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>>>> +}
>>>> +
>>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>>>> +	if (ret)
>>>> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
>>>> +}
>>>> +
>>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
>>>> +	{ .name	= RPMSG_CHAR_DEVNAME },
>>>> +	{ },
>>>> +};
>>>> +
>>>> +static struct rpmsg_driver rpmsg_chrdev_driver = {
>>>> +	.probe = rpmsg_chrdev_probe,
>>>> +	.remove = rpmsg_chrdev_remove,
>>>> +	.id_table = rpmsg_chrdev_id_table,
>>>> +	.drv = {
>>>> +		.name = "rpmsg_chrdev",
>>>> +	},
>>>> +};
>>>
>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
>>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
>>> there asn't been other users of it so I'm wondering if it is worth going through
>>> all this trouble.
>>
>> It is a good point.
>>
>> Just as a reminder, the need of ST and, I assume, some other companies, is to
>> have a basic/generic communication channel to control a remote processor
>> application.
>>
>> Nothing generic exists today for a virtio transport based implementation.
>> Companies have to create their own driver.
>>
>> The purpose of my work is to allow our customer to use RPMsg without developing
>> a specific driver to control remote applications.
>>
>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
>> inter-processor link to send and receive data. The rpmsg_tty is another one.
>>
>> Focusing on the rpmsg_chrdev:
>> We did a part of the work with the first patch set that would be in 5.13.
>> But is it simple to use it for virtio transport based platforms?
>> If we don't implement the NS announcement support in rpmsg_chrdev, using
>> rpmsg_chrdev for a user application seems rather tricky.
>> How to instantiate the communication?
> 
> Since we already have /dev/rpmsg_ctrlX user space can instantiate an 
> using that interface, which is how things are done in the GLINK/SMD world.
> 
> Wouldn't that cover the usecases you had in mind?

I have in mind that to make RPMsg easy to use, we need a generic driver with a
basic user interface to send end receive data, that supports the NS announcement:
-  remote side could instantiate it.
-  an instantiation of the device by a Linux application generates a NS
announcement sent to the remote side (for instance to create a channel for debug
trace).

On the other side, the initial work requested by Bjorn seems to be reached:
de-correlate the control part to be able to reuse it for other rpmsg devices.

I just have the feeling that we are stay in the middle of the road without the
patches 4,5 and 6 to have a first basic interface relying on RPMsg.

> 
> As you pointed out above rpmsg_chrdev should be light and simple - eliminating
> patches 4, 5 and 6 would yield that.
> 

My concern here is more about the complexity of using it by application, for
platforms that rely on virtio rpmsg transport. For instance applications need to
know the notion of local and remote RPMsg addressing.

Based on your feeling, here is my proposition for next steps:
 1- resend a version a version with only patch 1,2 3 + the patch to clean-up the
   #include in rpmsg_char
 2- switch back to the RPMsg TTY upstream.
 3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland.


Then, we can come back to patches 4, 5 and 6 depending on the feedback from the
users.

Does this proposition would be OK for you?

Thanks,
Arnaud


>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
>> determine the services and associated remote address.
>>
>> I don't think the QCOM drivers have the same problem because they seems to
>> initiate the communication and work directly with the RPMsg endpoints ( new
>> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
>>
>> By introducing the ability to instantiate rpmsg_chrdevs through the NS
>> announcement, we make this easy for applications to use.
>>
>> And without rpmsg_chrdevs instantiation, It also means that we can't create an
>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
>> right?
>>
>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
>> I'm not against leaving the rpmsg_char in this state and switching to the
>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
>> channels.
>>
>> We could come back on this if requested by someone else.
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
>>> service.  That way patches 4, 5 and 6 of this set can be dropped.
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +
>>>>  static int rpmsg_chrdev_init(void)
>>>>  {
>>>>  	int ret;
>>>> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
>>>>  	if (IS_ERR(rpmsg_class)) {
>>>>  		pr_err("failed to create rpmsg class\n");
>>>>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>> -		return PTR_ERR(rpmsg_class);
>>>> +		ret = PTR_ERR(rpmsg_class);
>>>> +		goto free_region;
>>>> +	}
>>>> +
>>>> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>>>> +	if (ret < 0) {
>>>> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
>>>> +		goto free_class;
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> +
>>>> +free_class:
>>>> +	class_destroy(rpmsg_class);
>>>> +free_region:
>>>> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>> +
>>>> +	return ret;
>>>>  }
>>>>  postcore_initcall(rpmsg_chrdev_init);
>>>>  
>>>>  static void rpmsg_chrdev_exit(void)
>>>>  {
>>>> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>>>>  	class_destroy(rpmsg_class);
>>>>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>>  }
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

Good morning,

On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 5/6/21 6:11 PM, Mathieu Poirier wrote:
> > Good day,
> > 
> > On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> >>> Hi Arnaud,
> >>>
> >>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> >>>> A rpmsg char device allows to probe the endpoint device on a remote name
> >>>> service announcement.
> >>>>
> >>>> With this patch the /dev/rpmsgX interface is created either by a user
> >>>> application or by the remote firmware.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>>
> >>>> ---
> >>>> update from V1:
> >>>>
> >>>>  - add missing unregister_rpmsg_driver call on module exit.
> >>>> ---
> >>>>  drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 52 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >>>> index 5c6a7da6e4d7..9166454c1310 100644
> >>>> --- a/drivers/rpmsg/rpmsg_char.c
> >>>> +++ b/drivers/rpmsg/rpmsg_char.c
> >>>> @@ -18,6 +18,8 @@
> >>>>  
> >>>>  #include "rpmsg_char.h"
> >>>>  
> >>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> >>>> +
> >>>>  static dev_t rpmsg_major;
> >>>>  static struct class *rpmsg_class;
> >>>>  
> >>>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
> >>>>  }
> >>>>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
> >>>>  
> >>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >>>> +{
> >>>> +	struct rpmsg_channel_info chinfo;
> >>>> +
> >>>> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >>>> +	chinfo.src = rpdev->src;
> >>>> +	chinfo.dst = rpdev->dst;
> >>>> +
> >>>> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> >>>> +}
> >>>> +
> >>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> >>>> +	if (ret)
> >>>> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
> >>>> +}
> >>>> +
> >>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
> >>>> +	{ .name	= RPMSG_CHAR_DEVNAME },
> >>>> +	{ },
> >>>> +};
> >>>> +
> >>>> +static struct rpmsg_driver rpmsg_chrdev_driver = {
> >>>> +	.probe = rpmsg_chrdev_probe,
> >>>> +	.remove = rpmsg_chrdev_remove,
> >>>> +	.id_table = rpmsg_chrdev_id_table,
> >>>> +	.drv = {
> >>>> +		.name = "rpmsg_chrdev",
> >>>> +	},
> >>>> +};
> >>>
> >>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> >>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> >>> there asn't been other users of it so I'm wondering if it is worth going through
> >>> all this trouble.
> >>
> >> It is a good point.
> >>
> >> Just as a reminder, the need of ST and, I assume, some other companies, is to
> >> have a basic/generic communication channel to control a remote processor
> >> application.
> >>
> >> Nothing generic exists today for a virtio transport based implementation.
> >> Companies have to create their own driver.
> >>
> >> The purpose of my work is to allow our customer to use RPMsg without developing
> >> a specific driver to control remote applications.
> >>
> >> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> >> inter-processor link to send and receive data. The rpmsg_tty is another one.
> >>
> >> Focusing on the rpmsg_chrdev:
> >> We did a part of the work with the first patch set that would be in 5.13.
> >> But is it simple to use it for virtio transport based platforms?
> >> If we don't implement the NS announcement support in rpmsg_chrdev, using
> >> rpmsg_chrdev for a user application seems rather tricky.
> >> How to instantiate the communication?
> > 
> > Since we already have /dev/rpmsg_ctrlX user space can instantiate an 
> > using that interface, which is how things are done in the GLINK/SMD world.
> > 
> > Wouldn't that cover the usecases you had in mind?
> 
> I have in mind that to make RPMsg easy to use, we need a generic driver with a
> basic user interface to send end receive data, that supports the NS announcement:
> -  remote side could instantiate it.
> -  an instantiation of the device by a Linux application generates a NS
> announcement sent to the remote side (for instance to create a channel for debug
> trace).
>

The communication using a rpmsg_chrdev should be happening in two different ways,
i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous
patchset). 

From user space communication using a rpmsg_chrdev should be initiated in two
different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you
had in a previous patchset). 

Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy
compatibility and I am quite happy with that.  In this case the driver works the
same way regardless of the transport mechanism - virtio, GLINK or SMD.

Then there is instantiation with RPMSG_CREATE_DEV_IOCTL.  That creates a new
channel (with endpoint) when coming from /dev/rpmsg_ctrlX.  When we have that
functionality we can make the rpmsg_chrdev available from the name service, making
sure the end result is the same regardless of source of the request (remote
processor or user space).  I was under the impression that functionality would
be part of an upcoming patchset.

Unless I'm missing parts of the story, proceeding this way should cover all the
requirements we talked about.

> On the other side, the initial work requested by Bjorn seems to be reached:
> de-correlate the control part to be able to reuse it for other rpmsg devices.
> 
> I just have the feeling that we are stay in the middle of the road without the
> patches 4,5 and 6 to have a first basic interface relying on RPMsg.
> 
> > 
> > As you pointed out above rpmsg_chrdev should be light and simple - eliminating
> > patches 4, 5 and 6 would yield that.
> > 
> 
> My concern here is more about the complexity of using it by application, for
> platforms that rely on virtio rpmsg transport. For instance applications need to
> know the notion of local and remote RPMsg addressing.
> 
> Based on your feeling, here is my proposition for next steps:
>  1- resend a version a version with only patch 1,2 3 + the patch to clean-up the
>    #include in rpmsg_char
>  2- switch back to the RPMsg TTY upstream.
>  3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland.
>

Introducing RPMSG_TTY makes sense if a serial controller is only accessible from
the remote processor.  On the flip side it is an overkill if we just want a raw
message passing mechanism.  For that the rpmsg_chrdev driver, with the above
extention, should be used.
 
> 
> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the
> users.
> 
> Does this proposition would be OK for you?
> 
> Thanks,
> Arnaud
> 
> 
> >> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> >> determine the services and associated remote address.
> >>
> >> I don't think the QCOM drivers have the same problem because they seems to
> >> initiate the communication and work directly with the RPMsg endpoints ( new
> >> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> >>
> >> By introducing the ability to instantiate rpmsg_chrdevs through the NS
> >> announcement, we make this easy for applications to use.
> >>
> >> And without rpmsg_chrdevs instantiation, It also means that we can't create an
> >> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> >> right?
> >>
> >> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> >> I'm not against leaving the rpmsg_char in this state and switching to the
> >> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> >> channels.
> >>
> >> We could come back on this if requested by someone else.
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
> >>> service.  That way patches 4, 5 and 6 of this set can be dropped.
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>> +
> >>>>  static int rpmsg_chrdev_init(void)
> >>>>  {
> >>>>  	int ret;
> >>>> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void)
> >>>>  	if (IS_ERR(rpmsg_class)) {
> >>>>  		pr_err("failed to create rpmsg class\n");
> >>>>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >>>> -		return PTR_ERR(rpmsg_class);
> >>>> +		ret = PTR_ERR(rpmsg_class);
> >>>> +		goto free_region;
> >>>> +	}
> >>>> +
> >>>> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> >>>> +	if (ret < 0) {
> >>>> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
> >>>> +		goto free_class;
> >>>>  	}
> >>>>  
> >>>>  	return 0;
> >>>> +
> >>>> +free_class:
> >>>> +	class_destroy(rpmsg_class);
> >>>> +free_region:
> >>>> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >>>> +
> >>>> +	return ret;
> >>>>  }
> >>>>  postcore_initcall(rpmsg_chrdev_init);
> >>>>  
> >>>>  static void rpmsg_chrdev_exit(void)
> >>>>  {
> >>>> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
> >>>>  	class_destroy(rpmsg_class);
> >>>>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >>>>  }
> >>>> -- 
> >>>> 2.17.1
> >>>>

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

Good morning Julien,

On Fri, May 07, 2021 at 10:17:12AM +0200, Julien Massot wrote:
> Hi Mathieu, Arnaud,
> 
> On 5/5/21 8:25 PM, Arnaud POULIQUEN wrote:
> > Hi Mathieu,
> > 
> > On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> > > Hi Arnaud,
> > > 
> > > On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> > > > A rpmsg char device allows to probe the endpoint device on a remote name
> > > > service announcement.
> > > > 
> > > > With this patch the /dev/rpmsgX interface is created either by a user
> > > > application or by the remote firmware.
> > > > 
> > > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> > > > 
> > > > ---
> > > > update from V1:
> > > > 
> > > >   - add missing unregister_rpmsg_driver call on module exit.
> > > > ---
> > > >   drivers/rpmsg/rpmsg_char.c | 53 +++++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 52 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> > > > index 5c6a7da6e4d7..9166454c1310 100644
> > > > --- a/drivers/rpmsg/rpmsg_char.c
> > > > +++ b/drivers/rpmsg/rpmsg_char.c
> > > > @@ -18,6 +18,8 @@
> > > >   #include "rpmsg_char.h"
> > > > +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> > > > +
> > > >   static dev_t rpmsg_major;
> > > >   static struct class *rpmsg_class;
> > > > @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
> > > >   }
> > > >   EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
> > > > +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> > > > +{
> > > > +	struct rpmsg_channel_info chinfo;
> > > > +
> > > > +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> > > > +	chinfo.src = rpdev->src;
> > > > +	chinfo.dst = rpdev->dst;
> > > > +
> > > > +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> > > > +}
> > > > +
> > > > +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> > > > +	if (ret)
> > > > +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
> > > > +}
> > > > +
> > > > +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
> > > > +	{ .name	= RPMSG_CHAR_DEVNAME },
> > > > +	{ },
> > > > +};
> > > > +
> > > > +static struct rpmsg_driver rpmsg_chrdev_driver = {
> > > > +	.probe = rpmsg_chrdev_probe,
> > > > +	.remove = rpmsg_chrdev_remove,
> > > > +	.id_table = rpmsg_chrdev_id_table,
> > > > +	.drv = {
> > > > +		.name = "rpmsg_chrdev",
> > > > +	},
> > > > +};
> > > 
> > > The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> > > name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> > > there asn't been other users of it so I'm wondering if it is worth going through
> > > all this trouble.
> > 
> > It is a good point.
> > 
> > Just as a reminder, the need of ST and, I assume, some other companies, is to
> > have a basic/generic communication channel to control a remote processor
> > application.
> > 
> > Nothing generic exists today for a virtio transport based implementation.
> > Companies have to create their own driver.
> > 
> > The purpose of my work is to allow our customer to use RPMsg without developing
> > a specific driver to control remote applications.
> > 
> > The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> > inter-processor link to send and receive data. The rpmsg_tty is another one.
> > 
> > Focusing on the rpmsg_chrdev:
> > We did a part of the work with the first patch set that would be in 5.13.
> > But is it simple to use it for virtio transport based platforms?
> > If we don't implement the NS announcement support in rpmsg_chrdev, using
> > rpmsg_chrdev for a user application seems rather tricky.
> > How to instantiate the communication?
> > The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> > determine the services and associated remote address.
> > 
> > I don't think the QCOM drivers have the same problem because they seems to
> > initiate the communication and work directly with the RPMsg endpoints ( new
> > channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> > 
> > By introducing the ability to instantiate rpmsg_chrdevs through the NS
> > announcement, we make this easy for applications to use.
> > 
> > And without rpmsg_chrdevs instantiation, It also means that we can't create an
> > RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> > right?
> > 
> > That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> > I'm not against leaving the rpmsg_char in this state and switching to the
> > rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> > channels.
> > 
> > We could come back on this if requested by someone else.
> 
> I'm personnaly following this thread, our project is to be able to do RPC call
> from Linux to an RTOS (Zephyr). Our plan is to do that in userspace using the nameservice
> announcement from virtio/rpmsg.

Good to know.  I highly encourage you to review patches and provide comments -
that will be very helpful to us.

Thanks,
Mathieu

> 
> We did an hackish patch to do that internally:
> https://github.com/iotbzh/meta-rcar-zephyr/blob/master/recipes-kernel/linux/linux-renesas/0001-Add-device-driver-for-rcar-r7-
> rpmsg.patch
> 
> That we will be really happy to drop by any cleaner solution.
> 
> Thanks for your work !
> Julien

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Arnaud POULIQUEN]

Hello Mathieu,

On 5/7/21 6:31 PM, Mathieu Poirier wrote:
> Good morning,
> 
> On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 5/6/21 6:11 PM, Mathieu Poirier wrote:
>>> Good day,
>>>
>>> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
>>>> Hi Mathieu,
>>>>
>>>> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
>>>>> Hi Arnaud,
>>>>>
>>>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:

[snip...]

>>>>>> +};
>>>>>
>>>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
>>>>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
>>>>> there asn't been other users of it so I'm wondering if it is worth going through
>>>>> all this trouble.
>>>>
>>>> It is a good point.
>>>>
>>>> Just as a reminder, the need of ST and, I assume, some other companies, is to
>>>> have a basic/generic communication channel to control a remote processor
>>>> application.
>>>>
>>>> Nothing generic exists today for a virtio transport based implementation.
>>>> Companies have to create their own driver.
>>>>
>>>> The purpose of my work is to allow our customer to use RPMsg without developing
>>>> a specific driver to control remote applications.
>>>>
>>>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
>>>> inter-processor link to send and receive data. The rpmsg_tty is another one.
>>>>
>>>> Focusing on the rpmsg_chrdev:
>>>> We did a part of the work with the first patch set that would be in 5.13.
>>>> But is it simple to use it for virtio transport based platforms?
>>>> If we don't implement the NS announcement support in rpmsg_chrdev, using
>>>> rpmsg_chrdev for a user application seems rather tricky.
>>>> How to instantiate the communication?
>>>
>>> Since we already have /dev/rpmsg_ctrlX user space can instantiate an 
>>> using that interface, which is how things are done in the GLINK/SMD world.
>>>
>>> Wouldn't that cover the usecases you had in mind?
>>
>> I have in mind that to make RPMsg easy to use, we need a generic driver with a
>> basic user interface to send end receive data, that supports the NS announcement:
>> -  remote side could instantiate it.
>> -  an instantiation of the device by a Linux application generates a NS
>> announcement sent to the remote side (for instance to create a channel for debug
>> trace).
>>
> 
> The communication using a rpmsg_chrdev should be happening in two different ways,
> i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous
> patchset). 
> 
> From user space communication using a rpmsg_chrdev should be initiated in two
> different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you
> had in a previous patchset). 
> 
> Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy
> compatibility and I am quite happy with that.  In this case the driver works the
> same way regardless of the transport mechanism - virtio, GLINK or SMD.

Ok i will send a new revision including only this ones, and continue the updates
in a new patchset.

> 
> Then there is instantiation with RPMSG_CREATE_DEV_IOCTL.  That creates a new
> channel (with endpoint) when coming from /dev/rpmsg_ctrlX.  When we have that
> functionality we can make the rpmsg_chrdev available from the name service, making
> sure the end result is the same regardless of source of the request (remote
> processor or user space).  I was under the impression that functionality would
> be part of an upcoming patchset.
> 
> Unless I'm missing parts of the story, proceeding this way should cover all the
> requirements we talked about.

From my windows, there are 3 remaining features:
- capability to instantiate rpmsg_chrdev from the remote side (NS announcement)
- capability to instantiate rpmsg_chrdev from local user application
  (RPMSG_CREATE_DEV_IOCTL)
- capability to send a NS announcement to the remote side on  rpmsg_chrdev local
instantiation using RPMSG_CREATE_DEV_IOCTL. This one could be more tricky to
implement as the endpoint can be created after the channel.

To simplify the review while keeping the overall picture in mind (and perhaps
prioritize based on other companies' interests), Please, just tell me what would
be your preference in term of splitting and next step.

> 
>> On the other side, the initial work requested by Bjorn seems to be reached:
>> de-correlate the control part to be able to reuse it for other rpmsg devices.
>>
>> I just have the feeling that we are stay in the middle of the road without the
>> patches 4,5 and 6 to have a first basic interface relying on RPMsg.
>>
>>>
>>> As you pointed out above rpmsg_chrdev should be light and simple - eliminating
>>> patches 4, 5 and 6 would yield that.
>>>
>>
>> My concern here is more about the complexity of using it by application, for
>> platforms that rely on virtio rpmsg transport. For instance applications need to
>> know the notion of local and remote RPMsg addressing.
>>
>> Based on your feeling, here is my proposition for next steps:
>>  1- resend a version a version with only patch 1,2 3 + the patch to clean-up the
>>    #include in rpmsg_char
>>  2- switch back to the RPMsg TTY upstream.
>>  3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland.
>>
> 
> Introducing RPMSG_TTY makes sense if a serial controller is only accessible from
> the remote processor.  On the flip side it is an overkill if we just want a raw
> message passing mechanism.  For that the rpmsg_chrdev driver, with the above
> extention, should be used.
>  

Yes the rpmsg_chrdev should be the default one to use for basic communication.
The main purpose of the RPMSG_TTY (from ST company POW) is to easy the
transition in term of communication between an external and an internal
processor based on a serial link. It provides an abstraction layer that the
application does not have to manage the transport layer.

Both seem to me interesting to implement, but let's continue to focus on
rpmsg_chrdev first.

Thanks,
Arnaud

>>
>> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the
>> users.
>>
>> Does this proposition would be OK for you?
>>
>> Thanks,
>> Arnaud
>>
>>
>>>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
>>>> determine the services and associated remote address.
>>>>
>>>> I don't think the QCOM drivers have the same problem because they seems to
>>>> initiate the communication and work directly with the RPMsg endpoints ( new
>>>> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
>>>>
>>>> By introducing the ability to instantiate rpmsg_chrdevs through the NS
>>>> announcement, we make this easy for applications to use.
>>>>
>>>> And without rpmsg_chrdevs instantiation, It also means that we can't create an
>>>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
>>>> right?
>>>>
>>>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
>>>> I'm not against leaving the rpmsg_char in this state and switching to the
>>>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
>>>> channels.
>>>>
>>>> We could come back on this if requested by someone else.
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>>
>>>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
>>>>> service.  That way patches 4, 5 and 6 of this set can be dropped.
>>>>>
>>>>> Thanks,
>>>>> Mathieu
>>>>>

Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

On Mon, May 17, 2021 at 12:04:29PM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 5/7/21 6:31 PM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> On 5/6/21 6:11 PM, Mathieu Poirier wrote:
> >>> Good day,
> >>>
> >>> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hi Mathieu,
> >>>>
> >>>> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> >>>>> Hi Arnaud,
> >>>>>
> >>>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> 
> [snip...]
> 
> >>>>>> +};
> >>>>>
> >>>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> >>>>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> >>>>> there asn't been other users of it so I'm wondering if it is worth going through
> >>>>> all this trouble.
> >>>>
> >>>> It is a good point.
> >>>>
> >>>> Just as a reminder, the need of ST and, I assume, some other companies, is to
> >>>> have a basic/generic communication channel to control a remote processor
> >>>> application.
> >>>>
> >>>> Nothing generic exists today for a virtio transport based implementation.
> >>>> Companies have to create their own driver.
> >>>>
> >>>> The purpose of my work is to allow our customer to use RPMsg without developing
> >>>> a specific driver to control remote applications.
> >>>>
> >>>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> >>>> inter-processor link to send and receive data. The rpmsg_tty is another one.
> >>>>
> >>>> Focusing on the rpmsg_chrdev:
> >>>> We did a part of the work with the first patch set that would be in 5.13.
> >>>> But is it simple to use it for virtio transport based platforms?
> >>>> If we don't implement the NS announcement support in rpmsg_chrdev, using
> >>>> rpmsg_chrdev for a user application seems rather tricky.
> >>>> How to instantiate the communication?
> >>>
> >>> Since we already have /dev/rpmsg_ctrlX user space can instantiate an 
> >>> using that interface, which is how things are done in the GLINK/SMD world.
> >>>
> >>> Wouldn't that cover the usecases you had in mind?
> >>
> >> I have in mind that to make RPMsg easy to use, we need a generic driver with a
> >> basic user interface to send end receive data, that supports the NS announcement:
> >> -  remote side could instantiate it.
> >> -  an instantiation of the device by a Linux application generates a NS
> >> announcement sent to the remote side (for instance to create a channel for debug
> >> trace).
> >>
> > 
> > The communication using a rpmsg_chrdev should be happening in two different ways,
> > i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous
> > patchset). 
> > 
> > From user space communication using a rpmsg_chrdev should be initiated in two
> > different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you
> > had in a previous patchset). 
> > 
> > Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy
> > compatibility and I am quite happy with that.  In this case the driver works the
> > same way regardless of the transport mechanism - virtio, GLINK or SMD.
> 
> Ok i will send a new revision including only this ones, and continue the updates
> in a new patchset.
> 
> > 
> > Then there is instantiation with RPMSG_CREATE_DEV_IOCTL.  That creates a new
> > channel (with endpoint) when coming from /dev/rpmsg_ctrlX.  When we have that
> > functionality we can make the rpmsg_chrdev available from the name service, making
> > sure the end result is the same regardless of source of the request (remote
> > processor or user space).  I was under the impression that functionality would
> > be part of an upcoming patchset.
> > 
> > Unless I'm missing parts of the story, proceeding this way should cover all the
> > requirements we talked about.
> 
> From my windows, there are 3 remaining features:
> - capability to instantiate rpmsg_chrdev from the remote side (NS announcement)

I think this should be #2.

> - capability to instantiate rpmsg_chrdev from local user application
>   (RPMSG_CREATE_DEV_IOCTL)

This should be #1.  Once this is firmly in place #2 (above) should be relatively
easy to implement.  #1 and #2 can be in the same patchset, or not, depending on
what you prefer. 

> - capability to send a NS announcement to the remote side on  rpmsg_chrdev local
> instantiation using RPMSG_CREATE_DEV_IOCTL. This one could be more tricky to
> implement as the endpoint can be created after the channel.

That should probably come after #1 and #2, and in a separate patchset.

> 
> To simplify the review while keeping the overall picture in mind (and perhaps
> prioritize based on other companies' interests), Please, just tell me what would
> be your preference in term of splitting and next step.
> 
> > 
> >> On the other side, the initial work requested by Bjorn seems to be reached:
> >> de-correlate the control part to be able to reuse it for other rpmsg devices.
> >>
> >> I just have the feeling that we are stay in the middle of the road without the
> >> patches 4,5 and 6 to have a first basic interface relying on RPMsg.
> >>
> >>>
> >>> As you pointed out above rpmsg_chrdev should be light and simple - eliminating
> >>> patches 4, 5 and 6 would yield that.
> >>>
> >>
> >> My concern here is more about the complexity of using it by application, for
> >> platforms that rely on virtio rpmsg transport. For instance applications need to
> >> know the notion of local and remote RPMsg addressing.
> >>
> >> Based on your feeling, here is my proposition for next steps:
> >>  1- resend a version a version with only patch 1,2 3 + the patch to clean-up the
> >>    #include in rpmsg_char
> >>  2- switch back to the RPMsg TTY upstream.
> >>  3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland.
> >>
> > 
> > Introducing RPMSG_TTY makes sense if a serial controller is only accessible from
> > the remote processor.  On the flip side it is an overkill if we just want a raw
> > message passing mechanism.  For that the rpmsg_chrdev driver, with the above
> > extention, should be used.
> >  
> 
> Yes the rpmsg_chrdev should be the default one to use for basic communication.

Perfect, we are on the same page.

> The main purpose of the RPMSG_TTY (from ST company POW) is to easy the
> transition in term of communication between an external and an internal
> processor based on a serial link. It provides an abstraction layer that the
> application does not have to manage the transport layer.
> 

Ok

> Both seem to me interesting to implement, but let's continue to focus on
> rpmsg_chrdev first.
> 
> Thanks,
> Arnaud
> 
> >>
> >> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the
> >> users.
> >>
> >> Does this proposition would be OK for you?
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>
> >>>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> >>>> determine the services and associated remote address.
> >>>>
> >>>> I don't think the QCOM drivers have the same problem because they seems to
> >>>> initiate the communication and work directly with the RPMsg endpoints ( new
> >>>> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> >>>>
> >>>> By introducing the ability to instantiate rpmsg_chrdevs through the NS
> >>>> announcement, we make this easy for applications to use.
> >>>>
> >>>> And without rpmsg_chrdevs instantiation, It also means that we can't create an
> >>>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> >>>> right?
> >>>>
> >>>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> >>>> I'm not against leaving the rpmsg_char in this state and switching to the
> >>>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> >>>> channels.
> >>>>
> >>>> We could come back on this if requested by someone else.
> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
> >>>>> service.  That way patches 4, 5 and 6 of this set can be dropped.
> >>>>>
> >>>>> Thanks,
> >>>>> Mathieu
> >>>>>
>