[PATCH v6 00/10] Restructure the rpmsg_char driver and introduce rpmsg_ctrl driver.

[PATCH v6 00/10] Restructure the rpmsg_char driver and introduce rpmsg_ctrl driver.

[Arnaud Pouliquen]

Main update from V5 [1] 
This series is a re-merging of 3 series in one (I increased patchset version based on the first one):

- Restructure the rpmsg char to decorrelate the control part (v5) [1].
- Introduce a generic IOCTL interface for RPMsg channels management (V4) [2].
- rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls (V3) [3]

With update based on Bjorn Andersson comments:
 - comment the use of the IS_REACHABLE macro
 - Migrate the creation of the rpmsg class from the rpmsg_char.c to rpmsg_core.c
 - refactor the rpmsg_chrdev_eptdev_create in two sub function to address potential race
   condition reported by Bjorn in rpmsg_chrdev_probe[4].

And a new patch to fix ns announcement on default endpoint creation.

Patchset description:

The current rpmsg_char module implements a /dev/rpmsg_ctrl interface that provides the ability to
instantiate char devices (/dev/rpmsgX) associated with an rpmsg endpoint for communication with the
remote processor.
This implementation fit with QCOM rpmsg backend but not with themagement by chanel implemented in 
the generic rpmsg virtio backend.
This series restructures the rpmsg_char driver to decorrelate the control part from the data part
in order to improve its compatible with the rpmsg virtio backend.

Objective:
- Expose a /dev/rpmsg_ctrlX interface for the application that is no longer dedicated to the
  rpmsg_char but generalized to all rpmsg services. This offers capability to create and destroy
  rpmsg channels from a user's application initiative (using the new RPMSG_CREATE_DEV_IOCTL and
  RPMSG_DESTROY_DEV_IOCTL controls).
  An application will be able to create/establish an rpmsg communication channel to communicate
  with the remote processor, and not only wait the remote processor initiative.
  This is interesting for example to establish a temporary communication link for diagnosis,
  calibration, debugging... or instantiate  new data flows on some user actions.
- Add capability to probe the rpmsg_char device at the initiative of the remote processor
 (rpmsg service announcement mechanism).
  This allows platforms based on the rpmsg virtio backend to create the /dev/rpmgX interface with
  a rpmsg name service announcement.

Subsets:
  - Extract the control part of the char dev and create the rpmsg_ctrl.c file (patches 1 to 4)
  - Introduce the "rpmsg-raw" channel in rpmsg_char(patches 5 to 8)
  - Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate RPMsg
    devices (patch 9)
    The application can then create or release a channel by specifying:
       - the name service of the device to instantiate.   
       - the source address.
       - the destination address.
  - Send a ns announcement to the remote processor on default endpoint creation (patche 10)

This series can be applied and tested on git/remoteproc/linux.git[5] for-next branch (6ee5808de074).

[1] https://lore.kernel.org/all/20210712123752.10449-1-arnaud.pouliquen@foss.st.com/
[2] https://lore.kernel.org/all/20210217132905.1485-1-arnaud.pouliquen@foss.st.com/
[3] https://lore.kernel.org/all/20210712132303.25058-1-arnaud.pouliquen@foss.st.com/
[4] https://lkml.org/lkml/2021/10/8/1158
[5] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git 

Arnaud Pouliquen (10):
  rpmsg: char: Export eptdev create an destroy functions
  rpmsg: create the rpmsg class in core instead of in rpmsg char
  rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl
  rpmsg: Update rpmsg_chrdev_register_device function
  rpmsg: char: Refactor rpmsg_chrdev_eptdev_create function
  rpmsg: Introduce rpmsg_create_default_ept function
  rpmsg: char: Add possibility to use default endpoint of the rpmsg
    device.
  rpmsg: char: Introduce the "rpmsg-raw" channel
  rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls
  rpmsg: core: send a ns announcement when a default endpoint is created

 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        | 247 +++++++++++-------------------
 drivers/rpmsg/rpmsg_char.h        |  59 +++++++
 drivers/rpmsg/rpmsg_core.c        |  88 ++++++++++-
 drivers/rpmsg/rpmsg_ctrl.c        | 245 +++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |   8 +-
 drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
 include/linux/rpmsg.h             |  23 +++
 include/uapi/linux/rpmsg.h        |  10 ++
 12 files changed, 530 insertions(+), 166 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

[PATCH v6 01/10] 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>
---
 update vs previous revision:
  - add comment to explain the IS_REACHABLE usage
  - remove Mathieu Poirier reviewed-by as patch updated
---
 drivers/rpmsg/rpmsg_char.c | 18 +++++++-----
 drivers/rpmsg/rpmsg_char.h | 57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 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..109c2c43005f
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) STMicroelectronics 2021.
+ */
+
+#ifndef __RPMSG_CHRDEV_H__
+#define __RPMSG_CHRDEV_H__
+
+/*
+ * The IS_REACHABLE macro is used here to prevent unresolved symbol error during link,
+ * building with RPMSG_CHAR=m and RPMSG_CTRL=y configuration.
+ * In such case a kernel warning is printed to help develloper to fix the issue.
+ */
+
+#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)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+	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 v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Arnaud Pouliquen]

Migrate the creation of the rpmsg class from the rpmsg_char
to the core that the class is usable by all rpmsg services.

Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 14 ++------------
 drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
 include/linux/rpmsg.h      | 10 ++++++++++
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 941c5c54dd72..327ed739a3a7 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -28,7 +28,6 @@
 #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);
@@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 	init_waitqueue_head(&eptdev->readq);
 
 	device_initialize(dev);
-	dev->class = rpmsg_class;
+	dev->class = rpmsg_get_class();
 	dev->parent = parent;
 	dev->groups = rpmsg_eptdev_groups;
 	dev_set_drvdata(dev, eptdev);
@@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	dev = &ctrldev->dev;
 	device_initialize(dev);
 	dev->parent = &rpdev->dev;
-	dev->class = rpmsg_class;
+	dev->class = rpmsg_get_class();
 
 	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
 	ctrldev->cdev.owner = THIS_MODULE;
@@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
 		return ret;
 	}
 
-	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
-	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 = 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);
 	}
 
@@ -579,7 +570,6 @@ 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);
 }
 module_exit(rpmsg_chrdev_exit);
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 9151836190ce..53162038254d 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,8 @@
 
 #include "rpmsg_internal.h"
 
+static struct class *rpmsg_class;
+
 /**
  * rpmsg_create_channel() - create a new rpmsg channel
  * using its name and address info.
@@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 }
 EXPORT_SYMBOL(rpmsg_poll);
 
+/**
+ * rpmsg_get_class() - get reference to the sysfs rpmsg class
+ *
+ * This function return the pointer to the "rpmsg" class created by the rpmsg core.
+ *
+ * Returns the struct class pointer
+ */
+struct class *rpmsg_get_class(void)
+{
+	return rpmsg_class;
+}
+EXPORT_SYMBOL(rpmsg_get_class);
+
 /**
  * rpmsg_trysend_offchannel() - send a message using explicit src/dst addresses
  * @ept: the rpmsg endpoint
@@ -629,10 +644,17 @@ static int __init rpmsg_init(void)
 {
 	int ret;
 
+	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
+	if (IS_ERR(rpmsg_class)) {
+		pr_err("failed to create rpmsg class\n");
+		return PTR_ERR(rpmsg_class);
+	}
+
 	ret = bus_register(&rpmsg_bus);
-	if (ret)
+	if (ret) {
 		pr_err("failed to register rpmsg bus: %d\n", ret);
-
+		class_destroy(rpmsg_class);
+	}
 	return ret;
 }
 postcore_initcall(rpmsg_init);
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index a8dcf8a9ae88..6fe51549d931 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 			poll_table *wait);
 
+struct class *rpmsg_get_class(void);
+
 #else
 
 static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
@@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
 	return 0;
 }
 
+static inline struct class *rpmsg_get_class(void)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return NULL;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG) */
 
 /* use a macro to avoid include chaining to get THIS_MODULE */
-- 
2.17.1

[PATCH v6 03/10] 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>

---
update vs previous version:
- set the ctrl device class with new rpmsg_get_class API for legacy support
---
 drivers/rpmsg/Kconfig      |   9 ++
 drivers/rpmsg/Makefile     |   1 +
 drivers/rpmsg/rpmsg_char.c | 169 +----------------------------
 drivers/rpmsg/rpmsg_char.h |   2 +
 drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
 5 files changed, 230 insertions(+), 167 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 327ed739a3a7..3607865d7be7 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -25,32 +25,14 @@
 
 #include "rpmsg_char.h"
 
-#define RPMSG_DEV_MAX	(MINORMASK + 1)
-
 static dev_t rpmsg_major;
 
-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
@@ -407,169 +389,22 @@ 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_get_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;
 	}
 
-	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
-	if (ret < 0) {
-		pr_err("rpmsgchr: failed to register rpmsg driver\n");
-		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);
 	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 }
 module_exit(rpmsg_chrdev_exit);
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
index 109c2c43005f..ff1acc42628a 100644
--- a/drivers/rpmsg/rpmsg_char.h
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -12,6 +12,8 @@
  * In such case a kernel warning is printed to help develloper to fix the issue.
  */
 
+#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..1d3c12e5cdcf
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,216 @@
+// 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;
+	dev->class = rpmsg_get_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_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 v6 04/10] 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 3f377a795b33..e6eb7fd126c9 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1715,7 +1715,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 1d3c12e5cdcf..4734ce9d927b 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -180,7 +180,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 5da622eb1c8f..2acec7f37474 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -839,7 +839,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 v6 05/10] rpmsg: char: Refactor rpmsg_chrdev_eptdev_create function

[Arnaud Pouliquen]

Introduce the rpmsg_chrdev_eptdev_alloc and  rpmsg_chrdev_eptdev_add
internal function to split the allocation part from the device add.

This patch prepares the introduction of a rpmsg channel device for the
char device. An default endpoint will be created,
referenced in the rpmsg_eptdev structure before adding the devices.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
delta vs previous revision
- re-split in two functions to avoid race condition  with uevent,
  described by Born here: https://lkml.org/lkml/2021/10/8/1158
- clean Reviewed-by and Tested-by due to non-minor update
---
 drivers/rpmsg/rpmsg_char.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3607865d7be7..74c96a8d33aa 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -322,20 +322,18 @@ 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 struct rpmsg_eptdev *rpmsg_chrdev_eptdev_alloc(struct rpmsg_device *rpdev,
+						      struct device *parent)
 {
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
-	int ret;
 
 	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
 	if (!eptdev)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dev = &eptdev->dev;
 	eptdev->rpdev = rpdev;
-	eptdev->chinfo = chinfo;
 
 	mutex_init(&eptdev->ept_lock);
 	spin_lock_init(&eptdev->queue_lock);
@@ -351,6 +349,16 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
 	eptdev->cdev.owner = THIS_MODULE;
 
+	return eptdev;
+}
+
+static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
+{
+	struct device *dev = &eptdev->dev;
+	int ret;
+
+	eptdev->chinfo = chinfo;
+
 	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
 	if (ret < 0)
 		goto free_eptdev;
@@ -387,6 +395,21 @@ 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)
+{
+	struct rpmsg_eptdev *eptdev;
+	int ret;
+
+	eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, parent);
+	if (IS_ERR(eptdev))
+		return PTR_ERR(eptdev);
+
+	ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+
+	return ret;
+}
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
 static int rpmsg_chrdev_init(void)
-- 
2.17.1

[PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Arnaud Pouliquen]

By providing a callback in the rpmsg_driver structure, the rpmsg devices
can be probed with a default endpoint created.

In this case, it is not possible to associated to this endpoint private data
that could allow the driver to retrieve the context.

This helper function allows rpmsg drivers to create a default endpoint
on runtime with an associated private context.

For example, a driver might create a context structure on the probe and
want to provide that context as private data for the default rpmsg
callback.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Tested-by: Julien Massot <julien.massot@iot.bzh>
---
 drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
 include/linux/rpmsg.h      | 13 ++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 53162038254d..92557c49d460 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);
 
+/**
+ * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
+ * @rpdev: rpmsg channel device
+ * @cb: rx callback handler
+ * @priv: private data for the driver's use
+ * @chinfo: channel_info with the local rpmsg address to bind with @cb
+ *
+ * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
+ * no endpoint is created when the device is probed by the rpmsg bus.
+ *
+ * This function returns a pointer to the default endpoint if already created or creates
+ * an endpoint and assign it as the default endpoint of the rpmsg device.
+ *
+ * Drivers should provide their @rpdev channel (so the new endpoint would belong
+ * to the same remote processor their channel belongs to), an rx callback
+ * function, an optional private data (which is provided back when the
+ * rx callback is invoked), and an address they want to bind with the
+ * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
+ * dynamically assign them an available rpmsg address (drivers should have
+ * a very good reason why not to always use RPMSG_ADDR_ANY here).
+ *
+ * Returns a pointer to the endpoint on success, or NULL on error.
+ */
+struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+						rpmsg_rx_cb_t cb, void *priv,
+						struct rpmsg_channel_info chinfo)
+{
+	struct rpmsg_endpoint *ept;
+
+	if (WARN_ON(!rpdev))
+		return NULL;
+
+	/* It does not make sense to create a default endpoint without a callback. */
+	if (!cb)
+		return NULL;
+
+	if (rpdev->ept)
+		return rpdev->ept;
+
+	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
+	if (!ept)
+		return NULL;
+
+	/* Assign the new endpoint as default endpoint */
+	rpdev->ept = ept;
+	rpdev->src = ept->addr;
+
+	return ept;
+}
+EXPORT_SYMBOL(rpmsg_create_default_ept);
+
 /**
  * rpmsg_send() - send a message across to the remote processor
  * @ept: the rpmsg endpoint
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 6fe51549d931..b071ac17ff78 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
 struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
 					rpmsg_rx_cb_t cb, void *priv,
 					struct rpmsg_channel_info chinfo);
+struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+						rpmsg_rx_cb_t cb, void *priv,
+						struct rpmsg_channel_info chinfo);
 
 int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
 int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
@@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
 	return NULL;
 }
 
+static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+							      rpmsg_rx_cb_t cb, void *priv,
+							      struct rpmsg_channel_info chinfo)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return NULL;
+}
+
 static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
 {
 	/* This shouldn't be possible */
-- 
2.17.1

[PATCH v6 07/10] rpmsg: char: Add possibility to use default endpoint of the rpmsg device.

[Arnaud Pouliquen]

Current implementation create/destroy a new endpoint on each
rpmsg_eptdev_open/rpmsg_eptdev_release calls.

For a rpmsg device created by the NS announcement mechanism we need to
use a unique static endpoint that is the default rpmsg device endpoint
associated to the channel.

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 default_ept field in rpmsg_eptdev structure.This pointer
determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.

- If default_ept == NULL:
  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 default_ept is set:
  use the rpmsg device default endpoint for the communication.

Address the update of _rpmsg_chrdev_eptdev_create in a separate patch for readability.

Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.

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

---
update vs previous version
 - replace static_ept boolean by default_ept endpoint pointer which
   can point to the default channel endpoint.
 - clean Reviewed-by and tested-by as code updated.
---
 drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 74c96a8d33aa..632279cd6c97 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -44,6 +44,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
  * @queue_lock:	synchronization of @queue operations
  * @queue:	incoming message queue
  * @readq:	wait object for incoming queue
+ * @default_ept: set to channel default endpoint if the default endpoint should be re-used
+ *              on device open to prevent endpoint address update.
  */
 struct rpmsg_eptdev {
 	struct device dev;
@@ -54,10 +56,12 @@ struct rpmsg_eptdev {
 
 	struct mutex ept_lock;
 	struct rpmsg_endpoint *ept;
+	struct rpmsg_endpoint *default_ept;
 
 	spinlock_t queue_lock;
 	struct sk_buff_head queue;
 	wait_queue_head_t readq;
+
 };
 
 int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
@@ -115,7 +119,15 @@ 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 the default_ept is set to true, the rpmsg device default endpoint is used.
+	 * Else a new endpoint is created on open that will be destroyed on release.
+	 */
+	if (eptdev->default_ept)
+		ept = eptdev->default_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);
@@ -136,7 +148,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->default_ept)
+			rpmsg_destroy_ept(eptdev->ept);
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
@@ -263,6 +276,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->default_ept)
+		return -EINVAL;
+
 	return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
 }
 
-- 
2.17.1

[PATCH v6 08/10] rpmsg: char: Introduce the "rpmsg-raw" channel

[Arnaud Pouliquen]

Allows to probe the endpoint device on a remote name service announcement,
by registering a rpmsg_driverfor the "rpmsg-raw" channel.

With this patch the /dev/rpmsgX interface can be instantiated by the remote
firmware.

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

---
update from last version:
- rework the probe function to create the default endpoint before
 adding the eptdev.
- clean Reviewed-by and tested-by as code updated.
---
 drivers/rpmsg/rpmsg_char.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 632279cd6c97..aef628c6b57c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -429,6 +429,58 @@ 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;
+	struct rpmsg_eptdev *eptdev;
+	struct device *dev = &rpdev->dev;
+
+	memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
+	chinfo.src = rpdev->src;
+	chinfo.dst = rpdev->dst;
+
+	eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, dev);
+	if (IS_ERR(eptdev))
+		return PTR_ERR(eptdev);
+
+	/*
+	 * Create the default endpoint associated to the rpmsg device and provide rpmsg_eptdev
+	 * structure as callback private data.
+	 * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
+	 * reuse the default endpoint instead
+	 */
+	eptdev->default_ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, chinfo);
+	if (!eptdev->default_ept) {
+		dev_err(&rpdev->dev, "failed to create %s\n", chinfo.name);
+		put_device(dev);
+		kfree(eptdev);
+		return -EINVAL;
+	}
+
+	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+}
+
+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-raw" },
+	{ },
+};
+
+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;
@@ -439,12 +491,24 @@ static int rpmsg_chrdev_init(void)
 		return ret;
 	}
 
+	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+	if (ret < 0) {
+		pr_err("rpmsg: failed to register rpmsg raw driver\n");
+		goto free_region;
+	}
+
 	return 0;
+
+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);
 	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 }
 module_exit(rpmsg_chrdev_exit);
-- 
2.17.1

[PATCH v6 09/10] rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls

[Arnaud Pouliquen]

Allow the user space application to create and release an rpmsg device
by adding RPMSG_CREATE_DEV_IOCTL and RPMSG_RELEASE_DEV_IOCTL ioctrls to
the /dev/rpmsg_ctrl interface

The RPMSG_CREATE_DEV_IOCTL Ioctl can be used to instantiate a local rpmsg
device.
Depending on the back-end implementation, the associated rpmsg driver is
probed and a NS announcement can be sent to the remote processor.

The RPMSG_RELEASE_DEV_IOCTL allows the user application to release a
rpmsg device created either by the remote processor or with the
RPMSG_CREATE_DEV_IOCTL call.
Depending on the back-end implementation, the associated rpmsg driver is
removed and a NS destroy rpmsg can be sent to the remote processor.

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_ctrl.c | 37 +++++++++++++++++++++++++++++++++----
 include/uapi/linux/rpmsg.h | 10 ++++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 4734ce9d927b..b9b925ed2f32 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -23,6 +23,7 @@
 #include <uapi/linux/rpmsg.h>
 
 #include "rpmsg_char.h"
+#include "rpmsg_internal.h"
 
 static dev_t rpmsg_major;
 
@@ -37,11 +38,13 @@ static DEFINE_IDA(rpmsg_minor_ida);
  * @rpdev:	underlaying rpmsg device
  * @cdev:	cdev for the ctrl device
  * @dev:	device for the ctrl device
+ * @ctrl_lock:	serialize the ioctrls.
  */
 struct rpmsg_ctrldev {
 	struct rpmsg_device *rpdev;
 	struct cdev cdev;
 	struct device dev;
+	struct mutex ctrl_lock;
 };
 
 static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
@@ -70,9 +73,8 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
-
-	if (cmd != RPMSG_CREATE_EPT_IOCTL)
-		return -EINVAL;
+	struct rpmsg_device *rpdev;
+	int ret = 0;
 
 	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
 		return -EFAULT;
@@ -82,7 +84,33 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	chinfo.src = eptinfo.src;
 	chinfo.dst = eptinfo.dst;
 
-	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
+	mutex_lock(&ctrldev->ctrl_lock);
+	switch (cmd) {
+	case RPMSG_CREATE_EPT_IOCTL:
+		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
+		break;
+
+	case RPMSG_CREATE_DEV_IOCTL:
+		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+		if (!rpdev) {
+			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
+			ret = -ENXIO;
+		}
+		break;
+
+	case RPMSG_RELEASE_DEV_IOCTL:
+		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
+		if (ret)
+			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
+				chinfo.name, ret);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&ctrldev->ctrl_lock);
+
+	return ret;
 };
 
 static const struct file_operations rpmsg_ctrldev_fops = {
@@ -120,6 +148,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
 	dev->parent = &rpdev->dev;
 	dev->class = rpmsg_get_class();
 
+	mutex_init(&ctrldev->ctrl_lock);
 	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
 	ctrldev->cdev.owner = THIS_MODULE;
 
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f5ca8740f3fb..1637e68177d9 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -33,4 +33,14 @@ struct rpmsg_endpoint_info {
  */
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
+/**
+ * Instantiate a new local rpmsg service device.
+ */
+#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
+
+/**
+ * Release a local rpmsg device.
+ */
+#define RPMSG_RELEASE_DEV_IOCTL	_IOW(0xb5, 0x4, struct rpmsg_endpoint_info)
+
 #endif
-- 
2.17.1

[PATCH v6 10/10] rpmsg: core: send a ns announcement when a default endpoint is created

[Arnaud Pouliquen]

When a channel is created by user space application with the
RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent
(depending on backend) to inform the remote side that a new service
is available.

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

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 92557c49d460..4c0c605473c7 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -160,6 +160,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
 						struct rpmsg_channel_info chinfo)
 {
 	struct rpmsg_endpoint *ept;
+	int err = 0;
 
 	if (WARN_ON(!rpdev))
 		return NULL;
@@ -179,6 +180,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
 	rpdev->ept = ept;
 	rpdev->src = ept->addr;
 
+	if (rpdev->ops->announce_create)
+		err = rpdev->ops->announce_create(rpdev);
+	if (err) {
+		rpmsg_destroy_ept(ept);
+		rpdev->ept = NULL;
+		rpdev->src = RPMSG_ADDR_ANY;
+
+		return NULL;
+	}
+
 	return ept;
 }
 EXPORT_SYMBOL(rpmsg_create_default_ept);
-- 
2.17.1

Re: [PATCH v6 01/10] rpmsg: char: Export eptdev create an destroy functions

[Bjorn Andersson]

On Fri 22 Oct 07:54 CDT 2021, 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.
> 
> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  update vs previous revision:
>   - add comment to explain the IS_REACHABLE usage
>   - remove Mathieu Poirier reviewed-by as patch updated
> ---
>  drivers/rpmsg/rpmsg_char.c | 18 +++++++-----
>  drivers/rpmsg/rpmsg_char.h | 57 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 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..109c2c43005f
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) STMicroelectronics 2021.
> + */
> +
> +#ifndef __RPMSG_CHRDEV_H__
> +#define __RPMSG_CHRDEV_H__
> +
> +/*
> + * The IS_REACHABLE macro is used here to prevent unresolved symbol error during link,
> + * building with RPMSG_CHAR=m and RPMSG_CTRL=y configuration.
> + * In such case a kernel warning is printed to help develloper to fix the issue.
> + */

I think we concluded that RPMSG_CHAR=n, RPMSG_CTRL=y is a valid
combination. If so I presume you don't want the user to get the kernel
log spammed by the WARN_ON()?

Afaict this should only ever be invoked by rpmsg_ctrl and with
RPMSG_CHAR disabled the related ioctls should just fail nicely.


As such. I think this should be #if IS_ENABLED() and we should put:

	depends on RPMSG_CHAR || RPMSG_CHAR=n

in the RPMSG_CTRL Kconfig.

Regards,
Bjorn

> +
> +#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)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +	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 v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Bjorn Andersson]

On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:

> Migrate the creation of the rpmsg class from the rpmsg_char
> to the core that the class is usable by all rpmsg services.
> 
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 14 ++------------
>  drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
>  include/linux/rpmsg.h      | 10 ++++++++++
>  3 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 941c5c54dd72..327ed739a3a7 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -28,7 +28,6 @@
>  #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);
> @@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>  	init_waitqueue_head(&eptdev->readq);
>  
>  	device_initialize(dev);
> -	dev->class = rpmsg_class;
> +	dev->class = rpmsg_get_class();
>  	dev->parent = parent;
>  	dev->groups = rpmsg_eptdev_groups;
>  	dev_set_drvdata(dev, eptdev);
> @@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	dev = &ctrldev->dev;
>  	device_initialize(dev);
>  	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;
> +	dev->class = rpmsg_get_class();
>  
>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>  	ctrldev->cdev.owner = THIS_MODULE;
> @@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
>  		return ret;
>  	}
>  
> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> -	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 = 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);
>  	}
>  
> @@ -579,7 +570,6 @@ 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);
>  }
>  module_exit(rpmsg_chrdev_exit);
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 9151836190ce..53162038254d 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,8 @@
>  
>  #include "rpmsg_internal.h"
>  
> +static struct class *rpmsg_class;
> +
>  /**
>   * rpmsg_create_channel() - create a new rpmsg channel
>   * using its name and address info.
> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>  }
>  EXPORT_SYMBOL(rpmsg_poll);
>  
> +/**
> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
> + *
> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
> + *
> + * Returns the struct class pointer
> + */
> +struct class *rpmsg_get_class(void)

What value does this helper function add? Can't we just expose
rpmsg_class directly?

> +{
> +	return rpmsg_class;
> +}
> +EXPORT_SYMBOL(rpmsg_get_class);
> +
>  /**
>   * rpmsg_trysend_offchannel() - send a message using explicit src/dst addresses
>   * @ept: the rpmsg endpoint
> @@ -629,10 +644,17 @@ static int __init rpmsg_init(void)
>  {
>  	int ret;
>  
> +	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> +	if (IS_ERR(rpmsg_class)) {
> +		pr_err("failed to create rpmsg class\n");
> +		return PTR_ERR(rpmsg_class);
> +	}
> +
>  	ret = bus_register(&rpmsg_bus);
> -	if (ret)
> +	if (ret) {
>  		pr_err("failed to register rpmsg bus: %d\n", ret);
> -
> +		class_destroy(rpmsg_class);
> +	}
>  	return ret;
>  }
>  postcore_initcall(rpmsg_init);
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h

Isn't this just going to be used by rpmsg_char and rpmsg_ctrl? Do we
really need to expose it in the client-facing API?

Regards,
Bjorn

> index a8dcf8a9ae88..6fe51549d931 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>  			poll_table *wait);
>  
> +struct class *rpmsg_get_class(void);
> +
>  #else
>  
>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>  	return 0;
>  }
>  
> +static inline struct class *rpmsg_get_class(void)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return NULL;
> +}
> +
>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>  
>  /* use a macro to avoid include chaining to get THIS_MODULE */
> -- 
> 2.17.1
> 

Re: [PATCH v6 03/10] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Bjorn Andersson]

On Fri 22 Oct 07:54 CDT 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.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> ---
> update vs previous version:
> - set the ctrl device class with new rpmsg_get_class API for legacy support
> ---
>  drivers/rpmsg/Kconfig      |   9 ++
>  drivers/rpmsg/Makefile     |   1 +
>  drivers/rpmsg/rpmsg_char.c | 169 +----------------------------
>  drivers/rpmsg/rpmsg_char.h |   2 +
>  drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 230 insertions(+), 167 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

We don't want select of user selectable config options.

>  	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

I still don't like the introduction of more Kconfig options - search the
list for the number of patches that has corrected Kconfig dependency
issues.

That said, if you get it right...

> +	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/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
[..]
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> index 109c2c43005f..ff1acc42628a 100644
> --- a/drivers/rpmsg/rpmsg_char.h
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -12,6 +12,8 @@
>   * In such case a kernel warning is printed to help develloper to fix the issue.
>   */
>  
> +#define RPMSG_DEV_MAX	(MINORMASK + 1)

This was used to define the minors of the rpmsg chdev, now you're
splitting that range in one for the ctrl and one for the char.

Moving this define to a common place gives an impression that there's a
relationship between the two, but I don't see any. So I think you should
duplicate this in the two files - just like the other stuff.

> +
>  #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..1d3c12e5cdcf
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021, STMicroelectronics

Did you actually change anything that warrant the explicit copyright
claim?

> + * 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.
> + */
[..]
> +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;
> +	dev->class = rpmsg_get_class();

Thank you.

Regards,
Bjorn

Re: [PATCH v6 04/10] rpmsg: Update rpmsg_chrdev_register_device function

[Bjorn Andersson]

On Fri 22 Oct 07:54 CDT 2021, 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>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  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 3f377a795b33..e6eb7fd126c9 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1715,7 +1715,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 1d3c12e5cdcf..4734ce9d927b 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -180,7 +180,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 5da622eb1c8f..2acec7f37474 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -839,7 +839,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 v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Bjorn Andersson]

On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:

> By providing a callback in the rpmsg_driver structure, the rpmsg devices
> can be probed with a default endpoint created.
> 
> In this case, it is not possible to associated to this endpoint private data
> that could allow the driver to retrieve the context.
> 
> This helper function allows rpmsg drivers to create a default endpoint
> on runtime with an associated private context.
> 
> For example, a driver might create a context structure on the probe and
> want to provide that context as private data for the default rpmsg
> callback.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Tested-by: Julien Massot <julien.massot@iot.bzh>
> ---
>  drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
>  include/linux/rpmsg.h      | 13 ++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 53162038254d..92557c49d460 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>  }
>  EXPORT_SYMBOL(rpmsg_destroy_ept);
>  
> +/**
> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
> + * @rpdev: rpmsg channel device
> + * @cb: rx callback handler
> + * @priv: private data for the driver's use
> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
> + *
> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
> + * no endpoint is created when the device is probed by the rpmsg bus.
> + *
> + * This function returns a pointer to the default endpoint if already created or creates
> + * an endpoint and assign it as the default endpoint of the rpmsg device.

But if the driver didn't specify a callback, when would this ever
happen?

> + *
> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
> + * to the same remote processor their channel belongs to), an rx callback
> + * function, an optional private data (which is provided back when the
> + * rx callback is invoked), and an address they want to bind with the
> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
> + * dynamically assign them an available rpmsg address (drivers should have
> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
> + *
> + * Returns a pointer to the endpoint on success, or NULL on error.

Correct kerneldoc is "Return: ..."

> + */
> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> +						rpmsg_rx_cb_t cb, void *priv,
> +						struct rpmsg_channel_info chinfo)
> +{
> +	struct rpmsg_endpoint *ept;
> +
> +	if (WARN_ON(!rpdev))
> +		return NULL;
> +
> +	/* It does not make sense to create a default endpoint without a callback. */
> +	if (!cb)
> +		return NULL;
> +
> +	if (rpdev->ept)
> +		return rpdev->ept;

How does the caller know if they should call rpmsg_destroy_ept() on the
returned ept or not?

> +
> +	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
> +	if (!ept)
> +		return NULL;
> +
> +	/* Assign the new endpoint as default endpoint */
> +	rpdev->ept = ept;
> +	rpdev->src = ept->addr;
> +
> +	return ept;
> +}
> +EXPORT_SYMBOL(rpmsg_create_default_ept);
> +
>  /**
>   * rpmsg_send() - send a message across to the remote processor
>   * @ept: the rpmsg endpoint
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 6fe51549d931..b071ac17ff78 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
>  					rpmsg_rx_cb_t cb, void *priv,
>  					struct rpmsg_channel_info chinfo);
> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,

Is there ever a case where someone outside drivers/rpmsg/ should call
this function?

Regards,
Bjorn

> +						rpmsg_rx_cb_t cb, void *priv,
> +						struct rpmsg_channel_info chinfo);
>  
>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
> @@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
>  	return NULL;
>  }
>  
> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> +							      rpmsg_rx_cb_t cb, void *priv,
> +							      struct rpmsg_channel_info chinfo)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return NULL;
> +}
> +
>  static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
>  {
>  	/* This shouldn't be possible */
> -- 
> 2.17.1
> 

Re: [PATCH v6 01/10] rpmsg: char: Export eptdev create an destroy functions

[Arnaud POULIQUEN]

Hi Bjorn,

On 11/1/21 5:55 PM, Bjorn Andersson wrote:
> On Fri 22 Oct 07:54 CDT 2021, 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.
>>
>> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  update vs previous revision:
>>   - add comment to explain the IS_REACHABLE usage
>>   - remove Mathieu Poirier reviewed-by as patch updated
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 18 +++++++-----
>>  drivers/rpmsg/rpmsg_char.h | 57 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 68 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..109c2c43005f
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_char.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (C) STMicroelectronics 2021.
>> + */
>> +
>> +#ifndef __RPMSG_CHRDEV_H__
>> +#define __RPMSG_CHRDEV_H__
>> +
>> +/*
>> + * The IS_REACHABLE macro is used here to prevent unresolved symbol error during link,
>> + * building with RPMSG_CHAR=m and RPMSG_CTRL=y configuration.
>> + * In such case a kernel warning is printed to help develloper to fix the issue.
>> + */
> 
> I think we concluded that RPMSG_CHAR=n, RPMSG_CTRL=y is a valid
> combination. If so I presume you don't want the user to get the kernel
> log spammed by the WARN_ON()?
> 
> Afaict this should only ever be invoked by rpmsg_ctrl and with
> RPMSG_CHAR disabled the related ioctls should just fail nicely.
> 
> 
> As such. I think this should be #if IS_ENABLED() and we should put:
> 
> 	depends on RPMSG_CHAR || RPMSG_CHAR=n
> 
> in the RPMSG_CTRL Kconfig.

Need to make more tests but that seems to me a very good way to solve the
dependency.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +
>> +#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)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +	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 v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Arnaud POULIQUEN]

On 11/1/21 5:57 PM, Bjorn Andersson wrote:
> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
> 
>> Migrate the creation of the rpmsg class from the rpmsg_char
>> to the core that the class is usable by all rpmsg services.
>>
>> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 14 ++------------
>>  drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
>>  include/linux/rpmsg.h      | 10 ++++++++++
>>  3 files changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 941c5c54dd72..327ed739a3a7 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -28,7 +28,6 @@
>>  #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);
>> @@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>  	init_waitqueue_head(&eptdev->readq);
>>  
>>  	device_initialize(dev);
>> -	dev->class = rpmsg_class;
>> +	dev->class = rpmsg_get_class();
>>  	dev->parent = parent;
>>  	dev->groups = rpmsg_eptdev_groups;
>>  	dev_set_drvdata(dev, eptdev);
>> @@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>  	dev = &ctrldev->dev;
>>  	device_initialize(dev);
>>  	dev->parent = &rpdev->dev;
>> -	dev->class = rpmsg_class;
>> +	dev->class = rpmsg_get_class();
>>  
>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>  	ctrldev->cdev.owner = THIS_MODULE;
>> @@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
>>  		return ret;
>>  	}
>>  
>> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
>> -	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 = 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);
>>  	}
>>  
>> @@ -579,7 +570,6 @@ 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);
>>  }
>>  module_exit(rpmsg_chrdev_exit);
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 9151836190ce..53162038254d 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -20,6 +20,8 @@
>>  
>>  #include "rpmsg_internal.h"
>>  
>> +static struct class *rpmsg_class;
>> +
>>  /**
>>   * rpmsg_create_channel() - create a new rpmsg channel
>>   * using its name and address info.
>> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>  }
>>  EXPORT_SYMBOL(rpmsg_poll);
>>  
>> +/**
>> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
>> + *
>> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
>> + *
>> + * Returns the struct class pointer
>> + */
>> +struct class *rpmsg_get_class(void)
> 
> What value does this helper function add? Can't we just expose
> rpmsg_class directly?

look to me cleaner to not expose directly the rpmsg_class in rpmsg.h as this
variable is read only for rpmsg services.

> 
>> +{
>> +	return rpmsg_class;
>> +}
>> +EXPORT_SYMBOL(rpmsg_get_class);
>> +
>>  /**
>>   * rpmsg_trysend_offchannel() - send a message using explicit src/dst addresses
>>   * @ept: the rpmsg endpoint
>> @@ -629,10 +644,17 @@ static int __init rpmsg_init(void)
>>  {
>>  	int ret;
>>  
>> +	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
>> +	if (IS_ERR(rpmsg_class)) {
>> +		pr_err("failed to create rpmsg class\n");
>> +		return PTR_ERR(rpmsg_class);
>> +	}
>> +
>>  	ret = bus_register(&rpmsg_bus);
>> -	if (ret)
>> +	if (ret) {
>>  		pr_err("failed to register rpmsg bus: %d\n", ret);
>> -
>> +		class_destroy(rpmsg_class);
>> +	}
>>  	return ret;
>>  }
>>  postcore_initcall(rpmsg_init);
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> 
> Isn't this just going to be used by rpmsg_char and rpmsg_ctrl? Do we
> really need to expose it in the client-facing API?

I based this dev on hypothesis that the class could be used by some other rpmsg
clients. But it is not mandatory. It can be extended later, on need.

What would you propose as an alternative to this API?

I can see 2 alternatives:
- Define the rpmsg_class in rpmsg_internal.h
  In current patchset rpmsg_char.c does not include the rpmsg_internal.h.
  I'm not sure if this include makes sense for an rpmsg service driver.

- Use "extern struct class *rpmsg_class; " in rpmsg_char and rpmsg_ctrl modules

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> index a8dcf8a9ae88..6fe51549d931 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>  			poll_table *wait);
>>  
>> +struct class *rpmsg_get_class(void);
>> +
>>  #else
>>  
>>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>>  	return 0;
>>  }
>>  
>> +static inline struct class *rpmsg_get_class(void)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return NULL;
>> +}
>> +
>>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>>  
>>  /* use a macro to avoid include chaining to get THIS_MODULE */
>> -- 
>> 2.17.1
>>

Re: [PATCH v6 03/10] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud POULIQUEN]

On 11/1/21 6:07 PM, Bjorn Andersson wrote:
> On Fri 22 Oct 07:54 CDT 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.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> ---
>> update vs previous version:
>> - set the ctrl device class with new rpmsg_get_class API for legacy support
>> ---
>>  drivers/rpmsg/Kconfig      |   9 ++
>>  drivers/rpmsg/Makefile     |   1 +
>>  drivers/rpmsg/rpmsg_char.c | 169 +----------------------------
>>  drivers/rpmsg/rpmsg_char.h |   2 +
>>  drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
>>  5 files changed, 230 insertions(+), 167 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
> 
> We don't want select of user selectable config options.
> 
>>  	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
> 
> I still don't like the introduction of more Kconfig options - search the
> list for the number of patches that has corrected Kconfig dependency
> issues.
> 
> That said, if you get it right...
> 
>> +	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/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> [..]
>> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
>> index 109c2c43005f..ff1acc42628a 100644
>> --- a/drivers/rpmsg/rpmsg_char.h
>> +++ b/drivers/rpmsg/rpmsg_char.h
>> @@ -12,6 +12,8 @@
>>   * In such case a kernel warning is printed to help develloper to fix the issue.
>>   */
>>  
>> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> 
> This was used to define the minors of the rpmsg chdev, now you're
> splitting that range in one for the ctrl and one for the char.
> 
> Moving this define to a common place gives an impression that there's a
> relationship between the two, but I don't see any. So I think you should
> duplicate this in the two files - just like the other stuff.

That's true, I will fix this

> 
>> +
>>  #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..1d3c12e5cdcf
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2021, STMicroelectronics
> 
> Did you actually change anything that warrant the explicit copyright
> claim?
> 

Mainly because i created a new file here, but right it is a copy-past, so
I will remove it.

Thanks,
Arnaud

>> + * 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.
>> + */
> [..]
>> +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;
>> +	dev->class = rpmsg_get_class();
> 
> Thank you.
> 
> Regards,
> Bjorn
> 

Re: [PATCH v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Bjorn Andersson]

On Tue 02 Nov 08:23 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 11/1/21 5:57 PM, Bjorn Andersson wrote:
> > On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
> > 
> >> Migrate the creation of the rpmsg class from the rpmsg_char
> >> to the core that the class is usable by all rpmsg services.
> >>
> >> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 14 ++------------
> >>  drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
> >>  include/linux/rpmsg.h      | 10 ++++++++++
> >>  3 files changed, 36 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 941c5c54dd72..327ed739a3a7 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -28,7 +28,6 @@
> >>  #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);
> >> @@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
> >>  	init_waitqueue_head(&eptdev->readq);
> >>  
> >>  	device_initialize(dev);
> >> -	dev->class = rpmsg_class;
> >> +	dev->class = rpmsg_get_class();
> >>  	dev->parent = parent;
> >>  	dev->groups = rpmsg_eptdev_groups;
> >>  	dev_set_drvdata(dev, eptdev);
> >> @@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >>  	dev = &ctrldev->dev;
> >>  	device_initialize(dev);
> >>  	dev->parent = &rpdev->dev;
> >> -	dev->class = rpmsg_class;
> >> +	dev->class = rpmsg_get_class();
> >>  
> >>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> >>  	ctrldev->cdev.owner = THIS_MODULE;
> >> @@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
> >>  		return ret;
> >>  	}
> >>  
> >> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> >> -	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 = 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);
> >>  	}
> >>  
> >> @@ -579,7 +570,6 @@ 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);
> >>  }
> >>  module_exit(rpmsg_chrdev_exit);
> >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> >> index 9151836190ce..53162038254d 100644
> >> --- a/drivers/rpmsg/rpmsg_core.c
> >> +++ b/drivers/rpmsg/rpmsg_core.c
> >> @@ -20,6 +20,8 @@
> >>  
> >>  #include "rpmsg_internal.h"
> >>  
> >> +static struct class *rpmsg_class;
> >> +
> >>  /**
> >>   * rpmsg_create_channel() - create a new rpmsg channel
> >>   * using its name and address info.
> >> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_poll);
> >>  
> >> +/**
> >> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
> >> + *
> >> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
> >> + *
> >> + * Returns the struct class pointer
> >> + */
> >> +struct class *rpmsg_get_class(void)
> > 
> > What value does this helper function add? Can't we just expose
> > rpmsg_class directly?
> 
> look to me cleaner to not expose directly the rpmsg_class in rpmsg.h as this
> variable is read only for rpmsg services.
> 

The pointer is read only, but the object isn't. So I think it's cleaner
to just share the pointer in the first place.

But that said, looking at this a little bit more, I don't think there's
any guarantee that class_create() has been executed before
rpmsg_ctrl_probe() is being invoked.

> > 
> >> +{
> >> +	return rpmsg_class;
> >> +}
> >> +EXPORT_SYMBOL(rpmsg_get_class);
[..]
> >> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> > 
> > Isn't this just going to be used by rpmsg_char and rpmsg_ctrl? Do we
> > really need to expose it in the client-facing API?
> 
> I based this dev on hypothesis that the class could be used by some other rpmsg
> clients. But it is not mandatory. It can be extended later, on need.
> 

That's a good hypothesis, it might be useful in other places as well.
But I think it's best to keep it local for now and make an explicit
decision about opening up when that need comes.

> What would you propose as an alternative to this API?
> 
> I can see 2 alternatives:
> - Define the rpmsg_class in rpmsg_internal.h
>   In current patchset rpmsg_char.c does not include the rpmsg_internal.h.
>   I'm not sure if this include makes sense for an rpmsg service driver.
> 

rpmsg_ctrl and rpmsg_char are more tightly coupled to rpmsg than typical
rpmsg drivers, so I think it's better to include rpmsg_internal.h than to
open up the API to the clients.

Thanks,
Bjorn

> - Use "extern struct class *rpmsg_class; " in rpmsg_char and rpmsg_ctrl modules
> 
> Regards,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> index a8dcf8a9ae88..6fe51549d931 100644
> >> --- a/include/linux/rpmsg.h
> >> +++ b/include/linux/rpmsg.h
> >> @@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> >>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
> >>  			poll_table *wait);
> >>  
> >> +struct class *rpmsg_get_class(void);
> >> +
> >>  #else
> >>  
> >>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
> >> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
> >>  	return 0;
> >>  }
> >>  
> >> +static inline struct class *rpmsg_get_class(void)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
> >>  
> >>  /* use a macro to avoid include chaining to get THIS_MODULE */
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Arnaud POULIQUEN]

On 11/1/21 6:37 PM, Bjorn Andersson wrote:
> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
> 
>> By providing a callback in the rpmsg_driver structure, the rpmsg devices
>> can be probed with a default endpoint created.
>>
>> In this case, it is not possible to associated to this endpoint private data
>> that could allow the driver to retrieve the context.
>>
>> This helper function allows rpmsg drivers to create a default endpoint
>> on runtime with an associated private context.
>>
>> For example, a driver might create a context structure on the probe and
>> want to provide that context as private data for the default rpmsg
>> callback.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Tested-by: Julien Massot <julien.massot@iot.bzh>
>> ---
>>  drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/rpmsg.h      | 13 ++++++++++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 53162038254d..92557c49d460 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>  }
>>  EXPORT_SYMBOL(rpmsg_destroy_ept);
>>  
>> +/**
>> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
>> + * @rpdev: rpmsg channel device
>> + * @cb: rx callback handler
>> + * @priv: private data for the driver's use
>> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
>> + *
>> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
>> + * no endpoint is created when the device is probed by the rpmsg bus.
>> + *
>> + * This function returns a pointer to the default endpoint if already created or creates
>> + * an endpoint and assign it as the default endpoint of the rpmsg device.
> 
> But if the driver didn't specify a callback, when would this ever
> happen?

Not sure to understand your point here...
Do you mean that something is missing in description such as:
 * On register_rpmsg_driver if no callback is provided in the rpmsg_driver
 * structure, no endpoint is created when the device is probed by the rpmsg bus.
 * The rpmsg driver can call rpmsg_create_default_ept during or after its
 * probing to register a default endpoint with an associated callback and @priv
 * context.

> 
>> + *
>> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
>> + * to the same remote processor their channel belongs to), an rx callback
>> + * function, an optional private data (which is provided back when the
>> + * rx callback is invoked), and an address they want to bind with the
>> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
>> + * dynamically assign them an available rpmsg address (drivers should have
>> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
>> + *
>> + * Returns a pointer to the endpoint on success, or NULL on error.
> 
> Correct kerneldoc is "Return: ..."

I will update this

> 
>> + */
>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>> +						rpmsg_rx_cb_t cb, void *priv,
>> +						struct rpmsg_channel_info chinfo)
>> +{
>> +	struct rpmsg_endpoint *ept;
>> +
>> +	if (WARN_ON(!rpdev))
>> +		return NULL;
>> +
>> +	/* It does not make sense to create a default endpoint without a callback. */
>> +	if (!cb)
>> +		return NULL;
>> +
>> +	if (rpdev->ept)
>> +		return rpdev->ept;
> 
> How does the caller know if they should call rpmsg_destroy_ept() on the
> returned ept or not?

This case is probably a bug. What about replacing the condition by
if(WARN_ON(rpdev->ept))?

> 
>> +
>> +	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
>> +	if (!ept)
>> +		return NULL;
>> +
>> +	/* Assign the new endpoint as default endpoint */
>> +	rpdev->ept = ept;
>> +	rpdev->src = ept->addr;
>> +
>> +	return ept;
>> +}
>> +EXPORT_SYMBOL(rpmsg_create_default_ept);
>> +
>>  /**
>>   * rpmsg_send() - send a message across to the remote processor
>>   * @ept: the rpmsg endpoint
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index 6fe51549d931..b071ac17ff78 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
>>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
>>  					rpmsg_rx_cb_t cb, void *priv,
>>  					struct rpmsg_channel_info chinfo);
>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> 
> Is there ever a case where someone outside drivers/rpmsg/ should call
> this function?

A rpmsg service driver could call it to generate the ns announcement after
the probe (for instance on a sysfs open).
(Please have a look to [PATCH v6 10/10] rpmsg: core: send a ns announcement when
a default endpoint is created)

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +						rpmsg_rx_cb_t cb, void *priv,
>> +						struct rpmsg_channel_info chinfo);
>>  
>>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
>>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
>> @@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
>>  	return NULL;
>>  }
>>  
>> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>> +							      rpmsg_rx_cb_t cb, void *priv,
>> +							      struct rpmsg_channel_info chinfo)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return NULL;
>> +}
>> +
>>  static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
>>  {
>>  	/* This shouldn't be possible */
>> -- 
>> 2.17.1
>>

Re: [PATCH v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Arnaud POULIQUEN]

On 11/2/21 5:38 PM, Bjorn Andersson wrote:
> On Tue 02 Nov 08:23 PDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 11/1/21 5:57 PM, Bjorn Andersson wrote:
>>> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> Migrate the creation of the rpmsg class from the rpmsg_char
>>>> to the core that the class is usable by all rpmsg services.
>>>>
>>>> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_char.c | 14 ++------------
>>>>  drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
>>>>  include/linux/rpmsg.h      | 10 ++++++++++
>>>>  3 files changed, 36 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index 941c5c54dd72..327ed739a3a7 100644
>>>> --- a/drivers/rpmsg/rpmsg_char.c
>>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>>> @@ -28,7 +28,6 @@
>>>>  #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);
>>>> @@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>>>  	init_waitqueue_head(&eptdev->readq);
>>>>  
>>>>  	device_initialize(dev);
>>>> -	dev->class = rpmsg_class;
>>>> +	dev->class = rpmsg_get_class();
>>>>  	dev->parent = parent;
>>>>  	dev->groups = rpmsg_eptdev_groups;
>>>>  	dev_set_drvdata(dev, eptdev);
>>>> @@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>>  	dev = &ctrldev->dev;
>>>>  	device_initialize(dev);
>>>>  	dev->parent = &rpdev->dev;
>>>> -	dev->class = rpmsg_class;
>>>> +	dev->class = rpmsg_get_class();
>>>>  
>>>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>>>  	ctrldev->cdev.owner = THIS_MODULE;
>>>> @@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
>>>> -	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 = 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);
>>>>  	}
>>>>  
>>>> @@ -579,7 +570,6 @@ 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);
>>>>  }
>>>>  module_exit(rpmsg_chrdev_exit);
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index 9151836190ce..53162038254d 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -20,6 +20,8 @@
>>>>  
>>>>  #include "rpmsg_internal.h"
>>>>  
>>>> +static struct class *rpmsg_class;
>>>> +
>>>>  /**
>>>>   * rpmsg_create_channel() - create a new rpmsg channel
>>>>   * using its name and address info.
>>>> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>>>  }
>>>>  EXPORT_SYMBOL(rpmsg_poll);
>>>>  
>>>> +/**
>>>> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
>>>> + *
>>>> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
>>>> + *
>>>> + * Returns the struct class pointer
>>>> + */
>>>> +struct class *rpmsg_get_class(void)
>>>
>>> What value does this helper function add? Can't we just expose
>>> rpmsg_class directly?
>>
>> look to me cleaner to not expose directly the rpmsg_class in rpmsg.h as this
>> variable is read only for rpmsg services.
>>
> 
> The pointer is read only, but the object isn't. So I think it's cleaner
> to just share the pointer in the first place.
> 
> But that said, looking at this a little bit more, I don't think there's
> any guarantee that class_create() has been executed before
> rpmsg_ctrl_probe() is being invoked.

class_create is called in in the rpmsg_init (rpmsg_core.c). as RPMSG_CTRL and
RPMSG_CHAR depend on RPMSG config this use case should not occurs, or did I miss
something?

> 
>>>
>>>> +{
>>>> +	return rpmsg_class;
>>>> +}
>>>> +EXPORT_SYMBOL(rpmsg_get_class);
> [..]
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>
>>> Isn't this just going to be used by rpmsg_char and rpmsg_ctrl? Do we
>>> really need to expose it in the client-facing API?
>>
>> I based this dev on hypothesis that the class could be used by some other rpmsg
>> clients. But it is not mandatory. It can be extended later, on need.
>>
> 
> That's a good hypothesis, it might be useful in other places as well.
> But I think it's best to keep it local for now and make an explicit
> decision about opening up when that need comes.
> 
>> What would you propose as an alternative to this API?
>>
>> I can see 2 alternatives:
>> - Define the rpmsg_class in rpmsg_internal.h
>>   In current patchset rpmsg_char.c does not include the rpmsg_internal.h.
>>   I'm not sure if this include makes sense for an rpmsg service driver.
>>
> 
> rpmsg_ctrl and rpmsg_char are more tightly coupled to rpmsg than typical
> rpmsg drivers, so I think it's better to include rpmsg_internal.h than to
> open up the API to the clients.

So i will declare the variable in rpmsg_internal.h and remove the
rpmsg_get_class to use directly the variable.

Thanks,
Arnaud

> 
> Thanks,
> Bjorn
> 
>> - Use "extern struct class *rpmsg_class; " in rpmsg_char and rpmsg_ctrl modules
>>
>> Regards,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> index a8dcf8a9ae88..6fe51549d931 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>>>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>>>  			poll_table *wait);
>>>>  
>>>> +struct class *rpmsg_get_class(void);
>>>> +
>>>>  #else
>>>>  
>>>>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>>>> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static inline struct class *rpmsg_get_class(void)
>>>> +{
>>>> +	/* This shouldn't be possible */
>>>> +	WARN_ON(1);
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>>>>  
>>>>  /* use a macro to avoid include chaining to get THIS_MODULE */
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Bjorn Andersson]

On Tue 02 Nov 11:56 CDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 11/1/21 6:37 PM, Bjorn Andersson wrote:
> > On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
> > 
> >> By providing a callback in the rpmsg_driver structure, the rpmsg devices
> >> can be probed with a default endpoint created.
> >>
> >> In this case, it is not possible to associated to this endpoint private data
> >> that could allow the driver to retrieve the context.
> >>
> >> This helper function allows rpmsg drivers to create a default endpoint
> >> on runtime with an associated private context.
> >>
> >> For example, a driver might create a context structure on the probe and
> >> want to provide that context as private data for the default rpmsg
> >> callback.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> Tested-by: Julien Massot <julien.massot@iot.bzh>
> >> ---
> >>  drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
> >>  include/linux/rpmsg.h      | 13 ++++++++++
> >>  2 files changed, 64 insertions(+)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> >> index 53162038254d..92557c49d460 100644
> >> --- a/drivers/rpmsg/rpmsg_core.c
> >> +++ b/drivers/rpmsg/rpmsg_core.c
> >> @@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_destroy_ept);
> >>  
> >> +/**
> >> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
> >> + * @rpdev: rpmsg channel device
> >> + * @cb: rx callback handler
> >> + * @priv: private data for the driver's use
> >> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
> >> + *
> >> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
> >> + * no endpoint is created when the device is probed by the rpmsg bus.
> >> + *
> >> + * This function returns a pointer to the default endpoint if already created or creates
> >> + * an endpoint and assign it as the default endpoint of the rpmsg device.
> > 
> > But if the driver didn't specify a callback, when would this ever
> > happen?
> 
> Not sure to understand your point here...
> Do you mean that something is missing in description such as:
>  * On register_rpmsg_driver if no callback is provided in the rpmsg_driver
>  * structure, no endpoint is created when the device is probed by the rpmsg bus.
>  * The rpmsg driver can call rpmsg_create_default_ept during or after its
>  * probing to register a default endpoint with an associated callback and @priv
>  * context.
> 

I was referring specifically to the case of rpmsg_create_default_ept()
being called on a rpmsg_device that already has a rpdev->ept.

Afaict this would either be because the driver did specify a callback or
because the driver didn't but has already called
rpmsg_create_default_ept().

Both cases sounds like invalid usage.

> > 
> >> + *
> >> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
> >> + * to the same remote processor their channel belongs to), an rx callback
> >> + * function, an optional private data (which is provided back when the
> >> + * rx callback is invoked), and an address they want to bind with the
> >> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
> >> + * dynamically assign them an available rpmsg address (drivers should have
> >> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
> >> + *
> >> + * Returns a pointer to the endpoint on success, or NULL on error.
> > 
> > Correct kerneldoc is "Return: ..."
> 
> I will update this
> 
> > 
> >> + */
> >> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> >> +						rpmsg_rx_cb_t cb, void *priv,
> >> +						struct rpmsg_channel_info chinfo)
> >> +{
> >> +	struct rpmsg_endpoint *ept;
> >> +
> >> +	if (WARN_ON(!rpdev))
> >> +		return NULL;
> >> +
> >> +	/* It does not make sense to create a default endpoint without a callback. */
> >> +	if (!cb)
> >> +		return NULL;
> >> +
> >> +	if (rpdev->ept)
> >> +		return rpdev->ept;
> > 
> > How does the caller know if they should call rpmsg_destroy_ept() on the
> > returned ept or not?
> 
> This case is probably a bug. What about replacing the condition by
> if(WARN_ON(rpdev->ept))?
> 

Right, I don't think it will be possible for the client driver to do the
right thing based on this logic.

> > 
> >> +
> >> +	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
> >> +	if (!ept)
> >> +		return NULL;
> >> +
> >> +	/* Assign the new endpoint as default endpoint */
> >> +	rpdev->ept = ept;
> >> +	rpdev->src = ept->addr;
> >> +
> >> +	return ept;
> >> +}
> >> +EXPORT_SYMBOL(rpmsg_create_default_ept);
> >> +
> >>  /**
> >>   * rpmsg_send() - send a message across to the remote processor
> >>   * @ept: the rpmsg endpoint
> >> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> >> index 6fe51549d931..b071ac17ff78 100644
> >> --- a/include/linux/rpmsg.h
> >> +++ b/include/linux/rpmsg.h
> >> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
> >>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
> >>  					rpmsg_rx_cb_t cb, void *priv,
> >>  					struct rpmsg_channel_info chinfo);
> >> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> > 
> > Is there ever a case where someone outside drivers/rpmsg/ should call
> > this function?
> 
> A rpmsg service driver could call it to generate the ns announcement after
> the probe (for instance on a sysfs open).
> (Please have a look to [PATCH v6 10/10] rpmsg: core: send a ns announcement when
> a default endpoint is created)
> 

I'm still not convinced that it's correct to do NS only for primary
endpoints.

In particular looking down the path where you are instantiating services
on the Linux side; e.g. what if you want your driver to probe on some
control channel but have the actual data flow on a separate channel
(something I believe Loic talked about earlier).

How would the remote side know about that second endpoint if the NS
doesn't announce it?

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> +						rpmsg_rx_cb_t cb, void *priv,
> >> +						struct rpmsg_channel_info chinfo);
> >>  
> >>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
> >>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
> >> @@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
> >>  	return NULL;
> >>  }
> >>  
> >> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> >> +							      rpmsg_rx_cb_t cb, void *priv,
> >> +							      struct rpmsg_channel_info chinfo)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >>  static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> >>  {
> >>  	/* This shouldn't be possible */
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Arnaud POULIQUEN]

On 11/3/21 5:57 PM, Bjorn Andersson wrote:
> On Tue 02 Nov 11:56 CDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 11/1/21 6:37 PM, Bjorn Andersson wrote:
>>> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> By providing a callback in the rpmsg_driver structure, the rpmsg devices
>>>> can be probed with a default endpoint created.
>>>>
>>>> In this case, it is not possible to associated to this endpoint private data
>>>> that could allow the driver to retrieve the context.
>>>>
>>>> This helper function allows rpmsg drivers to create a default endpoint
>>>> on runtime with an associated private context.
>>>>
>>>> For example, a driver might create a context structure on the probe and
>>>> want to provide that context as private data for the default rpmsg
>>>> callback.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Tested-by: Julien Massot <julien.massot@iot.bzh>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/rpmsg.h      | 13 ++++++++++
>>>>  2 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index 53162038254d..92557c49d460 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>>>  }
>>>>  EXPORT_SYMBOL(rpmsg_destroy_ept);
>>>>  
>>>> +/**
>>>> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
>>>> + * @rpdev: rpmsg channel device
>>>> + * @cb: rx callback handler
>>>> + * @priv: private data for the driver's use
>>>> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
>>>> + *
>>>> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
>>>> + * no endpoint is created when the device is probed by the rpmsg bus.
>>>> + *
>>>> + * This function returns a pointer to the default endpoint if already created or creates
>>>> + * an endpoint and assign it as the default endpoint of the rpmsg device.
>>>
>>> But if the driver didn't specify a callback, when would this ever
>>> happen?
>>
>> Not sure to understand your point here...
>> Do you mean that something is missing in description such as:
>>  * On register_rpmsg_driver if no callback is provided in the rpmsg_driver
>>  * structure, no endpoint is created when the device is probed by the rpmsg bus.
>>  * The rpmsg driver can call rpmsg_create_default_ept during or after its
>>  * probing to register a default endpoint with an associated callback and @priv
>>  * context.
>>
> 
> I was referring specifically to the case of rpmsg_create_default_ept()
> being called on a rpmsg_device that already has a rpdev->ept.
> 
> Afaict this would either be because the driver did specify a callback or
> because the driver didn't but has already called
> rpmsg_create_default_ept().
> 
> Both cases sounds like invalid usage.
> 
>>>
>>>> + *
>>>> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
>>>> + * to the same remote processor their channel belongs to), an rx callback
>>>> + * function, an optional private data (which is provided back when the
>>>> + * rx callback is invoked), and an address they want to bind with the
>>>> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
>>>> + * dynamically assign them an available rpmsg address (drivers should have
>>>> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
>>>> + *
>>>> + * Returns a pointer to the endpoint on success, or NULL on error.
>>>
>>> Correct kerneldoc is "Return: ..."
>>
>> I will update this
>>
>>>
>>>> + */
>>>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>> +						rpmsg_rx_cb_t cb, void *priv,
>>>> +						struct rpmsg_channel_info chinfo)
>>>> +{
>>>> +	struct rpmsg_endpoint *ept;
>>>> +
>>>> +	if (WARN_ON(!rpdev))
>>>> +		return NULL;
>>>> +
>>>> +	/* It does not make sense to create a default endpoint without a callback. */
>>>> +	if (!cb)
>>>> +		return NULL;
>>>> +
>>>> +	if (rpdev->ept)
>>>> +		return rpdev->ept;
>>>
>>> How does the caller know if they should call rpmsg_destroy_ept() on the
>>> returned ept or not?
>>
>> This case is probably a bug. What about replacing the condition by
>> if(WARN_ON(rpdev->ept))?
>>
> 
> Right, I don't think it will be possible for the client driver to do the
> right thing based on this logic.
> 
>>>
>>>> +
>>>> +	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
>>>> +	if (!ept)
>>>> +		return NULL;
>>>> +
>>>> +	/* Assign the new endpoint as default endpoint */
>>>> +	rpdev->ept = ept;
>>>> +	rpdev->src = ept->addr;
>>>> +
>>>> +	return ept;
>>>> +}
>>>> +EXPORT_SYMBOL(rpmsg_create_default_ept);
>>>> +
>>>>  /**
>>>>   * rpmsg_send() - send a message across to the remote processor
>>>>   * @ept: the rpmsg endpoint
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index 6fe51549d931..b071ac17ff78 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
>>>>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
>>>>  					rpmsg_rx_cb_t cb, void *priv,
>>>>  					struct rpmsg_channel_info chinfo);
>>>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>
>>> Is there ever a case where someone outside drivers/rpmsg/ should call
>>> this function?
>>
>> A rpmsg service driver could call it to generate the ns announcement after
>> the probe (for instance on a sysfs open).
>> (Please have a look to [PATCH v6 10/10] rpmsg: core: send a ns announcement when
>> a default endpoint is created)
>>
> 
> I'm still not convinced that it's correct to do NS only for primary
> endpoints.
> 
> In particular looking down the path where you are instantiating services
> on the Linux side; e.g. what if you want your driver to probe on some
> control channel but have the actual data flow on a separate channel
> (something I believe Loic talked about earlier).
> 
> How would the remote side know about that second endpoint if the NS
> doesn't announce it?

Right, today it is a limitation of the announcement mechanism. The service is
linked to rpmsg_dev_probe and rpmsg_dev_remove (except if the backend implements
an announcement on create_ept & destroy_ept ops).

The rpmsg_create_default_ept follows this behavior.
As result of this patch, the limitation will exist in both directions.

Another limitation is an ack of the NS announcement to provide the associated
remote address.

Extending rpmsg_ns.c to add new messages could solve these limitations, but that
would be a separate topic.

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> +						rpmsg_rx_cb_t cb, void *priv,
>>>> +						struct rpmsg_channel_info chinfo);
>>>>  
>>>>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
>>>>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
>>>> @@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
>>>>  	return NULL;
>>>>  }
>>>>  
>>>> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>> +							      rpmsg_rx_cb_t cb, void *priv,
>>>> +							      struct rpmsg_channel_info chinfo)
>>>> +{
>>>> +	/* This shouldn't be possible */
>>>> +	WARN_ON(1);
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
>>>>  {
>>>>  	/* This shouldn't be possible */
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Bjorn Andersson]

On Tue 02 Nov 12:11 CDT 2021, Arnaud POULIQUEN wrote:
> On 11/2/21 5:38 PM, Bjorn Andersson wrote:
> > On Tue 02 Nov 08:23 PDT 2021, Arnaud POULIQUEN wrote:
> >> On 11/1/21 5:57 PM, Bjorn Andersson wrote:
> >>> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
[..]
> >>>> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
> >>>>  }
> >>>>  EXPORT_SYMBOL(rpmsg_poll);
> >>>>  
> >>>> +/**
> >>>> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
> >>>> + *
> >>>> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
> >>>> + *
> >>>> + * Returns the struct class pointer
> >>>> + */
> >>>> +struct class *rpmsg_get_class(void)
> >>>
> >>> What value does this helper function add? Can't we just expose
> >>> rpmsg_class directly?
> >>
> >> look to me cleaner to not expose directly the rpmsg_class in rpmsg.h as this
> >> variable is read only for rpmsg services.
> >>
> > 
> > The pointer is read only, but the object isn't. So I think it's cleaner
> > to just share the pointer in the first place.
> > 
> > But that said, looking at this a little bit more, I don't think there's
> > any guarantee that class_create() has been executed before
> > rpmsg_ctrl_probe() is being invoked.
> 
> class_create is called in in the rpmsg_init (rpmsg_core.c). as RPMSG_CTRL and
> RPMSG_CHAR depend on RPMSG config this use case should not occurs, or did I miss
> something?
> 

Specifying "depends on RPMSG" in the Kconfig doesn't directly impact the
load order.

But in expanding my question I realized that rpmsg_ctrl_probe() will
only ever be invoked if rpmsg_ctrldev_init() was able to call
register_rpmsg_drive(), which in turn depends on rpmsg_init() having
initialized rpmsg_bus of the driver_register() will fail.

So we should be good.

Thanks,
Bjorn