[PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[Arnaud Pouliquen]

This series is a restructuring of the RPMsg char driver, to create a generic
RPMsg ioctl interface for all rpmsg services.

The RPMsg char driver provides interfaces that:
- expose a char RPMsg device for communication with the remote processor,
- expose controls interface for applications to create and release endpoints.

The objective of this series is to decorrelate the two interfaces:
  - Provide a char device for a RPMsg raw service in the rpmsg_char that can be
    probed by a RPMsg bus on a ns announcement.
  - Generalize the use of the ioctl for all RPMsg services by creating the
    rpmsg_ctrl, but keep it compatibile with the legacy.

If the V1 create a new rpmsg_raw driver in addition to the rpmsg_ctrl this
version try to reuse the rpmsg_char driver by addapting QCOM GLINK and SMD
drivers.
So a goal of this version is to help to determine the best strategy to move
forward:
  - reuse rpmsg_char.
  - introduce a new driver and keep rpmsg_char as a legacy driver for a while.

Notice that SMD and GLINK patches have to be tested, only build has been tested.

1) RPMsg control driver: rpmsg_ctrl.c
  This driver is based on the control part of the RPMsg_char driver. 
  On probe a /dev/rpmsg_ctrl<X> interface is created to allow to manage the
  channels.
  The principles are the following:
  - The RPMsg service driver registers it's name and the associated service
    using the rpmsg_ctrl_unregister_ctl API. The list of supported services
    is defined in  include/uapi/linux/rpmsg.h and exposed to the
    application thanks to a new field in rpmsg_endpoint_info struct.
  - On the RPMsg bus probe(e.g virtio bus) an rpmsg_ctrl device is
    registered that creates the control interface.
  - The application can then create or release a channel by specifying:
       - the name service
       - the source address.
       - the destination address.
  - The rpmsg_ctrl uses the same interface than the ns announcement to
    create and release the associated channel but using the driver_override
    field to force the service name.
    The  "driver_override" allows to force the name service associated to
    an RPMsg driver, bypassing the rpmsg_device_id based match check.
  - At least for virtio bus, an associated ns announcement is sent to the
    remote side.  

2) rpmsg char driver: rpmsg_char.c
    - The rpmsg class has not been removed. The associated attributes
      are already available in /sys/bus/rpmsg/.
    - The eptdev device is now an RPMsg device probed by a RPMsg bus driver
      (probed only by the ioctl in rpmsg_char driver).

Know current Limitations:
- Tested only with virtio RPMsg bus and for one vdev instance.
- The glink and smd drivers adaptations have not been tested (not able to test).
- To limit commit and not update the IOCT interface some features have been not
  implemented in this first step:
    - the NS announcement as not been updated, it is not possible to create an
      endpoint with a destibnation address set to RPMSG_ADDR_ANY (-1),
    - not possible to destroy the channel,
    - only the "rpmsg-raw" service is supported.

This series can be applied in Bjorn's rpmsg-next branch on top of the
RPMsg_ns series(4c0943255805).

This series can be tested using rpmsgexport tools available here:
https://github.com/andersson/rpmsgexport.
---
new from V1[1]:
- In V1 the rpmsg_char.c was not impacted, a rpmsg_raw.c has been created
  instead.
- IOCTL interface as not been updated (to go by steps).
- smd and glink drivers has been updated to support channels creation and
  release.

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

Arnaud Pouliquen (16):
  rpmsg: introduce RPMsg control driver for channel creation
  rpmsg: add RPMsg control API to register service
  rpmsg: add override field in channel info
  rpmsg: ctrl: implement the ioctl function to create device
  rpmsg: ns: initialize channel info override field
  rpmsg: add helper to register the rpmsg ctrl device
  rpmsg: char: clean up rpmsg class
  rpmsg: char: make char rpmsg a rpmsg device without the control part
  rpmsg: char: register RPMsg raw service to the ioctl interface.
  rpmsg: char: allow only one endpoint per device
  rpmsg: char: check destination address is not null
  rpmsg: virtio: use the driver_override in channel creation ops
  rpmsg: virtio: probe the rpmsg_ctl device
  rpmsg: glink: add create and release rpmsg channel ops
  rpmsg: smd: add create and release rpmsg channel ops
  rpmsg: replace rpmsg_chrdev_register_device use

 drivers/rpmsg/Kconfig             |   8 +
 drivers/rpmsg/Makefile            |   1 +
 drivers/rpmsg/qcom_glink_native.c |  96 +++++++--
 drivers/rpmsg/qcom_smd.c          |  59 +++++-
 drivers/rpmsg/rpmsg_char.c        | 246 ++++++-----------------
 drivers/rpmsg/rpmsg_ctrl.c        | 320 ++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  14 --
 drivers/rpmsg/rpmsg_ns.c          |   1 +
 drivers/rpmsg/virtio_rpmsg_bus.c  |  38 +++-
 include/linux/rpmsg.h             |  40 ++++
 include/uapi/linux/rpmsg.h        |  14 ++
 11 files changed, 606 insertions(+), 231 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

[PATCH v2 01/16] rpmsg: introduce RPMsg control driver for channel creation

[Arnaud Pouliquen]

The RPMsg_ctrl driver is a duplication of the ioctrl part of the
rpmsg_char driver to make generic the ioctl to manage channels by
the userspace applications.

As a first step, this driver just creates the /dev/rpmsg_ctl<x>
( <x> is the instance value). The ioctl is not implemented.

Notice that this driver is associated to a RPMsg device with no endpoint.
Instantiating this device as an RPMsg device allows to retrieve the
associated RPMsg backend.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/Kconfig      |   8 ++
 drivers/rpmsg/Makefile     |   1 +
 drivers/rpmsg/rpmsg_ctrl.c | 208 +++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0b4407abdf13..c9e602016c3b 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -23,6 +23,14 @@ config RPMSG_NS
 	  channel that probes the associated RPMsg device on remote endpoint
 	  service announcement.
 
+config RPMSG_CTRL
+	tristate "RPMSG control interface"
+	depends on RPMSG
+	help
+	  Say Y here to enable the support of the /dev/rpmsg_ctl API. this API
+	  allows user-space programs to create channels with specific name,
+	  source and destination addresses.
+
 config RPMSG_MTK_SCP
 	tristate "MediaTek SCP"
 	depends on MTK_SCP
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..3c1bce9d0228 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
 obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
+obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
 obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
 qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
new file mode 100644
index 000000000000..425c3e32ada4
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2020
+ */
+
+#include <linux/cdev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "rpmsg_internal.h"
+#include <uapi/linux/rpmsg.h>
+
+#define RPMSG_DEV_MAX	(MINORMASK + 1)
+
+#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl_dev, dev)
+#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl_dev, cdev)
+
+/**
+ * struct rpmsg_ctrl_dev - control device for instantiating endpoint devices
+ * @rpdev:	underlaying rpmsg device
+ * @cdev:	cdev for the ctrl device
+ * @dev:	device for the ctrl device
+ */
+struct rpmsg_ctrl_dev {
+	struct rpmsg_device *rpdev;
+	struct cdev cdev;
+	struct device dev;
+};
+
+static DEFINE_IDA(rpmsg_ctrl_ida);
+static DEFINE_IDA(rpmsg_minor_ida);
+
+static dev_t rpmsg_major;
+
+static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	get_device(&ctrldev->dev);
+	filp->private_data = ctrldev;
+
+	return 0;
+}
+
+static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
+
+	dev_info(&ctrldev->dev, "Control not yet implemented\n");
+
+	return 0;
+};
+
+static int rpmsg_ctrl_dev_release(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	put_device(&ctrldev->dev);
+
+	return 0;
+}
+
+static void rpmsg_ctrl_dev_release_device(struct device *dev)
+{
+	struct rpmsg_ctrl_dev *ctrldev = dev_to_ctrldev(dev);
+
+	dev_err(dev, "%s\n", __func__);
+
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+	cdev_del(&ctrldev->cdev);
+	kfree(ctrldev);
+}
+
+static const struct file_operations rpmsg_ctrl_fops = {
+	.owner = THIS_MODULE,
+	.open = rpmsg_ctrl_dev_open,
+	.release = rpmsg_ctrl_dev_release,
+	.unlocked_ioctl = rpmsg_ctrl_dev_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+static int rpmsg_ctrl_add_control(struct rpmsg_ctrl_dev *ctrldev)
+{
+	struct device *dev = &ctrldev->dev;
+	int ret;
+
+	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
+	ctrldev->cdev.owner = THIS_MODULE;
+
+	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
+	dev_set_name(dev, "rpmsg_ctrl%d", dev->id);
+
+	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
+	if (ret)
+		goto free_minor_ida;
+
+	dev_info(dev, "add %s control for %s driver\n",
+		 dev_name(dev),  dev_name(dev->parent));
+
+	return 0;
+
+free_minor_ida:
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+
+	return ret;
+}
+
+static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrl_dev *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;
+
+	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto free_ctrldev;
+
+	dev->id = ret;
+
+	ret = rpmsg_ctrl_add_control(ctrldev);
+	if (ret < 0)
+		goto free_ctrl_ida;
+
+	/* We can now rely on the release function for cleanup */
+	dev->release = rpmsg_ctrl_dev_release_device;
+
+	ret = device_add(dev);
+	if (ret) {
+		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
+		put_device(dev);
+		goto free_ctrl_ida;
+	}
+
+	dev_set_drvdata(dev, ctrldev);
+	dev_set_drvdata(&rpdev->dev, ctrldev);
+
+	return 0;
+
+free_ctrl_ida:
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+free_ctrldev:
+	put_device(dev);
+	kfree(ctrldev);
+
+	return ret;
+}
+
+static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrl_dev *ctrldev = dev_get_drvdata(&rpdev->dev);
+
+	device_del(&ctrldev->dev);
+	put_device(&ctrldev->dev);
+}
+
+static struct rpmsg_driver rpmsg_ctrl_driver = {
+	.drv.name = KBUILD_MODNAME,
+	.probe = rpmsg_ctrl_probe,
+	.remove = rpmsg_ctrl_remove,
+};
+
+static int rpmsg_ctrl_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
+	if (ret < 0) {
+		pr_err("rpmsg_ctrl: failed to allocate char dev region\n");
+		return ret;
+	}
+
+	ret = register_rpmsg_driver(&rpmsg_ctrl_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_ctrl_init);
+
+static void rpmsg_ctrl_exit(void)
+{
+	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+}
+module_exit(rpmsg_ctrl_exit);
+
+MODULE_DESCRIPTION("ioctl rpmsg driver");
+MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v2 02/16] rpmsg: add RPMsg control API to register service

[Arnaud Pouliquen]

Add API to register a RPMsg service to the control device.
The rpmsg_drv_ctrl_info structure links a service to its driver.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 57 ++++++++++++++++++++++++++++++++++++++
 include/linux/rpmsg.h      | 31 +++++++++++++++++++++
 include/uapi/linux/rpmsg.h | 14 ++++++++++
 3 files changed, 102 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 425c3e32ada4..065e2e304019 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -27,6 +27,20 @@ struct rpmsg_ctrl_dev {
 	struct device dev;
 };
 
+/**
+ * struct rpmsg_ctl_info - control info list node
+ * @ctrl:	control driver info
+ * @node:	list node
+ *
+ * This structure is used by rpmsg_ctl to list the registered drivers services
+ */
+struct rpmsg_ctl_info {
+	const struct rpmsg_drv_ctrl_info *ctrl;
+	struct list_head node;
+};
+
+static LIST_HEAD(rpmsg_drv_list);
+
 static DEFINE_IDA(rpmsg_ctrl_ida);
 static DEFINE_IDA(rpmsg_minor_ida);
 
@@ -175,6 +189,49 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
 	.remove = rpmsg_ctrl_remove,
 };
 
+/**
+ * rpmsg_ctrl_register_ctl() -register control for the associated service
+ * @ctrl: rpmsg driver information
+ *
+ * This function is called by the rpmsg driver to register a service that will
+ * be exposed to be instantiate by the application.
+ */
+int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
+{
+	struct rpmsg_ctl_info *drv_info;
+
+	drv_info =  kzalloc(sizeof(*drv_info), GFP_KERNEL);
+	if (!drv_info)
+		return -ENOMEM;
+
+	drv_info->ctrl = ctrl;
+
+	list_add_tail(&drv_info->node, &rpmsg_drv_list);
+
+	return 0;
+}
+EXPORT_SYMBOL(rpmsg_ctrl_register_ctl);
+
+/**
+ * rpmsg_ctrl_unregister_ctl() -unregister control for the associated service
+ * @ctrl: the rpmsg control information
+ *
+ * This function is called by the rpmsg driver to unregister the associated
+ * service.
+ */
+void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
+{
+	struct rpmsg_ctl_info *drv_info, *tmp;
+
+	list_for_each_entry_safe(drv_info, tmp, &rpmsg_drv_list, node) {
+		if (drv_info->ctrl == ctrl) {
+			list_del(&drv_info->node);
+			kfree(drv_info);
+		}
+	}
+}
+EXPORT_SYMBOL(rpmsg_ctrl_unregister_ctl);
+
 static int rpmsg_ctrl_init(void)
 {
 	int ret;
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index a5db828b2420..5d64704c2346 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -26,6 +26,19 @@ struct rpmsg_endpoint;
 struct rpmsg_device_ops;
 struct rpmsg_endpoint_ops;
 
+/**
+ * struct rpmsg_drv_ctrl_info - rpmsg ctrl structure
+ * @drv_name:	name of the associated driver
+ * @service:	the associated rpmsg service listed in @rpmsg_services
+ *
+ * This structure is used by rpmsg_ctl to link the endpoint creation to the
+ * service rpmsg driver.
+ */
+struct rpmsg_drv_ctrl_info {
+	const char *drv_name;
+	u32  service;
+};
+
 /**
  * struct rpmsg_channel_info - channel info representation
  * @name: name of service
@@ -315,4 +328,22 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
 	module_driver(__rpmsg_driver, register_rpmsg_driver, \
 			unregister_rpmsg_driver)
 
+#if IS_ENABLED(CONFIG_RPMSG_CTRL)
+
+int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
+void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
+
+#else
+
+static inline int rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
+{
+	return 0;
+}
+
+static inline void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_RPMSG_CTRL) */
+
 #endif /* _LINUX_RPMSG_H */
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..0b0cb028e0b3 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -9,6 +9,20 @@
 #include <linux/ioctl.h>
 #include <linux/types.h>
 
+/**
+ * enum rpmsg_services - list of supported RPMsg services
+ *
+ * @RPMSG_RAW_SERVICE: char device RPMSG service
+ * @RPMSG_START_PRIVATE_SERVICES: private services have to be declared after.
+ */
+enum rpmsg_services {
+	/* Reserved services */
+	RPMSG_RAW_SERVICE =  0,
+
+	/* Private services */
+	RPMSG_START_PRIVATE_SERVICES =  1024,
+};
+
 /**
  * struct rpmsg_endpoint_info - endpoint info representation
  * @name: name of service
-- 
2.17.1

[PATCH v2 03/16] rpmsg: add override field in channel info

[Arnaud Pouliquen]

The override field is already used in the rpmsg_device.
Adding this field in the channel info allows to force the channel
creation with a specified RPMsg service driver.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 include/linux/rpmsg.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 5d64704c2346..5681c585e235 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -44,11 +44,13 @@ struct rpmsg_drv_ctrl_info {
  * @name: name of service
  * @src: local address
  * @dst: destination address
+ * @driver_override: driver name to force a match
  */
 struct rpmsg_channel_info {
 	char name[RPMSG_NAME_SIZE];
 	u32 src;
 	u32 dst;
+	const char *driver_override;
 };
 
 /**
-- 
2.17.1

[PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Arnaud Pouliquen]

Implement the ioctl function that parses the list of
rpmsg drivers registered to create an associated device.
To be ISO user API, in a first step, the driver_override
is only allowed for the RPMsg raw service, supported by the
rpmsg_char driver.

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

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 065e2e304019..8381b5b2b794 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static const char *rpmsg_ctrl_get_drv_name(u32 service)
+{
+	struct rpmsg_ctl_info *drv_info;
+
+	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
+		if (drv_info->ctrl->service == service)
+			return drv_info->ctrl->drv_name;
+	}
+
+	return NULL;
+}
+
 static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
 				 unsigned long arg)
 {
 	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
-
-	dev_info(&ctrldev->dev, "Control not yet implemented\n");
+	void __user *argp = (void __user *)arg;
+	struct rpmsg_channel_info chinfo;
+	struct rpmsg_endpoint_info eptinfo;
+	struct rpmsg_device *newch;
+
+	if (cmd != RPMSG_CREATE_EPT_IOCTL)
+		return -EINVAL;
+
+	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+		return -EFAULT;
+
+	/*
+	 * In a frst step only the rpmsg_raw service is supported.
+	 * The override is foorced to RPMSG_RAW_SERVICE
+	 */
+	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
+	if (!chinfo.driver_override)
+		return -ENODEV;
+
+	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+	chinfo.src = eptinfo.src;
+	chinfo.dst = eptinfo.dst;
+
+	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+	if (!newch) {
+		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
+		return -ENXIO;
+	}
 
 	return 0;
 };
-- 
2.17.1

[PATCH v2 05/16] rpmsg: ns: initialize channel info override field

[Arnaud Pouliquen]

By default driver_override should be 0 to avoid to force
the channel creation with a specified name.The local variable
is not initialized.

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

diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 762ff1ae279f..a526bff62947 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
 	chinfo.src = RPMSG_ADDR_ANY;
 	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
+	chinfo.driver_override = NULL;
 
 	dev_info(dev, "%sing channel %s addr 0x%x\n",
 		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
-- 
2.17.1

[PATCH v2 06/16] rpmsg: add helper to register the rpmsg ctrl device

[Arnaud Pouliquen]

This function registers a rpmsg_ctl device and its associated
/dev/rpmsg_ctrl interface.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 16 ++++++++++++++++
 include/linux/rpmsg.h      |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 8381b5b2b794..e4b0ca6dffe3 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -271,6 +271,22 @@ void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
 }
 EXPORT_SYMBOL(rpmsg_ctrl_unregister_ctl);
 
+/**
+ * rpmsg_ctl_register_device() - register control device based on rpdev
+ * @rpdev:	prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * a control rpmsg driver forcing the channel name.
+ */
+int rpmsg_ctl_register_device(struct rpmsg_device *rpdev)
+{
+	strcpy(rpdev->id.name, KBUILD_MODNAME);
+	rpdev->driver_override = KBUILD_MODNAME;
+
+	return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_ctl_register_device);
+
 static int rpmsg_ctrl_init(void)
 {
 	int ret;
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 5681c585e235..86540528325f 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -335,6 +335,8 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
 int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
 void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
 
+int rpmsg_ctl_register_device(struct rpmsg_device *rpdev);
+
 #else
 
 static inline int rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
@@ -346,6 +348,11 @@ static inline void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *c
 {
 }
 
+static inline int rpmsg_ctl_register_device(struct rpmsg_device *rpdev)
+{
+	return 0;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG_CTRL) */
 
 #endif /* _LINUX_RPMSG_H */
-- 
2.17.1

[PATCH v2 07/16] rpmsg: char: clean up rpmsg class

[Arnaud Pouliquen]

Suppress the management of the rpmsg class as attribute. It is already
handled in /sys/bus rpmsg as specified in
documentation/ABI/testing/sysfs-bus-rpmsg.
This patch prepares the migration of the control device in rpmsg_ctrl.

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4bbbacdbf3bb..732f5caf068a 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -27,7 +27,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);
@@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {
 	.compat_ioctl = compat_ptr_ioctl,
 };
 
-static ssize_t name_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n", eptdev->chinfo.name);
-}
-static DEVICE_ATTR_RO(name);
-
-static ssize_t src_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", eptdev->chinfo.src);
-}
-static DEVICE_ATTR_RO(src);
-
-static ssize_t dst_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%d\n", eptdev->chinfo.dst);
-}
-static DEVICE_ATTR_RO(dst);
-
-static struct attribute *rpmsg_eptdev_attrs[] = {
-	&dev_attr_name.attr,
-	&dev_attr_src.attr,
-	&dev_attr_dst.attr,
-	NULL
-};
-ATTRIBUTE_GROUPS(rpmsg_eptdev);
-
 static void rpmsg_eptdev_release_device(struct device *dev)
 {
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
@@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	init_waitqueue_head(&eptdev->readq);
 
 	device_initialize(dev);
-	dev->class = rpmsg_class;
 	dev->parent = &ctrldev->dev;
-	dev->groups = rpmsg_eptdev_groups;
 	dev_set_drvdata(dev, eptdev);
 
 	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
@@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	dev = &ctrldev->dev;
 	device_initialize(dev);
 	dev->parent = &rpdev->dev;
-	dev->class = rpmsg_class;
 
 	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
 	ctrldev->cdev.owner = THIS_MODULE;
@@ -553,17 +514,9 @@ static int rpmsg_char_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);
 	}
 
@@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_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);
-- 
2.17.1

[PATCH v2 08/16] rpmsg: char: make char rpmsg a rpmsg device without the control part

[Arnaud Pouliquen]

The RPMsg control part is migrated to the rpmsg_ctrl.c. Clean up the
code associated to the support of the /dev/rpmsgctrl0 and update the
driver to only manage the char devices as a RPMsg generic service.

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 732f5caf068a..3d77f4d5fc32 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -28,16 +28,12 @@
 
 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
@@ -300,10 +296,8 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 	kfree(eptdev);
 }
 
-static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
-			       struct rpmsg_channel_info chinfo)
+static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 {
-	struct rpmsg_device *rpdev = ctrldev->rpdev;
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
 	int ret;
@@ -314,7 +308,9 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 
 	dev = &eptdev->dev;
 	eptdev->rpdev = rpdev;
-	eptdev->chinfo = chinfo;
+	strncpy(eptdev->chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
+	eptdev->chinfo.src = rpdev->src;
+	eptdev->chinfo.dst = rpdev->dst;
 
 	mutex_init(&eptdev->ept_lock);
 	spin_lock_init(&eptdev->queue_lock);
@@ -322,15 +318,18 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	init_waitqueue_head(&eptdev->readq);
 
 	device_initialize(dev);
-	dev->parent = &ctrldev->dev;
+	dev->parent = &rpdev->dev;
 	dev_set_drvdata(dev, eptdev);
 
 	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
 	eptdev->cdev.owner = THIS_MODULE;
 
 	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(dev, "failed to get IDA (%d)\n", ret);
 		goto free_eptdev;
+	}
+
 	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
 
 	ret = ida_simple_get(&rpmsg_ept_ida, 0, 0, GFP_KERNEL);
@@ -340,8 +339,10 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	dev_set_name(dev, "rpmsg%d", ret);
 
 	ret = cdev_add(&eptdev->cdev, dev->devt, 1);
-	if (ret)
+	if (ret) {
+		dev_err(&rpdev->dev, "failed to add char device(%d)\n", ret);
 		goto free_ept_ida;
+	}
 
 	/* We can now rely on the release function for cleanup */
 	dev->release = rpmsg_eptdev_release_device;
@@ -352,6 +353,8 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 		put_device(dev);
 	}
 
+	dev_set_drvdata(&rpdev->dev, eptdev);
+
 	return ret;
 
 free_ept_ida:
@@ -365,138 +368,25 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	return ret;
 }
 
-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)
+static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
 {
-	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+	struct rpmsg_eptdev *eptdev = dev_get_drvdata(&rpdev->dev);
 
-	put_device(&ctrldev->dev);
+	/* Wake up any blocked readers */
+	wake_up_interruptible(&eptdev->readq);
 
-	return 0;
+	device_del(&eptdev->dev);
+	put_device(&eptdev->dev);
 }
 
-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_eptdev_create(ctrldev, chinfo);
+static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
+	{ .name	= "rpmsg-raw" },
+	{ },
 };
-
-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;
-
-	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_eptdev_destroy);
-	if (ret)
-		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
-
-	device_del(&ctrldev->dev);
-	put_device(&ctrldev->dev);
-}
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_chrdev_id_table);
 
 static struct rpmsg_driver rpmsg_chrdev_driver = {
+	.id_table	= rpmsg_chrdev_id_table,
 	.probe = rpmsg_chrdev_probe,
 	.remove = rpmsg_chrdev_remove,
 	.drv = {
@@ -504,7 +394,7 @@ static struct rpmsg_driver rpmsg_chrdev_driver = {
 	},
 };
 
-static int rpmsg_char_init(void)
+static int rpmsg_chrdev_init(void)
 {
 	int ret;
 
@@ -516,13 +406,13 @@ static int rpmsg_char_init(void)
 
 	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
 	if (ret < 0) {
-		pr_err("rpmsgchr: failed to register rpmsg driver\n");
+		pr_err("rpmsg: failed to register rpmsg chrdev driver\n");
 		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 	}
 
 	return ret;
 }
-postcore_initcall(rpmsg_char_init);
+postcore_initcall(rpmsg_chrdev_init);
 
 static void rpmsg_chrdev_exit(void)
 {
-- 
2.17.1

[PATCH v2 09/16] rpmsg: char: register RPMsg raw service to the ioctl interface.

[Arnaud Pouliquen]

Associate the RPMsg char to the RPMSG_RAW_SERVICE, by registering it to the
RPMsg Control.

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3d77f4d5fc32..66e01b979e72 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -394,6 +394,11 @@ static struct rpmsg_driver rpmsg_chrdev_driver = {
 	},
 };
 
+const struct rpmsg_drv_ctrl_info  rpmsg_chrdev_ctrl = {
+	.drv_name = "rpmsg_chrdev",
+	.service = RPMSG_RAW_SERVICE,
+};
+
 static int rpmsg_chrdev_init(void)
 {
 	int ret;
@@ -407,15 +412,27 @@ static int rpmsg_chrdev_init(void)
 	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
 	if (ret < 0) {
 		pr_err("rpmsg: failed to register rpmsg chrdev driver\n");
-		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+		goto free_region;
 	}
 
+	ret = rpmsg_ctrl_register_ctl(&rpmsg_chrdev_ctrl);
+	if (ret < 0)
+		goto free_rpmsg;
+
+	return 0;
+
+free_rpmsg:
+	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
+free_region:
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+
 	return ret;
 }
 postcore_initcall(rpmsg_chrdev_init);
 
 static void rpmsg_chrdev_exit(void)
 {
+	rpmsg_ctrl_unregister_ctl(&rpmsg_chrdev_ctrl);
 	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
 	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
 }
-- 
2.17.1

[PATCH v2 10/16] rpmsg: char: allow only one endpoint per device

[Arnaud Pouliquen]

Only one endpoint can be created per device, prevent from multi open.

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 66e01b979e72..4b0674a2e3e9 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -122,6 +122,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 	struct rpmsg_device *rpdev = eptdev->rpdev;
 	struct device *dev = &eptdev->dev;
 
+	if (eptdev->ept)
+		return -EBUSY;
+
 	get_device(dev);
 
 	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
-- 
2.17.1

[PATCH v2 11/16] rpmsg: char: check destination address is not null

[Arnaud Pouliquen]

The name service announcement is not sent if no endpoint is created by
default. If the destination address is not precised by the
application when creating the device (thanks to the RPMsg CTRL interface),
it is not possible to have a valid RPMsg channel.

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4b0674a2e3e9..8b1928594d10 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -305,6 +305,16 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	struct device *dev;
 	int ret;
 
+	/* There is not ept created by default. As consequence there is not NS
+	 * announcement and the destination address has to be set.
+	 * this limitation could be solved in future by adding a helper in
+	 * rpmsg_ns.
+	 */
+	if (rpdev->dst == RPMSG_ADDR_ANY) {
+		dev_err(dev, "destination address invalid (%d)\n", rpdev->dst);
+		return -EINVAL;
+	}
+
 	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
 	if (!eptdev)
 		return -ENOMEM;
-- 
2.17.1

[PATCH v2 12/16] rpmsg: virtio: use the driver_override in channel creation ops

[Arnaud Pouliquen]

Use the override information from the channel info structure
to set the rpdev override and so links the channel to a specific
driver.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e87d4cf926eb..c1d4bc08b2a5 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -416,6 +416,7 @@ static struct rpmsg_device *__rpmsg_create_channel(struct virtproc_info *vrp,
 	rpdev->dst = chinfo->dst;
 	rpdev->ops = &virtio_rpmsg_ops;
 	rpdev->little_endian = virtio_is_little_endian(vrp->vdev);
+	rpdev->driver_override = (char *)chinfo->driver_override;
 
 	/*
 	 * rpmsg server channels has predefined local address (for now),
-- 
2.17.1

[PATCH v2 13/16] rpmsg: virtio: probe the rpmsg_ctl device

[Arnaud Pouliquen]

Probe the rpmsg_ctl driver on virtio rpmsg bus creation.
This provides the possibility to expose an ioctrl to create
RPMsg channels.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index c1d4bc08b2a5..e8caccb2d298 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -814,6 +814,35 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
 	wake_up_interruptible(&vrp->sendq);
 }
 
+static struct rpmsg_device *rpmsg_virtio_add_char_dev(struct virtio_device *vdev)
+{
+	struct virtproc_info *vrp = vdev->priv;
+	struct virtio_rpmsg_channel *vch;
+	struct rpmsg_device *rpdev_ctrl;
+	int err = 0;
+
+	vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+	if (!vch)
+		return ERR_PTR(-ENOMEM);
+
+	/* Link the channel to our vrp */
+	vch->vrp = vrp;
+
+	/* Assign public information to the rpmsg_device */
+	rpdev_ctrl = &vch->rpdev;
+	rpdev_ctrl->ops = &virtio_rpmsg_ops;
+
+	rpdev_ctrl->dev.parent = &vrp->vdev->dev;
+	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
+	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
+
+	err = rpmsg_ctl_register_device(rpdev_ctrl);
+	if (err)
+		return ERR_PTR(err);
+
+	return rpdev_ctrl;
+}
+
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -821,7 +850,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	struct virtqueue *vqs[2];
 	struct virtproc_info *vrp;
 	struct virtio_rpmsg_channel *vch;
-	struct rpmsg_device *rpdev_ns;
+	struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
@@ -919,6 +948,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
 			goto free_coherent;
 	}
 
+	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
+	if (IS_ERR(rpdev_ctrl)) {
+		err = PTR_ERR(rpdev_ctrl);
+		goto free_coherent;
+	}
 	/*
 	 * Prepare to kick but don't notify yet - we can't do this before
 	 * device is ready.
@@ -942,6 +976,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 free_coherent:
 	kfree(vch);
+	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
 	dma_free_coherent(vdev->dev.parent, total_buf_space,
 			  bufs_va, vrp->bufs_dma);
 vqs_del:
-- 
2.17.1

[PATCH v2 14/16] rpmsg: glink: add create and release rpmsg channel ops

[Arnaud Pouliquen]

Add the new ops introduced by the rpmsg_ns series and used
by the rpmsg_ctrl driver to instantiate a new rpmsg channel.

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

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 27a05167c18c..d74c338de077 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -205,6 +205,9 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
 #define GLINK_FEATURE_INTENTLESS	BIT(1)
 
 static void qcom_glink_rx_done_work(struct work_struct *work);
+static struct rpmsg_device *
+qcom_glink_create_rpdev(struct qcom_glink *glink,
+			struct rpmsg_channel_info *chinfo);
 
 static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
 						      const char *name)
@@ -1203,6 +1206,37 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 	return 0;
 }
 
+static struct rpmsg_device *
+qcom_glink_create_channel(struct rpmsg_device *rp_parent,
+			  struct rpmsg_channel_info *chinfo)
+{
+	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
+	struct qcom_glink *glink = channel->glink;
+	struct rpmsg_device *rpdev;
+	const char *name = chinfo->name;
+
+	channel = qcom_glink_alloc_channel(glink, name);
+	if (IS_ERR(channel))
+		return ERR_PTR(PTR_ERR(channel));
+
+	rpdev = qcom_glink_create_rpdev(glink, chinfo);
+	if (!IS_ERR(rpdev)) {
+		rpdev->ept = &channel->ept;
+		channel->rpdev = rpdev;
+	}
+
+	return rpdev;
+}
+
+static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
+				      struct rpmsg_channel_info *chinfo)
+{
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+	struct qcom_glink *glink = channel->glink;
+
+	return rpmsg_unregister_device(glink->dev, chinfo);
+}
+
 static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 {
 	struct glink_channel *channel = to_glink_channel(ept);
@@ -1359,6 +1393,8 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
 static const struct rpmsg_device_ops glink_device_ops = {
 	.create_ept = qcom_glink_create_ept,
 	.announce_create = qcom_glink_announce_create,
+	.create_channel = qcom_glink_create_channel,
+	.release_channel = qcom_glink_release_channel,
 };
 
 static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
@@ -1376,13 +1412,45 @@ static void qcom_glink_rpdev_release(struct device *dev)
 	kfree(rpdev);
 }
 
+static struct rpmsg_device *
+qcom_glink_create_rpdev(struct qcom_glink *glink,
+			struct rpmsg_channel_info *chinfo)
+{
+	struct rpmsg_device *rpdev;
+	struct device_node *node;
+	int ret;
+
+	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+	if (!rpdev)
+		return ERR_PTR(-ENOMEM);
+
+	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
+	rpdev->src = chinfo->src;
+	rpdev->dst = chinfo->dst;
+	rpdev->ops = &glink_device_ops;
+
+	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
+	rpdev->dev.of_node = node;
+	rpdev->dev.parent = glink->dev;
+	rpdev->dev.release = qcom_glink_rpdev_release;
+	rpdev->driver_override = (char *)chinfo->driver_override;
+
+	ret = rpmsg_register_device(rpdev);
+	if (ret) {
+		kfree(rpdev);
+		return ERR_PTR(ret);
+	}
+
+	return rpdev;
+}
+
 static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 			      char *name)
 {
 	struct glink_channel *channel;
 	struct rpmsg_device *rpdev;
 	bool create_device = false;
-	struct device_node *node;
+	struct rpmsg_channel_info chinfo;
 	int lcid;
 	int ret;
 	unsigned long flags;
@@ -1416,27 +1484,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 	complete_all(&channel->open_req);
 
 	if (create_device) {
-		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
-		if (!rpdev) {
-			ret = -ENOMEM;
+		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
+		if (IS_ERR(rpdev)) {
+			ret = PTR_ERR(rpdev);
 			goto rcid_remove;
 		}
-
 		rpdev->ept = &channel->ept;
-		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
-		rpdev->src = RPMSG_ADDR_ANY;
-		rpdev->dst = RPMSG_ADDR_ANY;
-		rpdev->ops = &glink_device_ops;
-
-		node = qcom_glink_match_channel(glink->dev->of_node, name);
-		rpdev->dev.of_node = node;
-		rpdev->dev.parent = glink->dev;
-		rpdev->dev.release = qcom_glink_rpdev_release;
-
-		ret = rpmsg_register_device(rpdev);
-		if (ret)
-			goto rcid_remove;
-
 		channel->rpdev = rpdev;
 	}
 
-- 
2.17.1

[PATCH v2 15/16] rpmsg: smd: add create and release rpmsg channel ops

[Arnaud Pouliquen]

Add the new ops introduced by the rpmsg_ns series and used
by the rpmsg_ctrl driver to instantiate a new rpmsg channel.

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

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 19903de6268d..40853702f54f 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -67,6 +67,7 @@ struct smd_channel_info_word;
 struct smd_channel_info_word_pair;
 
 static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops;
+static const struct rpmsg_device_ops qcom_smd_device_ops;
 
 #define SMD_ALLOC_TBL_COUNT	2
 #define SMD_ALLOC_TBL_SIZE	64
@@ -1013,6 +1014,52 @@ static struct device_node *qcom_smd_match_channel(struct device_node *edge_node,
 	return NULL;
 }
 
+static void qcom_smd_release_device(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct qcom_smd_device *qsdev = to_smd_device(rpdev);
+
+	kfree(qsdev);
+}
+
+static struct rpmsg_device *
+qcom_smd_create_rpmsg_ch(struct rpmsg_device *rp_parent,
+			 struct rpmsg_channel_info *chinfo)
+{
+	struct qcom_smd_device *qsdev, *qspdev = to_smd_device(rp_parent);
+	struct qcom_smd_edge *edge = qspdev->edge;
+	int ret;
+
+	qsdev = kzalloc(sizeof(*qsdev), GFP_KERNEL);
+	if (!qsdev)
+		return ERR_PTR(-ENOMEM);
+
+	qsdev->edge = edge;
+	strncpy(qsdev->rpdev.id.name, chinfo->name, RPMSG_NAME_SIZE);
+	qsdev->rpdev.src = chinfo->src;
+	qsdev->rpdev.dst = chinfo->dst;
+	qsdev->rpdev.ops = &qcom_smd_device_ops;
+	qsdev->rpdev.dev.parent = &edge->dev;
+	qsdev->rpdev.dev.release = qcom_smd_release_device;
+	qsdev->rpdev.driver_override = (char *)chinfo->driver_override;
+
+	ret = rpmsg_register_device(&qsdev->rpdev);
+	if (ret) {
+		kfree(qsdev);
+		return ERR_PTR(ret);
+	}
+
+	return &qsdev->rpdev;
+}
+
+static int qcom_smd_release_rpmsg_ch(struct rpmsg_device *rpdev,
+				     struct rpmsg_channel_info *chinfo)
+{
+	struct qcom_smd_device *qsdev = to_smd_device(rpdev);
+
+	return rpmsg_unregister_device(&qsdev->edge->dev, chinfo);
+}
+
 static int qcom_smd_announce_create(struct rpmsg_device *rpdev)
 {
 	struct qcom_smd_endpoint *qept = to_smd_endpoint(rpdev->ept);
@@ -1033,6 +1080,8 @@ static int qcom_smd_announce_create(struct rpmsg_device *rpdev)
 static const struct rpmsg_device_ops qcom_smd_device_ops = {
 	.create_ept = qcom_smd_create_ept,
 	.announce_create = qcom_smd_announce_create,
+	.create_channel = qcom_smd_create_rpmsg_ch,
+	.release_channel = qcom_smd_release_rpmsg_ch,
 };
 
 static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
@@ -1042,14 +1091,6 @@ static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
 	.poll = qcom_smd_poll,
 };
 
-static void qcom_smd_release_device(struct device *dev)
-{
-	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
-	struct qcom_smd_device *qsdev = to_smd_device(rpdev);
-
-	kfree(qsdev);
-}
-
 /*
  * Create a smd client device for channel that is being opened.
  */
-- 
2.17.1

[PATCH v2 16/16] rpmsg: replace rpmsg_chrdev_register_device use

[Arnaud Pouliquen]

Replace rpmsg_chrdev_register_device by the new helper
rpmsg_ctl_register_device to probe the new IOCTL interface.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/qcom_glink_native.c |  2 +-
 drivers/rpmsg/qcom_smd.c          |  2 +-
 drivers/rpmsg/rpmsg_internal.h    | 14 --------------
 3 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index d74c338de077..6c7bb84f7da9 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1681,7 +1681,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_ctl_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 40853702f54f..a39457c57705 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1138,7 +1138,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_ctl_register_device(&qsdev->rpdev);
 }
 
 /*
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index a76c344253bf..c81dfb374b64 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -81,19 +81,5 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
 					  struct rpmsg_channel_info *chinfo);
 int rpmsg_release_channel(struct rpmsg_device *rpdev,
 			  struct rpmsg_channel_info *chinfo);
-/**
- * rpmsg_chrdev_register_device() - register chrdev device 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)
-{
-	strcpy(rpdev->id.name, "rpmsg_chrdev");
-	rpdev->driver_override = "rpmsg_chrdev";
-
-	return rpmsg_register_device(rpdev);
-}
 
 #endif
-- 
2.17.1

Re: [PATCH v2 01/16] rpmsg: introduce RPMsg control driver for channel creation

[Randy Dunlap]

On 12/22/20 2:57 AM, Arnaud Pouliquen wrote:
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..c9e602016c3b 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -23,6 +23,14 @@ config RPMSG_NS
>  	  channel that probes the associated RPMsg device on remote endpoint
>  	  service announcement.
>  
> +config RPMSG_CTRL
> +	tristate "RPMSG control interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the /dev/rpmsg_ctl API. this API

	                                                              This

> +	  allows user-space programs to create channels with specific name,
> +	  source and destination addresses.


-- 
~Randy

Re: [PATCH v2 13/16] rpmsg: virtio: probe the rpmsg_ctl device

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 15524 bytes --]

Hi Arnaud,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc1 next-20201223]
[cannot apply to rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-generic-IOCTL-interface-for-RPMsg-channels-management/20201222-190521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8653b778e454a7708847aeafe689bce07aeeb94e
config: x86_64-randconfig-a016-20201221 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/532ff49403675dd41b19bcc2b03ca22a08443f8d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Arnaud-Pouliquen/introduce-generic-IOCTL-interface-for-RPMsg-channels-management/20201222-190521
        git checkout 532ff49403675dd41b19bcc2b03ca22a08443f8d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/rpmsg/virtio_rpmsg_bus.c:947:7: warning: variable 'rpdev_ctrl' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                   if (err)
                       ^~~
   drivers/rpmsg/virtio_rpmsg_bus.c:979:32: note: uninitialized use occurs here
           kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
                                         ^~~~~~~~~~
   drivers/rpmsg/virtio_rpmsg_bus.c:111:15: note: expanded from macro 'to_virtio_rpmsg_channel'
           container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
                        ^~~~~~
   include/linux/kernel.h:693:26: note: expanded from macro 'container_of'
           void *__mptr = (void *)(ptr);                                   \
                                   ^~~
   drivers/rpmsg/virtio_rpmsg_bus.c:947:3: note: remove the 'if' if its condition is always false
                   if (err)
                   ^~~~~~~~
   drivers/rpmsg/virtio_rpmsg_bus.c:930:7: warning: variable 'rpdev_ctrl' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                   if (!vch) {
                       ^~~~
   drivers/rpmsg/virtio_rpmsg_bus.c:979:32: note: uninitialized use occurs here
           kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
                                         ^~~~~~~~~~
   drivers/rpmsg/virtio_rpmsg_bus.c:111:15: note: expanded from macro 'to_virtio_rpmsg_channel'
           container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
                        ^~~~~~
   include/linux/kernel.h:693:26: note: expanded from macro 'container_of'
           void *__mptr = (void *)(ptr);                                   \
                                   ^~~
   drivers/rpmsg/virtio_rpmsg_bus.c:930:3: note: remove the 'if' if its condition is always false
                   if (!vch) {
                   ^~~~~~~~~~~
   drivers/rpmsg/virtio_rpmsg_bus.c:853:44: note: initialize the variable 'rpdev_ctrl' to silence this warning
           struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
                                                     ^
                                                      = NULL
   2 warnings generated.


vim +947 drivers/rpmsg/virtio_rpmsg_bus.c

532ff49403675dd Arnaud Pouliquen     2020-12-22  845  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  846  static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  847  {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  848  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3ca Stefan Hajnoczi      2015-12-17  849  	static const char * const names[] = { "input", "output" };
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  850  	struct virtqueue *vqs[2];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  851  	struct virtproc_info *vrp;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  852  	struct virtio_rpmsg_channel *vch;
532ff49403675dd Arnaud Pouliquen     2020-12-22  853  	struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  854  	void *bufs_va;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  855  	int err = 0, i;
b1b9891441fa33f Suman Anna           2014-09-16  856  	size_t total_buf_space;
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  857  	bool notify;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  858  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  859  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  860  	if (!vrp)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  861  		return -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  862  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  863  	vrp->vdev = vdev;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  864  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  865  	idr_init(&vrp->endpoints);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  866  	mutex_init(&vrp->endpoints_lock);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  867  	mutex_init(&vrp->tx_lock);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  868  	init_waitqueue_head(&vrp->sendq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  869  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  870  	/* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb22758845 Michael S. Tsirkin   2017-03-06  871  	err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  872  	if (err)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  873  		goto free_vrp;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  874  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  875  	vrp->rvq = vqs[0];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  876  	vrp->svq = vqs[1];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  877  
b1b9891441fa33f Suman Anna           2014-09-16  878  	/* we expect symmetric tx/rx vrings */
b1b9891441fa33f Suman Anna           2014-09-16  879  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33f Suman Anna           2014-09-16  880  		virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33f Suman Anna           2014-09-16  881  
b1b9891441fa33f Suman Anna           2014-09-16  882  	/* we need less buffers if vrings are small */
b1b9891441fa33f Suman Anna           2014-09-16  883  	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33f Suman Anna           2014-09-16  884  		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33f Suman Anna           2014-09-16  885  	else
b1b9891441fa33f Suman Anna           2014-09-16  886  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33f Suman Anna           2014-09-16  887  
f93848f9eeb0f87 Loic Pallardy        2017-03-28  888  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f87 Loic Pallardy        2017-03-28  889  
f93848f9eeb0f87 Loic Pallardy        2017-03-28  890  	total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33f Suman Anna           2014-09-16  891  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  892  	/* allocate coherent memory for the buffers */
d999b622fcfb392 Loic Pallardy        2019-01-10  893  	bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33f Suman Anna           2014-09-16  894  				     total_buf_space, &vrp->bufs_dma,
b1b9891441fa33f Suman Anna           2014-09-16  895  				     GFP_KERNEL);
3119b487e03650b Wei Yongjun          2013-04-29  896  	if (!bufs_va) {
3119b487e03650b Wei Yongjun          2013-04-29  897  		err = -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  898  		goto vqs_del;
3119b487e03650b Wei Yongjun          2013-04-29  899  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  900  
de4064af76537f1 Suman Anna           2018-10-23  901  	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b15 Anna, Suman          2016-08-12  902  		bufs_va, &vrp->bufs_dma);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  903  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  904  	/* half of the buffers is dedicated for RX */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  905  	vrp->rbufs = bufs_va;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  906  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  907  	/* and half is dedicated for TX */
b1b9891441fa33f Suman Anna           2014-09-16  908  	vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  909  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  910  	/* set up the receive buffers */
b1b9891441fa33f Suman Anna           2014-09-16  911  	for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  912  		struct scatterlist sg;
f93848f9eeb0f87 Loic Pallardy        2017-03-28  913  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  914  
9dd87c2af651b09 Loic Pallardy        2017-03-28  915  		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  916  
cee51d69a45b6ce Rusty Russell        2013-03-20  917  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  918  					  GFP_KERNEL);
57e1a37347d31c6 Rusty Russell        2012-10-16  919  		WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  920  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  921  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  922  	/* suppress "tx-complete" interrupts */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  923  	virtqueue_disable_cb(vrp->svq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  924  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  925  	vdev->priv = vrp;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  926  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  927  	/* if supported by the remote processor, enable the name service */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  928  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  929  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  930  		if (!vch) {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  931  			err = -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  932  			goto free_coherent;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  933  		}
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  934  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  935  		/* Link the channel to our vrp */
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  936  		vch->vrp = vrp;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  937  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  938  		/* Assign public information to the rpmsg_device */
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  939  		rpdev_ns = &vch->rpdev;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  940  		rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  941  		rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  942  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  943  		rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  944  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  945  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  946  		err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf77 Arnaud Pouliquen     2020-11-20 @947  		if (err)
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  948  			goto free_coherent;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  949  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  950  
532ff49403675dd Arnaud Pouliquen     2020-12-22  951  	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
532ff49403675dd Arnaud Pouliquen     2020-12-22  952  	if (IS_ERR(rpdev_ctrl)) {
532ff49403675dd Arnaud Pouliquen     2020-12-22  953  		err = PTR_ERR(rpdev_ctrl);
532ff49403675dd Arnaud Pouliquen     2020-12-22  954  		goto free_coherent;
532ff49403675dd Arnaud Pouliquen     2020-12-22  955  	}
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  956  	/*
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  957  	 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  958  	 * device is ready.
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  959  	 */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  960  	notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  961  
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  962  	/* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  963  	virtio_device_ready(vdev);
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  964  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  965  	/* tell the remote processor it can start sending messages */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  966  	/*
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  967  	 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  968  	 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  969  	 */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  970  	if (notify)
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  971  		virtqueue_notify(vrp->rvq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  972  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  973  	dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  974  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  975  	return 0;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  976  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  977  free_coherent:
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  978  	kfree(vch);
532ff49403675dd Arnaud Pouliquen     2020-12-22  979  	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
d999b622fcfb392 Loic Pallardy        2019-01-10  980  	dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab0 Fernando Guzman Lugo 2012-08-29  981  			  bufs_va, vrp->bufs_dma);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  982  vqs_del:
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  983  	vdev->config->del_vqs(vrp->vdev);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  984  free_vrp:
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  985  	kfree(vrp);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  986  	return err;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  987  }
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  988  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39751 bytes --]

Re: [PATCH v2 13/16] rpmsg: virtio: probe the rpmsg_ctl device

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 12624 bytes --]

Hi Arnaud,

url:    https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-generic-IOCTL-interface-for-RPMsg-channels-management/20201222-190521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8653b778e454a7708847aeafe689bce07aeeb94e
config: x86_64-randconfig-m001-20201221 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:978 rpmsg_probe() error: uninitialized symbol 'vch'.
drivers/rpmsg/virtio_rpmsg_bus.c:979 rpmsg_probe() error: uninitialized symbol 'rpdev_ctrl'.

vim +/vch +978 drivers/rpmsg/virtio_rpmsg_bus.c

bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  846  static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  847  {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  848  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3ca Stefan Hajnoczi      2015-12-17  849  	static const char * const names[] = { "input", "output" };
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  850  	struct virtqueue *vqs[2];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  851  	struct virtproc_info *vrp;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  852  	struct virtio_rpmsg_channel *vch;
                                                                                     ^^^
532ff49403675dd Arnaud Pouliquen     2020-12-22  853  	struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
                                                                                        ^^^^^^^^^^

bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  854  	void *bufs_va;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  855  	int err = 0, i;
b1b9891441fa33f Suman Anna           2014-09-16  856  	size_t total_buf_space;
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  857  	bool notify;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  858  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  859  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  860  	if (!vrp)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  861  		return -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  862  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  863  	vrp->vdev = vdev;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  864  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  865  	idr_init(&vrp->endpoints);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  866  	mutex_init(&vrp->endpoints_lock);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  867  	mutex_init(&vrp->tx_lock);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  868  	init_waitqueue_head(&vrp->sendq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  869  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  870  	/* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb22758845 Michael S. Tsirkin   2017-03-06  871  	err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  872  	if (err)
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  873  		goto free_vrp;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  874  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  875  	vrp->rvq = vqs[0];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  876  	vrp->svq = vqs[1];
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  877  
b1b9891441fa33f Suman Anna           2014-09-16  878  	/* we expect symmetric tx/rx vrings */
b1b9891441fa33f Suman Anna           2014-09-16  879  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33f Suman Anna           2014-09-16  880  		virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33f Suman Anna           2014-09-16  881  
b1b9891441fa33f Suman Anna           2014-09-16  882  	/* we need less buffers if vrings are small */
b1b9891441fa33f Suman Anna           2014-09-16  883  	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33f Suman Anna           2014-09-16  884  		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33f Suman Anna           2014-09-16  885  	else
b1b9891441fa33f Suman Anna           2014-09-16  886  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33f Suman Anna           2014-09-16  887  
f93848f9eeb0f87 Loic Pallardy        2017-03-28  888  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f87 Loic Pallardy        2017-03-28  889  
f93848f9eeb0f87 Loic Pallardy        2017-03-28  890  	total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33f Suman Anna           2014-09-16  891  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  892  	/* allocate coherent memory for the buffers */
d999b622fcfb392 Loic Pallardy        2019-01-10  893  	bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33f Suman Anna           2014-09-16  894  				     total_buf_space, &vrp->bufs_dma,
b1b9891441fa33f Suman Anna           2014-09-16  895  				     GFP_KERNEL);
3119b487e03650b Wei Yongjun          2013-04-29  896  	if (!bufs_va) {
3119b487e03650b Wei Yongjun          2013-04-29  897  		err = -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  898  		goto vqs_del;
3119b487e03650b Wei Yongjun          2013-04-29  899  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  900  
de4064af76537f1 Suman Anna           2018-10-23  901  	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b15 Anna, Suman          2016-08-12  902  		bufs_va, &vrp->bufs_dma);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  903  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  904  	/* half of the buffers is dedicated for RX */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  905  	vrp->rbufs = bufs_va;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  906  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  907  	/* and half is dedicated for TX */
b1b9891441fa33f Suman Anna           2014-09-16  908  	vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  909  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  910  	/* set up the receive buffers */
b1b9891441fa33f Suman Anna           2014-09-16  911  	for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  912  		struct scatterlist sg;
f93848f9eeb0f87 Loic Pallardy        2017-03-28  913  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  914  
9dd87c2af651b09 Loic Pallardy        2017-03-28  915  		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  916  
cee51d69a45b6ce Rusty Russell        2013-03-20  917  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  918  					  GFP_KERNEL);
57e1a37347d31c6 Rusty Russell        2012-10-16  919  		WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  920  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  921  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  922  	/* suppress "tx-complete" interrupts */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  923  	virtqueue_disable_cb(vrp->svq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  924  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  925  	vdev->priv = vrp;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  926  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  927  	/* if supported by the remote processor, enable the name service */
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  928  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  929  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);

Not initialized if virtio_has_feature() is false.

950a7388f02bf77 Arnaud Pouliquen     2020-11-20  930  		if (!vch) {
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  931  			err = -ENOMEM;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  932  			goto free_coherent;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  933  		}
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  934  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  935  		/* Link the channel to our vrp */
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  936  		vch->vrp = vrp;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  937  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  938  		/* Assign public information to the rpmsg_device */
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  939  		rpdev_ns = &vch->rpdev;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  940  		rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  941  		rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  942  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  943  		rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  944  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  945  
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  946  		err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  947  		if (err)
950a7388f02bf77 Arnaud Pouliquen     2020-11-20  948  			goto free_coherent;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  949  	}
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  950  
532ff49403675dd Arnaud Pouliquen     2020-12-22  951  	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
532ff49403675dd Arnaud Pouliquen     2020-12-22  952  	if (IS_ERR(rpdev_ctrl)) {
532ff49403675dd Arnaud Pouliquen     2020-12-22  953  		err = PTR_ERR(rpdev_ctrl);
532ff49403675dd Arnaud Pouliquen     2020-12-22  954  		goto free_coherent;
532ff49403675dd Arnaud Pouliquen     2020-12-22  955  	}
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  956  	/*
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  957  	 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  958  	 * device is ready.
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  959  	 */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  960  	notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  961  
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  962  	/* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  963  	virtio_device_ready(vdev);
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  964  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  965  	/* tell the remote processor it can start sending messages */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  966  	/*
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  967  	 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  968  	 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  969  	 */
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  970  	if (notify)
71e4b8bf0482fc7 Michael S. Tsirkin   2015-03-12  971  		virtqueue_notify(vrp->rvq);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  972  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  973  	dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  974  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  975  	return 0;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  976  
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  977  free_coherent:
950a7388f02bf77 Arnaud Pouliquen     2020-11-20 @978  	kfree(vch);
                                                        ^^^^^^^^^^^

532ff49403675dd Arnaud Pouliquen     2020-12-22 @979  	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
                                                                                      ^^^^^^^^^^

d999b622fcfb392 Loic Pallardy        2019-01-10  980  	dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab0 Fernando Guzman Lugo 2012-08-29  981  			  bufs_va, vrp->bufs_dma);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  982  vqs_del:
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  983  	vdev->config->del_vqs(vrp->vdev);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  984  free_vrp:
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  985  	kfree(vrp);
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  986  	return err;
bcabbccabffe732 Ohad Ben-Cohen       2011-10-20  987  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32418 bytes --]

Re: [PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> This series is a restructuring of the RPMsg char driver, to create a generic
> RPMsg ioctl interface for all rpmsg services.
> 
> The RPMsg char driver provides interfaces that:
> - expose a char RPMsg device for communication with the remote processor,
> - expose controls interface for applications to create and release endpoints.
> 
> The objective of this series is to decorrelate the two interfaces:
>   - Provide a char device for a RPMsg raw service in the rpmsg_char that can be
>     probed by a RPMsg bus on a ns announcement.
>   - Generalize the use of the ioctl for all RPMsg services by creating the
>     rpmsg_ctrl, but keep it compatibile with the legacy.
> 
> If the V1 create a new rpmsg_raw driver in addition to the rpmsg_ctrl this
> version try to reuse the rpmsg_char driver by addapting QCOM GLINK and SMD
> drivers.
> So a goal of this version is to help to determine the best strategy to move
> forward:
>   - reuse rpmsg_char.
>   - introduce a new driver and keep rpmsg_char as a legacy driver for a while.
> 
> Notice that SMD and GLINK patches have to be tested, only build has been tested.
> 
> 1) RPMsg control driver: rpmsg_ctrl.c
>   This driver is based on the control part of the RPMsg_char driver. 
>   On probe a /dev/rpmsg_ctrl<X> interface is created to allow to manage the
>   channels.
>   The principles are the following:
>   - The RPMsg service driver registers it's name and the associated service
>     using the rpmsg_ctrl_unregister_ctl API. The list of supported services
>     is defined in  include/uapi/linux/rpmsg.h and exposed to the
>     application thanks to a new field in rpmsg_endpoint_info struct.
>   - On the RPMsg bus probe(e.g virtio bus) an rpmsg_ctrl device is
>     registered that creates the control interface.
>   - The application can then create or release a channel by specifying:
>        - the name service
>        - the source address.
>        - the destination address.

Why is this useful?

>   - The rpmsg_ctrl uses the same interface than the ns announcement to
>     create and release the associated channel but using the driver_override
>     field to force the service name.
>     The  "driver_override" allows to force the name service associated to
>     an RPMsg driver, bypassing the rpmsg_device_id based match check.

You mean, the chinfo driver_override allows the ioctl to specify which
driver should be bound to the device created for the newly registered
endpoint?

>   - At least for virtio bus, an associated ns announcement is sent to the
>     remote side.  
> 
> 2) rpmsg char driver: rpmsg_char.c
>     - The rpmsg class has not been removed. The associated attributes
>       are already available in /sys/bus/rpmsg/.

So today a rpmsg_device gets the same attributes both from the class and
the bus? So the only difference is that there will no longer be a
/sys/class/rpmsg ?

Regards,
Bjorn

>     - The eptdev device is now an RPMsg device probed by a RPMsg bus driver
>       (probed only by the ioctl in rpmsg_char driver).
> 
> Know current Limitations:
> - Tested only with virtio RPMsg bus and for one vdev instance.
> - The glink and smd drivers adaptations have not been tested (not able to test).
> - To limit commit and not update the IOCT interface some features have been not
>   implemented in this first step:
>     - the NS announcement as not been updated, it is not possible to create an
>       endpoint with a destibnation address set to RPMSG_ADDR_ANY (-1),
>     - not possible to destroy the channel,
>     - only the "rpmsg-raw" service is supported.
> 
> This series can be applied in Bjorn's rpmsg-next branch on top of the
> RPMsg_ns series(4c0943255805).
> 
> This series can be tested using rpmsgexport tools available here:
> https://github.com/andersson/rpmsgexport.
> ---
> new from V1[1]:
> - In V1 the rpmsg_char.c was not impacted, a rpmsg_raw.c has been created
>   instead.
> - IOCTL interface as not been updated (to go by steps).
> - smd and glink drivers has been updated to support channels creation and
>   release.
> 
> [1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=327277
> 
> Arnaud Pouliquen (16):
>   rpmsg: introduce RPMsg control driver for channel creation
>   rpmsg: add RPMsg control API to register service
>   rpmsg: add override field in channel info
>   rpmsg: ctrl: implement the ioctl function to create device
>   rpmsg: ns: initialize channel info override field
>   rpmsg: add helper to register the rpmsg ctrl device
>   rpmsg: char: clean up rpmsg class
>   rpmsg: char: make char rpmsg a rpmsg device without the control part
>   rpmsg: char: register RPMsg raw service to the ioctl interface.
>   rpmsg: char: allow only one endpoint per device
>   rpmsg: char: check destination address is not null
>   rpmsg: virtio: use the driver_override in channel creation ops
>   rpmsg: virtio: probe the rpmsg_ctl device
>   rpmsg: glink: add create and release rpmsg channel ops
>   rpmsg: smd: add create and release rpmsg channel ops
>   rpmsg: replace rpmsg_chrdev_register_device use
> 
>  drivers/rpmsg/Kconfig             |   8 +
>  drivers/rpmsg/Makefile            |   1 +
>  drivers/rpmsg/qcom_glink_native.c |  96 +++++++--
>  drivers/rpmsg/qcom_smd.c          |  59 +++++-
>  drivers/rpmsg/rpmsg_char.c        | 246 ++++++-----------------
>  drivers/rpmsg/rpmsg_ctrl.c        | 320 ++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |  14 --
>  drivers/rpmsg/rpmsg_ns.c          |   1 +
>  drivers/rpmsg/virtio_rpmsg_bus.c  |  38 +++-
>  include/linux/rpmsg.h             |  40 ++++
>  include/uapi/linux/rpmsg.h        |  14 ++
>  11 files changed, 606 insertions(+), 231 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 01/16] rpmsg: introduce RPMsg control driver for channel creation

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> The RPMsg_ctrl driver is a duplication of the ioctrl part of the
> rpmsg_char driver to make generic the ioctl to manage channels by
> the userspace applications.
> 
> As a first step, this driver just creates the /dev/rpmsg_ctl<x>
> ( <x> is the instance value). The ioctl is not implemented.
> 
> Notice that this driver is associated to a RPMsg device with no endpoint.
> Instantiating this device as an RPMsg device allows to retrieve the
> associated RPMsg backend.
> 

I think it would make more sense to make the rpmsg_char driver do what
you want and then cleanly extract the "control" code out of it to a
separate file - than to create a duplicate implementation and then
retire the other one.

In particular I think that would show the difference between the two
better.

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/Kconfig      |   8 ++
>  drivers/rpmsg/Makefile     |   1 +
>  drivers/rpmsg/rpmsg_ctrl.c | 208 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..c9e602016c3b 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -23,6 +23,14 @@ config RPMSG_NS
>  	  channel that probes the associated RPMsg device on remote endpoint
>  	  service announcement.
>  
> +config RPMSG_CTRL
> +	tristate "RPMSG control interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the /dev/rpmsg_ctl API. this API
> +	  allows user-space programs to create channels with specific name,
> +	  source and destination addresses.
> +
>  config RPMSG_MTK_SCP
>  	tristate "MediaTek SCP"
>  	depends on MTK_SCP
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..3c1bce9d0228 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> new file mode 100644
> index 000000000000..425c3e32ada4
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020

This is pretty much a verbatim copy of rpmsg_char and as such
STMicroelectronics is not the appropriate/sole copyright holder of the
content.

Regards,
Bjorn

> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "rpmsg_internal.h"
> +#include <uapi/linux/rpmsg.h>
> +
> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> +
> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl_dev, dev)
> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl_dev, cdev)
> +
> +/**
> + * struct rpmsg_ctrl_dev - control device for instantiating endpoint devices
> + * @rpdev:	underlaying rpmsg device
> + * @cdev:	cdev for the ctrl device
> + * @dev:	device for the ctrl device
> + */
> +struct rpmsg_ctrl_dev {
> +	struct rpmsg_device *rpdev;
> +	struct cdev cdev;
> +	struct device dev;
> +};
> +
> +static DEFINE_IDA(rpmsg_ctrl_ida);
> +static DEFINE_IDA(rpmsg_minor_ida);
> +
> +static dev_t rpmsg_major;
> +
> +static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	get_device(&ctrldev->dev);
> +	filp->private_data = ctrldev;
> +
> +	return 0;
> +}
> +
> +static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> +
> +	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +
> +	return 0;
> +};
> +
> +static int rpmsg_ctrl_dev_release(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	put_device(&ctrldev->dev);
> +
> +	return 0;
> +}
> +
> +static void rpmsg_ctrl_dev_release_device(struct device *dev)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = dev_to_ctrldev(dev);
> +
> +	dev_err(dev, "%s\n", __func__);
> +
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +	cdev_del(&ctrldev->cdev);
> +	kfree(ctrldev);
> +}
> +
> +static const struct file_operations rpmsg_ctrl_fops = {
> +	.owner = THIS_MODULE,
> +	.open = rpmsg_ctrl_dev_open,
> +	.release = rpmsg_ctrl_dev_release,
> +	.unlocked_ioctl = rpmsg_ctrl_dev_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +static int rpmsg_ctrl_add_control(struct rpmsg_ctrl_dev *ctrldev)
> +{
> +	struct device *dev = &ctrldev->dev;
> +	int ret;
> +
> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
> +	ctrldev->cdev.owner = THIS_MODULE;
> +
> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
> +	dev_set_name(dev, "rpmsg_ctrl%d", dev->id);
> +
> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> +	if (ret)
> +		goto free_minor_ida;
> +
> +	dev_info(dev, "add %s control for %s driver\n",
> +		 dev_name(dev),  dev_name(dev->parent));
> +
> +	return 0;
> +
> +free_minor_ida:
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +
> +	return ret;
> +}
> +
> +static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl_dev *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;
> +
> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_ctrldev;
> +
> +	dev->id = ret;
> +
> +	ret = rpmsg_ctrl_add_control(ctrldev);
> +	if (ret < 0)
> +		goto free_ctrl_ida;
> +
> +	/* We can now rely on the release function for cleanup */
> +	dev->release = rpmsg_ctrl_dev_release_device;
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> +		put_device(dev);
> +		goto free_ctrl_ida;
> +	}
> +
> +	dev_set_drvdata(dev, ctrldev);
> +	dev_set_drvdata(&rpdev->dev, ctrldev);
> +
> +	return 0;
> +
> +free_ctrl_ida:
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +free_ctrldev:
> +	put_device(dev);
> +	kfree(ctrldev);
> +
> +	return ret;
> +}
> +
> +static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = dev_get_drvdata(&rpdev->dev);
> +
> +	device_del(&ctrldev->dev);
> +	put_device(&ctrldev->dev);
> +}
> +
> +static struct rpmsg_driver rpmsg_ctrl_driver = {
> +	.drv.name = KBUILD_MODNAME,
> +	.probe = rpmsg_ctrl_probe,
> +	.remove = rpmsg_ctrl_remove,
> +};
> +
> +static int rpmsg_ctrl_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
> +	if (ret < 0) {
> +		pr_err("rpmsg_ctrl: failed to allocate char dev region\n");
> +		return ret;
> +	}
> +
> +	ret = register_rpmsg_driver(&rpmsg_ctrl_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_ctrl_init);
> +
> +static void rpmsg_ctrl_exit(void)
> +{
> +	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +}
> +module_exit(rpmsg_ctrl_exit);
> +
> +MODULE_DESCRIPTION("ioctl rpmsg driver");
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH v2 02/16] rpmsg: add RPMsg control API to register service

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Add API to register a RPMsg service to the control device.
> The rpmsg_drv_ctrl_info structure links a service to its driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 57 ++++++++++++++++++++++++++++++++++++++
>  include/linux/rpmsg.h      | 31 +++++++++++++++++++++
>  include/uapi/linux/rpmsg.h | 14 ++++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 425c3e32ada4..065e2e304019 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -27,6 +27,20 @@ struct rpmsg_ctrl_dev {
>  	struct device dev;
>  };
>  
> +/**
> + * struct rpmsg_ctl_info - control info list node
> + * @ctrl:	control driver info
> + * @node:	list node
> + *
> + * This structure is used by rpmsg_ctl to list the registered drivers services
> + */
> +struct rpmsg_ctl_info {
> +	const struct rpmsg_drv_ctrl_info *ctrl;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(rpmsg_drv_list);
> +
>  static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_minor_ida);
>  
> @@ -175,6 +189,49 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
>  	.remove = rpmsg_ctrl_remove,
>  };
>  
> +/**
> + * rpmsg_ctrl_register_ctl() -register control for the associated service
> + * @ctrl: rpmsg driver information
> + *
> + * This function is called by the rpmsg driver to register a service that will
> + * be exposed to be instantiate by the application.
> + */
> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	drv_info =  kzalloc(sizeof(*drv_info), GFP_KERNEL);
> +	if (!drv_info)
> +		return -ENOMEM;
> +
> +	drv_info->ctrl = ctrl;
> +
> +	list_add_tail(&drv_info->node, &rpmsg_drv_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rpmsg_ctrl_register_ctl);
> +
> +/**
> + * rpmsg_ctrl_unregister_ctl() -unregister control for the associated service
> + * @ctrl: the rpmsg control information
> + *
> + * This function is called by the rpmsg driver to unregister the associated
> + * service.
> + */
> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +	struct rpmsg_ctl_info *drv_info, *tmp;
> +
> +	list_for_each_entry_safe(drv_info, tmp, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl == ctrl) {
> +			list_del(&drv_info->node);
> +			kfree(drv_info);
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(rpmsg_ctrl_unregister_ctl);
> +
>  static int rpmsg_ctrl_init(void)
>  {
>  	int ret;
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index a5db828b2420..5d64704c2346 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -26,6 +26,19 @@ struct rpmsg_endpoint;
>  struct rpmsg_device_ops;
>  struct rpmsg_endpoint_ops;
>  
> +/**
> + * struct rpmsg_drv_ctrl_info - rpmsg ctrl structure
> + * @drv_name:	name of the associated driver
> + * @service:	the associated rpmsg service listed in @rpmsg_services
> + *
> + * This structure is used by rpmsg_ctl to link the endpoint creation to the
> + * service rpmsg driver.
> + */
> +struct rpmsg_drv_ctrl_info {
> +	const char *drv_name;
> +	u32  service;
> +};
> +
>  /**
>   * struct rpmsg_channel_info - channel info representation
>   * @name: name of service
> @@ -315,4 +328,22 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>  	module_driver(__rpmsg_driver, register_rpmsg_driver, \
>  			unregister_rpmsg_driver)
>  
> +#if IS_ENABLED(CONFIG_RPMSG_CTRL)
> +
> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
> +
> +#else
> +
> +static inline int rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +	return 0;
> +}
> +
> +static inline void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +}
> +
> +#endif /* IS_ENABLED(CONFIG_RPMSG_CTRL) */
> +
>  #endif /* _LINUX_RPMSG_H */
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index e14c6dab4223..0b0cb028e0b3 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -9,6 +9,20 @@
>  #include <linux/ioctl.h>
>  #include <linux/types.h>
>  
> +/**
> + * enum rpmsg_services - list of supported RPMsg services
> + *
> + * @RPMSG_RAW_SERVICE: char device RPMSG service
> + * @RPMSG_START_PRIVATE_SERVICES: private services have to be declared after.
> + */
> +enum rpmsg_services {
> +	/* Reserved services */
> +	RPMSG_RAW_SERVICE =  0,
> +

What kind of things do you envision this list to contain in a year from
now?

Regards,
Bjorn

> +	/* Private services */
> +	RPMSG_START_PRIVATE_SERVICES =  1024,
> +};
> +
>  /**
>   * struct rpmsg_endpoint_info - endpoint info representation
>   * @name: name of service
> -- 
> 2.17.1
> 

Re: [PATCH v2 05/16] rpmsg: ns: initialize channel info override field

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> By default driver_override should be 0 to avoid to force
> the channel creation with a specified name.The local variable
> is not initialized.
> 

The same problem exists in qcom_glink_native, qcom_smd and rpmsg_char.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ns.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 762ff1ae279f..a526bff62947 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>  	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
>  	chinfo.src = RPMSG_ADDR_ANY;
>  	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> +	chinfo.driver_override = NULL;
>  
>  	dev_info(dev, "%sing channel %s addr 0x%x\n",
>  		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> -- 
> 2.17.1
> 

Re: [PATCH v2 07/16] rpmsg: char: clean up rpmsg class

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

This patch doesn't "clean up" the class, as described in $subject. It
just removes it.

> Suppress the management of the rpmsg class as attribute. It is already
> handled in /sys/bus rpmsg as specified in
> documentation/ABI/testing/sysfs-bus-rpmsg.

Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes
should be associated with the rpmsg_device (and thereby present in its
sysfs directory). But if these attributes are also added by the bus,
then why do we have this code? If that's the case this seems like a nice
cleanup that we should do outside/before merging the other patches.

> This patch prepares the migration of the control device in rpmsg_ctrl.
> 

It would be useful if the commit message described how it prepares for
the migration and why.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------
>  1 file changed, 48 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 4bbbacdbf3bb..732f5caf068a 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -27,7 +27,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);
> @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> -static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%s\n", eptdev->chinfo.name);
> -}
> -static DEVICE_ATTR_RO(name);
> -
> -static ssize_t src_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%d\n", eptdev->chinfo.src);
> -}
> -static DEVICE_ATTR_RO(src);
> -
> -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);
> -}
> -static DEVICE_ATTR_RO(dst);
> -
> -static struct attribute *rpmsg_eptdev_attrs[] = {
> -	&dev_attr_name.attr,
> -	&dev_attr_src.attr,
> -	&dev_attr_dst.attr,
> -	NULL
> -};
> -ATTRIBUTE_GROUPS(rpmsg_eptdev);
> -
>  static void rpmsg_eptdev_release_device(struct device *dev)
>  {
>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
> @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  	init_waitqueue_head(&eptdev->readq);
>  
>  	device_initialize(dev);
> -	dev->class = rpmsg_class;
>  	dev->parent = &ctrldev->dev;
> -	dev->groups = rpmsg_eptdev_groups;
>  	dev_set_drvdata(dev, eptdev);
>  
>  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
> @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	dev = &ctrldev->dev;
>  	device_initialize(dev);
>  	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;
>  
>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>  	ctrldev->cdev.owner = THIS_MODULE;
> @@ -553,17 +514,9 @@ static int rpmsg_char_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);
>  	}
>  
> @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_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);
> -- 
> 2.17.1
> 

Re: [PATCH v2 07/16] rpmsg: char: clean up rpmsg class

[Bjorn Andersson]

On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:

> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
> This patch doesn't "clean up" the class, as described in $subject. It
> just removes it.
> 
> > Suppress the management of the rpmsg class as attribute. It is already
> > handled in /sys/bus rpmsg as specified in
> > documentation/ABI/testing/sysfs-bus-rpmsg.
> 
> Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes
> should be associated with the rpmsg_device (and thereby present in its
> sysfs directory). But if these attributes are also added by the bus,
> then why do we have this code? If that's the case this seems like a nice
> cleanup that we should do outside/before merging the other patches.
> 
> > This patch prepares the migration of the control device in rpmsg_ctrl.
> > 
> 
> It would be useful if the commit message described how it prepares for
> the migration and why.
> 

Now I see what this patch does, it removes the attributes from the
character device's struct device, because they are provided by the
struct rpmsg_device's bus!

I wish your commit message made this obvious.

Also, this implies that for a few patches here rpmsg_char is just
broken - which I don't like.

Regards,
Bjorn

> Regards,
> Bjorn
> 
> > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> > ---
> >  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------
> >  1 file changed, 48 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> > index 4bbbacdbf3bb..732f5caf068a 100644
> > --- a/drivers/rpmsg/rpmsg_char.c
> > +++ b/drivers/rpmsg/rpmsg_char.c
> > @@ -27,7 +27,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);
> > @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {
> >  	.compat_ioctl = compat_ptr_ioctl,
> >  };
> >  
> > -static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> > -			 char *buf)
> > -{
> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> > -
> > -	return sprintf(buf, "%s\n", eptdev->chinfo.name);
> > -}
> > -static DEVICE_ATTR_RO(name);
> > -
> > -static ssize_t src_show(struct device *dev, struct device_attribute *attr,
> > -			 char *buf)
> > -{
> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> > -
> > -	return sprintf(buf, "%d\n", eptdev->chinfo.src);
> > -}
> > -static DEVICE_ATTR_RO(src);
> > -
> > -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,
> > -			 char *buf)
> > -{
> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> > -
> > -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);
> > -}
> > -static DEVICE_ATTR_RO(dst);
> > -
> > -static struct attribute *rpmsg_eptdev_attrs[] = {
> > -	&dev_attr_name.attr,
> > -	&dev_attr_src.attr,
> > -	&dev_attr_dst.attr,
> > -	NULL
> > -};
> > -ATTRIBUTE_GROUPS(rpmsg_eptdev);
> > -
> >  static void rpmsg_eptdev_release_device(struct device *dev)
> >  {
> >  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
> > @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> >  	init_waitqueue_head(&eptdev->readq);
> >  
> >  	device_initialize(dev);
> > -	dev->class = rpmsg_class;
> >  	dev->parent = &ctrldev->dev;
> > -	dev->groups = rpmsg_eptdev_groups;
> >  	dev_set_drvdata(dev, eptdev);
> >  
> >  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
> > @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >  	dev = &ctrldev->dev;
> >  	device_initialize(dev);
> >  	dev->parent = &rpdev->dev;
> > -	dev->class = rpmsg_class;
> >  
> >  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> >  	ctrldev->cdev.owner = THIS_MODULE;
> > @@ -553,17 +514,9 @@ static int rpmsg_char_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);
> >  	}
> >  
> > @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_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);
> > -- 
> > 2.17.1
> > 

Re: [PATCH v2 10/16] rpmsg: char: allow only one endpoint per device

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Only one endpoint can be created per device, prevent from multi open.
> 

Having multiple invocations of rpmsg_create_ept() with the same chinfo
sounds like a bad idea. I think in the SMD and GLINK case the underlying
transport would complain that the related chinfo is already "busy", but
this seems like an appropriate fix regardless.

Please add a proper Fixes: tag and send this outside of this patch
series.

Thanks,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 66e01b979e72..4b0674a2e3e9 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -122,6 +122,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>  	struct device *dev = &eptdev->dev;
>  
> +	if (eptdev->ept)
> +		return -EBUSY;
> +
>  	get_device(dev);
>  
>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> -- 
> 2.17.1
> 

Re: [PATCH v2 11/16] rpmsg: char: check destination address is not null

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> The name service announcement is not sent if no endpoint is created by
> default. If the destination address is not precised by the
> application when creating the device (thanks to the RPMsg CTRL interface),
> it is not possible to have a valid RPMsg channel.
> 

In the Qualcomm transports, the chinfo.name is used to identify the
channel, so there it's valid to create a endpoint without dst.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 4b0674a2e3e9..8b1928594d10 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -305,6 +305,16 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	struct device *dev;
>  	int ret;
>  
> +	/* There is not ept created by default. As consequence there is not NS
> +	 * announcement and the destination address has to be set.
> +	 * this limitation could be solved in future by adding a helper in
> +	 * rpmsg_ns.
> +	 */
> +	if (rpdev->dst == RPMSG_ADDR_ANY) {
> +		dev_err(dev, "destination address invalid (%d)\n", rpdev->dst);
> +		return -EINVAL;
> +	}
> +
>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>  	if (!eptdev)
>  		return -ENOMEM;
> -- 
> 2.17.1
> 

Re: [PATCH v2 14/16] rpmsg: glink: add create and release rpmsg channel ops

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Add the new ops introduced by the rpmsg_ns series and used
> by the rpmsg_ctrl driver to instantiate a new rpmsg channel.
> 

This is nice for completeness sake, but I don't think it makes sense for
transports that has the nameserver "builtin" to the transport itself.

I.e. if we have the NS sitting on top of GLINK and the remote firmware
sends a "create channel" message, then this code would cause Linux to
send a in-transport "create channel" message back to the remote side in
hope that it would be willing to talk on that channel - but that would
imply that the remote side needs to have registered a rpmsg device
related to that service name - in which case it already sent a
in-transport "create channel" message.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 94 ++++++++++++++++++++++++-------
>  1 file changed, 75 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 27a05167c18c..d74c338de077 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -205,6 +205,9 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>  #define GLINK_FEATURE_INTENTLESS	BIT(1)
>  
>  static void qcom_glink_rx_done_work(struct work_struct *work);
> +static struct rpmsg_device *
> +qcom_glink_create_rpdev(struct qcom_glink *glink,
> +			struct rpmsg_channel_info *chinfo);
>  
>  static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
>  						      const char *name)
> @@ -1203,6 +1206,37 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
>  	return 0;
>  }
>  
> +static struct rpmsg_device *
> +qcom_glink_create_channel(struct rpmsg_device *rp_parent,
> +			  struct rpmsg_channel_info *chinfo)
> +{
> +	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
> +	struct qcom_glink *glink = channel->glink;
> +	struct rpmsg_device *rpdev;
> +	const char *name = chinfo->name;
> +
> +	channel = qcom_glink_alloc_channel(glink, name);
> +	if (IS_ERR(channel))
> +		return ERR_PTR(PTR_ERR(channel));
> +
> +	rpdev = qcom_glink_create_rpdev(glink, chinfo);
> +	if (!IS_ERR(rpdev)) {
> +		rpdev->ept = &channel->ept;
> +		channel->rpdev = rpdev;
> +	}
> +
> +	return rpdev;
> +}
> +
> +static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
> +				      struct rpmsg_channel_info *chinfo)
> +{
> +	struct glink_channel *channel = to_glink_channel(rpdev->ept);
> +	struct qcom_glink *glink = channel->glink;
> +
> +	return rpmsg_unregister_device(glink->dev, chinfo);
> +}
> +
>  static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>  {
>  	struct glink_channel *channel = to_glink_channel(ept);
> @@ -1359,6 +1393,8 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
>  static const struct rpmsg_device_ops glink_device_ops = {
>  	.create_ept = qcom_glink_create_ept,
>  	.announce_create = qcom_glink_announce_create,
> +	.create_channel = qcom_glink_create_channel,
> +	.release_channel = qcom_glink_release_channel,
>  };
>  
>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
> @@ -1376,13 +1412,45 @@ static void qcom_glink_rpdev_release(struct device *dev)
>  	kfree(rpdev);
>  }
>  
> +static struct rpmsg_device *
> +qcom_glink_create_rpdev(struct qcom_glink *glink,
> +			struct rpmsg_channel_info *chinfo)
> +{
> +	struct rpmsg_device *rpdev;
> +	struct device_node *node;
> +	int ret;
> +
> +	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> +	if (!rpdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
> +	rpdev->src = chinfo->src;
> +	rpdev->dst = chinfo->dst;
> +	rpdev->ops = &glink_device_ops;
> +
> +	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
> +	rpdev->dev.of_node = node;
> +	rpdev->dev.parent = glink->dev;
> +	rpdev->dev.release = qcom_glink_rpdev_release;
> +	rpdev->driver_override = (char *)chinfo->driver_override;
> +
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret) {
> +		kfree(rpdev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return rpdev;
> +}
> +
>  static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>  			      char *name)
>  {
>  	struct glink_channel *channel;
>  	struct rpmsg_device *rpdev;
>  	bool create_device = false;
> -	struct device_node *node;
> +	struct rpmsg_channel_info chinfo;
>  	int lcid;
>  	int ret;
>  	unsigned long flags;
> @@ -1416,27 +1484,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>  	complete_all(&channel->open_req);
>  
>  	if (create_device) {
> -		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> -		if (!rpdev) {
> -			ret = -ENOMEM;
> +		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
> +		chinfo.src = RPMSG_ADDR_ANY;
> +		chinfo.dst = RPMSG_ADDR_ANY;
> +		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
> +		if (IS_ERR(rpdev)) {
> +			ret = PTR_ERR(rpdev);
>  			goto rcid_remove;
>  		}
> -
>  		rpdev->ept = &channel->ept;
> -		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
> -		rpdev->src = RPMSG_ADDR_ANY;
> -		rpdev->dst = RPMSG_ADDR_ANY;
> -		rpdev->ops = &glink_device_ops;
> -
> -		node = qcom_glink_match_channel(glink->dev->of_node, name);
> -		rpdev->dev.of_node = node;
> -		rpdev->dev.parent = glink->dev;
> -		rpdev->dev.release = qcom_glink_rpdev_release;
> -
> -		ret = rpmsg_register_device(rpdev);
> -		if (ret)
> -			goto rcid_remove;
> -
>  		channel->rpdev = rpdev;
>  	}
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2 16/16] rpmsg: replace rpmsg_chrdev_register_device use

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Replace rpmsg_chrdev_register_device by the new helper
> rpmsg_ctl_register_device to probe the new IOCTL interface.
> 

This again implies that rpmsg_char was broken in SMD and GLINK during a
large part of this series. Your strategy also has the side effect of
having changed the name from /dev/rpmsg_ctrlX to /dev/rpmsg_ctlX,
breaking my userspace.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c |  2 +-
>  drivers/rpmsg/qcom_smd.c          |  2 +-
>  drivers/rpmsg/rpmsg_internal.h    | 14 --------------
>  3 files changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index d74c338de077..6c7bb84f7da9 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1681,7 +1681,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_ctl_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 40853702f54f..a39457c57705 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -1138,7 +1138,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_ctl_register_device(&qsdev->rpdev);
>  }
>  
>  /*
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index a76c344253bf..c81dfb374b64 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -81,19 +81,5 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
>  					  struct rpmsg_channel_info *chinfo);
>  int rpmsg_release_channel(struct rpmsg_device *rpdev,
>  			  struct rpmsg_channel_info *chinfo);
> -/**
> - * rpmsg_chrdev_register_device() - register chrdev device 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)
> -{
> -	strcpy(rpdev->id.name, "rpmsg_chrdev");
> -	rpdev->driver_override = "rpmsg_chrdev";
> -
> -	return rpmsg_register_device(rpdev);
> -}
>  
>  #endif
> -- 
> 2.17.1
> 

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Bjorn Andersson]

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Implement the ioctl function that parses the list of
> rpmsg drivers registered to create an associated device.
> To be ISO user API, in a first step, the driver_override
> is only allowed for the RPMsg raw service, supported by the
> rpmsg_char driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 065e2e304019..8381b5b2b794 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl->service == service)
> +			return drv_info->ctrl->drv_name;
> +	}
> +
> +	return NULL;
> +}
> +
>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>  				 unsigned long arg)
>  {
>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> -
> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_channel_info chinfo;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_device *newch;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	/*
> +	 * In a frst step only the rpmsg_raw service is supported.
> +	 * The override is foorced to RPMSG_RAW_SERVICE
> +	 */
> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> +	if (!chinfo.driver_override)
> +		return -ENODEV;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);

Afaict this would create and announce and endpoint (or possibly find a
endpoint announced by the other side of the link).

In the case of the Qualcomm transports, and as been discussed to
introduce for virtio in the past, the channel actually have a state. So
opening/announcing it here means that we have no way to close and reopen
this channel later?


It would also mean that we announce to the firmware that there's an
application in Linux now ready to receive data on this channel - but
that won't be the case until someone actually open the created cdev (or
tty in your case) - which quite likely will result in data loss.

I think instead of piggybacking on the rpmsg_device we should just carry
these "raw exports to userspace" in some other construct - perhaps a
auxiliary_bus, or if we still only care for char and tty, not split them
up at all using the device model.

Regards,
Bjorn

> +	if (!newch) {
> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> +		return -ENXIO;
> +	}
>  
>  	return 0;
>  };
> -- 
> 2.17.1
> 

Re: [PATCH v2 02/16] rpmsg: add RPMsg control API to register service

[Arnaud POULIQUEN]

On 1/5/21 1:34 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Add API to register a RPMsg service to the control device.
>> The rpmsg_drv_ctrl_info structure links a service to its driver.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 57 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/rpmsg.h      | 31 +++++++++++++++++++++
>>  include/uapi/linux/rpmsg.h | 14 ++++++++++
>>  3 files changed, 102 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 425c3e32ada4..065e2e304019 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -27,6 +27,20 @@ struct rpmsg_ctrl_dev {
>>  	struct device dev;
>>  };
>>  
>> +/**
>> + * struct rpmsg_ctl_info - control info list node
>> + * @ctrl:	control driver info
>> + * @node:	list node
>> + *
>> + * This structure is used by rpmsg_ctl to list the registered drivers services
>> + */
>> +struct rpmsg_ctl_info {
>> +	const struct rpmsg_drv_ctrl_info *ctrl;
>> +	struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(rpmsg_drv_list);
>> +
>>  static DEFINE_IDA(rpmsg_ctrl_ida);
>>  static DEFINE_IDA(rpmsg_minor_ida);
>>  
>> @@ -175,6 +189,49 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
>>  	.remove = rpmsg_ctrl_remove,
>>  };
>>  
>> +/**
>> + * rpmsg_ctrl_register_ctl() -register control for the associated service
>> + * @ctrl: rpmsg driver information
>> + *
>> + * This function is called by the rpmsg driver to register a service that will
>> + * be exposed to be instantiate by the application.
>> + */
>> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
>> +{
>> +	struct rpmsg_ctl_info *drv_info;
>> +
>> +	drv_info =  kzalloc(sizeof(*drv_info), GFP_KERNEL);
>> +	if (!drv_info)
>> +		return -ENOMEM;
>> +
>> +	drv_info->ctrl = ctrl;
>> +
>> +	list_add_tail(&drv_info->node, &rpmsg_drv_list);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(rpmsg_ctrl_register_ctl);
>> +
>> +/**
>> + * rpmsg_ctrl_unregister_ctl() -unregister control for the associated service
>> + * @ctrl: the rpmsg control information
>> + *
>> + * This function is called by the rpmsg driver to unregister the associated
>> + * service.
>> + */
>> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
>> +{
>> +	struct rpmsg_ctl_info *drv_info, *tmp;
>> +
>> +	list_for_each_entry_safe(drv_info, tmp, &rpmsg_drv_list, node) {
>> +		if (drv_info->ctrl == ctrl) {
>> +			list_del(&drv_info->node);
>> +			kfree(drv_info);
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL(rpmsg_ctrl_unregister_ctl);
>> +
>>  static int rpmsg_ctrl_init(void)
>>  {
>>  	int ret;
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index a5db828b2420..5d64704c2346 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -26,6 +26,19 @@ struct rpmsg_endpoint;
>>  struct rpmsg_device_ops;
>>  struct rpmsg_endpoint_ops;
>>  
>> +/**
>> + * struct rpmsg_drv_ctrl_info - rpmsg ctrl structure
>> + * @drv_name:	name of the associated driver
>> + * @service:	the associated rpmsg service listed in @rpmsg_services
>> + *
>> + * This structure is used by rpmsg_ctl to link the endpoint creation to the
>> + * service rpmsg driver.
>> + */
>> +struct rpmsg_drv_ctrl_info {
>> +	const char *drv_name;
>> +	u32  service;
>> +};
>> +
>>  /**
>>   * struct rpmsg_channel_info - channel info representation
>>   * @name: name of service
>> @@ -315,4 +328,22 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>>  	module_driver(__rpmsg_driver, register_rpmsg_driver, \
>>  			unregister_rpmsg_driver)
>>  
>> +#if IS_ENABLED(CONFIG_RPMSG_CTRL)
>> +
>> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
>> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
>> +
>> +#else
>> +
>> +static inline int rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
>> +{
>> +}
>> +
>> +#endif /* IS_ENABLED(CONFIG_RPMSG_CTRL) */
>> +
>>  #endif /* _LINUX_RPMSG_H */
>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>> index e14c6dab4223..0b0cb028e0b3 100644
>> --- a/include/uapi/linux/rpmsg.h
>> +++ b/include/uapi/linux/rpmsg.h
>> @@ -9,6 +9,20 @@
>>  #include <linux/ioctl.h>
>>  #include <linux/types.h>
>>  
>> +/**
>> + * enum rpmsg_services - list of supported RPMsg services
>> + *
>> + * @RPMSG_RAW_SERVICE: char device RPMSG service
>> + * @RPMSG_START_PRIVATE_SERVICES: private services have to be declared after.
>> + */
>> +enum rpmsg_services {
>> +	/* Reserved services */
>> +	RPMSG_RAW_SERVICE =  0,
>> +
> 
> What kind of things do you envision this list to contain in a year from
> now?

The next one is the TTY, and perhaps the RPMsg I2C that allows to share a I2C
bus between the main and a remote processor.

Please notice that this field will also has to be added in a second step to the
rpmsg_endpoint_info struct, to allow the applications to select the expected
service.

Regards
Arnaud

> 
> Regards,
> Bjorn
> 
>> +	/* Private services */
>> +	RPMSG_START_PRIVATE_SERVICES =  1024,
>> +};
>> +
>>  /**
>>   * struct rpmsg_endpoint_info - endpoint info representation
>>   * @name: name of service
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[Arnaud POULIQUEN]

Hello Bjorn,

On 1/5/21 12:03 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> This series is a restructuring of the RPMsg char driver, to create a generic
>> RPMsg ioctl interface for all rpmsg services.
>>
>> The RPMsg char driver provides interfaces that:
>> - expose a char RPMsg device for communication with the remote processor,
>> - expose controls interface for applications to create and release endpoints.
>>
>> The objective of this series is to decorrelate the two interfaces:
>>   - Provide a char device for a RPMsg raw service in the rpmsg_char that can be
>>     probed by a RPMsg bus on a ns announcement.
>>   - Generalize the use of the ioctl for all RPMsg services by creating the
>>     rpmsg_ctrl, but keep it compatibile with the legacy.
>>
>> If the V1 create a new rpmsg_raw driver in addition to the rpmsg_ctrl this
>> version try to reuse the rpmsg_char driver by addapting QCOM GLINK and SMD
>> drivers.
>> So a goal of this version is to help to determine the best strategy to move
>> forward:
>>   - reuse rpmsg_char.
>>   - introduce a new driver and keep rpmsg_char as a legacy driver for a while.
>>
>> Notice that SMD and GLINK patches have to be tested, only build has been tested.
>>
>> 1) RPMsg control driver: rpmsg_ctrl.c
>>   This driver is based on the control part of the RPMsg_char driver. 
>>   On probe a /dev/rpmsg_ctrl<X> interface is created to allow to manage the
>>   channels.
>>   The principles are the following:
>>   - The RPMsg service driver registers it's name and the associated service
>>     using the rpmsg_ctrl_unregister_ctl API. The list of supported services
>>     is defined in  include/uapi/linux/rpmsg.h and exposed to the
>>     application thanks to a new field in rpmsg_endpoint_info struct.
>>   - On the RPMsg bus probe(e.g virtio bus) an rpmsg_ctrl device is
>>     registered that creates the control interface.
>>   - The application can then create or release a channel by specifying:
>>        - the name service
>>        - the source address.
>>        - the destination address.
> 
> Why is this useful?
I'm not sure to understand what is behind your question.
I guess the question is why is it useful to create a channel?
Mainly to use same way to probe a RPMsg service than the NS announcement.

> 
>>   - The rpmsg_ctrl uses the same interface than the ns announcement to
>>     create and release the associated channel but using the driver_override
>>     field to force the service name.
>>     The  "driver_override" allows to force the name service associated to
>>     an RPMsg driver, bypassing the rpmsg_device_id based match check.
> 
> You mean, the chinfo driver_override allows the ioctl to specify which
> driver should be bound to the device created for the newly registered
> endpoint?

Yes exactly, this is the main point of the proposal. Having the same RPMsg
driver that can be probed either by the remote side using the NS announcement
mechanism or by the rpmsg ctrl interface.
The driver "just" has to register to the RPMsg ctrl which service it supports.

> 
>>   - At least for virtio bus, an associated ns announcement is sent to the
>>     remote side.  
>>
>> 2) rpmsg char driver: rpmsg_char.c
>>     - The rpmsg class has not been removed. The associated attributes
>>       are already available in /sys/bus/rpmsg/.
> 
> So today a rpmsg_device gets the same attributes both from the class and
> the bus? So the only difference is that there will no longer be a
> /sys/class/rpmsg ?

Yes, if the rpmsg_char is probed by the bus,
My proposal is to suppress attributes in /sys/class/rpmsg as they are already
defined in /sys/bus/rpmsg/devices/
But class attribute can be kept if needed...

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>>     - The eptdev device is now an RPMsg device probed by a RPMsg bus driver
>>       (probed only by the ioctl in rpmsg_char driver).
>>
>> Know current Limitations:
>> - Tested only with virtio RPMsg bus and for one vdev instance.
>> - The glink and smd drivers adaptations have not been tested (not able to test).
>> - To limit commit and not update the IOCT interface some features have been not
>>   implemented in this first step:
>>     - the NS announcement as not been updated, it is not possible to create an
>>       endpoint with a destibnation address set to RPMSG_ADDR_ANY (-1),
>>     - not possible to destroy the channel,
>>     - only the "rpmsg-raw" service is supported.
>>
>> This series can be applied in Bjorn's rpmsg-next branch on top of the
>> RPMsg_ns series(4c0943255805).
>>
>> This series can be tested using rpmsgexport tools available here:
>> https://github.com/andersson/rpmsgexport.
>> ---
>> new from V1[1]:
>> - In V1 the rpmsg_char.c was not impacted, a rpmsg_raw.c has been created
>>   instead.
>> - IOCTL interface as not been updated (to go by steps).
>> - smd and glink drivers has been updated to support channels creation and
>>   release.
>>
>> [1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=327277
>>
>> Arnaud Pouliquen (16):
>>   rpmsg: introduce RPMsg control driver for channel creation
>>   rpmsg: add RPMsg control API to register service
>>   rpmsg: add override field in channel info
>>   rpmsg: ctrl: implement the ioctl function to create device
>>   rpmsg: ns: initialize channel info override field
>>   rpmsg: add helper to register the rpmsg ctrl device
>>   rpmsg: char: clean up rpmsg class
>>   rpmsg: char: make char rpmsg a rpmsg device without the control part
>>   rpmsg: char: register RPMsg raw service to the ioctl interface.
>>   rpmsg: char: allow only one endpoint per device
>>   rpmsg: char: check destination address is not null
>>   rpmsg: virtio: use the driver_override in channel creation ops
>>   rpmsg: virtio: probe the rpmsg_ctl device
>>   rpmsg: glink: add create and release rpmsg channel ops
>>   rpmsg: smd: add create and release rpmsg channel ops
>>   rpmsg: replace rpmsg_chrdev_register_device use
>>
>>  drivers/rpmsg/Kconfig             |   8 +
>>  drivers/rpmsg/Makefile            |   1 +
>>  drivers/rpmsg/qcom_glink_native.c |  96 +++++++--
>>  drivers/rpmsg/qcom_smd.c          |  59 +++++-
>>  drivers/rpmsg/rpmsg_char.c        | 246 ++++++-----------------
>>  drivers/rpmsg/rpmsg_ctrl.c        | 320 ++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h    |  14 --
>>  drivers/rpmsg/rpmsg_ns.c          |   1 +
>>  drivers/rpmsg/virtio_rpmsg_bus.c  |  38 +++-
>>  include/linux/rpmsg.h             |  40 ++++
>>  include/uapi/linux/rpmsg.h        |  14 ++
>>  11 files changed, 606 insertions(+), 231 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 05/16] rpmsg: ns: initialize channel info override field

[Arnaud POULIQUEN]

On 1/5/21 1:38 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> By default driver_override should be 0 to avoid to force
>> the channel creation with a specified name.The local variable
>> is not initialized.
>>
> 
> The same problem exists in qcom_glink_native, qcom_smd and rpmsg_char.

Right! And perhaps initializing the structure on declaration would be a better
method.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ns.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 762ff1ae279f..a526bff62947 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -55,6 +55,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>>  	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
>>  	chinfo.src = RPMSG_ADDR_ANY;
>>  	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
>> +	chinfo.driver_override = NULL;
>>  
>>  	dev_info(dev, "%sing channel %s addr 0x%x\n",
>>  		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 07/16] rpmsg: char: clean up rpmsg class

[Arnaud POULIQUEN]

On 1/5/21 1:54 AM, Bjorn Andersson wrote:
> On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:
> 
>> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
>>
>> This patch doesn't "clean up" the class, as described in $subject. It
>> just removes it.
>>
>>> Suppress the management of the rpmsg class as attribute. It is already
>>> handled in /sys/bus rpmsg as specified in
>>> documentation/ABI/testing/sysfs-bus-rpmsg.
>>
>> Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes
>> should be associated with the rpmsg_device (and thereby present in its
>> sysfs directory). But if these attributes are also added by the bus,
>> then why do we have this code? If that's the case this seems like a nice
>> cleanup that we should do outside/before merging the other patches.
>>
>>> This patch prepares the migration of the control device in rpmsg_ctrl.
>>>
>>
>> It would be useful if the commit message described how it prepares for
>> the migration and why.
>>
> 
> Now I see what this patch does, it removes the attributes from the
> character device's struct device, because they are provided by the
> struct rpmsg_device's bus!
> 
> I wish your commit message made this obvious.
> 
> Also, this implies that for a few patches here rpmsg_char is just
> broken - which I don't like.

I will move this patch at the end of the series and clarify commit message

Thanks
Arnaud

> 
> Regards,
> Bjorn
> 
>> Regards,
>> Bjorn
>>
>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>> ---
>>>  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------
>>>  1 file changed, 48 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index 4bbbacdbf3bb..732f5caf068a 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -27,7 +27,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);
>>> @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {
>>>  	.compat_ioctl = compat_ptr_ioctl,
>>>  };
>>>  
>>> -static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>>> -			 char *buf)
>>> -{
>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
>>> -
>>> -	return sprintf(buf, "%s\n", eptdev->chinfo.name);
>>> -}
>>> -static DEVICE_ATTR_RO(name);
>>> -
>>> -static ssize_t src_show(struct device *dev, struct device_attribute *attr,
>>> -			 char *buf)
>>> -{
>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
>>> -
>>> -	return sprintf(buf, "%d\n", eptdev->chinfo.src);
>>> -}
>>> -static DEVICE_ATTR_RO(src);
>>> -
>>> -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,
>>> -			 char *buf)
>>> -{
>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
>>> -
>>> -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);
>>> -}
>>> -static DEVICE_ATTR_RO(dst);
>>> -
>>> -static struct attribute *rpmsg_eptdev_attrs[] = {
>>> -	&dev_attr_name.attr,
>>> -	&dev_attr_src.attr,
>>> -	&dev_attr_dst.attr,
>>> -	NULL
>>> -};
>>> -ATTRIBUTE_GROUPS(rpmsg_eptdev);
>>> -
>>>  static void rpmsg_eptdev_release_device(struct device *dev)
>>>  {
>>>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>>> @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>>  	init_waitqueue_head(&eptdev->readq);
>>>  
>>>  	device_initialize(dev);
>>> -	dev->class = rpmsg_class;
>>>  	dev->parent = &ctrldev->dev;
>>> -	dev->groups = rpmsg_eptdev_groups;
>>>  	dev_set_drvdata(dev, eptdev);
>>>  
>>>  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
>>> @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>  	dev = &ctrldev->dev;
>>>  	device_initialize(dev);
>>>  	dev->parent = &rpdev->dev;
>>> -	dev->class = rpmsg_class;
>>>  
>>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>>  	ctrldev->cdev.owner = THIS_MODULE;
>>> @@ -553,17 +514,9 @@ static int rpmsg_char_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);
>>>  	}
>>>  
>>> @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_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);
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH v2 10/16] rpmsg: char: allow only one endpoint per device

[Arnaud POULIQUEN]

On 1/5/21 1:59 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Only one endpoint can be created per device, prevent from multi open.
>>
> 
> Having multiple invocations of rpmsg_create_ept() with the same chinfo
> sounds like a bad idea. I think in the SMD and GLINK case the underlying
> transport would complain that the related chinfo is already "busy", but
> this seems like an appropriate fix regardless.
> 
> Please add a proper Fixes: tag and send this outside of this patch
> series.

I will send it in a separate patch.

Regards,
Arnaud

> 
> Thanks,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 66e01b979e72..4b0674a2e3e9 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -122,6 +122,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>>  	struct device *dev = &eptdev->dev;
>>  
>> +	if (eptdev->ept)
>> +		return -EBUSY;
>> +
>>  	get_device(dev);
>>  
>>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 11/16] rpmsg: char: check destination address is not null

[Arnaud POULIQUEN]

On 1/5/21 2:03 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> The name service announcement is not sent if no endpoint is created by
>> default. If the destination address is not precised by the
>> application when creating the device (thanks to the RPMsg CTRL interface),
>> it is not possible to have a valid RPMsg channel.
>>
> 
> In the Qualcomm transports, the chinfo.name is used to identify the
> channel, so there it's valid to create a endpoint without dst.

So to be move in rpmsg virtio...either reporting an error or generating a NS
announcement.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 4b0674a2e3e9..8b1928594d10 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -305,6 +305,16 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>  	struct device *dev;
>>  	int ret;
>>  
>> +	/* There is not ept created by default. As consequence there is not NS
>> +	 * announcement and the destination address has to be set.
>> +	 * this limitation could be solved in future by adding a helper in
>> +	 * rpmsg_ns.
>> +	 */
>> +	if (rpdev->dst == RPMSG_ADDR_ANY) {
>> +		dev_err(dev, "destination address invalid (%d)\n", rpdev->dst);
>> +		return -EINVAL;
>> +	}
>> +
>>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>>  	if (!eptdev)
>>  		return -ENOMEM;
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 14/16] rpmsg: glink: add create and release rpmsg channel ops

[Arnaud POULIQUEN]

On 1/5/21 2:08 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Add the new ops introduced by the rpmsg_ns series and used
>> by the rpmsg_ctrl driver to instantiate a new rpmsg channel.
>>
> 
> This is nice for completeness sake, but I don't think it makes sense for
> transports that has the nameserver "builtin" to the transport itself.
> 
> I.e. if we have the NS sitting on top of GLINK and the remote firmware
> sends a "create channel" message, then this code would cause Linux to
> send a in-transport "create channel" message back to the remote side in
> hope that it would be willing to talk on that channel - but that would
> imply that the remote side needs to have registered a rpmsg device
> related to that service name - in which case it already sent a
> in-transport "create channel" message.

That was one of my main concerns about this series. I'm not familiar enough with
these drivers to make sure my proposal was 100% compatible...
How is the mapping between between the local and remote endpoints when the DST
address is not specified by the Linux application?

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/qcom_glink_native.c | 94 ++++++++++++++++++++++++-------
>>  1 file changed, 75 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 27a05167c18c..d74c338de077 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -205,6 +205,9 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>>  #define GLINK_FEATURE_INTENTLESS	BIT(1)
>>  
>>  static void qcom_glink_rx_done_work(struct work_struct *work);
>> +static struct rpmsg_device *
>> +qcom_glink_create_rpdev(struct qcom_glink *glink,
>> +			struct rpmsg_channel_info *chinfo);
>>  
>>  static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
>>  						      const char *name)
>> @@ -1203,6 +1206,37 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
>>  	return 0;
>>  }
>>  
>> +static struct rpmsg_device *
>> +qcom_glink_create_channel(struct rpmsg_device *rp_parent,
>> +			  struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
>> +	struct qcom_glink *glink = channel->glink;
>> +	struct rpmsg_device *rpdev;
>> +	const char *name = chinfo->name;
>> +
>> +	channel = qcom_glink_alloc_channel(glink, name);
>> +	if (IS_ERR(channel))
>> +		return ERR_PTR(PTR_ERR(channel));
>> +
>> +	rpdev = qcom_glink_create_rpdev(glink, chinfo);
>> +	if (!IS_ERR(rpdev)) {
>> +		rpdev->ept = &channel->ept;
>> +		channel->rpdev = rpdev;
>> +	}
>> +
>> +	return rpdev;
>> +}
>> +
>> +static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
>> +				      struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(rpdev->ept);
>> +	struct qcom_glink *glink = channel->glink;
>> +
>> +	return rpmsg_unregister_device(glink->dev, chinfo);
>> +}
>> +
>>  static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>>  {
>>  	struct glink_channel *channel = to_glink_channel(ept);
>> @@ -1359,6 +1393,8 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
>>  static const struct rpmsg_device_ops glink_device_ops = {
>>  	.create_ept = qcom_glink_create_ept,
>>  	.announce_create = qcom_glink_announce_create,
>> +	.create_channel = qcom_glink_create_channel,
>> +	.release_channel = qcom_glink_release_channel,
>>  };
>>  
>>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>> @@ -1376,13 +1412,45 @@ static void qcom_glink_rpdev_release(struct device *dev)
>>  	kfree(rpdev);
>>  }
>>  
>> +static struct rpmsg_device *
>> +qcom_glink_create_rpdev(struct qcom_glink *glink,
>> +			struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct rpmsg_device *rpdev;
>> +	struct device_node *node;
>> +	int ret;
>> +
>> +	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
>> +	if (!rpdev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
>> +	rpdev->src = chinfo->src;
>> +	rpdev->dst = chinfo->dst;
>> +	rpdev->ops = &glink_device_ops;
>> +
>> +	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
>> +	rpdev->dev.of_node = node;
>> +	rpdev->dev.parent = glink->dev;
>> +	rpdev->dev.release = qcom_glink_rpdev_release;
>> +	rpdev->driver_override = (char *)chinfo->driver_override;
>> +
>> +	ret = rpmsg_register_device(rpdev);
>> +	if (ret) {
>> +		kfree(rpdev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return rpdev;
>> +}
>> +
>>  static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>>  			      char *name)
>>  {
>>  	struct glink_channel *channel;
>>  	struct rpmsg_device *rpdev;
>>  	bool create_device = false;
>> -	struct device_node *node;
>> +	struct rpmsg_channel_info chinfo;
>>  	int lcid;
>>  	int ret;
>>  	unsigned long flags;
>> @@ -1416,27 +1484,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>>  	complete_all(&channel->open_req);
>>  
>>  	if (create_device) {
>> -		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
>> -		if (!rpdev) {
>> -			ret = -ENOMEM;
>> +		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
>> +		chinfo.src = RPMSG_ADDR_ANY;
>> +		chinfo.dst = RPMSG_ADDR_ANY;
>> +		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
>> +		if (IS_ERR(rpdev)) {
>> +			ret = PTR_ERR(rpdev);
>>  			goto rcid_remove;
>>  		}
>> -
>>  		rpdev->ept = &channel->ept;
>> -		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
>> -		rpdev->src = RPMSG_ADDR_ANY;
>> -		rpdev->dst = RPMSG_ADDR_ANY;
>> -		rpdev->ops = &glink_device_ops;
>> -
>> -		node = qcom_glink_match_channel(glink->dev->of_node, name);
>> -		rpdev->dev.of_node = node;
>> -		rpdev->dev.parent = glink->dev;
>> -		rpdev->dev.release = qcom_glink_rpdev_release;
>> -
>> -		ret = rpmsg_register_device(rpdev);
>> -		if (ret)
>> -			goto rcid_remove;
>> -
>>  		channel->rpdev = rpdev;
>>  	}
>>  
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Arnaud POULIQUEN]

On 1/5/21 2:33 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Implement the ioctl function that parses the list of
>> rpmsg drivers registered to create an associated device.
>> To be ISO user API, in a first step, the driver_override
>> is only allowed for the RPMsg raw service, supported by the
>> rpmsg_char driver.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 065e2e304019..8381b5b2b794 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>  	return 0;
>>  }
>>  
>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>> +{
>> +	struct rpmsg_ctl_info *drv_info;
>> +
>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>> +		if (drv_info->ctrl->service == service)
>> +			return drv_info->ctrl->drv_name;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>  				 unsigned long arg)
>>  {
>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>> -
>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>> +	void __user *argp = (void __user *)arg;
>> +	struct rpmsg_channel_info chinfo;
>> +	struct rpmsg_endpoint_info eptinfo;
>> +	struct rpmsg_device *newch;
>> +
>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * In a frst step only the rpmsg_raw service is supported.
>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>> +	 */
>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>> +	if (!chinfo.driver_override)
>> +		return -ENODEV;
>> +
>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>> +	chinfo.src = eptinfo.src;
>> +	chinfo.dst = eptinfo.dst;
>> +
>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> 
> Afaict this would create and announce and endpoint (or possibly find a
> endpoint announced by the other side of the link).

It depends on how rpdev is initialized[1].
 - For the rpmsg_char no default endpoint is created. The endpoint is created on
/dev/rpmsgX open. So the channel is created but not announced.
=> both sides have to know the the destination address for virtio implementation.

- For the rpmsg TTY the endpoint should be created by default by the RPMsg core
and associated to the rpdev. An announcement is sent to the remote side.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L445

> 
> In the case of the Qualcomm transports, and as been discussed to
> introduce for virtio in the past, the channel actually have a state. So
> opening/announcing it here means that we have no way to close and reopen
> this channel later?

In this first series I just focused to de-correlate the control part from the
rpmsg char. A main difference is that a channel is associated to a cdev.

But the ioctrl can be extended to close the cdev and the associated channel
(implemented in my V1).
else the rpmsg device is automatically remove by the rpmsg bus.
> 
> 
> It would also mean that we announce to the firmware that there's an
> application in Linux now ready to receive data on this channel - but
> that won't be the case until someone actually open the created cdev (or
> tty in your case) - which quite likely will result in data loss.

With the virtio implementation it is potentially already the case. When Linux
receive an NS announcement, there is no mechanism to inform the remote firmware
that Linux is ready to receive data. Some OpenAMP lib user already point out
this issue.
In glink driver seems that there is no such issue as
qcom_glink_send_open/close_req allow to provide information on endpoint state.

I would propose to address this in a next step.

> 
> I think instead of piggybacking on the rpmsg_device we should just carry
> these "raw exports to userspace" in some other construct - perhaps a
> auxiliary_bus, 

I'm not familiar with auxilary-bus but seems very similar to the rpmsg_bus...
I wonder if this could lead to code duplication in RPMsg service drivers to
support the control but also the NS announcement.

or if we still only care for char and tty, not split them
> up at all using the device model.

The initial requirement was to extend the control interface implemented in
rpmsg_char to other services before introducing new one.

So probably as a first step we have to clarify the requirements to determine the
solution to implement.

Here is my point of view on the induced requirements:
- Allow to create a service from Linux user application:
	- with a specific name
	- with or without name service announcement.
- Allow to probe the same service by receiving either a NS announcement from the
remote firmware or a Linux user application request.
- Use these services independently of the RPMsg transport implementation (e.g be
able to use RPMSg char with the RPMsg virtio bus).

This requirements explain my approach: associate a service to a RPMsg device in
order to be able to probe using the same driver either by the remote firmware NS
announcement or by a Linux user application.

Is the requirements I detailed match with what you had in mind?

Please, could you detail your views on the use of the auxilary bus in this context?

We can also think about an alternative to keep rpmsg_char unchanged for legacy
support.
 - only create a RPMsg ctrl for new RPMsg services
 - enable it for virtio_rpmsg_bus (In this case the rpmsg char cannot be probed
by remote firmware, but allows communication between fixed addresses)

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +	if (!newch) {
>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>> +		return -ENXIO;
>> +	}
>>  
>>  	return 0;
>>  };
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[Mathieu Poirier]

Hi Arnaud,

[...]

> 
> Arnaud Pouliquen (16):
>   rpmsg: introduce RPMsg control driver for channel creation
>   rpmsg: add RPMsg control API to register service
>   rpmsg: add override field in channel info
>   rpmsg: ctrl: implement the ioctl function to create device
>   rpmsg: ns: initialize channel info override field
>   rpmsg: add helper to register the rpmsg ctrl device
>   rpmsg: char: clean up rpmsg class
>   rpmsg: char: make char rpmsg a rpmsg device without the control part
>   rpmsg: char: register RPMsg raw service to the ioctl interface.
>   rpmsg: char: allow only one endpoint per device
>   rpmsg: char: check destination address is not null
>   rpmsg: virtio: use the driver_override in channel creation ops
>   rpmsg: virtio: probe the rpmsg_ctl device
>   rpmsg: glink: add create and release rpmsg channel ops
>   rpmsg: smd: add create and release rpmsg channel ops
>   rpmsg: replace rpmsg_chrdev_register_device use
> 
>  drivers/rpmsg/Kconfig             |   8 +
>  drivers/rpmsg/Makefile            |   1 +
>  drivers/rpmsg/qcom_glink_native.c |  96 +++++++--
>  drivers/rpmsg/qcom_smd.c          |  59 +++++-
>  drivers/rpmsg/rpmsg_char.c        | 246 ++++++-----------------
>  drivers/rpmsg/rpmsg_ctrl.c        | 320 ++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |  14 --
>  drivers/rpmsg/rpmsg_ns.c          |   1 +
>  drivers/rpmsg/virtio_rpmsg_bus.c  |  38 +++-
>  include/linux/rpmsg.h             |  40 ++++
>  include/uapi/linux/rpmsg.h        |  14 ++
>  11 files changed, 606 insertions(+), 231 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

I am finally coming around to review this set.  I see that you already had an
extensive conversation with Bjorn - did you want me to have a look as well or
should I wait for the next revision?

Thanks,
Mathieu

> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

[Arnaud POULIQUEN]

Hi Mathieu,

On 1/13/21 9:31 PM, Mathieu Poirier wrote:
> Hi Arnaud,
> 
> [...]
> 
>>
>> Arnaud Pouliquen (16):
>>   rpmsg: introduce RPMsg control driver for channel creation
>>   rpmsg: add RPMsg control API to register service
>>   rpmsg: add override field in channel info
>>   rpmsg: ctrl: implement the ioctl function to create device
>>   rpmsg: ns: initialize channel info override field
>>   rpmsg: add helper to register the rpmsg ctrl device
>>   rpmsg: char: clean up rpmsg class
>>   rpmsg: char: make char rpmsg a rpmsg device without the control part
>>   rpmsg: char: register RPMsg raw service to the ioctl interface.
>>   rpmsg: char: allow only one endpoint per device
>>   rpmsg: char: check destination address is not null
>>   rpmsg: virtio: use the driver_override in channel creation ops
>>   rpmsg: virtio: probe the rpmsg_ctl device
>>   rpmsg: glink: add create and release rpmsg channel ops
>>   rpmsg: smd: add create and release rpmsg channel ops
>>   rpmsg: replace rpmsg_chrdev_register_device use
>>
>>  drivers/rpmsg/Kconfig             |   8 +
>>  drivers/rpmsg/Makefile            |   1 +
>>  drivers/rpmsg/qcom_glink_native.c |  96 +++++++--
>>  drivers/rpmsg/qcom_smd.c          |  59 +++++-
>>  drivers/rpmsg/rpmsg_char.c        | 246 ++++++-----------------
>>  drivers/rpmsg/rpmsg_ctrl.c        | 320 ++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h    |  14 --
>>  drivers/rpmsg/rpmsg_ns.c          |   1 +
>>  drivers/rpmsg/virtio_rpmsg_bus.c  |  38 +++-
>>  include/linux/rpmsg.h             |  40 ++++
>>  include/uapi/linux/rpmsg.h        |  14 ++
>>  11 files changed, 606 insertions(+), 231 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> I am finally coming around to review this set.  I see that you already had an
> extensive conversation with Bjorn - did you want me to have a look as well or
> should I wait for the next revision?

Based on Bjorn first feedback, my understanding is that the management based on
create/destroy channel does not match with the QCOM RPMsg backend
implementation. I think this is the blocking point of my V2 implementation.

Before sending a new revision i would hope that we have a roundtable discussion
to clarify the direction to move forward, to avoid sending useless revisions.

As discussed in [1], there are different alternatives, that probably depend on
the features we expect to support.
I tried to sum-up the requirement I have in mind in [1].

The 2 main directions I can see are:
- rework the rpmsg_char to match with all rpmsg backend (V2 implementation)
    to be honest i don't know how to move forward in this direction as QCOM and
    virtio backends are rather different.
- not modify the rpmsg_char but create the rpmsg_ctrl (and perhaps also a
rpmsg_raw for a /dev/rpmsg data interface) that would use the create/destroy
channel such as the rpmsg ns (V1 implementation).
    one advantage of this solution is that this does not impact QCOM drivers.
    one drawback is that we duplicate the code.

[1]
https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-5-arnaud.pouliquen@foss.st.com/

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

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>>
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 01/16] rpmsg: introduce RPMsg control driver for channel creation

[Mathieu Poirier]

Hi Arnaud,

On Tue, Dec 22, 2020 at 11:57:11AM +0100, Arnaud Pouliquen wrote:
> The RPMsg_ctrl driver is a duplication of the ioctrl part of the
> rpmsg_char driver to make generic the ioctl to manage channels by
> the userspace applications.
> 
> As a first step, this driver just creates the /dev/rpmsg_ctl<x>
> ( <x> is the instance value). The ioctl is not implemented.
> 
> Notice that this driver is associated to a RPMsg device with no endpoint.
> Instantiating this device as an RPMsg device allows to retrieve the
> associated RPMsg backend.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/Kconfig      |   8 ++
>  drivers/rpmsg/Makefile     |   1 +
>  drivers/rpmsg/rpmsg_ctrl.c | 208 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..c9e602016c3b 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -23,6 +23,14 @@ config RPMSG_NS
>  	  channel that probes the associated RPMsg device on remote endpoint
>  	  service announcement.
>  
> +config RPMSG_CTRL
> +	tristate "RPMSG control interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the /dev/rpmsg_ctl API. this API
> +	  allows user-space programs to create channels with specific name,
> +	  source and destination addresses.
> +
>  config RPMSG_MTK_SCP
>  	tristate "MediaTek SCP"
>  	depends on MTK_SCP
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..3c1bce9d0228 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> new file mode 100644
> index 000000000000..425c3e32ada4
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "rpmsg_internal.h"
> +#include <uapi/linux/rpmsg.h>
> +
> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> +
> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl_dev, dev)
> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl_dev, cdev)
> +
> +/**
> + * struct rpmsg_ctrl_dev - control device for instantiating endpoint devices
> + * @rpdev:	underlaying rpmsg device
> + * @cdev:	cdev for the ctrl device
> + * @dev:	device for the ctrl device
> + */
> +struct rpmsg_ctrl_dev {
> +	struct rpmsg_device *rpdev;
> +	struct cdev cdev;
> +	struct device dev;
> +};

As Bjorn pointed out it would be better modify rpmsg_char.c to what you want it
to be rather than spinning off a parallel implementation.  One immediate
advantage would be to avoid the creation of new structures that are identical to
existing ones.  There's also the problem of bisecting...  If I remember
correctly there are a few instances where applying this set would break a bisect
session.

> +
> +static DEFINE_IDA(rpmsg_ctrl_ida);
> +static DEFINE_IDA(rpmsg_minor_ida);
> +
> +static dev_t rpmsg_major;
> +
> +static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	get_device(&ctrldev->dev);
> +	filp->private_data = ctrldev;
> +
> +	return 0;
> +}
> +
> +static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> +
> +	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +
> +	return 0;
> +};
> +
> +static int rpmsg_ctrl_dev_release(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	put_device(&ctrldev->dev);
> +
> +	return 0;
> +}
> +
> +static void rpmsg_ctrl_dev_release_device(struct device *dev)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = dev_to_ctrldev(dev);
> +
> +	dev_err(dev, "%s\n", __func__);

Debug left over.

> +
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +	cdev_del(&ctrldev->cdev);
> +	kfree(ctrldev);
> +}
> +
> +static const struct file_operations rpmsg_ctrl_fops = {
> +	.owner = THIS_MODULE,
> +	.open = rpmsg_ctrl_dev_open,
> +	.release = rpmsg_ctrl_dev_release,
> +	.unlocked_ioctl = rpmsg_ctrl_dev_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +static int rpmsg_ctrl_add_control(struct rpmsg_ctrl_dev *ctrldev)
> +{
> +	struct device *dev = &ctrldev->dev;
> +	int ret;
> +
> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
> +	ctrldev->cdev.owner = THIS_MODULE;
> +
> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
> +	dev_set_name(dev, "rpmsg_ctrl%d", dev->id);

Here "rpmsg_ctrl" is used but the changelog and refers to "rpmsg_ctl".  Using
the latter would, as Bjorn pointed out, break existing implementation.

> +
> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> +	if (ret)
> +		goto free_minor_ida;
> +
> +	dev_info(dev, "add %s control for %s driver\n",
> +		 dev_name(dev),  dev_name(dev->parent));
> +
> +	return 0;
> +
> +free_minor_ida:
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +
> +	return ret;
> +}
> +
> +static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl_dev *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;
> +
> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_ctrldev;
> +
> +	dev->id = ret;
> +
> +	ret = rpmsg_ctrl_add_control(ctrldev);
> +	if (ret < 0)
> +		goto free_ctrl_ida;
> +
> +	/* We can now rely on the release function for cleanup */
> +	dev->release = rpmsg_ctrl_dev_release_device;
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> +		put_device(dev);
> +		goto free_ctrl_ida;
> +	}
> +
> +	dev_set_drvdata(dev, ctrldev);
> +	dev_set_drvdata(&rpdev->dev, ctrldev);
> +
> +	return 0;
> +
> +free_ctrl_ida:
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +free_ctrldev:
> +	put_device(dev);
> +	kfree(ctrldev);
> +
> +	return ret;
> +}
> +
> +static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl_dev *ctrldev = dev_get_drvdata(&rpdev->dev);
> +
> +	device_del(&ctrldev->dev);
> +	put_device(&ctrldev->dev);
> +}
> +
> +static struct rpmsg_driver rpmsg_ctrl_driver = {
> +	.drv.name = KBUILD_MODNAME,
> +	.probe = rpmsg_ctrl_probe,
> +	.remove = rpmsg_ctrl_remove,
> +};
> +
> +static int rpmsg_ctrl_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
> +	if (ret < 0) {
> +		pr_err("rpmsg_ctrl: failed to allocate char dev region\n");
> +		return ret;
> +	}
> +
> +	ret = register_rpmsg_driver(&rpmsg_ctrl_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_ctrl_init);
> +
> +static void rpmsg_ctrl_exit(void)
> +{
> +	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +}
> +module_exit(rpmsg_ctrl_exit);
> +
> +MODULE_DESCRIPTION("ioctl rpmsg driver");
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH v2 02/16] rpmsg: add RPMsg control API to register service

[Mathieu Poirier]

On Tue, Dec 22, 2020 at 11:57:12AM +0100, Arnaud Pouliquen wrote:
> Add API to register a RPMsg service to the control device.
> The rpmsg_drv_ctrl_info structure links a service to its driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 57 ++++++++++++++++++++++++++++++++++++++
>  include/linux/rpmsg.h      | 31 +++++++++++++++++++++
>  include/uapi/linux/rpmsg.h | 14 ++++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 425c3e32ada4..065e2e304019 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -27,6 +27,20 @@ struct rpmsg_ctrl_dev {
>  	struct device dev;
>  };
>  
> +/**
> + * struct rpmsg_ctl_info - control info list node
> + * @ctrl:	control driver info
> + * @node:	list node
> + *
> + * This structure is used by rpmsg_ctl to list the registered drivers services
> + */
> +struct rpmsg_ctl_info {

struct rpmsg_ctrl_info

> +	const struct rpmsg_drv_ctrl_info *ctrl;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(rpmsg_drv_list);
> +
>  static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_minor_ida);
>  
> @@ -175,6 +189,49 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
>  	.remove = rpmsg_ctrl_remove,
>  };
>  
> +/**
> + * rpmsg_ctrl_register_ctl() -register control for the associated service
> + * @ctrl: rpmsg driver information
> + *
> + * This function is called by the rpmsg driver to register a service that will
> + * be exposed to be instantiate by the application.
> + */
> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)

Here an rpmsg_drv is registered with the infrastructure and as such I propose
rpmsg_ctrl_register_driver_ctrl().

> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	drv_info =  kzalloc(sizeof(*drv_info), GFP_KERNEL);
> +	if (!drv_info)
> +		return -ENOMEM;

When this driver is remove all drv_info left in rpmsg_drv_list should be free'd.
We can't count on users to call rpmsg_ctrl_unregister_ctl().

> +
> +	drv_info->ctrl = ctrl;

This has the potential of creating problems - we don't know then the memory
assocaited with @ctrl will disappear on us.

> +
> +	list_add_tail(&drv_info->node, &rpmsg_drv_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rpmsg_ctrl_register_ctl);
> +
> +/**
> + * rpmsg_ctrl_unregister_ctl() -unregister control for the associated service
> + * @ctrl: the rpmsg control information
> + *
> + * This function is called by the rpmsg driver to unregister the associated
> + * service.
> + */
> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +	struct rpmsg_ctl_info *drv_info, *tmp;
> +
> +	list_for_each_entry_safe(drv_info, tmp, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl == ctrl) {
> +			list_del(&drv_info->node);
> +			kfree(drv_info);
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(rpmsg_ctrl_unregister_ctl);
> +
>  static int rpmsg_ctrl_init(void)
>  {
>  	int ret;
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index a5db828b2420..5d64704c2346 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -26,6 +26,19 @@ struct rpmsg_endpoint;
>  struct rpmsg_device_ops;
>  struct rpmsg_endpoint_ops;
>  
> +/**
> + * struct rpmsg_drv_ctrl_info - rpmsg ctrl structure
> + * @drv_name:	name of the associated driver
> + * @service:	the associated rpmsg service listed in @rpmsg_services
> + *
> + * This structure is used by rpmsg_ctl to link the endpoint creation to the
> + * service rpmsg driver.
> + */
> +struct rpmsg_drv_ctrl_info {
> +	const char *drv_name;
> +	u32  service;
> +};
> +
>  /**
>   * struct rpmsg_channel_info - channel info representation
>   * @name: name of service
> @@ -315,4 +328,22 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>  	module_driver(__rpmsg_driver, register_rpmsg_driver, \
>  			unregister_rpmsg_driver)
>  
> +#if IS_ENABLED(CONFIG_RPMSG_CTRL)
> +
> +int  rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
> +void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl);
> +
> +#else
> +
> +static inline int rpmsg_ctrl_register_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +	return 0;
> +}
> +
> +static inline void rpmsg_ctrl_unregister_ctl(const struct rpmsg_drv_ctrl_info *ctrl)
> +{
> +}
> +
> +#endif /* IS_ENABLED(CONFIG_RPMSG_CTRL) */

Shouldn't this be in its own header, something like rpmsg_ctrl.h?

> +
>  #endif /* _LINUX_RPMSG_H */
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index e14c6dab4223..0b0cb028e0b3 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -9,6 +9,20 @@
>  #include <linux/ioctl.h>
>  #include <linux/types.h>
>  
> +/**
> + * enum rpmsg_services - list of supported RPMsg services
> + *
> + * @RPMSG_RAW_SERVICE: char device RPMSG service
> + * @RPMSG_START_PRIVATE_SERVICES: private services have to be declared after.
> + */
> +enum rpmsg_services {
> +	/* Reserved services */
> +	RPMSG_RAW_SERVICE =  0,
> +
> +	/* Private services */
> +	RPMSG_START_PRIVATE_SERVICES =  1024,

We have plenty of bits in an enum type - I would set this to 4096 just to be on
the safe side.

> +};
> +
>  /**
>   * struct rpmsg_endpoint_info - endpoint info representation
>   * @name: name of service
> -- 
> 2.17.1
> 

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Mathieu Poirier]

On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
> Implement the ioctl function that parses the list of
> rpmsg drivers registered to create an associated device.
> To be ISO user API, in a first step, the driver_override
> is only allowed for the RPMsg raw service, supported by the
> rpmsg_char driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 065e2e304019..8381b5b2b794 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl->service == service)
> +			return drv_info->ctrl->drv_name;
> +	}
> +

I'm unsure about the above... To me this looks like what the .match() function
of a bus would do.  And when I read Bjorn's comment he brought up the
auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
into.  Registering with a bus would streamline a lot of the code in this
patchset.

I'm out of time for today - I will continue tomorrow.

Thanks,
Mathieu

> +	return NULL;
> +}
> +
>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>  				 unsigned long arg)
>  {
>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> -
> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_channel_info chinfo;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_device *newch;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	/*
> +	 * In a frst step only the rpmsg_raw service is supported.
> +	 * The override is foorced to RPMSG_RAW_SERVICE
> +	 */
> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> +	if (!chinfo.driver_override)
> +		return -ENODEV;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> +	if (!newch) {
> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> +		return -ENXIO;
> +	}
>  
>  	return 0;
>  };
> -- 
> 2.17.1
> 

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Arnaud POULIQUEN]

Hi Mathieu,

On 1/22/21 12:52 AM, Mathieu Poirier wrote:
> On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
>> Implement the ioctl function that parses the list of
>> rpmsg drivers registered to create an associated device.
>> To be ISO user API, in a first step, the driver_override
>> is only allowed for the RPMsg raw service, supported by the
>> rpmsg_char driver.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 065e2e304019..8381b5b2b794 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>  	return 0;
>>  }
>>  
>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>> +{
>> +	struct rpmsg_ctl_info *drv_info;
>> +
>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>> +		if (drv_info->ctrl->service == service)
>> +			return drv_info->ctrl->drv_name;
>> +	}
>> +
> 
> I'm unsure about the above... To me this looks like what the .match() function
> of a bus would do.  And when I read Bjorn's comment he brought up the
> auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
> into.  Registering with a bus would streamline a lot of the code in this
> patchset.

As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
Look like duplication from my POV, except if the IOCTL does not manage channel
but only endpoint.

In my design I considered that the rpmsg_ctrl creates a channel associated to a
rpmsg_device such as the RPMsg ns_announcement.

Based on this assumption, if we implement the auxiliary_bus (or other) for the
rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
auxillary bus. The probe from the auxiliary bus would lead to the creation of an
RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
would probably make tricky the remove part.

That said, I think the design depends on the functionality that should be
implemented in the rpmsg_ctrl. Here is an alternative approach based on the
auxiliary bus, which I'm starting to think about:

The current approach of the rpmsg_char driver is to use the IOCTRL interface to
instantiate a cdev with an endpoint (the RPMsg device is associated with the
ioctl dev). This would correspond to the use of an auxiliary bus to manage local
endpoint creations.

We could therefore consider an RPMsg name service based on an RPmsg device. This
RPMsg device would register a kind of "RPMsg service endpoint" driver on the
auxiliary rpmsg_ioctl bus.
The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
on user application request the rpmsg_ctrl will call the appropriate auxiliary
device to create an endpoint.

If we consider that one objective of this series is to allow application to
initiate the communication with the remote processor, so to be able to initiate
the service (ns announcement sent to the remote processor).
This implies that:
-either the RPMsg device has been probed first by a remote ns announcement or by
a Linux kernel driver using the "driver_override", to register an auxiliary
device. In this case an endpoint will be created associated to the RPMsg service
- or create a RPMsg device on first ioctl endpoint creation request, if it does
not exist (that could trig a NS announcement to remote processor).

But I'm not sure that this approach would work with QCOM RPMsg backends...

> 
> I'm out of time for today - I will continue tomorrow.

It seems to me that the main point to step forward is to clarify the global
design and features of the rpmsg-ctrl.
Depending on the decision taken, this series could be trashed and rewritten from
a blank page...To not lost to much time on the series don't hesitate to limit
the review to the minimum.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>> +	return NULL;
>> +}
>> +
>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>  				 unsigned long arg)
>>  {
>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>> -
>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>> +	void __user *argp = (void __user *)arg;
>> +	struct rpmsg_channel_info chinfo;
>> +	struct rpmsg_endpoint_info eptinfo;
>> +	struct rpmsg_device *newch;
>> +
>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * In a frst step only the rpmsg_raw service is supported.
>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>> +	 */
>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>> +	if (!chinfo.driver_override)
>> +		return -ENODEV;
>> +
>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>> +	chinfo.src = eptinfo.src;
>> +	chinfo.dst = eptinfo.dst;
>> +
>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>> +	if (!newch) {
>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>> +		return -ENXIO;
>> +	}
>>  
>>  	return 0;
>>  };
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Mathieu Poirier]

On Fri, Jan 22, 2021 at 02:05:27PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 1/22/21 12:52 AM, Mathieu Poirier wrote:
> > On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
> >> Implement the ioctl function that parses the list of
> >> rpmsg drivers registered to create an associated device.
> >> To be ISO user API, in a first step, the driver_override
> >> is only allowed for the RPMsg raw service, supported by the
> >> rpmsg_char driver.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 41 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >> index 065e2e304019..8381b5b2b794 100644
> >> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
> >>  	return 0;
> >>  }
> >>  
> >> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> >> +{
> >> +	struct rpmsg_ctl_info *drv_info;
> >> +
> >> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> >> +		if (drv_info->ctrl->service == service)
> >> +			return drv_info->ctrl->drv_name;
> >> +	}
> >> +
> > 
> > I'm unsure about the above... To me this looks like what the .match() function
> > of a bus would do.  And when I read Bjorn's comment he brought up the
> > auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
> > into.  Registering with a bus would streamline a lot of the code in this
> > patchset.
> 
> As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
> Look like duplication from my POV, except if the IOCTL does not manage channel
> but only endpoint.
> 
> In my design I considered that the rpmsg_ctrl creates a channel associated to a
> rpmsg_device such as the RPMsg ns_announcement.
> 
> Based on this assumption, if we implement the auxiliary_bus (or other) for the
> rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
> auxillary bus. The probe from the auxiliary bus would lead to the creation of an
> RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
> would probably make tricky the remove part.
> 
> That said, I think the design depends on the functionality that should be
> implemented in the rpmsg_ctrl. Here is an alternative approach based on the
> auxiliary bus, which I'm starting to think about:
> 
> The current approach of the rpmsg_char driver is to use the IOCTRL interface to
> instantiate a cdev with an endpoint (the RPMsg device is associated with the
> ioctl dev). This would correspond to the use of an auxiliary bus to manage local
> endpoint creations.
> 
> We could therefore consider an RPMsg name service based on an RPmsg device. This
> RPMsg device would register a kind of "RPMsg service endpoint" driver on the
> auxiliary rpmsg_ioctl bus.
> The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
> on user application request the rpmsg_ctrl will call the appropriate auxiliary
> device to create an endpoint.
> 
> If we consider that one objective of this series is to allow application to
> initiate the communication with the remote processor, so to be able to initiate
> the service (ns announcement sent to the remote processor).
> This implies that:
> -either the RPMsg device has been probed first by a remote ns announcement or by
> a Linux kernel driver using the "driver_override", to register an auxiliary
> device. In this case an endpoint will be created associated to the RPMsg service
> - or create a RPMsg device on first ioctl endpoint creation request, if it does
> not exist (that could trig a NS announcement to remote processor).
> 
> But I'm not sure that this approach would work with QCOM RPMsg backends...
>

I don't think there is a way forward with this set without a clear understanding
of the Glink and SMD drivers.  I have already spent a fair amount of time in the
Glink driver and will continue on Monday with SMD.  

> > 
> > I'm out of time for today - I will continue tomorrow.
> 
> It seems to me that the main point to step forward is to clarify the global
> design and features of the rpmsg-ctrl.
> Depending on the decision taken, this series could be trashed and rewritten from
> a blank page...To not lost to much time on the series don't hesitate to limit
> the review to the minimum.
> 

I doubt you will ever get clear guidelines on the whole solution.  I will get
back to you once I am done with the SMD driver, which should be in the
latter part of next week.

> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >> +	return NULL;
> >> +}
> >> +
> >>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> >>  				 unsigned long arg)
> >>  {
> >>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> >> -
> >> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> >> +	void __user *argp = (void __user *)arg;
> >> +	struct rpmsg_channel_info chinfo;
> >> +	struct rpmsg_endpoint_info eptinfo;
> >> +	struct rpmsg_device *newch;
> >> +
> >> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> >> +		return -EINVAL;
> >> +
> >> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> +		return -EFAULT;
> >> +
> >> +	/*
> >> +	 * In a frst step only the rpmsg_raw service is supported.
> >> +	 * The override is foorced to RPMSG_RAW_SERVICE
> >> +	 */
> >> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> >> +	if (!chinfo.driver_override)
> >> +		return -ENODEV;
> >> +
> >> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> >> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> >> +	chinfo.src = eptinfo.src;
> >> +	chinfo.dst = eptinfo.dst;
> >> +
> >> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> >> +	if (!newch) {
> >> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> >> +		return -ENXIO;
> >> +	}
> >>  
> >>  	return 0;
> >>  };
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Arnaud POULIQUEN]

Hi Mathieu,

On 1/22/21 9:59 PM, Mathieu Poirier wrote:
> On Fri, Jan 22, 2021 at 02:05:27PM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 1/22/21 12:52 AM, Mathieu Poirier wrote:
>>> On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
>>>> Implement the ioctl function that parses the list of
>>>> rpmsg drivers registered to create an associated device.
>>>> To be ISO user API, in a first step, the driver_override
>>>> is only allowed for the RPMsg raw service, supported by the
>>>> rpmsg_char driver.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>> index 065e2e304019..8381b5b2b794 100644
>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>>>> +{
>>>> +	struct rpmsg_ctl_info *drv_info;
>>>> +
>>>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>>>> +		if (drv_info->ctrl->service == service)
>>>> +			return drv_info->ctrl->drv_name;
>>>> +	}
>>>> +
>>>
>>> I'm unsure about the above... To me this looks like what the .match() function
>>> of a bus would do.  And when I read Bjorn's comment he brought up the
>>> auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
>>> into.  Registering with a bus would streamline a lot of the code in this
>>> patchset.
>>
>> As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
>> Look like duplication from my POV, except if the IOCTL does not manage channel
>> but only endpoint.
>>
>> In my design I considered that the rpmsg_ctrl creates a channel associated to a
>> rpmsg_device such as the RPMsg ns_announcement.
>>
>> Based on this assumption, if we implement the auxiliary_bus (or other) for the
>> rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
>> auxillary bus. The probe from the auxiliary bus would lead to the creation of an
>> RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
>> would probably make tricky the remove part.
>>
>> That said, I think the design depends on the functionality that should be
>> implemented in the rpmsg_ctrl. Here is an alternative approach based on the
>> auxiliary bus, which I'm starting to think about:
>>
>> The current approach of the rpmsg_char driver is to use the IOCTRL interface to
>> instantiate a cdev with an endpoint (the RPMsg device is associated with the
>> ioctl dev). This would correspond to the use of an auxiliary bus to manage local
>> endpoint creations.
>>
>> We could therefore consider an RPMsg name service based on an RPmsg device. This
>> RPMsg device would register a kind of "RPMsg service endpoint" driver on the
>> auxiliary rpmsg_ioctl bus.
>> The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
>> on user application request the rpmsg_ctrl will call the appropriate auxiliary
>> device to create an endpoint.
>>
>> If we consider that one objective of this series is to allow application to
>> initiate the communication with the remote processor, so to be able to initiate
>> the service (ns announcement sent to the remote processor).
>> This implies that:
>> -either the RPMsg device has been probed first by a remote ns announcement or by
>> a Linux kernel driver using the "driver_override", to register an auxiliary
>> device. In this case an endpoint will be created associated to the RPMsg service
>> - or create a RPMsg device on first ioctl endpoint creation request, if it does
>> not exist (that could trig a NS announcement to remote processor).
>>
>> But I'm not sure that this approach would work with QCOM RPMsg backends...
>>
> 
> I don't think there is a way forward with this set without a clear understanding
> of the Glink and SMD drivers.  I have already spent a fair amount of time in the
> Glink driver and will continue on Monday with SMD.  
> 
>>>
>>> I'm out of time for today - I will continue tomorrow.
>>
>> It seems to me that the main point to step forward is to clarify the global
>> design and features of the rpmsg-ctrl.
>> Depending on the decision taken, this series could be trashed and rewritten from
>> a blank page...To not lost to much time on the series don't hesitate to limit
>> the review to the minimum.
>>
> 
> I doubt you will ever get clear guidelines on the whole solution.  I will get
> back to you once I am done with the SMD driver, which should be in the
> latter part of next week.


Thanks for your time past on this topic!

I don't expect a clear guidance but that we clarify the objective of this RPMsg
IOCTL. A first step would be sure that we are in line with the objective of the
RPMsg IOCTL.
For instance should we continue in a way to have the rpmsg_char more "rpmsg
service" generic, relying on a rpmsg_ioctl for the control part? Or should we
implement something independent (with is own API) to limit dependency with QCOM
backends constraints?
At the end, if implementing a IOCTL interface directly in the RPMsg TTY seems to
you and Bjorn simpler, I can also go on this way...

On my side I expect to find time this week to prototype a RPMSg ioctl using the
auxiliary bus to better understand involved mechanism.

Thanks,
Arnaud

> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>>>  				 unsigned long arg)
>>>>  {
>>>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>>>> -
>>>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>>>> +	void __user *argp = (void __user *)arg;
>>>> +	struct rpmsg_channel_info chinfo;
>>>> +	struct rpmsg_endpoint_info eptinfo;
>>>> +	struct rpmsg_device *newch;
>>>> +
>>>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>> +		return -EFAULT;
>>>> +
>>>> +	/*
>>>> +	 * In a frst step only the rpmsg_raw service is supported.
>>>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>>>> +	 */
>>>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>>>> +	if (!chinfo.driver_override)
>>>> +		return -ENODEV;
>>>> +
>>>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>>> +	chinfo.src = eptinfo.src;
>>>> +	chinfo.dst = eptinfo.dst;
>>>> +
>>>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>>>> +	if (!newch) {
>>>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>>>> +		return -ENXIO;
>>>> +	}
>>>>  
>>>>  	return 0;
>>>>  };
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Mathieu Poirier]

[...]

> > It seems to me that the main point to step forward is to clarify the global
> > design and features of the rpmsg-ctrl.
> > Depending on the decision taken, this series could be trashed and rewritten from
> > a blank page...To not lost to much time on the series don't hesitate to limit
> > the review to the minimum.
> > 
> 
> I doubt you will ever get clear guidelines on the whole solution.  I will get
> back to you once I am done with the SMD driver, which should be in the
> latter part of next week.
>

After looking at the rpmsg_chrdev driver, its current customers (i.e the Qcom
drivers), the rpmsg name service and considering the long term goals of this
patchset I have the following guidelines: 

1) I thought long and hard about how to split the current rpmsg_chrdev driver
between the control plane and the raw device plane and the end solution looks
much slimpler than I expected.  Exporting function rpmsg_eptdev_create() after
moving it to another file (along with other dependencies) should be all we need.
Calling rpmsg_eptdev_create() from rpmsg_ctrldev_ioctl() will automatically load
the new driver, the same way calling rpmsg_ns_register_device() from
rpmsg_probe() took care of loading the rpmsg_ns driver.

2) While keeping the control plane functionality related to
RPMSG_CREATE_EPT_IOCTL intact, introduce a new RPMSG_CREATE_DEV_IOCTL that will
allow for the instantiation of rpmsg_devices, exactly the same way a name service
announcement from a remote processor does.  I envision that code path to
eventually call rpmsg_create_channel().

3) Leave the rpmsg_channel_info structure intact and use the
rpmsg_channel_info::name to bind to a rpmsg_driver, exactly how it is currently
done for name service driver selection.  That will allow us to re-use the
current rpmsg_bus intrastructure, i.e rpmsg_bus::match(), without having to deal
with yet another bus type.  Proceeding this way gives us the opportunity to keep
the current channel name convention for other rpmch_chrdev users untouched.

4) In a prior conversation you indicated the intention of instantiating the
rpmsg_chrdev from the name service interface.  I agree with doing so but 
conjugating that with the RPMSG_CHAR kenrel define may be tricky.  I will wait
to see what you come up with.

I hope this helps.

Thanks,
Mathieu


 
> > Thanks,
> > Arnaud
> > 
> > > 
> > > Thanks,
> > > Mathieu
> > > 
> > >> +	return NULL;
> > >> +}
> > >> +
> > >>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> > >>  				 unsigned long arg)
> > >>  {
> > >>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> > >> -
> > >> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> > >> +	void __user *argp = (void __user *)arg;
> > >> +	struct rpmsg_channel_info chinfo;
> > >> +	struct rpmsg_endpoint_info eptinfo;
> > >> +	struct rpmsg_device *newch;
> > >> +
> > >> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> > >> +		return -EINVAL;
> > >> +
> > >> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> > >> +		return -EFAULT;
> > >> +
> > >> +	/*
> > >> +	 * In a frst step only the rpmsg_raw service is supported.
> > >> +	 * The override is foorced to RPMSG_RAW_SERVICE
> > >> +	 */
> > >> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> > >> +	if (!chinfo.driver_override)
> > >> +		return -ENODEV;
> > >> +
> > >> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> > >> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> > >> +	chinfo.src = eptinfo.src;
> > >> +	chinfo.dst = eptinfo.dst;
> > >> +
> > >> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> > >> +	if (!newch) {
> > >> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> > >> +		return -ENXIO;
> > >> +	}
> > >>  
> > >>  	return 0;
> > >>  };
> > >> -- 
> > >> 2.17.1
> > >>

Re: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device

[Arnaud POULIQUEN]

On 1/29/21 1:13 AM, Mathieu Poirier wrote:
> [...]
> 
>>> It seems to me that the main point to step forward is to clarify the global
>>> design and features of the rpmsg-ctrl.
>>> Depending on the decision taken, this series could be trashed and rewritten from
>>> a blank page...To not lost to much time on the series don't hesitate to limit
>>> the review to the minimum.
>>>
>>
>> I doubt you will ever get clear guidelines on the whole solution.  I will get
>> back to you once I am done with the SMD driver, which should be in the
>> latter part of next week.
>>
> 
> After looking at the rpmsg_chrdev driver, its current customers (i.e the Qcom
> drivers), the rpmsg name service and considering the long term goals of this
> patchset I have the following guidelines: 
> 
> 1) I thought long and hard about how to split the current rpmsg_chrdev driver
> between the control plane and the raw device plane and the end solution looks
> much slimpler than I expected.  Exporting function rpmsg_eptdev_create() after
> moving it to another file (along with other dependencies) should be all we need.
> Calling rpmsg_eptdev_create() from rpmsg_ctrldev_ioctl() will automatically load
> the new driver, the same way calling rpmsg_ns_register_device() from
> rpmsg_probe() took care of loading the rpmsg_ns driver.
> 
> 2) While keeping the control plane functionality related to
> RPMSG_CREATE_EPT_IOCTL intact, introduce a new RPMSG_CREATE_DEV_IOCTL that will
> allow for the instantiation of rpmsg_devices, exactly the same way a name service
> announcement from a remote processor does.  I envision that code path to
> eventually call rpmsg_create_channel().
> 
> 3) Leave the rpmsg_channel_info structure intact and use the
> rpmsg_channel_info::name to bind to a rpmsg_driver, exactly how it is currently
> done for name service driver selection.  That will allow us to re-use the
> current rpmsg_bus intrastructure, i.e rpmsg_bus::match(), without having to deal
> with yet another bus type.  Proceeding this way gives us the opportunity to keep
> the current channel name convention for other rpmch_chrdev users untouched.
> 
> 4) In a prior conversation you indicated the intention of instantiating the
> rpmsg_chrdev from the name service interface.  I agree with doing so but 
> conjugating that with the RPMSG_CHAR kenrel define may be tricky.  I will wait
> to see what you come up with.
> 
> I hope this helps.

Thank you for these guidelines! It need a bit of time to look at the details
(especially point 1) ), but your suggestion seems to me to be a good compromise.
I hope to come back soon with a new revision based on this point.

Regards,
Arnaud

> 
> Thanks,
> Mathieu
> 
> 
>  
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>> +	return NULL;
>>>>> +}
>>>>> +
>>>>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  				 unsigned long arg)
>>>>>  {
>>>>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>>>>> -
>>>>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>>>>> +	void __user *argp = (void __user *)arg;
>>>>> +	struct rpmsg_channel_info chinfo;
>>>>> +	struct rpmsg_endpoint_info eptinfo;
>>>>> +	struct rpmsg_device *newch;
>>>>> +
>>>>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>>> +		return -EFAULT;
>>>>> +
>>>>> +	/*
>>>>> +	 * In a frst step only the rpmsg_raw service is supported.
>>>>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>>>>> +	 */
>>>>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>>>>> +	if (!chinfo.driver_override)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>>>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>>>> +	chinfo.src = eptinfo.src;
>>>>> +	chinfo.dst = eptinfo.dst;
>>>>> +
>>>>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>>>>> +	if (!newch) {
>>>>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>>>>> +		return -ENXIO;
>>>>> +	}
>>>>>  
>>>>>  	return 0;
>>>>>  };
>>>>> -- 
>>>>> 2.17.1
>>>>>