[PATCH 0/9] introduce name service announcement rpmsg driver

[PATCH 0/9] introduce name service announcement rpmsg driver

[Arnaud Pouliquen]

The NS announcement is implemented by several backends, but could be
considered as part the RPMsg protocol. 
In this case it should be managed as a reserved rpmsg service and so
implemented on top of the rpmsg protocol.

This series introduces the rpmsg_ns driver that handles the name service
announcement. The virtio backend is updated in consequence to use this
service.

Applies cleanly on Bjorn rpmsg-next branch (ddd1930d6e3e)

Arnaud Pouliquen (9):
  rpmsg: virtio: rename rpmsg_create_channel
  rpmsg: core: add channel creation internal API
  rpmsg: virtio: add rpmsg channel device ops
  rpmsg: define the name service announcement as reserved address
  rpmsg: introduce reserved rpmsg driver for ns announcement
  rpmsg: virtio: use rpmsg ns device for the ns announcement
  rpmsg: ns: add name service announcement service
  rpmsg: virtio: use rpmsg_ns driver to manage ns announcement
  rpmsg: ns: name service announcement endianness

 drivers/rpmsg/Kconfig            |   9 ++
 drivers/rpmsg/Makefile           |   1 +
 drivers/rpmsg/rpmsg_core.c       |  37 ++++++
 drivers/rpmsg/rpmsg_internal.h   |  32 +++++
 drivers/rpmsg/rpmsg_ns.c         | 175 +++++++++++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 213 +++++++++----------------------
 include/linux/rpmsg.h            |   9 ++
 7 files changed, 325 insertions(+), 151 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ns.c

-- 
2.17.1

[PATCH 1/9] rpmsg: virtio: rename rpmsg_create_channel

[Arnaud Pouliquen]

Rename the internal function as it is internal, and as
the name will be used in rpmsg_core.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 9006fc7f73d0..736bc7b9dea8 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -390,8 +390,9 @@ static void virtio_rpmsg_release_device(struct device *dev)
  * this function will be used to create both static and dynamic
  * channels.
  */
-static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
-						 struct rpmsg_channel_info *chinfo)
+static struct rpmsg_device *
+__rpmsg_create_channel(struct virtproc_info *vrp,
+		       struct rpmsg_channel_info *chinfo)
 {
 	struct virtio_rpmsg_channel *vch;
 	struct rpmsg_device *rpdev;
@@ -864,7 +865,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 		if (ret)
 			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
 	} else {
-		newch = rpmsg_create_channel(vrp, &chinfo);
+		newch = __rpmsg_create_channel(vrp, &chinfo);
 		if (!newch)
 			dev_err(dev, "rpmsg_create_channel failed\n");
 	}
-- 
2.17.1

[PATCH 2/9] rpmsg: core: add channel creation internal API

[Arnaud Pouliquen]

Add the channel creation API as a first step to be able to define the
name service announcement as a rpmsg driver independent from the RPMsg
virtio bus.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/rpmsg_core.c     | 37 ++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h | 12 +++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index a6361cad608b..ae7da4a2e528 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,43 @@
 
 #include "rpmsg_internal.h"
 
+/**
+ * rpmsg_create_channel() - create a new rpmsg channel
+ * using its name and address info.
+ * @rpdev: rpmsg driver
+ * @chinfo: channel_info to bind
+ *
+ * Returns a pointer to the new rpmsg device on success, or NULL on error.
+ */
+struct rpmsg_device *
+	rpmsg_create_channel(struct rpmsg_device *rpdev,
+			     struct rpmsg_channel_info *chinfo)
+{
+	if (WARN_ON(!rpdev) || !rpdev->ops->create_channel)
+		return NULL;
+
+	return rpdev->ops->create_channel(rpdev, chinfo);
+}
+EXPORT_SYMBOL(rpmsg_create_channel);
+
+/**
+ * rpmsg_release_channel() - release a rpmsg channel
+ * using its name and address info.
+ * @rpdev: rpmsg driver
+ * @chinfo: channel_info to bind
+ *
+ * Returns a pointer to the new rpmsg device on success, or NULL on error.
+ */
+int rpmsg_release_channel(struct rpmsg_device *rpdev,
+			  struct rpmsg_channel_info *chinfo)
+{
+	if (WARN_ON(!rpdev) || !rpdev->ops->release_channel)
+		return 0;
+
+	return rpdev->ops->release_channel(rpdev, chinfo);
+}
+EXPORT_SYMBOL(rpmsg_release_channel);
+
 /**
  * rpmsg_create_ept() - create a new rpmsg_endpoint
  * @rpdev: rpmsg channel device
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3fc83cd50e98..d5ab286d0e5e 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -20,6 +20,8 @@
 
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @create_channel:	create backend-specific channel, optional
+ * @release_channel:	release backend-specific channel, optional
  * @create_ept:		create backend-specific endpoint, required
  * @announce_create:	announce presence of new channel, optional
  * @announce_destroy:	announce destruction of channel, optional
@@ -29,6 +31,11 @@
  * advertise new channels implicitly by creating the endpoints.
  */
 struct rpmsg_device_ops {
+	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
+					     struct rpmsg_channel_info *chinfo);
+	int (*release_channel)(struct rpmsg_device *rpdev,
+			       struct rpmsg_channel_info *chinfo);
+
 	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
 					    rpmsg_rx_cb_t cb, void *priv,
 					    struct rpmsg_channel_info chinfo);
@@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
 struct device *rpmsg_find_device(struct device *parent,
 				 struct rpmsg_channel_info *chinfo);
 
+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
-- 
2.17.1

Re: [PATCH 2/9] rpmsg: core: add channel creation internal API

[Mathieu Poirier]

Hi Arnaud,

On Fri, Jul 31, 2020 at 01:47:25PM +0200, Arnaud Pouliquen wrote:
> Add the channel creation API as a first step to be able to define the
> name service announcement as a rpmsg driver independent from the RPMsg
> virtio bus.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 37 ++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index a6361cad608b..ae7da4a2e528 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,43 @@
>  
>  #include "rpmsg_internal.h"
>  
> +/**
> + * rpmsg_create_channel() - create a new rpmsg channel
> + * using its name and address info.
> + * @rpdev: rpmsg driver
> + * @chinfo: channel_info to bind
> + *
> + * Returns a pointer to the new rpmsg device on success, or NULL on error.
> + */
> +struct rpmsg_device *
> +	rpmsg_create_channel(struct rpmsg_device *rpdev,
> +			     struct rpmsg_channel_info *chinfo)
> +{
> +	if (WARN_ON(!rpdev) || !rpdev->ops->create_channel)
> +		return NULL;

Ok for the WARN_ON().  In another if(), I would check for ops and
ops->create_channel().  Same for the release() operation.

> +
> +	return rpdev->ops->create_channel(rpdev, chinfo);
> +}
> +EXPORT_SYMBOL(rpmsg_create_channel);
> +
> +/**
> + * rpmsg_release_channel() - release a rpmsg channel
> + * using its name and address info.
> + * @rpdev: rpmsg driver
> + * @chinfo: channel_info to bind
> + *
> + * Returns a pointer to the new rpmsg device on success, or NULL on error.
> + */
> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
> +			  struct rpmsg_channel_info *chinfo)
> +{
> +	if (WARN_ON(!rpdev) || !rpdev->ops->release_channel)
> +		return 0;
> +
> +	return rpdev->ops->release_channel(rpdev, chinfo);
> +}
> +EXPORT_SYMBOL(rpmsg_release_channel);
> +
>  /**
>   * rpmsg_create_ept() - create a new rpmsg_endpoint
>   * @rpdev: rpmsg channel device
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3fc83cd50e98..d5ab286d0e5e 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -20,6 +20,8 @@
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @create_channel:	create backend-specific channel, optional
> + * @release_channel:	release backend-specific channel, optional
>   * @create_ept:		create backend-specific endpoint, required
>   * @announce_create:	announce presence of new channel, optional
>   * @announce_destroy:	announce destruction of channel, optional
> @@ -29,6 +31,11 @@
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
> +					     struct rpmsg_channel_info *chinfo);
> +	int (*release_channel)(struct rpmsg_device *rpdev,
> +			       struct rpmsg_channel_info *chinfo);
> +
>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>  					    rpmsg_rx_cb_t cb, void *priv,
>  					    struct rpmsg_channel_info chinfo);
> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>  struct device *rpmsg_find_device(struct device *parent,
>  				 struct rpmsg_channel_info *chinfo);
>  
> +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
> -- 
> 2.17.1
> 

[PATCH 3/9] rpmsg: virtio: add rpmsg channel device ops

[Arnaud Pouliquen]

Implement the create and release of the RPMsg channel
for the RPMsg virtio bus.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 736bc7b9dea8..910d30818901 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -176,6 +176,12 @@ static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
 				  int len, u32 dst);
 static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 					   u32 dst, void *data, int len);
+static struct rpmsg_device *
+	virtio_rpmsg_create_channel(struct rpmsg_device *rpdev,
+				    struct rpmsg_channel_info *chinfo);
+static int virtio_rpmsg_release_channel(struct rpmsg_device *rpdev,
+					struct rpmsg_channel_info *chinfo);
+
 
 static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
 	.destroy_ept = virtio_rpmsg_destroy_ept,
@@ -372,6 +378,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 }
 
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
+	.create_channel = virtio_rpmsg_create_channel,
+	.release_channel = virtio_rpmsg_release_channel,
 	.create_ept = virtio_rpmsg_create_ept,
 	.announce_create = virtio_rpmsg_announce_create,
 	.announce_destroy = virtio_rpmsg_announce_destroy,
@@ -439,6 +447,25 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
 	return rpdev;
 }
 
+static struct rpmsg_device *
+	virtio_rpmsg_create_channel(struct rpmsg_device *rpdev,
+				    struct rpmsg_channel_info *chinfo)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return __rpmsg_create_channel(vrp, chinfo);
+}
+
+static int virtio_rpmsg_release_channel(struct rpmsg_device *rpdev,
+					struct rpmsg_channel_info *chinfo)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return rpmsg_unregister_device(&vrp->vdev->dev, chinfo);
+}
+
 /* super simple buffer "allocator" that is just enough for now */
 static void *get_a_tx_buf(struct virtproc_info *vrp)
 {
-- 
2.17.1

[PATCH 4/9] rpmsg: define the name service announcement as reserved address

[Arnaud Pouliquen]

The address 53 is reserved for the dynamic RPMsg device management
on name service announcement.
Define this address in a reserved enum list.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 3 ---
 include/linux/rpmsg.h            | 9 +++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 910d30818901..b2927661868c 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -162,9 +162,6 @@ struct virtio_rpmsg_channel {
  */
 #define RPMSG_RESERVED_ADDRESSES	(1024)
 
-/* Address 53 is reserved for advertising remote services */
-#define RPMSG_NS_ADDR			(53)
-
 static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
 static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
 static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 9fe156d1c018..5496d27c6f8c 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -20,6 +20,15 @@
 
 #define RPMSG_ADDR_ANY		0xFFFFFFFF
 
+/**
+ * enum rpmsg_ns_flags - List of reserved RPMsg addresses
+ *
+ * @RPMSG_NS_ADDR: remote services advertising for dynamic allocation
+ */
+enum rpmsg_reserved_addr {
+	RPMSG_NS_ADDR		= 53,
+};
+
 struct rpmsg_device;
 struct rpmsg_endpoint;
 struct rpmsg_device_ops;
-- 
2.17.1

[PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement

[Arnaud Pouliquen]

The name service announcement should not be linked to the RPMsg virtio bus
but to the RPMsg protocol itself.

This patch proposes to break the dependency with the RPmsg virtio bus by
the introduction of the reserved RPMsg name service driver which will be in
charge of managing the RPMsg name service announcement.

This first patch only implements the probe and the RPMsg endpoint to
manage create and release channels remote requests.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/Kconfig          |   8 ++
 drivers/rpmsg/Makefile         |   1 +
 drivers/rpmsg/rpmsg_internal.h |  17 +++++
 drivers/rpmsg/rpmsg_ns.c       | 135 +++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+)
 create mode 100644 drivers/rpmsg/rpmsg_ns.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index f96716893c2a..140faa975ea1 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -15,6 +15,14 @@ config RPMSG_CHAR
 	  in /dev. They make it possible for user-space programs to send and
 	  receive rpmsg packets.
 
+config RPMSG_NS
+	tristate "RPMSG name service announcement"
+	depends on RPMSG
+	help
+	  Say Y here to enable the support of the name service announcement
+	  channel that probes the associate RPMsg device on remote endpoint
+	  service announcement.
+
 config RPMSG_MTK_SCP
 	tristate "MediaTek SCP"
 	depends on MTK_SCP
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index ffe932ef6050..8d452656f0ee 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
+obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.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_internal.h b/drivers/rpmsg/rpmsg_internal.h
index d5ab286d0e5e..641b48f6bf2a 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
 	return rpmsg_register_device(rpdev);
 }
 
+/**
+ * rpmsg_ns_register_device() - register name service 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 name service device.
+ */
+static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+	strcpy(rpdev->id.name, "rpmsg_ns");
+	rpdev->driver_override = "rpmsg_ns";
+	rpdev->src = RPMSG_NS_ADDR;
+	rpdev->dst = RPMSG_NS_ADDR;
+
+	return rpmsg_register_device(rpdev);
+}
+
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
new file mode 100644
index 000000000000..fe7713e737c2
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "rpmsg_internal.h"
+
+/**
+ * enum rpmsg_ns_flags - dynamic name service announcement flags
+ *
+ * @RPMSG_NS_CREATE: a new remote service was just created
+ * @RPMSG_NS_DESTROY: a known remote service was just destroyed
+ */
+enum rpmsg_ns_flags {
+	RPMSG_NS_CREATE		= 0,
+	RPMSG_NS_DESTROY	= 1,
+};
+
+/**
+ * struct rpmsg_ns_msg - dynamic name service announcement message
+ * @name: name of remote service that is published
+ * @addr: address of remote service that is published
+ * @flags: indicates whether service is created or destroyed
+ *
+ * This message is sent across to publish a new service, or announce
+ * about its removal. When we receive these messages, an appropriate
+ * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
+ * or ->remove() handler of the appropriate rpmsg driver will be invoked
+ * (if/as-soon-as one is registered).
+ */
+struct rpmsg_ns_msg {
+	char name[RPMSG_NAME_SIZE];
+	u32 addr;
+	u32 flags;
+} __packed;
+
+/* invoked when a name service announcement arrives */
+static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
+		       void *priv, u32 src)
+{
+	struct rpmsg_ns_msg *msg = data;
+	struct rpmsg_device *newch;
+	struct rpmsg_channel_info chinfo;
+	struct device *dev = &rpdev->dev;
+	int ret;
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
+			 data, len, true);
+#endif
+
+	if (len != sizeof(*msg)) {
+		dev_err(dev, "malformed ns msg (%d)\n", len);
+		return -EINVAL;
+	}
+
+	/* don't trust the remote processor for null terminating the name */
+	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
+
+	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
+	chinfo.src = RPMSG_ADDR_ANY;
+	chinfo.dst = msg->addr;
+
+	dev_info(dev, "%sing channel %s addr 0x%x\n",
+		 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
+		 msg->name, msg->addr);
+
+	if (msg->flags & RPMSG_NS_DESTROY) {
+		ret = rpmsg_release_channel(rpdev, &chinfo);
+		if (ret)
+			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
+	} else {
+		newch = rpmsg_create_channel(rpdev, &chinfo);
+		if (!newch)
+			dev_err(dev, "rpmsg_create_channel failed\n");
+	}
+
+	return 0;
+}
+
+static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_channel_info ns_chinfo;
+	struct rpmsg_endpoint *ns_ept;
+
+	ns_chinfo.src = RPMSG_NS_ADDR;
+	ns_chinfo.dst = RPMSG_NS_ADDR;
+	strcpy(ns_chinfo.name, "name_service");
+
+	/*
+	 * create and attach the endpoint to the rpmsg device that it would be
+	 * destroy when the rpmsg device will be deleted
+	 */
+	ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo);
+	if (!ns_ept) {
+		dev_err(&rpdev->dev, "failed to create the ns ept\n");
+		return -ENOMEM;
+	}
+	rpdev->ept = ns_ept;
+
+	rpdev->src = RPMSG_NS_ADDR;
+
+	return 0;
+}
+
+static struct rpmsg_driver rpmsg_ns_driver = {
+	.drv.name = "rpmsg_ns",
+	.probe = rpmsg_ns_probe,
+};
+
+static int rpmsg_ns_init(void)
+{
+	int ret;
+
+	ret = register_rpmsg_driver(&rpmsg_ns_driver);
+	if (ret < 0)
+		pr_err("%s: Failed to register rpmsg driver\n", __func__);
+
+	return ret;
+}
+postcore_initcall(rpmsg_ns_init);
+
+static void rpmsg_ns_exit(void)
+{
+	unregister_rpmsg_driver(&rpmsg_ns_driver);
+}
+module_exit(rpmsg_ns_exit);
+
+MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
+MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
+MODULE_ALIAS("rpmsg_ns");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement

[kernel test robot]

Hi Arnaud,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20200730]
[also build test WARNING on v5.8]
[cannot apply to linux/master linus/master rpmsg/for-next v5.8-rc7 v5.8-rc6 v5.8-rc5]
[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-name-service-announcement-rpmsg-driver/20200731-195014
base:    7b287a5c6ac518c415a258f2aa7b1ebb25c263d2
compiler: nds32le-linux-gcc (GCC) 9.3.0

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


cppcheck warnings: (new ones prefixed by >>)

>> drivers/rpmsg/rpmsg_ns.c:68:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
      msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
                                    ^

vim +68 drivers/rpmsg/rpmsg_ns.c

    54	
    55		if (len != sizeof(*msg)) {
    56			dev_err(dev, "malformed ns msg (%d)\n", len);
    57			return -EINVAL;
    58		}
    59	
    60		/* don't trust the remote processor for null terminating the name */
    61		msg->name[RPMSG_NAME_SIZE - 1] = '\0';
    62	
    63		strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
    64		chinfo.src = RPMSG_ADDR_ANY;
    65		chinfo.dst = msg->addr;
    66	
    67		dev_info(dev, "%sing channel %s addr 0x%x\n",
  > 68			 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
    69			 msg->name, msg->addr);
    70	
    71		if (msg->flags & RPMSG_NS_DESTROY) {
    72			ret = rpmsg_release_channel(rpdev, &chinfo);
    73			if (ret)
    74				dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
    75		} else {
    76			newch = rpmsg_create_channel(rpdev, &chinfo);
    77			if (!newch)
    78				dev_err(dev, "rpmsg_create_channel failed\n");
    79		}
    80	
    81		return 0;
    82	}
    83	

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

Re: [PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement

[Mathieu Poirier]

On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote:
> The name service announcement should not be linked to the RPMsg virtio bus
> but to the RPMsg protocol itself.
> 
> This patch proposes to break the dependency with the RPmsg virtio bus by
> the introduction of the reserved RPMsg name service driver which will be in
> charge of managing the RPMsg name service announcement.
> 
> This first patch only implements the probe and the RPMsg endpoint to
> manage create and release channels remote requests.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/Kconfig          |   8 ++
>  drivers/rpmsg/Makefile         |   1 +
>  drivers/rpmsg/rpmsg_internal.h |  17 +++++
>  drivers/rpmsg/rpmsg_ns.c       | 135 +++++++++++++++++++++++++++++++++
>  4 files changed, 161 insertions(+)
>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index f96716893c2a..140faa975ea1 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -15,6 +15,14 @@ config RPMSG_CHAR
>  	  in /dev. They make it possible for user-space programs to send and
>  	  receive rpmsg packets.
>  
> +config RPMSG_NS
> +	tristate "RPMSG name service announcement"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the name service announcement
> +	  channel that probes the associate RPMsg device on remote endpoint

s/associate/associated

> +	  service announcement.
> +
>  config RPMSG_MTK_SCP
>  	tristate "MediaTek SCP"
>  	depends on MTK_SCP
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index ffe932ef6050..8d452656f0ee 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
> +obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.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_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index d5ab286d0e5e..641b48f6bf2a 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>  	return rpmsg_register_device(rpdev);
>  }
>  
> +/**
> + * rpmsg_ns_register_device() - register name service 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 name service device.
> + */
> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> +	strcpy(rpdev->id.name, "rpmsg_ns");
> +	rpdev->driver_override = "rpmsg_ns";
> +	rpdev->src = RPMSG_NS_ADDR;
> +	rpdev->dst = RPMSG_NS_ADDR;
> +
> +	return rpmsg_register_device(rpdev);
> +}
> +
>  #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index 000000000000..fe7713e737c2
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + */
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "rpmsg_internal.h"
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> +	RPMSG_NS_CREATE		= 0,
> +	RPMSG_NS_DESTROY	= 1,
> +};
> +
> +/**
> + * struct rpmsg_ns_msg - dynamic name service announcement message
> + * @name: name of remote service that is published
> + * @addr: address of remote service that is published
> + * @flags: indicates whether service is created or destroyed
> + *
> + * This message is sent across to publish a new service, or announce
> + * about its removal. When we receive these messages, an appropriate
> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> + * (if/as-soon-as one is registered).
> + */
> +struct rpmsg_ns_msg {
> +	char name[RPMSG_NAME_SIZE];
> +	u32 addr;
> +	u32 flags;
> +} __packed;
> +
> +/* invoked when a name service announcement arrives */
> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> +		       void *priv, u32 src)
> +{
> +	struct rpmsg_ns_msg *msg = data;
> +	struct rpmsg_device *newch;
> +	struct rpmsg_channel_info chinfo;
> +	struct device *dev = &rpdev->dev;
> +	int ret;
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> +	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> +			 data, len, true);
> +#endif
> +
> +	if (len != sizeof(*msg)) {
> +		dev_err(dev, "malformed ns msg (%d)\n", len);
> +		return -EINVAL;
> +	}
> +
> +	/* don't trust the remote processor for null terminating the name */
> +	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> +
> +	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> +	chinfo.src = RPMSG_ADDR_ANY;
> +	chinfo.dst = msg->addr;
> +
> +	dev_info(dev, "%sing channel %s addr 0x%x\n",
> +		 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> +		 msg->name, msg->addr);
> +
> +	if (msg->flags & RPMSG_NS_DESTROY) {
> +		ret = rpmsg_release_channel(rpdev, &chinfo);
> +		if (ret)
> +			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> +	} else {
> +		newch = rpmsg_create_channel(rpdev, &chinfo);
> +		if (!newch)
> +			dev_err(dev, "rpmsg_create_channel failed\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_channel_info ns_chinfo;
> +	struct rpmsg_endpoint *ns_ept;
> +
> +	ns_chinfo.src = RPMSG_NS_ADDR;
> +	ns_chinfo.dst = RPMSG_NS_ADDR;
> +	strcpy(ns_chinfo.name, "name_service");
> +
> +	/*
> +	 * create and attach the endpoint to the rpmsg device that it would be
> +	 * destroy when the rpmsg device will be deleted
> +	 */

This comment doesn't work, please revise.

> +	ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo);
> +	if (!ns_ept) {
> +		dev_err(&rpdev->dev, "failed to create the ns ept\n");
> +		return -ENOMEM;
> +	}
> +	rpdev->ept = ns_ept;
> +
> +	rpdev->src = RPMSG_NS_ADDR;

I think this is already done in rpmsg_ns_register_device().

> +
> +	return 0;
> +}
> +
> +static struct rpmsg_driver rpmsg_ns_driver = {
> +	.drv.name = "rpmsg_ns",
> +	.probe = rpmsg_ns_probe,
> +};
> +
> +static int rpmsg_ns_init(void)
> +{
> +	int ret;
> +
> +	ret = register_rpmsg_driver(&rpmsg_ns_driver);
> +	if (ret < 0)
> +		pr_err("%s: Failed to register rpmsg driver\n", __func__);
> +
> +	return ret;
> +}
> +postcore_initcall(rpmsg_ns_init);
> +
> +static void rpmsg_ns_exit(void)
> +{
> +	unregister_rpmsg_driver(&rpmsg_ns_driver);
> +}
> +module_exit(rpmsg_ns_exit);
> +
> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> +MODULE_ALIAS("rpmsg_ns");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement

[Arnaud POULIQUEN]

Hi Mathieu

Thanks for the review! please find few comments below.

On 8/25/20 12:47 AM, Mathieu Poirier wrote:
> On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote:
>> The name service announcement should not be linked to the RPMsg virtio bus
>> but to the RPMsg protocol itself.
>>
>> This patch proposes to break the dependency with the RPmsg virtio bus by
>> the introduction of the reserved RPMsg name service driver which will be in
>> charge of managing the RPMsg name service announcement.
>>
>> This first patch only implements the probe and the RPMsg endpoint to
>> manage create and release channels remote requests.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/Kconfig          |   8 ++
>>  drivers/rpmsg/Makefile         |   1 +
>>  drivers/rpmsg/rpmsg_internal.h |  17 +++++
>>  drivers/rpmsg/rpmsg_ns.c       | 135 +++++++++++++++++++++++++++++++++
>>  4 files changed, 161 insertions(+)
>>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index f96716893c2a..140faa975ea1 100644
>> --- a/drivers/rpmsg/Kconfig
>> +++ b/drivers/rpmsg/Kconfig
>> @@ -15,6 +15,14 @@ config RPMSG_CHAR
>>  	  in /dev. They make it possible for user-space programs to send and
>>  	  receive rpmsg packets.
>>  
>> +config RPMSG_NS
>> +	tristate "RPMSG name service announcement"
>> +	depends on RPMSG
>> +	help
>> +	  Say Y here to enable the support of the name service announcement
>> +	  channel that probes the associate RPMsg device on remote endpoint
> 
> s/associate/associated
> 
>> +	  service announcement.
>> +
>>  config RPMSG_MTK_SCP
>>  	tristate "MediaTek SCP"
>>  	depends on MTK_SCP
>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>> index ffe932ef6050..8d452656f0ee 100644
>> --- a/drivers/rpmsg/Makefile
>> +++ b/drivers/rpmsg/Makefile
>> @@ -1,6 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
>> +obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.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_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index d5ab286d0e5e..641b48f6bf2a 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>>  	return rpmsg_register_device(rpdev);
>>  }
>>  
>> +/**
>> + * rpmsg_ns_register_device() - register name service 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 name service device.
>> + */
>> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> +	strcpy(rpdev->id.name, "rpmsg_ns");
>> +	rpdev->driver_override = "rpmsg_ns";
>> +	rpdev->src = RPMSG_NS_ADDR;
>> +	rpdev->dst = RPMSG_NS_ADDR;
>> +
>> +	return rpmsg_register_device(rpdev);
>> +}
>> +
>>  #endif
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> new file mode 100644
>> index 000000000000..fe7713e737c2
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -0,0 +1,135 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> + */
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include "rpmsg_internal.h"
>> +
>> +/**
>> + * enum rpmsg_ns_flags - dynamic name service announcement flags
>> + *
>> + * @RPMSG_NS_CREATE: a new remote service was just created
>> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>> + */
>> +enum rpmsg_ns_flags {
>> +	RPMSG_NS_CREATE		= 0,
>> +	RPMSG_NS_DESTROY	= 1,
>> +};
>> +
>> +/**
>> + * struct rpmsg_ns_msg - dynamic name service announcement message
>> + * @name: name of remote service that is published
>> + * @addr: address of remote service that is published
>> + * @flags: indicates whether service is created or destroyed
>> + *
>> + * This message is sent across to publish a new service, or announce
>> + * about its removal. When we receive these messages, an appropriate
>> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
>> + * (if/as-soon-as one is registered).
>> + */
>> +struct rpmsg_ns_msg {
>> +	char name[RPMSG_NAME_SIZE];
>> +	u32 addr;
>> +	u32 flags;
>> +} __packed;
>> +
>> +/* invoked when a name service announcement arrives */
>> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> +		       void *priv, u32 src)
>> +{
>> +	struct rpmsg_ns_msg *msg = data;
>> +	struct rpmsg_device *newch;
>> +	struct rpmsg_channel_info chinfo;
>> +	struct device *dev = &rpdev->dev;
>> +	int ret;
>> +
>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>> +	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
>> +			 data, len, true);
>> +#endif
>> +
>> +	if (len != sizeof(*msg)) {
>> +		dev_err(dev, "malformed ns msg (%d)\n", len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* don't trust the remote processor for null terminating the name */
>> +	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
>> +
>> +	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
>> +	chinfo.src = RPMSG_ADDR_ANY;
>> +	chinfo.dst = msg->addr;
>> +
>> +	dev_info(dev, "%sing channel %s addr 0x%x\n",
>> +		 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
>> +		 msg->name, msg->addr);
>> +
>> +	if (msg->flags & RPMSG_NS_DESTROY) {
>> +		ret = rpmsg_release_channel(rpdev, &chinfo);
>> +		if (ret)
>> +			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
>> +	} else {
>> +		newch = rpmsg_create_channel(rpdev, &chinfo);
>> +		if (!newch)
>> +			dev_err(dev, "rpmsg_create_channel failed\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_channel_info ns_chinfo;
>> +	struct rpmsg_endpoint *ns_ept;
>> +
>> +	ns_chinfo.src = RPMSG_NS_ADDR;
>> +	ns_chinfo.dst = RPMSG_NS_ADDR;
>> +	strcpy(ns_chinfo.name, "name_service");
>> +
>> +	/*
>> +	 * create and attach the endpoint to the rpmsg device that it would be
>> +	 * destroy when the rpmsg device will be deleted
>> +	 */
> 
> This comment doesn't work, please revise.

Could you clarify what does not work, from your POV?
in view of your comment, it seems I should at least rephrase it... 
proposal:
	/*
 	 * Create the NS service endpoint associated to the rpmsg device.
         * The endpoint will be automatically destroyed when the rpmsg device
         * will be deleted.
	 */

> 
>> +	ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo);
>> +	if (!ns_ept) {
>> +		dev_err(&rpdev->dev, "failed to create the ns ept\n");
>> +		return -ENOMEM;
>> +	}
>> +	rpdev->ept = ns_ept;
>> +
>> +	rpdev->src = RPMSG_NS_ADDR;
> 
> I think this is already done in rpmsg_ns_register_device().

You are right!

thanks,
Arnaud
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct rpmsg_driver rpmsg_ns_driver = {
>> +	.drv.name = "rpmsg_ns",
>> +	.probe = rpmsg_ns_probe,
>> +};
>> +
>> +static int rpmsg_ns_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = register_rpmsg_driver(&rpmsg_ns_driver);
>> +	if (ret < 0)
>> +		pr_err("%s: Failed to register rpmsg driver\n", __func__);
>> +
>> +	return ret;
>> +}
>> +postcore_initcall(rpmsg_ns_init);
>> +
>> +static void rpmsg_ns_exit(void)
>> +{
>> +	unregister_rpmsg_driver(&rpmsg_ns_driver);
>> +}
>> +module_exit(rpmsg_ns_exit);
>> +
>> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>> +MODULE_ALIAS("rpmsg_ns");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.17.1
>>

Re: [PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement

[Mathieu Poirier]

On Tue, 25 Aug 2020 at 05:57, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Mathieu
>
> Thanks for the review! please find few comments below.
>
> On 8/25/20 12:47 AM, Mathieu Poirier wrote:
> > On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote:
> >> The name service announcement should not be linked to the RPMsg virtio bus
> >> but to the RPMsg protocol itself.
> >>
> >> This patch proposes to break the dependency with the RPmsg virtio bus by
> >> the introduction of the reserved RPMsg name service driver which will be in
> >> charge of managing the RPMsg name service announcement.
> >>
> >> This first patch only implements the probe and the RPMsg endpoint to
> >> manage create and release channels remote requests.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >> ---
> >>  drivers/rpmsg/Kconfig          |   8 ++
> >>  drivers/rpmsg/Makefile         |   1 +
> >>  drivers/rpmsg/rpmsg_internal.h |  17 +++++
> >>  drivers/rpmsg/rpmsg_ns.c       | 135 +++++++++++++++++++++++++++++++++
> >>  4 files changed, 161 insertions(+)
> >>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> >>
> >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> >> index f96716893c2a..140faa975ea1 100644
> >> --- a/drivers/rpmsg/Kconfig
> >> +++ b/drivers/rpmsg/Kconfig
> >> @@ -15,6 +15,14 @@ config RPMSG_CHAR
> >>        in /dev. They make it possible for user-space programs to send and
> >>        receive rpmsg packets.
> >>
> >> +config RPMSG_NS
> >> +    tristate "RPMSG name service announcement"
> >> +    depends on RPMSG
> >> +    help
> >> +      Say Y here to enable the support of the name service announcement
> >> +      channel that probes the associate RPMsg device on remote endpoint
> >
> > s/associate/associated
> >
> >> +      service announcement.
> >> +
> >>  config RPMSG_MTK_SCP
> >>      tristate "MediaTek SCP"
> >>      depends on MTK_SCP
> >> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> >> index ffe932ef6050..8d452656f0ee 100644
> >> --- a/drivers/rpmsg/Makefile
> >> +++ b/drivers/rpmsg/Makefile
> >> @@ -1,6 +1,7 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  obj-$(CONFIG_RPMSG)         += rpmsg_core.o
> >>  obj-$(CONFIG_RPMSG_CHAR)    += rpmsg_char.o
> >> +obj-$(CONFIG_RPMSG_NS)              += rpmsg_ns.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_internal.h b/drivers/rpmsg/rpmsg_internal.h
> >> index d5ab286d0e5e..641b48f6bf2a 100644
> >> --- a/drivers/rpmsg/rpmsg_internal.h
> >> +++ b/drivers/rpmsg/rpmsg_internal.h
> >> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> >>      return rpmsg_register_device(rpdev);
> >>  }
> >>
> >> +/**
> >> + * rpmsg_ns_register_device() - register name service 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 name service device.
> >> + */
> >> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> +    strcpy(rpdev->id.name, "rpmsg_ns");
> >> +    rpdev->driver_override = "rpmsg_ns";
> >> +    rpdev->src = RPMSG_NS_ADDR;
> >> +    rpdev->dst = RPMSG_NS_ADDR;
> >> +
> >> +    return rpmsg_register_device(rpdev);
> >> +}
> >> +
> >>  #endif
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> new file mode 100644
> >> index 000000000000..fe7713e737c2
> >> --- /dev/null
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -0,0 +1,135 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> >> + */
> >> +#include <linux/device.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include "rpmsg_internal.h"
> >> +
> >> +/**
> >> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> >> + *
> >> + * @RPMSG_NS_CREATE: a new remote service was just created
> >> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> >> + */
> >> +enum rpmsg_ns_flags {
> >> +    RPMSG_NS_CREATE         = 0,
> >> +    RPMSG_NS_DESTROY        = 1,
> >> +};
> >> +
> >> +/**
> >> + * struct rpmsg_ns_msg - dynamic name service announcement message
> >> + * @name: name of remote service that is published
> >> + * @addr: address of remote service that is published
> >> + * @flags: indicates whether service is created or destroyed
> >> + *
> >> + * This message is sent across to publish a new service, or announce
> >> + * about its removal. When we receive these messages, an appropriate
> >> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> >> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> >> + * (if/as-soon-as one is registered).
> >> + */
> >> +struct rpmsg_ns_msg {
> >> +    char name[RPMSG_NAME_SIZE];
> >> +    u32 addr;
> >> +    u32 flags;
> >> +} __packed;
> >> +
> >> +/* invoked when a name service announcement arrives */
> >> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >> +                   void *priv, u32 src)
> >> +{
> >> +    struct rpmsg_ns_msg *msg = data;
> >> +    struct rpmsg_device *newch;
> >> +    struct rpmsg_channel_info chinfo;
> >> +    struct device *dev = &rpdev->dev;
> >> +    int ret;
> >> +
> >> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +    dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> >> +                     data, len, true);
> >> +#endif
> >> +
> >> +    if (len != sizeof(*msg)) {
> >> +            dev_err(dev, "malformed ns msg (%d)\n", len);
> >> +            return -EINVAL;
> >> +    }
> >> +
> >> +    /* don't trust the remote processor for null terminating the name */
> >> +    msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> >> +
> >> +    strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> >> +    chinfo.src = RPMSG_ADDR_ANY;
> >> +    chinfo.dst = msg->addr;
> >> +
> >> +    dev_info(dev, "%sing channel %s addr 0x%x\n",
> >> +             msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> >> +             msg->name, msg->addr);
> >> +
> >> +    if (msg->flags & RPMSG_NS_DESTROY) {
> >> +            ret = rpmsg_release_channel(rpdev, &chinfo);
> >> +            if (ret)
> >> +                    dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> >> +    } else {
> >> +            newch = rpmsg_create_channel(rpdev, &chinfo);
> >> +            if (!newch)
> >> +                    dev_err(dev, "rpmsg_create_channel failed\n");
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> >> +{
> >> +    struct rpmsg_channel_info ns_chinfo;
> >> +    struct rpmsg_endpoint *ns_ept;
> >> +
> >> +    ns_chinfo.src = RPMSG_NS_ADDR;
> >> +    ns_chinfo.dst = RPMSG_NS_ADDR;
> >> +    strcpy(ns_chinfo.name, "name_service");
> >> +
> >> +    /*
> >> +     * create and attach the endpoint to the rpmsg device that it would be
> >> +     * destroy when the rpmsg device will be deleted
> >> +     */
> >
> > This comment doesn't work, please revise.
>
> Could you clarify what does not work, from your POV?
> in view of your comment, it seems I should at least rephrase it...
> proposal:
>         /*
>          * Create the NS service endpoint associated to the rpmsg device.
>          * The endpoint will be automatically destroyed when the rpmsg device
>          * will be deleted.
>          */

Yes, now it is perfectly clear - thanks for revisiting.

>
> >
> >> +    ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo);
> >> +    if (!ns_ept) {
> >> +            dev_err(&rpdev->dev, "failed to create the ns ept\n");
> >> +            return -ENOMEM;
> >> +    }
> >> +    rpdev->ept = ns_ept;
> >> +
> >> +    rpdev->src = RPMSG_NS_ADDR;
> >
> > I think this is already done in rpmsg_ns_register_device().
>
> You are right!
>
> thanks,
> Arnaud
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static struct rpmsg_driver rpmsg_ns_driver = {
> >> +    .drv.name = "rpmsg_ns",
> >> +    .probe = rpmsg_ns_probe,
> >> +};
> >> +
> >> +static int rpmsg_ns_init(void)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = register_rpmsg_driver(&rpmsg_ns_driver);
> >> +    if (ret < 0)
> >> +            pr_err("%s: Failed to register rpmsg driver\n", __func__);
> >> +
> >> +    return ret;
> >> +}
> >> +postcore_initcall(rpmsg_ns_init);
> >> +
> >> +static void rpmsg_ns_exit(void)
> >> +{
> >> +    unregister_rpmsg_driver(&rpmsg_ns_driver);
> >> +}
> >> +module_exit(rpmsg_ns_exit);
> >> +
> >> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> >> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> >> +MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_LICENSE("GPL v2");
> >> --
> >> 2.17.1
> >>

[PATCH 6/9] rpmsg: virtio: use rpmsg ns device for the ns announcement

[Arnaud Pouliquen]

As generic NS driver is available, rely on it for NS management instead of
managing it in RPMsg virtio bus.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/Kconfig            |  1 +
 drivers/rpmsg/virtio_rpmsg_bus.c | 86 ++++++++------------------------
 2 files changed, 21 insertions(+), 66 deletions(-)

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 140faa975ea1..0143c9864c45 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -71,5 +71,6 @@ config RPMSG_VIRTIO
 	depends on HAS_DMA
 	select RPMSG
 	select VIRTIO
+	select RPMSG_NS
 
 endmenu
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index b2927661868c..f771fdae150e 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -48,7 +48,6 @@
  * @endpoints_lock: lock of the endpoints set
  * @sendq:	wait queue of sending contexts waiting for a tx buffers
  * @sleepers:	number of senders that are waiting for a tx buffer
- * @ns_ept:	the bus's name service endpoint
  *
  * This structure stores the rpmsg state of a given virtio remote processor
  * device (there might be several virtio proc devices for each physical
@@ -67,7 +66,6 @@ struct virtproc_info {
 	struct mutex endpoints_lock;
 	wait_queue_head_t sendq;
 	atomic_t sleepers;
-	struct rpmsg_endpoint *ns_ept;
 };
 
 /* The feature bitmap for virtio rpmsg */
@@ -841,68 +839,14 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
 	wake_up_interruptible(&vrp->sendq);
 }
 
-/* invoked when a name service announcement arrives */
-static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
-		       void *priv, u32 src)
-{
-	struct rpmsg_ns_msg *msg = data;
-	struct rpmsg_device *newch;
-	struct rpmsg_channel_info chinfo;
-	struct virtproc_info *vrp = priv;
-	struct device *dev = &vrp->vdev->dev;
-	int ret;
-
-#if defined(CONFIG_DYNAMIC_DEBUG)
-	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
-			 data, len, true);
-#endif
-
-	if (len != sizeof(*msg)) {
-		dev_err(dev, "malformed ns msg (%d)\n", len);
-		return -EINVAL;
-	}
-
-	/*
-	 * the name service ept does _not_ belong to a real rpmsg channel,
-	 * and is handled by the rpmsg bus itself.
-	 * for sanity reasons, make sure a valid rpdev has _not_ sneaked
-	 * in somehow.
-	 */
-	if (rpdev) {
-		dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
-		return -EINVAL;
-	}
-
-	/* don't trust the remote processor for null terminating the name */
-	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
-
-	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
-	chinfo.src = RPMSG_ADDR_ANY;
-	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
-
-	dev_info(dev, "%sing channel %s addr 0x%x\n",
-		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
-		 "destroy" : "creat", msg->name, chinfo.dst);
-
-	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
-		ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
-		if (ret)
-			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
-	} else {
-		newch = __rpmsg_create_channel(vrp, &chinfo);
-		if (!newch)
-			dev_err(dev, "rpmsg_create_channel failed\n");
-	}
-
-	return 0;
-}
-
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
 	static const char * const names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct virtproc_info *vrp;
+	struct virtio_rpmsg_channel *vch;
+	struct rpmsg_device *rpdev_ns;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
@@ -978,14 +922,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 	/* if supported by the remote processor, enable the name service */
 	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
-		/* a dedicated endpoint handles the name service msgs */
-		vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
-						vrp, RPMSG_NS_ADDR);
-		if (!vrp->ns_ept) {
-			dev_err(&vdev->dev, "failed to create the ns ept\n");
+		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+		if (!vch) {
 			err = -ENOMEM;
 			goto free_coherent;
 		}
+
+		/* Link the channel to our vrp */
+		vch->vrp = vrp;
+
+		/* Assign public information to the rpmsg_device */
+		rpdev_ns = &vch->rpdev;
+		rpdev_ns->ops = &virtio_rpmsg_ops;
+
+		rpdev_ns->dev.parent = &vrp->vdev->dev;
+		rpdev_ns->dev.release = virtio_rpmsg_release_device;
+
+		err = rpmsg_ns_register_device(rpdev_ns);
+		if (err) {
+			kfree(vch);
+			goto free_coherent;
+		}
 	}
 
 	/*
@@ -1038,9 +995,6 @@ static void rpmsg_remove(struct virtio_device *vdev)
 	if (ret)
 		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
 
-	if (vrp->ns_ept)
-		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
-
 	idr_destroy(&vrp->endpoints);
 
 	vdev->config->del_vqs(vrp->vdev);
-- 
2.17.1

Re: [PATCH 6/9] rpmsg: virtio: use rpmsg ns device for the ns announcement

[Mathieu Poirier]

On Fri, Jul 31, 2020 at 01:47:29PM +0200, Arnaud Pouliquen wrote:
> As generic NS driver is available, rely on it for NS management instead of
> managing it in RPMsg virtio bus.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/Kconfig            |  1 +
>  drivers/rpmsg/virtio_rpmsg_bus.c | 86 ++++++++------------------------
>  2 files changed, 21 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 140faa975ea1..0143c9864c45 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -71,5 +71,6 @@ config RPMSG_VIRTIO
>  	depends on HAS_DMA
>  	select RPMSG
>  	select VIRTIO
> +	select RPMSG_NS
>  
>  endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index b2927661868c..f771fdae150e 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -48,7 +48,6 @@
>   * @endpoints_lock: lock of the endpoints set
>   * @sendq:	wait queue of sending contexts waiting for a tx buffers
>   * @sleepers:	number of senders that are waiting for a tx buffer
> - * @ns_ept:	the bus's name service endpoint
>   *
>   * This structure stores the rpmsg state of a given virtio remote processor
>   * device (there might be several virtio proc devices for each physical
> @@ -67,7 +66,6 @@ struct virtproc_info {
>  	struct mutex endpoints_lock;
>  	wait_queue_head_t sendq;
>  	atomic_t sleepers;
> -	struct rpmsg_endpoint *ns_ept;
>  };
>  
>  /* The feature bitmap for virtio rpmsg */
> @@ -841,68 +839,14 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>  	wake_up_interruptible(&vrp->sendq);
>  }
>  
> -/* invoked when a name service announcement arrives */
> -static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> -		       void *priv, u32 src)
> -{
> -	struct rpmsg_ns_msg *msg = data;
> -	struct rpmsg_device *newch;
> -	struct rpmsg_channel_info chinfo;
> -	struct virtproc_info *vrp = priv;
> -	struct device *dev = &vrp->vdev->dev;
> -	int ret;
> -
> -#if defined(CONFIG_DYNAMIC_DEBUG)
> -	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> -			 data, len, true);
> -#endif
> -
> -	if (len != sizeof(*msg)) {
> -		dev_err(dev, "malformed ns msg (%d)\n", len);
> -		return -EINVAL;
> -	}
> -
> -	/*
> -	 * the name service ept does _not_ belong to a real rpmsg channel,
> -	 * and is handled by the rpmsg bus itself.
> -	 * for sanity reasons, make sure a valid rpdev has _not_ sneaked
> -	 * in somehow.
> -	 */
> -	if (rpdev) {
> -		dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
> -		return -EINVAL;
> -	}
> -
> -	/* don't trust the remote processor for null terminating the name */
> -	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> -
> -	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> -	chinfo.src = RPMSG_ADDR_ANY;
> -	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
> -
> -	dev_info(dev, "%sing channel %s addr 0x%x\n",
> -		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> -		 "destroy" : "creat", msg->name, chinfo.dst);
> -
> -	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> -		ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> -		if (ret)
> -			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> -	} else {
> -		newch = __rpmsg_create_channel(vrp, &chinfo);
> -		if (!newch)
> -			dev_err(dev, "rpmsg_create_channel failed\n");
> -	}
> -
> -	return 0;
> -}
> -
>  static int rpmsg_probe(struct virtio_device *vdev)
>  {
>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>  	static const char * const names[] = { "input", "output" };
>  	struct virtqueue *vqs[2];
>  	struct virtproc_info *vrp;
> +	struct virtio_rpmsg_channel *vch;
> +	struct rpmsg_device *rpdev_ns;
>  	void *bufs_va;
>  	int err = 0, i;
>  	size_t total_buf_space;
> @@ -978,14 +922,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  	/* if supported by the remote processor, enable the name service */
>  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> -		/* a dedicated endpoint handles the name service msgs */
> -		vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
> -						vrp, RPMSG_NS_ADDR);
> -		if (!vrp->ns_ept) {
> -			dev_err(&vdev->dev, "failed to create the ns ept\n");
> +		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> +		if (!vch) {
>  			err = -ENOMEM;
>  			goto free_coherent;
>  		}
> +
> +		/* Link the channel to our vrp */
> +		vch->vrp = vrp;
> +
> +		/* Assign public information to the rpmsg_device */
> +		rpdev_ns = &vch->rpdev;
> +		rpdev_ns->ops = &virtio_rpmsg_ops;
> +
> +		rpdev_ns->dev.parent = &vrp->vdev->dev;
> +		rpdev_ns->dev.release = virtio_rpmsg_release_device;
> +
> +		err = rpmsg_ns_register_device(rpdev_ns);
> +		if (err) {
> +			kfree(vch);
> +			goto free_coherent;
> +		}

This patch doesn't apply to Bjorn's rproc-next anymore - would you mind
rebasing?

>  	}
>  
>  	/*
> @@ -1038,9 +995,6 @@ static void rpmsg_remove(struct virtio_device *vdev)
>  	if (ret)
>  		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
>  
> -	if (vrp->ns_ept)
> -		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
> -
>  	idr_destroy(&vrp->endpoints);
>  
>  	vdev->config->del_vqs(vrp->vdev);
> -- 
> 2.17.1
> 

Re: [PATCH 6/9] rpmsg: virtio: use rpmsg ns device for the ns announcement

[Arnaud POULIQUEN]

On 8/25/20 12:48 AM, Mathieu Poirier wrote:
> On Fri, Jul 31, 2020 at 01:47:29PM +0200, Arnaud Pouliquen wrote:
>> As generic NS driver is available, rely on it for NS management instead of
>> managing it in RPMsg virtio bus.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/Kconfig            |  1 +
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 86 ++++++++------------------------
>>  2 files changed, 21 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index 140faa975ea1..0143c9864c45 100644
>> --- a/drivers/rpmsg/Kconfig
>> +++ b/drivers/rpmsg/Kconfig
>> @@ -71,5 +71,6 @@ config RPMSG_VIRTIO
>>  	depends on HAS_DMA
>>  	select RPMSG
>>  	select VIRTIO
>> +	select RPMSG_NS
>>  
>>  endmenu
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index b2927661868c..f771fdae150e 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -48,7 +48,6 @@
>>   * @endpoints_lock: lock of the endpoints set
>>   * @sendq:	wait queue of sending contexts waiting for a tx buffers
>>   * @sleepers:	number of senders that are waiting for a tx buffer
>> - * @ns_ept:	the bus's name service endpoint
>>   *
>>   * This structure stores the rpmsg state of a given virtio remote processor
>>   * device (there might be several virtio proc devices for each physical
>> @@ -67,7 +66,6 @@ struct virtproc_info {
>>  	struct mutex endpoints_lock;
>>  	wait_queue_head_t sendq;
>>  	atomic_t sleepers;
>> -	struct rpmsg_endpoint *ns_ept;
>>  };
>>  
>>  /* The feature bitmap for virtio rpmsg */
>> @@ -841,68 +839,14 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>>  	wake_up_interruptible(&vrp->sendq);
>>  }
>>  
>> -/* invoked when a name service announcement arrives */
>> -static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> -		       void *priv, u32 src)
>> -{
>> -	struct rpmsg_ns_msg *msg = data;
>> -	struct rpmsg_device *newch;
>> -	struct rpmsg_channel_info chinfo;
>> -	struct virtproc_info *vrp = priv;
>> -	struct device *dev = &vrp->vdev->dev;
>> -	int ret;
>> -
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> -	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
>> -			 data, len, true);
>> -#endif
>> -
>> -	if (len != sizeof(*msg)) {
>> -		dev_err(dev, "malformed ns msg (%d)\n", len);
>> -		return -EINVAL;
>> -	}
>> -
>> -	/*
>> -	 * the name service ept does _not_ belong to a real rpmsg channel,
>> -	 * and is handled by the rpmsg bus itself.
>> -	 * for sanity reasons, make sure a valid rpdev has _not_ sneaked
>> -	 * in somehow.
>> -	 */
>> -	if (rpdev) {
>> -		dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	/* don't trust the remote processor for null terminating the name */
>> -	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
>> -
>> -	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
>> -	chinfo.src = RPMSG_ADDR_ANY;
>> -	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
>> -
>> -	dev_info(dev, "%sing channel %s addr 0x%x\n",
>> -		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
>> -		 "destroy" : "creat", msg->name, chinfo.dst);
>> -
>> -	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
>> -		ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
>> -		if (ret)
>> -			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
>> -	} else {
>> -		newch = __rpmsg_create_channel(vrp, &chinfo);
>> -		if (!newch)
>> -			dev_err(dev, "rpmsg_create_channel failed\n");
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static int rpmsg_probe(struct virtio_device *vdev)
>>  {
>>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>>  	static const char * const names[] = { "input", "output" };
>>  	struct virtqueue *vqs[2];
>>  	struct virtproc_info *vrp;
>> +	struct virtio_rpmsg_channel *vch;
>> +	struct rpmsg_device *rpdev_ns;
>>  	void *bufs_va;
>>  	int err = 0, i;
>>  	size_t total_buf_space;
>> @@ -978,14 +922,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  	/* if supported by the remote processor, enable the name service */
>>  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>> -		/* a dedicated endpoint handles the name service msgs */
>> -		vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
>> -						vrp, RPMSG_NS_ADDR);
>> -		if (!vrp->ns_ept) {
>> -			dev_err(&vdev->dev, "failed to create the ns ept\n");
>> +		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> +		if (!vch) {
>>  			err = -ENOMEM;
>>  			goto free_coherent;
>>  		}
>> +
>> +		/* Link the channel to our vrp */
>> +		vch->vrp = vrp;
>> +
>> +		/* Assign public information to the rpmsg_device */
>> +		rpdev_ns = &vch->rpdev;
>> +		rpdev_ns->ops = &virtio_rpmsg_ops;
>> +
>> +		rpdev_ns->dev.parent = &vrp->vdev->dev;
>> +		rpdev_ns->dev.release = virtio_rpmsg_release_device;
>> +
>> +		err = rpmsg_ns_register_device(rpdev_ns);
>> +		if (err) {
>> +			kfree(vch);
>> +			goto free_coherent;
>> +		}
> 
> This patch doesn't apply to Bjorn's rproc-next anymore - would you mind
> rebasing?

I will base my V2 on Bjorn's for-next (ca69dba7f13a293f83b08f003cd935c6abfee249)

Thanks,
Arnaud

> 
>>  	}
>>  
>>  	/*
>> @@ -1038,9 +995,6 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>  	if (ret)
>>  		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
>>  
>> -	if (vrp->ns_ept)
>> -		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
>> -
>>  	idr_destroy(&vrp->endpoints);
>>  
>>  	vdev->config->del_vqs(vrp->vdev);
>> -- 
>> 2.17.1
>>

[PATCH 7/9] rpmsg: ns: add name service announcement service

[Arnaud Pouliquen]

As the RPMsg driver is in charge of the name service announcement,
create an API to send channel creation and destruction to the remote
processor.
Notice that the source address of the message sent is now RPMSG_NS_ADDR.
Legacy implementation was to send the message with source address
corresponding to the ept created.
RPMSG_NS_ADDR as source address make sense as we want to send a message
belonging to the NS announcement service and the created ept address is
already in the message payload.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/rpmsg_internal.h |  3 +++
 drivers/rpmsg/rpmsg_ns.c       | 39 ++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 641b48f6bf2a..d1549e5cb607 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -119,4 +119,7 @@ static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
 	return rpmsg_register_device(rpdev);
 }
 
+int rpmsg_ns_announce_create(struct rpmsg_device *rpdev);
+int rpmsg_ns_announce_destroy(struct rpmsg_device *rpdev);
+
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index fe7713e737c2..cc2bd47c415a 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -37,6 +37,45 @@ struct rpmsg_ns_msg {
 	u32 flags;
 } __packed;
 
+/**
+ * rpmsg_ns_announce_create() -announce to the remote processor
+ * the service creation or destruction
+ * @rpdev: the rpmsg channel
+ * @ns_flag: related to the @rpmsg_ns_flags enum
+ *
+ * This function can be called on a channel creation or destruction to inform
+ * the remote processor, using the reserved name service anouncement channel.
+ */
+static int rpmsg_ns_channel_announce(struct rpmsg_device *rpdev, int ns_flag)
+{
+	struct rpmsg_ns_msg nsm;
+
+	if (!rpdev->announce || !rpdev->ept)
+		return 0;
+
+	if (ns_flag != RPMSG_NS_CREATE && ns_flag != RPMSG_NS_DESTROY)
+		return -EINVAL;
+
+	strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
+	nsm.addr = rpdev->ept->addr;
+	nsm.flags = ns_flag;
+
+	return rpmsg_send_offchannel(rpdev->ept, RPMSG_NS_ADDR, RPMSG_NS_ADDR,
+				     &nsm, sizeof(nsm));
+}
+
+int rpmsg_ns_announce_create(struct rpmsg_device *rpdev)
+{
+	return rpmsg_ns_channel_announce(rpdev, RPMSG_NS_CREATE);
+}
+EXPORT_SYMBOL(rpmsg_ns_announce_create);
+
+int rpmsg_ns_announce_destroy(struct rpmsg_device *rpdev)
+{
+	return rpmsg_ns_channel_announce(rpdev, RPMSG_NS_DESTROY);
+}
+EXPORT_SYMBOL(rpmsg_ns_announce_destroy);
+
 /* invoked when a name service announcement arrives */
 static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 		       void *priv, u32 src)
-- 
2.17.1

[PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement

[Arnaud Pouliquen]

Use the new rpmsg_ns API to send the name service announcements if
the VIRTIO_RPMSG_F_NS is set, else just not implement the ops.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index f771fdae150e..3c771a6392be 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -91,35 +91,6 @@ struct rpmsg_hdr {
 	u8 data[];
 } __packed;
 
-/**
- * struct rpmsg_ns_msg - dynamic name service announcement message
- * @name: name of remote service that is published
- * @addr: address of remote service that is published
- * @flags: indicates whether service is created or destroyed
- *
- * This message is sent across to publish a new service, or announce
- * about its removal. When we receive these messages, an appropriate
- * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
- * or ->remove() handler of the appropriate rpmsg driver will be invoked
- * (if/as-soon-as one is registered).
- */
-struct rpmsg_ns_msg {
-	char name[RPMSG_NAME_SIZE];
-	__virtio32 addr;
-	__virtio32 flags;
-} __packed;
-
-/**
- * enum rpmsg_ns_flags - dynamic name service announcement flags
- *
- * @RPMSG_NS_CREATE: a new remote service was just created
- * @RPMSG_NS_DESTROY: a known remote service was just destroyed
- */
-enum rpmsg_ns_flags {
-	RPMSG_NS_CREATE		= 0,
-	RPMSG_NS_DESTROY	= 1,
-};
-
 /**
  * @vrp: the remote processor this channel belongs to
  */
@@ -324,60 +295,18 @@ static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 	__rpmsg_destroy_ept(vch->vrp, ept);
 }
 
-static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
-{
-	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
-	struct virtproc_info *vrp = vch->vrp;
-	struct device *dev = &rpdev->dev;
-	int err = 0;
-
-	/* need to tell remote processor's name service about this channel ? */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
-		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
-		if (err)
-			dev_err(dev, "failed to announce service %d\n", err);
-	}
-
-	return err;
-}
-
-static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
-{
-	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
-	struct virtproc_info *vrp = vch->vrp;
-	struct device *dev = &rpdev->dev;
-	int err = 0;
-
-	/* tell remote processor's name service we're removing this channel */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
-		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
-		if (err)
-			dev_err(dev, "failed to announce service %d\n", err);
-	}
-
-	return err;
-}
-
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
 	.create_channel = virtio_rpmsg_create_channel,
 	.release_channel = virtio_rpmsg_release_channel,
 	.create_ept = virtio_rpmsg_create_ept,
-	.announce_create = virtio_rpmsg_announce_create,
-	.announce_destroy = virtio_rpmsg_announce_destroy,
+};
+
+static const struct rpmsg_device_ops virtio_rpmsg_w_nsa_ops = {
+	.create_channel = virtio_rpmsg_create_channel,
+	.release_channel = virtio_rpmsg_release_channel,
+	.create_ept = virtio_rpmsg_create_ept,
+	.announce_create = rpmsg_ns_announce_create,
+	.announce_destroy = rpmsg_ns_announce_destroy,
 };
 
 static void virtio_rpmsg_release_device(struct device *dev)
@@ -423,7 +352,10 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
 	rpdev = &vch->rpdev;
 	rpdev->src = chinfo->src;
 	rpdev->dst = chinfo->dst;
-	rpdev->ops = &virtio_rpmsg_ops;
+	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
+		rpdev->ops = &virtio_rpmsg_w_nsa_ops;
+	else
+		rpdev->ops = &virtio_rpmsg_ops;
 
 	/*
 	 * rpmsg server channels has predefined local address (for now),
@@ -933,7 +865,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 		/* Assign public information to the rpmsg_device */
 		rpdev_ns = &vch->rpdev;
-		rpdev_ns->ops = &virtio_rpmsg_ops;
+		rpdev_ns->ops = &virtio_rpmsg_w_nsa_ops;
 
 		rpdev_ns->dev.parent = &vrp->vdev->dev;
 		rpdev_ns->dev.release = virtio_rpmsg_release_device;
-- 
2.17.1

Re: [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement

[Mathieu Poirier]

Hi Arnaud,

On Fri, Jul 31, 2020 at 01:47:31PM +0200, Arnaud Pouliquen wrote:
> Use the new rpmsg_ns API to send the name service announcements if
> the VIRTIO_RPMSG_F_NS is set, else just not implement the ops.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 94 +++++---------------------------
>  1 file changed, 13 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index f771fdae150e..3c771a6392be 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -91,35 +91,6 @@ struct rpmsg_hdr {
>  	u8 data[];
>  } __packed;
>  
> -/**
> - * struct rpmsg_ns_msg - dynamic name service announcement message
> - * @name: name of remote service that is published
> - * @addr: address of remote service that is published
> - * @flags: indicates whether service is created or destroyed
> - *
> - * This message is sent across to publish a new service, or announce
> - * about its removal. When we receive these messages, an appropriate
> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
> - * (if/as-soon-as one is registered).
> - */
> -struct rpmsg_ns_msg {
> -	char name[RPMSG_NAME_SIZE];
> -	__virtio32 addr;
> -	__virtio32 flags;
> -} __packed;
> -
> -/**
> - * enum rpmsg_ns_flags - dynamic name service announcement flags
> - *
> - * @RPMSG_NS_CREATE: a new remote service was just created
> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> - */
> -enum rpmsg_ns_flags {
> -	RPMSG_NS_CREATE		= 0,
> -	RPMSG_NS_DESTROY	= 1,
> -};
> -
>  /**
>   * @vrp: the remote processor this channel belongs to
>   */
> @@ -324,60 +295,18 @@ static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>  	__rpmsg_destroy_ept(vch->vrp, ept);
>  }
>  
> -static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
> -{
> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> -	struct virtproc_info *vrp = vch->vrp;
> -	struct device *dev = &rpdev->dev;
> -	int err = 0;
> -
> -	/* need to tell remote processor's name service about this channel ? */
> -	if (rpdev->announce && rpdev->ept &&
> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> -		struct rpmsg_ns_msg nsm;
> -
> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
> -
> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> -		if (err)
> -			dev_err(dev, "failed to announce service %d\n", err);
> -	}
> -
> -	return err;
> -}
> -
> -static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> -{
> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> -	struct virtproc_info *vrp = vch->vrp;
> -	struct device *dev = &rpdev->dev;
> -	int err = 0;
> -
> -	/* tell remote processor's name service we're removing this channel */
> -	if (rpdev->announce && rpdev->ept &&
> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> -		struct rpmsg_ns_msg nsm;
> -
> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
> -
> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> -		if (err)
> -			dev_err(dev, "failed to announce service %d\n", err);
> -	}
> -
> -	return err;
> -}
> -
>  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
>  	.create_channel = virtio_rpmsg_create_channel,
>  	.release_channel = virtio_rpmsg_release_channel,
>  	.create_ept = virtio_rpmsg_create_ept,
> -	.announce_create = virtio_rpmsg_announce_create,
> -	.announce_destroy = virtio_rpmsg_announce_destroy,
> +};
> +
> +static const struct rpmsg_device_ops virtio_rpmsg_w_nsa_ops = {
> +	.create_channel = virtio_rpmsg_create_channel,
> +	.release_channel = virtio_rpmsg_release_channel,
> +	.create_ept = virtio_rpmsg_create_ept,
> +	.announce_create = rpmsg_ns_announce_create,
> +	.announce_destroy = rpmsg_ns_announce_destroy,
>  };
>  
>  static void virtio_rpmsg_release_device(struct device *dev)
> @@ -423,7 +352,10 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
>  	rpdev = &vch->rpdev;
>  	rpdev->src = chinfo->src;
>  	rpdev->dst = chinfo->dst;
> -	rpdev->ops = &virtio_rpmsg_ops;
> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
> +		rpdev->ops = &virtio_rpmsg_w_nsa_ops;
> +	else
> +		rpdev->ops = &virtio_rpmsg_ops;

Yesterday I struggled with this part and I still do this morning.  Function
__rpmsg_create_channel() can only be called if VIRTIO_RPMSG_F_NS is set so there
is no need to check it again.  I would also have expected this patch to be a
simple replace of the .announce_create/destroy functions.  Adding an ops that
doesn't have the .announce_create/destroy functions looks like a feature to me,
and one that I don't quite get.

Do you think you could expand on the motivation behind this patch?

Thanks,
Mathieu 

>  
>  	/*
>  	 * rpmsg server channels has predefined local address (for now),
> @@ -933,7 +865,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  		/* Assign public information to the rpmsg_device */
>  		rpdev_ns = &vch->rpdev;
> -		rpdev_ns->ops = &virtio_rpmsg_ops;
> +		rpdev_ns->ops = &virtio_rpmsg_w_nsa_ops;
>  
>  		rpdev_ns->dev.parent = &vrp->vdev->dev;
>  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
> -- 
> 2.17.1
> 

Re: [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement

[Arnaud POULIQUEN]

Hi mathieu,

I Sent my V2 few seconds before receiving your comment :)
Please find my answer below

On 8/25/20 6:54 PM, Mathieu Poirier wrote:
> Hi Arnaud,
> 
> On Fri, Jul 31, 2020 at 01:47:31PM +0200, Arnaud Pouliquen wrote:
>> Use the new rpmsg_ns API to send the name service announcements if
>> the VIRTIO_RPMSG_F_NS is set, else just not implement the ops.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 94 +++++---------------------------
>>  1 file changed, 13 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index f771fdae150e..3c771a6392be 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -91,35 +91,6 @@ struct rpmsg_hdr {
>>  	u8 data[];
>>  } __packed;
>>  
>> -/**
>> - * struct rpmsg_ns_msg - dynamic name service announcement message
>> - * @name: name of remote service that is published
>> - * @addr: address of remote service that is published
>> - * @flags: indicates whether service is created or destroyed
>> - *
>> - * This message is sent across to publish a new service, or announce
>> - * about its removal. When we receive these messages, an appropriate
>> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
>> - * (if/as-soon-as one is registered).
>> - */
>> -struct rpmsg_ns_msg {
>> -	char name[RPMSG_NAME_SIZE];
>> -	__virtio32 addr;
>> -	__virtio32 flags;
>> -} __packed;
>> -
>> -/**
>> - * enum rpmsg_ns_flags - dynamic name service announcement flags
>> - *
>> - * @RPMSG_NS_CREATE: a new remote service was just created
>> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>> - */
>> -enum rpmsg_ns_flags {
>> -	RPMSG_NS_CREATE		= 0,
>> -	RPMSG_NS_DESTROY	= 1,
>> -};
>> -
>>  /**
>>   * @vrp: the remote processor this channel belongs to
>>   */
>> @@ -324,60 +295,18 @@ static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>  	__rpmsg_destroy_ept(vch->vrp, ept);
>>  }
>>  
>> -static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
>> -{
>> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> -	struct virtproc_info *vrp = vch->vrp;
>> -	struct device *dev = &rpdev->dev;
>> -	int err = 0;
>> -
>> -	/* need to tell remote processor's name service about this channel ? */
>> -	if (rpdev->announce && rpdev->ept &&
>> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>> -		struct rpmsg_ns_msg nsm;
>> -
>> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
>> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
>> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
>> -
>> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>> -		if (err)
>> -			dev_err(dev, "failed to announce service %d\n", err);
>> -	}
>> -
>> -	return err;
>> -}
>> -
>> -static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>> -{
>> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> -	struct virtproc_info *vrp = vch->vrp;
>> -	struct device *dev = &rpdev->dev;
>> -	int err = 0;
>> -
>> -	/* tell remote processor's name service we're removing this channel */
>> -	if (rpdev->announce && rpdev->ept &&
>> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>> -		struct rpmsg_ns_msg nsm;
>> -
>> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
>> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
>> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
>> -
>> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>> -		if (err)
>> -			dev_err(dev, "failed to announce service %d\n", err);
>> -	}
>> -
>> -	return err;
>> -}
>> -
>>  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
>>  	.create_channel = virtio_rpmsg_create_channel,
>>  	.release_channel = virtio_rpmsg_release_channel,
>>  	.create_ept = virtio_rpmsg_create_ept,
>> -	.announce_create = virtio_rpmsg_announce_create,
>> -	.announce_destroy = virtio_rpmsg_announce_destroy,
>> +};
>> +
>> +static const struct rpmsg_device_ops virtio_rpmsg_w_nsa_ops = {
>> +	.create_channel = virtio_rpmsg_create_channel,
>> +	.release_channel = virtio_rpmsg_release_channel,
>> +	.create_ept = virtio_rpmsg_create_ept,
>> +	.announce_create = rpmsg_ns_announce_create,
>> +	.announce_destroy = rpmsg_ns_announce_destroy,
>>  };
>>  
>>  static void virtio_rpmsg_release_device(struct device *dev)
>> @@ -423,7 +352,10 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
>>  	rpdev = &vch->rpdev;
>>  	rpdev->src = chinfo->src;
>>  	rpdev->dst = chinfo->dst;
>> -	rpdev->ops = &virtio_rpmsg_ops;
>> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
>> +		rpdev->ops = &virtio_rpmsg_w_nsa_ops;
>> +	else
>> +		rpdev->ops = &virtio_rpmsg_ops;
> 
> Yesterday I struggled with this part and I still do this morning.  Function
> __rpmsg_create_channel() can only be called if VIRTIO_RPMSG_F_NS is set so there
> is no need to check it again.

That's right if rpmsg_create_channel is called by the rpmsg_ns only. But it 
could not be the case in future, for instance with the rpmsg_ctrl series [1]
As the channel creation is decorrelate from the ns annoucement we need to check it.

[1]https://patchwork.kernel.org/patch/11694787/

I would also have expected this patch to be a
> simple replace of the .announce_create/destroy functions.  Adding an ops that
> doesn't have the .announce_create/destroy functions looks like a feature to me,
> and one that I don't quite get.
> 
> Do you think you could expand on the motivation behind this patch?

It was my first implementation. It is more of a phylosophical point than anything else.
With this path i tried to implement the following rule:
  if VIRTIO_RPMSG_F_NS is not set
      no ns announcement support
  else 
      delegate to the ns announcement RPMsg service

Rather, legacy implementation is about to implement the announce ops even if not supported.

If this implementation is confusing i can go back to my first implementation which was
only an update the virtio_rpmsg_announce_xx functions:

@@ -322,15 +304,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
 	int err = 0;
 
 	/* need to tell remote processor's name service about this channel ? */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = rpdev->ept->addr;
-		nsm.flags = RPMSG_NS_CREATE;
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
+	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_CREATE);
 		if (err)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}
@@ -346,15 +321,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 	int err = 0;
 
 	/* tell remote processor's name service we're removing this channel */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = rpdev->ept->addr;
-		nsm.flags = RPMSG_NS_DESTROY;
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
+	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_DESTROY);
 		if (err)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}

Regards,
Arnaud

> 
> Thanks,
> Mathieu 
> 
>>  
>>  	/*
>>  	 * rpmsg server channels has predefined local address (for now),
>> @@ -933,7 +865,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  		/* Assign public information to the rpmsg_device */
>>  		rpdev_ns = &vch->rpdev;
>> -		rpdev_ns->ops = &virtio_rpmsg_ops;
>> +		rpdev_ns->ops = &virtio_rpmsg_w_nsa_ops;
>>  
>>  		rpdev_ns->dev.parent = &vrp->vdev->dev;
>>  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
>> -- 
>> 2.17.1
>>

Re: [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement

[Mathieu Poirier]

On Wed, Aug 26, 2020 at 09:42:46AM +0200, Arnaud POULIQUEN wrote:
> Hi mathieu,
> 
> I Sent my V2 few seconds before receiving your comment :)
> Please find my answer below

Seconds!

> 
> On 8/25/20 6:54 PM, Mathieu Poirier wrote:
> > Hi Arnaud,
> > 
> > On Fri, Jul 31, 2020 at 01:47:31PM +0200, Arnaud Pouliquen wrote:
> >> Use the new rpmsg_ns API to send the name service announcements if
> >> the VIRTIO_RPMSG_F_NS is set, else just not implement the ops.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >> ---
> >>  drivers/rpmsg/virtio_rpmsg_bus.c | 94 +++++---------------------------
> >>  1 file changed, 13 insertions(+), 81 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index f771fdae150e..3c771a6392be 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -91,35 +91,6 @@ struct rpmsg_hdr {
> >>  	u8 data[];
> >>  } __packed;
> >>  
> >> -/**
> >> - * struct rpmsg_ns_msg - dynamic name service announcement message
> >> - * @name: name of remote service that is published
> >> - * @addr: address of remote service that is published
> >> - * @flags: indicates whether service is created or destroyed
> >> - *
> >> - * This message is sent across to publish a new service, or announce
> >> - * about its removal. When we receive these messages, an appropriate
> >> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> >> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
> >> - * (if/as-soon-as one is registered).
> >> - */
> >> -struct rpmsg_ns_msg {
> >> -	char name[RPMSG_NAME_SIZE];
> >> -	__virtio32 addr;
> >> -	__virtio32 flags;
> >> -} __packed;
> >> -
> >> -/**
> >> - * enum rpmsg_ns_flags - dynamic name service announcement flags
> >> - *
> >> - * @RPMSG_NS_CREATE: a new remote service was just created
> >> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> >> - */
> >> -enum rpmsg_ns_flags {
> >> -	RPMSG_NS_CREATE		= 0,
> >> -	RPMSG_NS_DESTROY	= 1,
> >> -};
> >> -
> >>  /**
> >>   * @vrp: the remote processor this channel belongs to
> >>   */
> >> @@ -324,60 +295,18 @@ static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
> >>  	__rpmsg_destroy_ept(vch->vrp, ept);
> >>  }
> >>  
> >> -static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
> >> -{
> >> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> >> -	struct virtproc_info *vrp = vch->vrp;
> >> -	struct device *dev = &rpdev->dev;
> >> -	int err = 0;
> >> -
> >> -	/* need to tell remote processor's name service about this channel ? */
> >> -	if (rpdev->announce && rpdev->ept &&
> >> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> >> -		struct rpmsg_ns_msg nsm;
> >> -
> >> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> >> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> >> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
> >> -
> >> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> >> -		if (err)
> >> -			dev_err(dev, "failed to announce service %d\n", err);
> >> -	}
> >> -
> >> -	return err;
> >> -}
> >> -
> >> -static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> >> -{
> >> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> >> -	struct virtproc_info *vrp = vch->vrp;
> >> -	struct device *dev = &rpdev->dev;
> >> -	int err = 0;
> >> -
> >> -	/* tell remote processor's name service we're removing this channel */
> >> -	if (rpdev->announce && rpdev->ept &&
> >> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> >> -		struct rpmsg_ns_msg nsm;
> >> -
> >> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> >> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> >> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
> >> -
> >> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> >> -		if (err)
> >> -			dev_err(dev, "failed to announce service %d\n", err);
> >> -	}
> >> -
> >> -	return err;
> >> -}
> >> -
> >>  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
> >>  	.create_channel = virtio_rpmsg_create_channel,
> >>  	.release_channel = virtio_rpmsg_release_channel,
> >>  	.create_ept = virtio_rpmsg_create_ept,
> >> -	.announce_create = virtio_rpmsg_announce_create,
> >> -	.announce_destroy = virtio_rpmsg_announce_destroy,
> >> +};
> >> +
> >> +static const struct rpmsg_device_ops virtio_rpmsg_w_nsa_ops = {
> >> +	.create_channel = virtio_rpmsg_create_channel,
> >> +	.release_channel = virtio_rpmsg_release_channel,
> >> +	.create_ept = virtio_rpmsg_create_ept,
> >> +	.announce_create = rpmsg_ns_announce_create,
> >> +	.announce_destroy = rpmsg_ns_announce_destroy,
> >>  };
> >>  
> >>  static void virtio_rpmsg_release_device(struct device *dev)
> >> @@ -423,7 +352,10 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
> >>  	rpdev = &vch->rpdev;
> >>  	rpdev->src = chinfo->src;
> >>  	rpdev->dst = chinfo->dst;
> >> -	rpdev->ops = &virtio_rpmsg_ops;
> >> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
> >> +		rpdev->ops = &virtio_rpmsg_w_nsa_ops;
> >> +	else
> >> +		rpdev->ops = &virtio_rpmsg_ops;
> > 
> > Yesterday I struggled with this part and I still do this morning.  Function
> > __rpmsg_create_channel() can only be called if VIRTIO_RPMSG_F_NS is set so there
> > is no need to check it again.
> 
> That's right if rpmsg_create_channel is called by the rpmsg_ns only. But it 
> could not be the case in future, for instance with the rpmsg_ctrl series [1]
> As the channel creation is decorrelate from the ns annoucement we need to check it.
> 
> [1]https://patchwork.kernel.org/patch/11694787/
> 
> I would also have expected this patch to be a
> > simple replace of the .announce_create/destroy functions.  Adding an ops that
> > doesn't have the .announce_create/destroy functions looks like a feature to me,
> > and one that I don't quite get.
> > 
> > Do you think you could expand on the motivation behind this patch?
> 
> It was my first implementation. It is more of a phylosophical point than anything else.
> With this path i tried to implement the following rule:
>   if VIRTIO_RPMSG_F_NS is not set
>       no ns announcement support
>   else 
>       delegate to the ns announcement RPMsg service

I had another very long look at this...  I haven't had the time to look in your
next serie so the end result is not yet clear in my head.  But...

In __rpmsg_create_channel() the new code is testing for VIRTIO_RPMSG_F_NS in
order to allocate the ops, which means that the rpdev that is fed to
virtio_rpmsg_announce_create/destroy() are associated with a virtproc_info.
Moreover there is a test in virtio_rpmsg_announce_create/destroy() that checks 
for VIRTIO_RPMSG_F_NS already.  So if the rpdev was created from another
interface than the name announcement then no message would be sent.  

So it seems to me that the last two patches could be dropped and things would
still work properly.

The good news is that I reviewed V2 today and things look good.  I will wait to
review your next set before adding RB tags.  The down side is that having spent
a fair amount of time on your set, I need to look at other people's work if I
want to be fair to everyone.  As such I have to push back the review of your
other set to next week.

Thanks,
Mathieu

> 
> Rather, legacy implementation is about to implement the announce ops even if not supported.
> 
> If this implementation is confusing i can go back to my first implementation which was
> only an update the virtio_rpmsg_announce_xx functions:
> 
> @@ -322,15 +304,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
>  	int err = 0;
>  
>  	/* need to tell remote processor's name service about this channel ? */
> -	if (rpdev->announce && rpdev->ept &&
> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> -		struct rpmsg_ns_msg nsm;
> -
> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> -		nsm.addr = rpdev->ept->addr;
> -		nsm.flags = RPMSG_NS_CREATE;
> -
> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> +		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_CREATE);
>  		if (err)
>  			dev_err(dev, "failed to announce service %d\n", err);
>  	}
> @@ -346,15 +321,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>  	int err = 0;
>  
>  	/* tell remote processor's name service we're removing this channel */
> -	if (rpdev->announce && rpdev->ept &&
> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> -		struct rpmsg_ns_msg nsm;
> -
> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> -		nsm.addr = rpdev->ept->addr;
> -		nsm.flags = RPMSG_NS_DESTROY;
> -
> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> +		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_DESTROY);
>  		if (err)
>  			dev_err(dev, "failed to announce service %d\n", err);
>  	}
> 
> Regards,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu 
> > 
> >>  
> >>  	/*
> >>  	 * rpmsg server channels has predefined local address (for now),
> >> @@ -933,7 +865,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>  
> >>  		/* Assign public information to the rpmsg_device */
> >>  		rpdev_ns = &vch->rpdev;
> >> -		rpdev_ns->ops = &virtio_rpmsg_ops;
> >> +		rpdev_ns->ops = &virtio_rpmsg_w_nsa_ops;
> >>  
> >>  		rpdev_ns->dev.parent = &vrp->vdev->dev;
> >>  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement

[Arnaud POULIQUEN]

On 8/27/20 12:10 AM, Mathieu Poirier wrote:
> I had another very long look at this...  I haven't had the time to look in your
> next serie so the end result is not yet clear in my head.  But...
> 
> In __rpmsg_create_channel() the new code is testing for VIRTIO_RPMSG_F_NS in
> order to allocate the ops, which means that the rpdev that is fed to
> virtio_rpmsg_announce_create/destroy() are associated with a virtproc_info.
> Moreover there is a test in virtio_rpmsg_announce_create/destroy() that checks 
> for VIRTIO_RPMSG_F_NS already.  So if the rpdev was created from another
> interface than the name announcement then no message would be sent.  
> 
> So it seems to me that the last two patches could be dropped and things would
> still work properly.

Yes the last 2 patches concerns the TX part. Drop them could also be an option.
  
> 
> The good news is that I reviewed V2 today and things look good.  I will wait to
> review your next set before adding RB tags.  The down side is that having spent
> a fair amount of time on your set, I need to look at other people's work if I
> want to be fair to everyone.  As such I have to push back the review of your
> other set to next week.

That's fair! And this will give me time to have a look to the detach series. 

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 

[PATCH 9/9] rpmsg: ns: name service announcement endianness

[Arnaud Pouliquen]

The endianness has to be fixed to ensure that both sides can
decode the message. The Little endian format is used according
to the generic virtio implementation.

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

diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index cc2bd47c415a..b478e2b55213 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -33,8 +33,8 @@ enum rpmsg_ns_flags {
  */
 struct rpmsg_ns_msg {
 	char name[RPMSG_NAME_SIZE];
-	u32 addr;
-	u32 flags;
+	__le32 addr;
+	__le32 flags;
 } __packed;
 
 /**
@@ -57,8 +57,8 @@ static int rpmsg_ns_channel_announce(struct rpmsg_device *rpdev, int ns_flag)
 		return -EINVAL;
 
 	strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-	nsm.addr = rpdev->ept->addr;
-	nsm.flags = ns_flag;
+	nsm.addr = cpu_to_le32(rpdev->ept->addr);
+	nsm.flags = cpu_to_le32(ns_flag);
 
 	return rpmsg_send_offchannel(rpdev->ept, RPMSG_NS_ADDR, RPMSG_NS_ADDR,
 				     &nsm, sizeof(nsm));
@@ -84,6 +84,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 	struct rpmsg_device *newch;
 	struct rpmsg_channel_info chinfo;
 	struct device *dev = &rpdev->dev;
+	unsigned int flags = le32_to_cpu(msg->flags);
 	int ret;
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
@@ -101,13 +102,13 @@ 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 = msg->addr;
+	chinfo.dst = le32_to_cpu(msg->addr);
 
 	dev_info(dev, "%sing channel %s addr 0x%x\n",
-		 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
-		 msg->name, msg->addr);
+		 flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
+		 msg->name, chinfo.dst);
 
-	if (msg->flags & RPMSG_NS_DESTROY) {
+	if (flags & RPMSG_NS_DESTROY) {
 		ret = rpmsg_release_channel(rpdev, &chinfo);
 		if (ret)
 			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
-- 
2.17.1

Re: [PATCH 9/9] rpmsg: ns: name service announcement endianness

[Mathieu Poirier]

On Fri, Jul 31, 2020 at 01:47:32PM +0200, Arnaud Pouliquen wrote:
> The endianness has to be fixed to ensure that both sides can
> decode the message. The Little endian format is used according
> to the generic virtio implementation.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/rpmsg_ns.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index cc2bd47c415a..b478e2b55213 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -33,8 +33,8 @@ enum rpmsg_ns_flags {
>   */
>  struct rpmsg_ns_msg {
>  	char name[RPMSG_NAME_SIZE];
> -	u32 addr;
> -	u32 flags;
> +	__le32 addr;
> +	__le32 flags;
>  } __packed;
>  
>  /**
> @@ -57,8 +57,8 @@ static int rpmsg_ns_channel_announce(struct rpmsg_device *rpdev, int ns_flag)
>  		return -EINVAL;
>  
>  	strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> -	nsm.addr = rpdev->ept->addr;
> -	nsm.flags = ns_flag;
> +	nsm.addr = cpu_to_le32(rpdev->ept->addr);
> +	nsm.flags = cpu_to_le32(ns_flag);
>  
>  	return rpmsg_send_offchannel(rpdev->ept, RPMSG_NS_ADDR, RPMSG_NS_ADDR,
>  				     &nsm, sizeof(nsm));
> @@ -84,6 +84,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>  	struct rpmsg_device *newch;
>  	struct rpmsg_channel_info chinfo;
>  	struct device *dev = &rpdev->dev;
> +	unsigned int flags = le32_to_cpu(msg->flags);
>  	int ret;
>  
>  #if defined(CONFIG_DYNAMIC_DEBUG)
> @@ -101,13 +102,13 @@ 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 = msg->addr;
> +	chinfo.dst = le32_to_cpu(msg->addr);
>  
>  	dev_info(dev, "%sing channel %s addr 0x%x\n",
> -		 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> -		 msg->name, msg->addr);
> +		 flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> +		 msg->name, chinfo.dst);
>  
> -	if (msg->flags & RPMSG_NS_DESTROY) {
> +	if (flags & RPMSG_NS_DESTROY) {
>  		ret = rpmsg_release_channel(rpdev, &chinfo);
>  		if (ret)
>  			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);

Please merge this patch to patch 5, I don't see a need for doing this
separately.

Thanks,
Mathieu

> -- 
> 2.17.1
> 

Re: [PATCH 0/9] introduce name service announcement rpmsg driver

[Mathieu Poirier]

On Fri, Jul 31, 2020 at 01:47:23PM +0200, Arnaud Pouliquen wrote:
> The NS announcement is implemented by several backends, but could be
> considered as part the RPMsg protocol. 
> In this case it should be managed as a reserved rpmsg service and so
> implemented on top of the rpmsg protocol.
> 
> This series introduces the rpmsg_ns driver that handles the name service
> announcement. The virtio backend is updated in consequence to use this
> service.

I have started to review this set, comments will be spread over a few days. 

Thanks,
Mathieu

> 
> Applies cleanly on Bjorn rpmsg-next branch (ddd1930d6e3e)
> 
> Arnaud Pouliquen (9):
>   rpmsg: virtio: rename rpmsg_create_channel
>   rpmsg: core: add channel creation internal API
>   rpmsg: virtio: add rpmsg channel device ops
>   rpmsg: define the name service announcement as reserved address
>   rpmsg: introduce reserved rpmsg driver for ns announcement
>   rpmsg: virtio: use rpmsg ns device for the ns announcement
>   rpmsg: ns: add name service announcement service
>   rpmsg: virtio: use rpmsg_ns driver to manage ns announcement
>   rpmsg: ns: name service announcement endianness
> 
>  drivers/rpmsg/Kconfig            |   9 ++
>  drivers/rpmsg/Makefile           |   1 +
>  drivers/rpmsg/rpmsg_core.c       |  37 ++++++
>  drivers/rpmsg/rpmsg_internal.h   |  32 +++++
>  drivers/rpmsg/rpmsg_ns.c         | 175 +++++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 213 +++++++++----------------------
>  include/linux/rpmsg.h            |   9 ++
>  7 files changed, 325 insertions(+), 151 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> 
> -- 
> 2.17.1
>