[PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[Arnaud Pouliquen]

Main update from V4 [1] 
 - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
 - rebased on kernel V.14-rc1.

This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch

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 code related to the control
and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.
This split is an intermediate step to extend the controls to allow user applications to
instantiate rpmsg devices.
    
Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
and RPMSG_DESTROY_EPT_IOCTL controls.
  
The next step should be to add the capability to:
- instantiate rpmsg_chrdev from the remote side (NS announcement),
- instantiate rpmsg_chrdev from local user application by introducing the
  IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
- send a NS announcement to the remote side on rpmsg_chrdev local instantiation.

[1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
[2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523

Arnaud Pouliquen (4):
  rpmsg: char: Remove useless include
  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

 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        | 184 ++-----------------------
 drivers/rpmsg/rpmsg_char.h        |  51 +++++++
 drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |   8 +-
 drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
 9 files changed, 293 insertions(+), 181 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

[PATCH v5 1/4] rpmsg: char: Remove useless include

[Arnaud Pouliquen]

No facility requests the include of rpmsg_internal.h header file.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_char.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 2bebc9b2d163..b5907b80727c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -22,8 +22,6 @@
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
-#include "rpmsg_internal.h"
-
 #define RPMSG_DEV_MAX	(MINORMASK + 1)
 
 static dev_t rpmsg_major;
-- 
2.17.1

Re: (subset) [PATCH v5 1/4] rpmsg: char: Remove useless include

[Bjorn Andersson]

On Mon, 12 Jul 2021 14:37:49 +0200, Arnaud Pouliquen wrote:
> No facility requests the include of rpmsg_internal.h header file.
> 
> 

Applied, thanks!

[1/4] rpmsg: char: Remove useless include
      commit: bc774a3887cb513be08e846726bc4402897b267a

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

[PATCH v5 2/4] 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.

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>
---
 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 b5907b80727c..941c5c54dd72 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
@@ -22,6 +23,8 @@
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
+#include "rpmsg_char.h"
+
 #define RPMSG_DEV_MAX	(MINORMASK + 1)
 
 static dev_t rpmsg_major;
@@ -76,7 +79,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);
 
@@ -95,6 +98,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)
@@ -278,7 +282,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 = {
@@ -337,10 +341,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;
@@ -360,7 +363,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);
 
@@ -403,6 +406,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)
 {
@@ -442,7 +446,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 = {
@@ -528,7 +532,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..22573b60e008
--- /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: associated 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

Re: [PATCH v5 2/4] rpmsg: char: Export eptdev create an destroy functions

[Bjorn Andersson]

On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
[..]
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> new file mode 100644
> index 000000000000..22573b60e008
> --- /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)

This does allow you to build a kernel with RPMSG_CHAR=m and RPMSG_CTRL=y
without link failures. Any modules in the system will be able to call
rpmsg_chrdev_eptdev_create(), but the rpmsg_ctrl driver will only end up
in the stub.

This sounds like a recipe for a terrible debugging session...

Regards,
Bjorn

> +/**
> + * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
> + * @rpdev:  prepared rpdev to be used for creating endpoints
> + * @parent: parent device
> + * @chinfo: associated 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
> 

Re: [PATCH v5 2/4] rpmsg: char: Export eptdev create an destroy functions

[Arnaud POULIQUEN]

On 10/9/21 1:29 AM, Bjorn Andersson wrote:
> On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
> [..]
>> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
>> new file mode 100644
>> index 000000000000..22573b60e008
>> --- /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)
> 
> This does allow you to build a kernel with RPMSG_CHAR=m and RPMSG_CTRL=y
> without link failures. Any modules in the system will be able to call
> rpmsg_chrdev_eptdev_create(), but the rpmsg_ctrl driver will only end up
> in the stub.
> 
> This sounds like a recipe for a terrible debugging session...

The RPMSG_CREATE_EPT_IOCTL control create a dependency between the rpmsg_ctrl
and the rpmsg_char. The build error option seems not a good alternative.
And in case of RPMSG_CHAR=m and RPMSG_CTRL=y, an error is returned so not only a
stub.

What about adding a WARN_ON(1) in rpmsg_chrdev_eptdev_create
with an associated comment to ease the debug?

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +/**
>> + * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
>> + * @rpdev:  prepared rpdev to be used for creating endpoints
>> + * @parent: parent device
>> + * @chinfo: associated 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 v5 3/4] 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>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 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 | 215 +++++++++++++++++++++++++++++++++++++
 5 files changed, 229 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 941c5c54dd72..fbe10d527c5c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -25,33 +25,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
@@ -408,151 +390,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;
@@ -565,20 +407,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 22573b60e008..c328eb250b87 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..87a1746367eb
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,215 @@
+// 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/device.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/rpmsg.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 v5 3/4] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Bjorn Andersson]

On Mon 12 Jul 05:37 PDT 2021, 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.
> 

As I said in the cover letter, the only reason I can see for doing this
refactoring is in relation to the introduction of
RPMSG_CREATE_DEV_IOCTL. So I would like this patch to go together with
that patch, together with a good motivation why there's merit to
creating yet another kernel module (and by bind/unbind can't be used).

Perhaps I'm just missing some good usecase related to this?

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  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 | 215 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 229 insertions(+), 168 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
[..]
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
[..]
> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> -{
[..]
> -	dev = &ctrldev->dev;
> -	device_initialize(dev);
> -	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;
[..]
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
[..]
> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
> +{
[..]
> +	dev = &ctrldev->dev;
> +	device_initialize(dev);
> +	dev->parent = &rpdev->dev;

You lost the assignment of dev->class here, which breaks the udev rules
we use to invoke rpmsgexport to create endpoints and it causes udevadm
to complain that rpmsg_ctrlN doesn't have a "subsystem".

Regards,
Bjorn

Re: [PATCH v5 3/4] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud POULIQUEN]

On 10/9/21 1:35 AM, Bjorn Andersson wrote:
> On Mon 12 Jul 05:37 PDT 2021, 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.
>>
> 
> As I said in the cover letter, the only reason I can see for doing this
> refactoring is in relation to the introduction of
> RPMSG_CREATE_DEV_IOCTL. So I would like this patch to go together with
> that patch, together with a good motivation why there's merit to
> creating yet another kernel module (and by bind/unbind can't be used).
> 
> Perhaps I'm just missing some good usecase related to this?


> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  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 | 215 +++++++++++++++++++++++++++++++++++++
>>  5 files changed, 229 insertions(+), 168 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>
> [..]
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> [..]
>> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>> -{
> [..]
>> -	dev = &ctrldev->dev;
>> -	device_initialize(dev);
>> -	dev->parent = &rpdev->dev;
>> -	dev->class = rpmsg_class;
> [..]
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> [..]
>> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
>> +{
> [..]
>> +	dev = &ctrldev->dev;
>> +	device_initialize(dev);
>> +	dev->parent = &rpdev->dev;
> 
> You lost the assignment of dev->class here, which breaks the udev rules
> we use to invoke rpmsgexport to create endpoints and it causes udevadm
> to complain that rpmsg_ctrlN doesn't have a "subsystem".

We discussed this point with Mathieu, as a first step i kept the class, but that
generated another dependency with the rpmsg_char device while information was
available on the rpmsg bus. The char device and ctrl device should share the
same class. As rpmsg_ctrl is created first it would have to create the class,and
provide an API to rpmsg char

Please could you details what does means "rpmsg_ctrlN doesn't have a
"subsystem"." What exactly the udev is looking for? could it base it check on
the /dev/rpmsg_ctrl0 or /sys/bus/rpmsg/devices/...?

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 

Re: [PATCH v5 3/4] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Bjorn Andersson]

On Mon 11 Oct 05:46 CDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/9/21 1:35 AM, Bjorn Andersson wrote:
> > On Mon 12 Jul 05:37 PDT 2021, 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.
> >>
> > 
> > As I said in the cover letter, the only reason I can see for doing this
> > refactoring is in relation to the introduction of
> > RPMSG_CREATE_DEV_IOCTL. So I would like this patch to go together with
> > that patch, together with a good motivation why there's merit to
> > creating yet another kernel module (and by bind/unbind can't be used).
> > 
> > Perhaps I'm just missing some good usecase related to this?
> 
> 
> > 
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> ---
> >>  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 | 215 +++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 229 insertions(+), 168 deletions(-)
> >>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> >>
> > [..]
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> > [..]
> >> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >> -{
> > [..]
> >> -	dev = &ctrldev->dev;
> >> -	device_initialize(dev);
> >> -	dev->parent = &rpdev->dev;
> >> -	dev->class = rpmsg_class;
> > [..]
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> > [..]
> >> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
> >> +{
> > [..]
> >> +	dev = &ctrldev->dev;
> >> +	device_initialize(dev);
> >> +	dev->parent = &rpdev->dev;
> > 
> > You lost the assignment of dev->class here, which breaks the udev rules
> > we use to invoke rpmsgexport to create endpoints and it causes udevadm
> > to complain that rpmsg_ctrlN doesn't have a "subsystem".
> 
> We discussed this point with Mathieu, as a first step i kept the class, but that
> generated another dependency with the rpmsg_char device while information was
> available on the rpmsg bus. The char device and ctrl device should share the
> same class. As rpmsg_ctrl is created first it would have to create the class,and
> provide an API to rpmsg char
> 

Perhaps if this is considered a common piece shared between multiple
rpmsg modules we can create such class in the rpmsg "core" itself?

> Please could you details what does means "rpmsg_ctrlN doesn't have a
> "subsystem"." What exactly the udev is looking for? could it base it check on
> the /dev/rpmsg_ctrl0 or /sys/bus/rpmsg/devices/...?
> 

If I read the uevent messages correctly they seem to contain a SUBSYTEM=
property when the class is provided. But I'm not sure about the reasons
for that.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 

Re: [PATCH v5 3/4] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud POULIQUEN]

On 10/16/21 6:46 AM, Bjorn Andersson wrote:
> On Mon 11 Oct 05:46 CDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 10/9/21 1:35 AM, Bjorn Andersson wrote:
>>> On Mon 12 Jul 05:37 PDT 2021, 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.
>>>>
>>>
>>> As I said in the cover letter, the only reason I can see for doing this
>>> refactoring is in relation to the introduction of
>>> RPMSG_CREATE_DEV_IOCTL. So I would like this patch to go together with
>>> that patch, together with a good motivation why there's merit to
>>> creating yet another kernel module (and by bind/unbind can't be used).
>>>
>>> Perhaps I'm just missing some good usecase related to this?
>>
>>
>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> ---
>>>>  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 | 215 +++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 229 insertions(+), 168 deletions(-)
>>>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>>>
>>> [..]
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> [..]
>>>> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>> -{
>>> [..]
>>>> -	dev = &ctrldev->dev;
>>>> -	device_initialize(dev);
>>>> -	dev->parent = &rpdev->dev;
>>>> -	dev->class = rpmsg_class;
>>> [..]
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>> [..]
>>>> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
>>>> +{
>>> [..]
>>>> +	dev = &ctrldev->dev;
>>>> +	device_initialize(dev);
>>>> +	dev->parent = &rpdev->dev;
>>>
>>> You lost the assignment of dev->class here, which breaks the udev rules
>>> we use to invoke rpmsgexport to create endpoints and it causes udevadm
>>> to complain that rpmsg_ctrlN doesn't have a "subsystem".
>>
>> We discussed this point with Mathieu, as a first step i kept the class, but that
>> generated another dependency with the rpmsg_char device while information was
>> available on the rpmsg bus. The char device and ctrl device should share the
>> same class. As rpmsg_ctrl is created first it would have to create the class,and
>> provide an API to rpmsg char
>>
> 
> Perhaps if this is considered a common piece shared between multiple
> rpmsg modules we can create such class in the rpmsg "core" itself?

Yes that seems a good alternative

> 
>> Please could you details what does means "rpmsg_ctrlN doesn't have a
>> "subsystem"." What exactly the udev is looking for? could it base it check on
>> the /dev/rpmsg_ctrl0 or /sys/bus/rpmsg/devices/...?
>>
> 
> If I read the uevent messages correctly they seem to contain a SUBSYTEM=
> property when the class is provided. But I'm not sure about the reasons
> for that.

If it part of the udev requirement, i suppose that it is mandatory, and in this
case, declare the class in the core make sense.

I will send a new patchset that will squash all the remaining patches, taking
into account your comment.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>

Re: [PATCH v5 3/4] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Bjorn Andersson]

On Mon 18 Oct 02:13 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/16/21 6:46 AM, Bjorn Andersson wrote:
> > On Mon 11 Oct 05:46 CDT 2021, Arnaud POULIQUEN wrote:
> > 
> >>
> >>
> >> On 10/9/21 1:35 AM, Bjorn Andersson wrote:
> >>> On Mon 12 Jul 05:37 PDT 2021, 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.
> >>>>
> >>>
> >>> As I said in the cover letter, the only reason I can see for doing this
> >>> refactoring is in relation to the introduction of
> >>> RPMSG_CREATE_DEV_IOCTL. So I would like this patch to go together with
> >>> that patch, together with a good motivation why there's merit to
> >>> creating yet another kernel module (and by bind/unbind can't be used).
> >>>
> >>> Perhaps I'm just missing some good usecase related to this?
> >>
> >>
> >>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>> ---
> >>>>  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 | 215 +++++++++++++++++++++++++++++++++++++
> >>>>  5 files changed, 229 insertions(+), 168 deletions(-)
> >>>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> >>>>
> >>> [..]
> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >>> [..]
> >>>> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >>>> -{
> >>> [..]
> >>>> -	dev = &ctrldev->dev;
> >>>> -	device_initialize(dev);
> >>>> -	dev->parent = &rpdev->dev;
> >>>> -	dev->class = rpmsg_class;
> >>> [..]
> >>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >>> [..]
> >>>> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
> >>>> +{
> >>> [..]
> >>>> +	dev = &ctrldev->dev;
> >>>> +	device_initialize(dev);
> >>>> +	dev->parent = &rpdev->dev;
> >>>
> >>> You lost the assignment of dev->class here, which breaks the udev rules
> >>> we use to invoke rpmsgexport to create endpoints and it causes udevadm
> >>> to complain that rpmsg_ctrlN doesn't have a "subsystem".
> >>
> >> We discussed this point with Mathieu, as a first step i kept the class, but that
> >> generated another dependency with the rpmsg_char device while information was
> >> available on the rpmsg bus. The char device and ctrl device should share the
> >> same class. As rpmsg_ctrl is created first it would have to create the class,and
> >> provide an API to rpmsg char
> >>
> > 
> > Perhaps if this is considered a common piece shared between multiple
> > rpmsg modules we can create such class in the rpmsg "core" itself?
> 
> Yes that seems a good alternative
> 
> > 
> >> Please could you details what does means "rpmsg_ctrlN doesn't have a
> >> "subsystem"." What exactly the udev is looking for? could it base it check on
> >> the /dev/rpmsg_ctrl0 or /sys/bus/rpmsg/devices/...?
> >>
> > 
> > If I read the uevent messages correctly they seem to contain a SUBSYTEM=
> > property when the class is provided. But I'm not sure about the reasons
> > for that.
> 
> If it part of the udev requirement, i suppose that it is mandatory, and in this
> case, declare the class in the core make sense.
> 

I don't know if it's a requirement. But I think it's worth keeping the
class around, as it's the only problem I've found with existing users.

> I will send a new patchset that will squash all the remaining patches, taking
> into account your comment.
> 

Thanks,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> Regards,
> >>> Bjorn
> >>>

[PATCH v5 4/4] 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 87a1746367eb..eeb1708548c1 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -179,7 +179,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..d6056f09bcd8 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_ctrldev_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

Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[Bjorn Andersson]

On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:

> Main update from V4 [1] 
>  - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>  - rebased on kernel V.14-rc1.
> 
> This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch
> 
> 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 code related to the control
> and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.

I'm not convinced about the merits for this refactoring, you're creating
yet another kernel module which is fairly tightly coupled with
the rpmsg_char kernel module and the only case I can see where this
would be useful is if you want to be able to create reach
RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_EPT_IOCTL without having to
include the rpmsg_char part in your kernel.

> This split is an intermediate step to extend the controls to allow user applications to
> instantiate rpmsg devices.
>     

Can you give a concrete example of when this would be used?

Per our previous discussions I believe you intend to use this to bind
your rpmsg_tty driver to arbitrary channels in runtime, which to me
sounds like you're reinventing the bind/unbind sysfs attrs.

Regards,
Bjorn

> Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
> and RPMSG_DESTROY_EPT_IOCTL controls.
>   
> The next step should be to add the capability to:
> - instantiate rpmsg_chrdev from the remote side (NS announcement),
> - instantiate rpmsg_chrdev from local user application by introducing the
>   IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
> - send a NS announcement to the remote side on rpmsg_chrdev local instantiation.
> 
> [1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
> [2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
> 
> Arnaud Pouliquen (4):
>   rpmsg: char: Remove useless include
>   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
> 
>  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        | 184 ++-----------------------
>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
>  drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |   8 +-
>  drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
>  9 files changed, 293 insertions(+), 181 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_char.h
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> -- 
> 2.17.1
> 

Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[Arnaud POULIQUEN]

On 10/9/21 1:21 AM, Bjorn Andersson wrote:
> On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
> 
>> Main update from V4 [1] 
>>  - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>  - rebased on kernel V.14-rc1.
>>
>> This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch
>>
>> 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 code related to the control
>> and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.
> 
> I'm not convinced about the merits for this refactoring, you're creating
> yet another kernel module which is fairly tightly coupled with
> the rpmsg_char kernel module and the only case I can see where this
> would be useful is if you want to be able to create reach
> RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_EPT_IOCTL without having to
> include the rpmsg_char part in your kernel.

This is what we discussed during a meeting we had on the rpsmg_tty subject the
July 7, 2020. [1] sump-up what you requested from me before introducing the
rpmsg tty. But we miss-understood your requirement?

This work is the result of our discussion:
- decorrelate the control and stream part of the rpmsg_char to be able to reuse
the control for other rpmsg services such as the rpmsg_tty.
- Add capability to instantiate other rpmsg service from Linux user land
applications.

The correlation between the rpmsg_char and the rpmsg_ctrl is due to the support
of the RPMSG_CREATE_EPT_IOCTL RPMSG_DESTROY_EPT_IOCTL legacy controls for the
QCOM driver.

At the end I guess the rpmsg_ctrl could become, in the future, a channel for
endpoint signaling between processors.

[1] https://lkml.org/lkml/2020/7/15/868

> 
>> This split is an intermediate step to extend the controls to allow user applications to
>> instantiate rpmsg devices.

>>     
> 
> Can you give a concrete example of when this would be used?

Similar to what it is done with the RPMSG_CREATE_EPT_IOCTL but based on the
channel not the endpoint (as the rpmsg_bus virtio is channel based).

For instance we received several issue reports from customer on rpmsg
communication. The reason was that the coprocessor creates an unidirectional
channel to transfer data to the main processor. But nothing works because the
coprocessor doesn't have the remote address until the main processor send a
first message. The workaround is to send a fake message from the Linux to
provide is ept address.
Making this in the other direction allows the Linux application to initiate such
link when it is ready to receive data.

Other examples of usage:
- Create a temporary channel to get for instance logs of the remotre proc
- destroy and re-create some channels on Linux suspend/resume.

As the proposal of exposing the capability to userland to initiate the link (if
i well remember) is coming from you, don't hesitate if you have some extra
uscase that i can add in the cover letter.

> 
> Per our previous discussions I believe you intend to use this to bind
> your rpmsg_tty driver to arbitrary channels in runtime, which to me
> sounds like you're reinventing the bind/unbind sysfs attrs.

Please tell me if I wrong, but the bind /unbind allows to probe/remove an
exiting device. the RPMSG_CREATE_DEV_IOCTL creates a new one on the rpmsg bus,
so not exactly the same use case.

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
>> and RPMSG_DESTROY_EPT_IOCTL controls.
>>   
>> The next step should be to add the capability to:
>> - instantiate rpmsg_chrdev from the remote side (NS announcement),
>> - instantiate rpmsg_chrdev from local user application by introducing the
>>   IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
>> - send a NS announcement to the remote side on rpmsg_chrdev local instantiation.
>>
>> [1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
>> [2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
>>
>> Arnaud Pouliquen (4):
>>   rpmsg: char: Remove useless include
>>   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
>>
>>  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        | 184 ++-----------------------
>>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
>>  drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h    |   8 +-
>>  drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
>>  9 files changed, 293 insertions(+), 181 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_char.h
>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[Bjorn Andersson]

On Mon 11 Oct 03:38 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/9/21 1:21 AM, Bjorn Andersson wrote:
> > On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
> > 
> >> Main update from V4 [1] 
> >>  - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>  - rebased on kernel V.14-rc1.
> >>
> >> This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch
> >>
> >> 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 code related to the control
> >> and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.
> > 
> > I'm not convinced about the merits for this refactoring, you're creating
> > yet another kernel module which is fairly tightly coupled with
> > the rpmsg_char kernel module and the only case I can see where this
> > would be useful is if you want to be able to create reach
> > RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_EPT_IOCTL without having to
> > include the rpmsg_char part in your kernel.
> 
> This is what we discussed during a meeting we had on the rpsmg_tty subject the
> July 7, 2020. [1] sump-up what you requested from me before introducing the
> rpmsg tty. But we miss-understood your requirement?
> 
> This work is the result of our discussion:
> - decorrelate the control and stream part of the rpmsg_char to be able to reuse
> the control for other rpmsg services such as the rpmsg_tty.
> - Add capability to instantiate other rpmsg service from Linux user land
> applications.
> 
> The correlation between the rpmsg_char and the rpmsg_ctrl is due to the support
> of the RPMSG_CREATE_EPT_IOCTL RPMSG_DESTROY_EPT_IOCTL legacy controls for the
> QCOM driver.
> 
> At the end I guess the rpmsg_ctrl could become, in the future, a channel for
> endpoint signaling between processors.
> 
> [1] https://lkml.org/lkml/2020/7/15/868
> 
> > 
> >> This split is an intermediate step to extend the controls to allow user applications to
> >> instantiate rpmsg devices.
> 
> >>     
> > 
> > Can you give a concrete example of when this would be used?
> 
> Similar to what it is done with the RPMSG_CREATE_EPT_IOCTL but based on the
> channel not the endpoint (as the rpmsg_bus virtio is channel based).
> 

I've always seen the rpmsg_endpoint as some form of pipe (with the
special case in virtio rpmsg of it possibly not being connected to
anything) and then the rpmsg_channel being essentially the glue between
a "primary" endpoint and an rpmsg_device.

As such I assumed that it would make sense to do NS announcements of
rpmsg_endpoints in general, not only rpmsg_channels.

> For instance we received several issue reports from customer on rpmsg
> communication. The reason was that the coprocessor creates an unidirectional
> channel to transfer data to the main processor. But nothing works because the
> coprocessor doesn't have the remote address until the main processor send a
> first message. The workaround is to send a fake message from the Linux to
> provide is ept address.
> Making this in the other direction allows the Linux application to initiate such
> link when it is ready to receive data.
> 
> Other examples of usage:
> - Create a temporary channel to get for instance logs of the remotre proc
> - destroy and re-create some channels on Linux suspend/resume.
> 

What's the context these two sets of channels live in? A separate
rpmsg_device or you're having some userspace entity invoke the
create/destroy during suspend and resume?

> As the proposal of exposing the capability to userland to initiate the link (if
> i well remember) is coming from you, don't hesitate if you have some extra
> uscase that i can add in the cover letter.
> 

Right, I remember expressing the need to extend rpmsg_char (somehow) to
make it possible for userspace to initiate the creation of a channel to
the other side.

It's the part where userspace pokes the kernel, so that the kernel goes
and create the rpmsg_device, which magically ends up probing some driver
that I'm wondering about...

> > 
> > Per our previous discussions I believe you intend to use this to bind
> > your rpmsg_tty driver to arbitrary channels in runtime, which to me
> > sounds like you're reinventing the bind/unbind sysfs attrs.
> 
> Please tell me if I wrong, but the bind /unbind allows to probe/remove an
> exiting device. the RPMSG_CREATE_DEV_IOCTL creates a new one on the rpmsg bus,
> so not exactly the same use case.
> 

You're correct, it wouldn't allow you to locally create a new
channel/endpoint and have some driver attached to that.

Regards,
Bjorn

> Regards,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
> >> and RPMSG_DESTROY_EPT_IOCTL controls.
> >>   
> >> The next step should be to add the capability to:
> >> - instantiate rpmsg_chrdev from the remote side (NS announcement),
> >> - instantiate rpmsg_chrdev from local user application by introducing the
> >>   IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
> >> - send a NS announcement to the remote side on rpmsg_chrdev local instantiation.
> >>
> >> [1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
> >> [2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
> >>
> >> Arnaud Pouliquen (4):
> >>   rpmsg: char: Remove useless include
> >>   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
> >>
> >>  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        | 184 ++-----------------------
> >>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
> >>  drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
> >>  drivers/rpmsg/rpmsg_internal.h    |   8 +-
> >>  drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
> >>  9 files changed, 293 insertions(+), 181 deletions(-)
> >>  create mode 100644 drivers/rpmsg/rpmsg_char.h
> >>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> >>
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.

[Arnaud POULIQUEN]

On 10/19/21 5:28 AM, Bjorn Andersson wrote:
> On Mon 11 Oct 03:38 PDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 10/9/21 1:21 AM, Bjorn Andersson wrote:
>>> On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> Main update from V4 [1] 
>>>>  - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>  - rebased on kernel V.14-rc1.
>>>>
>>>> This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch
>>>>
>>>> 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 code related to the control
>>>> and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.
>>>
>>> I'm not convinced about the merits for this refactoring, you're creating
>>> yet another kernel module which is fairly tightly coupled with
>>> the rpmsg_char kernel module and the only case I can see where this
>>> would be useful is if you want to be able to create reach
>>> RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_EPT_IOCTL without having to
>>> include the rpmsg_char part in your kernel.
>>
>> This is what we discussed during a meeting we had on the rpsmg_tty subject the
>> July 7, 2020. [1] sump-up what you requested from me before introducing the
>> rpmsg tty. But we miss-understood your requirement?
>>
>> This work is the result of our discussion:
>> - decorrelate the control and stream part of the rpmsg_char to be able to reuse
>> the control for other rpmsg services such as the rpmsg_tty.
>> - Add capability to instantiate other rpmsg service from Linux user land
>> applications.
>>
>> The correlation between the rpmsg_char and the rpmsg_ctrl is due to the support
>> of the RPMSG_CREATE_EPT_IOCTL RPMSG_DESTROY_EPT_IOCTL legacy controls for the
>> QCOM driver.
>>
>> At the end I guess the rpmsg_ctrl could become, in the future, a channel for
>> endpoint signaling between processors.
>>
>> [1] https://lkml.org/lkml/2020/7/15/868
>>
>>>
>>>> This split is an intermediate step to extend the controls to allow user applications to
>>>> instantiate rpmsg devices.
>>
>>>>     
>>>
>>> Can you give a concrete example of when this would be used?
>>
>> Similar to what it is done with the RPMSG_CREATE_EPT_IOCTL but based on the
>> channel not the endpoint (as the rpmsg_bus virtio is channel based).
>>
> 
> I've always seen the rpmsg_endpoint as some form of pipe (with the
> special case in virtio rpmsg of it possibly not being connected to
> anything) and then the rpmsg_channel being essentially the glue between
> a "primary" endpoint and an rpmsg_device.

Referencing the documentation channel should be more than that:
"Channels are identified by a textual name and have a local ("source") rpmsg
address, and remote ("destination") rpmsg
address."
On the other side when creating an endpoint we provide channel info...
The border between the channel and the endpoint is quite blurred, and can
therefore be interpreted in different ways...

> 
> As such I assumed that it would make sense to do NS announcements of
> rpmsg_endpoints in general, not only rpmsg_channels.
> 
>> For instance we received several issue reports from customer on rpmsg
>> communication. The reason was that the coprocessor creates an unidirectional
>> channel to transfer data to the main processor. But nothing works because the
>> coprocessor doesn't have the remote address until the main processor send a
>> first message. The workaround is to send a fake message from the Linux to
>> provide is ept address.
>> Making this in the other direction allows the Linux application to initiate such
>> link when it is ready to receive data.
>>
>> Other examples of usage:
>> - Create a temporary channel to get for instance logs of the remotre proc
>> - destroy and re-create some channels on Linux suspend/resume.
>>
> 
> What's the context these two sets of channels live in? A separate
> rpmsg_device or you're having some userspace entity invoke the
> create/destroy during suspend and resume?
> 

Exemples are not concrete use cases but a projection of what our (mass market)
customers could do with such services.
I have in mind userland services/applications that would dynamically manage the
interaction with the co-processor. some other examples:
- For telematics control, the main processor could create temporary channels for
diagnostics.
- A low power tools that performs measurements using the coprocessor. The main
processor would be activated only to manage a HUI. When the HUI is enabled, the
main processor would be start a rpmsg service to get data and display it.
- more generally, by destroying a service some processing could be suspended on
coprocessor.

>> As the proposal of exposing the capability to userland to initiate the link (if
>> i well remember) is coming from you, don't hesitate if you have some extra
>> uscase that i can add in the cover letter.
>>
> 
> Right, I remember expressing the need to extend rpmsg_char (somehow) to
> make it possible for userspace to initiate the creation of a channel to
> the other side.
> 
> It's the part where userspace pokes the kernel, so that the kernel goes
> and create the rpmsg_device, which magically ends up probing some driver
> that I'm wondering about...

The RPMSG_CREATE_EPT_IOCTL do quite the same, it creates a device.
The difference is that the  RPMSG_CREATE_DEV_IOCTL create an additional parent
rpmsg device, while the RPMSG_CREATE_EPT_IOCTL attach this device to the rpmsg
chrdev device.

In term of device creation, the RPMSG_CREATE_DEV_IOCTL does nothing else more
than the NS announcement does.

Regards,
Arnaud

> 
>>>
>>> Per our previous discussions I believe you intend to use this to bind
>>> your rpmsg_tty driver to arbitrary channels in runtime, which to me
>>> sounds like you're reinventing the bind/unbind sysfs attrs.
>>
>> Please tell me if I wrong, but the bind /unbind allows to probe/remove an
>> exiting device. the RPMSG_CREATE_DEV_IOCTL creates a new one on the rpmsg bus,
>> so not exactly the same use case.
>>
> 
> You're correct, it wouldn't allow you to locally create a new
> channel/endpoint and have some driver attached to that.
> 
> Regards,
> Bjorn
> 
>> Regards,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
>>>> and RPMSG_DESTROY_EPT_IOCTL controls.
>>>>   
>>>> The next step should be to add the capability to:
>>>> - instantiate rpmsg_chrdev from the remote side (NS announcement),
>>>> - instantiate rpmsg_chrdev from local user application by introducing the
>>>>   IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
>>>> - send a NS announcement to the remote side on rpmsg_chrdev local instantiation.
>>>>
>>>> [1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
>>>> [2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
>>>>
>>>> Arnaud Pouliquen (4):
>>>>   rpmsg: char: Remove useless include
>>>>   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
>>>>
>>>>  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        | 184 ++-----------------------
>>>>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
>>>>  drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
>>>>  drivers/rpmsg/rpmsg_internal.h    |   8 +-
>>>>  drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
>>>>  9 files changed, 293 insertions(+), 181 deletions(-)
>>>>  create mode 100644 drivers/rpmsg/rpmsg_char.h
>>>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>>>
>>>> -- 
>>>> 2.17.1
>>>>