[PATCH v2 0/9] rpmsg: Make RPMSG name service modular

[PATCH v2 0/9] rpmsg: Make RPMSG name service modular

[Mathieu Poirier]

Good afternoon,

This set starts by making the RPMSG protocol transport agnostic by
moving the headers it uses to generic types and using those in the
current implementation.  From there it re-uses the work that Arnaud
published[1] to make the name service modular. 

Tested on stm32mp157 with the RPMSG client sample application.  Applies
cleanly on v5.9.

Thanks,
Mathieu

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

------
New for V2:
- Created new RPMSG types (Guennadi).
- Re-worked byte conversion functions(Guennadi).
- Added a single ->is_little_endian() operation (Arnaud).
- Fixed byte conversion before making name service modular.

Arnaud Pouliquen (4):
  rpmsg: virtio: Rename rpmsg_create_channel
  rpmsg: core: Add channel creation internal API
  rpmsg: virtio: Add rpmsg channel device ops
  rpmsg: Turn name service into a stand alone driver

Mathieu Poirier (5):
  rpmsg: Move rpmsg_endpoint_ops to rpmsg.h
  rpmsg: Introduce __rpmsg{16|32|64} types
  rpmsg: virtio: Move from virtio to rpmsg byte conversion
  rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file
  rpmsg: Make rpmsg_{register|unregister}_device() public

 drivers/rpmsg/Kconfig            |   8 ++
 drivers/rpmsg/Makefile           |   1 +
 drivers/rpmsg/rpmsg_core.c       |  44 +++++++
 drivers/rpmsg/rpmsg_internal.h   |  27 +---
 drivers/rpmsg/rpmsg_ns.c         | 110 +++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 206 ++++++++++---------------------
 include/linux/rpmsg.h            |  93 ++++++++++++--
 include/linux/rpmsg_byteorder.h  |  67 ++++++++++
 include/linux/rpmsg_ns.h         |  79 ++++++++++++
 include/uapi/linux/rpmsg.h       |   3 +
 include/uapi/linux/rpmsg_types.h |  11 ++
 11 files changed, 481 insertions(+), 168 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ns.c
 create mode 100644 include/linux/rpmsg_byteorder.h
 create mode 100644 include/linux/rpmsg_ns.h
 create mode 100644 include/uapi/linux/rpmsg_types.h

-- 
2.25.1

[PATCH v2 1/9] rpmsg: Move rpmsg_endpoint_ops to rpmsg.h

[Mathieu Poirier]

Move structure rpmsg_endpoint_ops to header rpmsg.h so that it can
be used by other entities.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_internal.h | 19 -------------------
 include/linux/rpmsg.h          | 24 +++++++++++++++++++++---
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3fc83cd50e98..094cf968d2d3 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -18,25 +18,6 @@
 #define to_rpmsg_device(d) container_of(d, struct rpmsg_device, dev)
 #define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
 
-/**
- * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
- * @create_ept:		create backend-specific endpoint, required
- * @announce_create:	announce presence of new channel, optional
- * @announce_destroy:	announce destruction of channel, optional
- *
- * Indirection table for the operations that a rpmsg backend should implement.
- * @announce_create and @announce_destroy are optional as the backend might
- * advertise new channels implicitly by creating the endpoints.
- */
-struct rpmsg_device_ops {
-	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
-					    rpmsg_rx_cb_t cb, void *priv,
-					    struct rpmsg_channel_info chinfo);
-
-	int (*announce_create)(struct rpmsg_device *ept);
-	int (*announce_destroy)(struct rpmsg_device *ept);
-};
-
 /**
  * struct rpmsg_endpoint_ops - indirection table for rpmsg_endpoint operations
  * @destroy_ept:	see @rpmsg_destroy_ept(), required
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 9fe156d1c018..9fe1c54ae995 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -22,7 +22,6 @@
 
 struct rpmsg_device;
 struct rpmsg_endpoint;
-struct rpmsg_device_ops;
 struct rpmsg_endpoint_ops;
 
 /**
@@ -37,6 +36,27 @@ struct rpmsg_channel_info {
 	u32 dst;
 };
 
+typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
+
+/**
+ * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @create_ept:		create backend-specific endpoint, required
+ * @announce_create:	announce presence of new channel, optional
+ * @announce_destroy:	announce destruction of channel, optional
+ *
+ * Indirection table for the operations that a rpmsg backend should implement.
+ * @announce_create and @announce_destroy are optional as the backend might
+ * advertise new channels implicitly by creating the endpoints.
+ */
+struct rpmsg_device_ops {
+	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
+					    rpmsg_rx_cb_t cb, void *priv,
+					    struct rpmsg_channel_info chinfo);
+
+	int (*announce_create)(struct rpmsg_device *ept);
+	int (*announce_destroy)(struct rpmsg_device *ept);
+};
+
 /**
  * rpmsg_device - device that belong to the rpmsg bus
  * @dev: the device struct
@@ -59,8 +79,6 @@ struct rpmsg_device {
 	const struct rpmsg_device_ops *ops;
 };
 
-typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
-
 /**
  * struct rpmsg_endpoint - binds a local rpmsg address to its user
  * @rpdev: rpmsg channel device
-- 
2.25.1

[PATCH v2 2/9] rpmsg: Introduce __rpmsg{16|32|64} types

[Mathieu Poirier]

Introduce __rpmsg{16|32|64} types along with byte order conversion
functions based on an rpmsg_device operation as a foundation to
make RPMSG modular and transport agnostic.

Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/rpmsg.h            | 51 ++++++++++++++++++++++++
 include/linux/rpmsg_byteorder.h  | 67 ++++++++++++++++++++++++++++++++
 include/uapi/linux/rpmsg_types.h | 11 ++++++
 3 files changed, 129 insertions(+)
 create mode 100644 include/linux/rpmsg_byteorder.h
 create mode 100644 include/uapi/linux/rpmsg_types.h

diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 9fe1c54ae995..165e4c6d4cd3 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -17,6 +17,7 @@
 #include <linux/kref.h>
 #include <linux/mutex.h>
 #include <linux/poll.h>
+#include <linux/rpmsg_byteorder.h>
 
 #define RPMSG_ADDR_ANY		0xFFFFFFFF
 
@@ -40,6 +41,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
 
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @is_little_endian:	returns true if using little endian byte ordering
  * @create_ept:		create backend-specific endpoint, required
  * @announce_create:	announce presence of new channel, optional
  * @announce_destroy:	announce destruction of channel, optional
@@ -49,6 +51,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
  * advertise new channels implicitly by creating the endpoints.
  */
 struct rpmsg_device_ops {
+	bool (*is_little_endian)(struct rpmsg_device *rpdev);
 	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
 					    rpmsg_rx_cb_t cb, void *priv,
 					    struct rpmsg_channel_info chinfo);
@@ -129,6 +132,54 @@ struct rpmsg_driver {
 	int (*callback)(struct rpmsg_device *, void *, int, void *, u32);
 };
 
+static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __rpmsg16_to_cpu(rpmsg_is_little_endian(), val);
+	else
+		return __rpmsg16_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
+}
+
+static inline __rpmsg16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __cpu_to_rpmsg16(rpmsg_is_little_endian(), val);
+	else
+		return __cpu_to_rpmsg16(rpdev->ops->is_little_endian(rpdev), val);
+}
+
+static inline u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, __rpmsg32 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __rpmsg32_to_cpu(rpmsg_is_little_endian(), val);
+	else
+		return __rpmsg32_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
+}
+
+static inline __rpmsg32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __cpu_to_rpmsg32(rpmsg_is_little_endian(), val);
+	else
+		return __cpu_to_rpmsg32(rpdev->ops->is_little_endian(rpdev), val);
+}
+
+static inline u64 rpmsg64_to_cpu(struct rpmsg_device *rpdev, __rpmsg64 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __rpmsg64_to_cpu(rpmsg_is_little_endian(), val);
+	else
+		return __rpmsg64_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
+}
+
+static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
+{
+	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
+		return __cpu_to_rpmsg64(rpmsg_is_little_endian(), val);
+	else
+		return __cpu_to_rpmsg64(rpdev->ops->is_little_endian(rpdev), val);
+}
+
 #if IS_ENABLED(CONFIG_RPMSG)
 
 int register_rpmsg_device(struct rpmsg_device *dev);
diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg_byteorder.h
new file mode 100644
index 000000000000..c0f565dbad6d
--- /dev/null
+++ b/include/linux/rpmsg_byteorder.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Follows implementation found in linux/virtio_byteorder.h
+ */
+#ifndef _LINUX_RPMSG_BYTEORDER_H
+#define _LINUX_RPMSG_BYTEORDER_H
+#include <linux/types.h>
+#include <uapi/linux/rpmsg_types.h>
+
+static inline bool rpmsg_is_little_endian(void)
+{
+#ifdef __LITTLE_ENDIAN
+	return true;
+#else
+	return false;
+#endif
+}
+
+static inline u16 __rpmsg16_to_cpu(bool little_endian, __rpmsg16 val)
+{
+	if (little_endian)
+		return le16_to_cpu((__force __le16)val);
+	else
+		return be16_to_cpu((__force __be16)val);
+}
+
+static inline __rpmsg16 __cpu_to_rpmsg16(bool little_endian, u16 val)
+{
+	if (little_endian)
+		return (__force __rpmsg16)cpu_to_le16(val);
+	else
+		return (__force __rpmsg16)cpu_to_be16(val);
+}
+
+static inline u32 __rpmsg32_to_cpu(bool little_endian, __rpmsg32 val)
+{
+	if (little_endian)
+		return le32_to_cpu((__force __le32)val);
+	else
+		return be32_to_cpu((__force __be32)val);
+}
+
+static inline __rpmsg32 __cpu_to_rpmsg32(bool little_endian, u32 val)
+{
+	if (little_endian)
+		return (__force __rpmsg32)cpu_to_le32(val);
+	else
+		return (__force __rpmsg32)cpu_to_be32(val);
+}
+
+static inline u64 __rpmsg64_to_cpu(bool little_endian, __rpmsg64 val)
+{
+	if (little_endian)
+		return le64_to_cpu((__force __le64)val);
+	else
+		return be64_to_cpu((__force __be64)val);
+}
+
+static inline __rpmsg64 __cpu_to_rpmsg64(bool little_endian, u64 val)
+{
+	if (little_endian)
+		return (__force __rpmsg64)cpu_to_le64(val);
+	else
+		return (__force __rpmsg64)cpu_to_be64(val);
+}
+
+#endif /* _LINUX_RPMSG_BYTEORDER_H */
diff --git a/include/uapi/linux/rpmsg_types.h b/include/uapi/linux/rpmsg_types.h
new file mode 100644
index 000000000000..36e3b9404391
--- /dev/null
+++ b/include/uapi/linux/rpmsg_types.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RPMSG_TYPES_H
+#define _UAPI_LINUX_RPMSG_TYPES_H
+
+#include <linux/types.h>
+
+typedef __u16 __bitwise __rpmsg16;
+typedef __u32 __bitwise __rpmsg32;
+typedef __u64 __bitwise __rpmsg64;
+
+#endif /* _UAPI_LINUX_RPMSG_TYPES_H */
-- 
2.25.1

[PATCH v2 3/9] rpmsg: virtio: Move from virtio to rpmsg byte conversion

[Mathieu Poirier]

Use rpmsg byte conversion functions in order for the RPMSG
headers and generic functions to be used by external entities.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 60 +++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 9006fc7f73d0..793fe924671f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,11 +19,11 @@
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/rpmsg.h>
+#include <linux/rpmsg_byteorder.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/virtio.h>
-#include <linux/virtio_byteorder.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 #include <linux/wait.h>
@@ -85,11 +85,11 @@ struct virtproc_info {
  * Every message sent(/received) on the rpmsg bus begins with this header.
  */
 struct rpmsg_hdr {
-	__virtio32 src;
-	__virtio32 dst;
-	__virtio32 reserved;
-	__virtio16 len;
-	__virtio16 flags;
+	__rpmsg32 src;
+	__rpmsg32 dst;
+	__rpmsg32 reserved;
+	__rpmsg16 len;
+	__rpmsg16 flags;
 	u8 data[];
 } __packed;
 
@@ -107,8 +107,8 @@ struct rpmsg_hdr {
  */
 struct rpmsg_ns_msg {
 	char name[RPMSG_NAME_SIZE];
-	__virtio32 addr;
-	__virtio32 flags;
+	__rpmsg32 addr;
+	__rpmsg32 flags;
 } __packed;
 
 /**
@@ -280,6 +280,14 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
 	return NULL;
 }
 
+static bool virtio_rpmsg_is_little_endian(struct rpmsg_device *rpdev)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return virtio_is_little_endian(vrp->vdev);
+}
+
 static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
 						      rpmsg_rx_cb_t cb,
 						      void *priv,
@@ -336,8 +344,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
 		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);
+		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
+		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_CREATE);
 
 		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
 		if (err)
@@ -360,8 +368,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 		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);
+		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
+		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_DESTROY);
 
 		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
 		if (err)
@@ -372,6 +380,7 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 }
 
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
+	.is_little_endian = virtio_rpmsg_is_little_endian,
 	.create_ept = virtio_rpmsg_create_ept,
 	.announce_create = virtio_rpmsg_announce_create,
 	.announce_destroy = virtio_rpmsg_announce_destroy,
@@ -613,10 +622,10 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 		}
 	}
 
-	msg->len = cpu_to_virtio16(vrp->vdev, len);
+	msg->len = cpu_to_rpmsg16(rpdev, len);
 	msg->flags = 0;
-	msg->src = cpu_to_virtio32(vrp->vdev, src);
-	msg->dst = cpu_to_virtio32(vrp->vdev, dst);
+	msg->src = cpu_to_rpmsg32(rpdev, src);
+	msg->dst = cpu_to_rpmsg32(rpdev, dst);
 	msg->reserved = 0;
 	memcpy(msg->data, data, len);
 
@@ -705,14 +714,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 {
 	struct rpmsg_endpoint *ept;
 	struct scatterlist sg;
-	unsigned int msg_len = virtio16_to_cpu(vrp->vdev, msg->len);
+	bool little_endian = rpmsg_is_little_endian();
+	unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
 	int err;
 
 	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
-		virtio32_to_cpu(vrp->vdev, msg->src),
-		virtio32_to_cpu(vrp->vdev, msg->dst), msg_len,
-		virtio16_to_cpu(vrp->vdev, msg->flags),
-		virtio32_to_cpu(vrp->vdev, msg->reserved));
+		__rpmsg32_to_cpu(little_endian, msg->src),
+		__rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
+		__rpmsg16_to_cpu(little_endian, msg->flags),
+		__rpmsg32_to_cpu(little_endian, msg->reserved));
 #if defined(CONFIG_DYNAMIC_DEBUG)
 	dynamic_hex_dump("rpmsg_virtio RX: ", DUMP_PREFIX_NONE, 16, 1,
 			 msg, sizeof(*msg) + msg_len, true);
@@ -731,7 +741,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 	/* use the dst addr to fetch the callback of the appropriate user */
 	mutex_lock(&vrp->endpoints_lock);
 
-	ept = idr_find(&vrp->endpoints, virtio32_to_cpu(vrp->vdev, msg->dst));
+	ept = idr_find(&vrp->endpoints, __rpmsg32_to_cpu(little_endian, msg->dst));
 
 	/* let's make sure no one deallocates ept while we use it */
 	if (ept)
@@ -745,7 +755,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 
 		if (ept->cb)
 			ept->cb(ept->rpdev, msg->data, msg_len, ept->priv,
-				virtio32_to_cpu(vrp->vdev, msg->src));
+				__rpmsg32_to_cpu(little_endian, msg->src));
 
 		mutex_unlock(&ept->cb_lock);
 
@@ -853,13 +863,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 = virtio32_to_cpu(vrp->vdev, msg->addr);
+	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
 
 	dev_info(dev, "%sing channel %s addr 0x%x\n",
-		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
+		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
 		 "destroy" : "creat", msg->name, chinfo.dst);
 
-	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
+	if (rpmsg32_to_cpu(rpdev, 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);
-- 
2.25.1

[PATCH v2 4/9] rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file

[Mathieu Poirier]

Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
so that they can be used by other entities.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
 include/linux/rpmsg_ns.h         | 62 ++++++++++++++++++++++++++++++++
 include/uapi/linux/rpmsg.h       |  3 ++
 3 files changed, 67 insertions(+), 56 deletions(-)
 create mode 100644 include/linux/rpmsg_ns.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 793fe924671f..85f2acc4ed9f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,7 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/rpmsg.h>
-#include <linux/rpmsg_byteorder.h>
+#include <linux/rpmsg_ns.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -27,6 +27,7 @@
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 #include <linux/wait.h>
+#include <uapi/linux/rpmsg.h>
 
 #include "rpmsg_internal.h"
 
@@ -70,58 +71,6 @@ struct virtproc_info {
 	struct rpmsg_endpoint *ns_ept;
 };
 
-/* The feature bitmap for virtio rpmsg */
-#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
-
-/**
- * struct rpmsg_hdr - common header for all rpmsg messages
- * @src: source address
- * @dst: destination address
- * @reserved: reserved for future use
- * @len: length of payload (in bytes)
- * @flags: message flags
- * @data: @len bytes of message payload data
- *
- * Every message sent(/received) on the rpmsg bus begins with this header.
- */
-struct rpmsg_hdr {
-	__rpmsg32 src;
-	__rpmsg32 dst;
-	__rpmsg32 reserved;
-	__rpmsg16 len;
-	__rpmsg16 flags;
-	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];
-	__rpmsg32 addr;
-	__rpmsg32 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
  */
@@ -162,9 +111,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_ns.h b/include/linux/rpmsg_ns.h
new file mode 100644
index 000000000000..3d836b8580b2
--- /dev/null
+++ b/include/linux/rpmsg_ns.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_RPMSG_NS_H
+#define _LINUX_RPMSG_NS_H
+
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+#include <linux/rpmsg_byteorder.h>
+
+/**
+ * struct rpmsg_hdr - common header for all rpmsg messages
+ * @src: source address
+ * @dst: destination address
+ * @reserved: reserved for future use
+ * @len: length of payload (in bytes)
+ * @flags: message flags
+ * @data: @len bytes of message payload data
+ *
+ * Every message sent(/received) on the rpmsg bus begins with this header.
+ */
+struct rpmsg_hdr {
+	__rpmsg32 src;
+	__rpmsg32 dst;
+	__rpmsg32 reserved;
+	__rpmsg16 len;
+	__rpmsg16 flags;
+	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];
+	__rpmsg32 addr;
+	__rpmsg32 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,
+};
+
+/* Address 53 is reserved for advertising remote services */
+#define RPMSG_NS_ADDR			(53)
+
+#endif
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..d669c04ef289 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
 #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
+
 #endif
-- 
2.25.1

[PATCH v2 5/9] rpmsg: virtio: Rename rpmsg_create_channel

[Mathieu Poirier]

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

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>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 85f2acc4ed9f..fca982b61c3b 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -345,8 +345,8 @@ 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;
@@ -820,7 +820,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.25.1

[PATCH v2 6/9] rpmsg: core: Add channel creation internal API

[Mathieu Poirier]

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

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>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_core.c     | 44 ++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h |  4 ++++
 include/linux/rpmsg.h          |  6 +++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index a6361cad608b..a5c4b80debf3 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,50 @@
 
 #include "rpmsg_internal.h"
 
+/**
+ * rpmsg_create_channel() - create a new rpmsg channel
+ * using its name and address info.
+ * @rpdev: rpmsg device
+ * @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))
+		return NULL;
+	if (!rpdev->ops || !rpdev->ops->create_channel) {
+		dev_err(&rpdev->dev, "no create_channel ops found\n");
+		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 device
+ * @chinfo: channel_info to bind
+ *
+ * Returns 0 on success or an appropriate error value.
+ */
+int rpmsg_release_channel(struct rpmsg_device *rpdev,
+			  struct rpmsg_channel_info *chinfo)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->release_channel) {
+		dev_err(&rpdev->dev, "no release_channel ops found\n");
+		return -ENXIO;
+	}
+
+	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 094cf968d2d3..b9b34b416b7b 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -56,6 +56,10 @@ 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
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 165e4c6d4cd3..eb70463a9f2e 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -42,6 +42,8 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
  * @is_little_endian:	returns true if using little endian byte ordering
+ * @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
@@ -52,6 +54,10 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
  */
 struct rpmsg_device_ops {
 	bool (*is_little_endian)(struct rpmsg_device *rpdev);
+	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);
-- 
2.25.1

[PATCH v2 7/9] rpmsg: virtio: Add rpmsg channel device ops

[Mathieu Poirier]

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

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

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index fca982b61c3b..1488b9ddc18d 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -122,6 +122,8 @@ 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 * __rpmsg_create_channel(struct virtproc_info *vrp,
+						    struct rpmsg_channel_info *chinfo);
 
 static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
 	.destroy_ept = virtio_rpmsg_destroy_ept,
@@ -234,6 +236,24 @@ static bool virtio_rpmsg_is_little_endian(struct rpmsg_device *rpdev)
 	return virtio_is_little_endian(vrp->vdev);
 }
 
+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);
+}
+
 static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
 						      rpmsg_rx_cb_t cb,
 						      void *priv,
@@ -327,6 +347,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
 	.is_little_endian = virtio_rpmsg_is_little_endian,
+	.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,
-- 
2.25.1

[PATCH v2 8/9] rpmsg: Make rpmsg_{register|unregister}_device() public

[Mathieu Poirier]

Make function rpmsg_register_device() and rpmsg_unregister_device()
functions public so that they can be used by other clients.  While
doing so get rid of two obsolete function, i.e register_rpmsg_device()
and unregister_rpmsg_device(), to prevent confusion.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_internal.h |  4 ----
 include/linux/rpmsg.h          | 12 ++++++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index b9b34b416b7b..70de053581bd 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -49,10 +49,6 @@ struct rpmsg_endpoint_ops {
 			     poll_table *wait);
 };
 
-int rpmsg_register_device(struct rpmsg_device *rpdev);
-int rpmsg_unregister_device(struct device *parent,
-			    struct rpmsg_channel_info *chinfo);
-
 struct device *rpmsg_find_device(struct device *parent,
 				 struct rpmsg_channel_info *chinfo);
 
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index eb70463a9f2e..0b3ec18ddbaa 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -188,8 +188,9 @@ static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
 
 #if IS_ENABLED(CONFIG_RPMSG)
 
-int register_rpmsg_device(struct rpmsg_device *dev);
-void unregister_rpmsg_device(struct rpmsg_device *dev);
+int rpmsg_register_device(struct rpmsg_device *rpdev);
+int rpmsg_unregister_device(struct device *parent,
+			    struct rpmsg_channel_info *chinfo);
 int __register_rpmsg_driver(struct rpmsg_driver *drv, struct module *owner);
 void unregister_rpmsg_driver(struct rpmsg_driver *drv);
 void rpmsg_destroy_ept(struct rpmsg_endpoint *);
@@ -212,15 +213,18 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 
 #else
 
-static inline int register_rpmsg_device(struct rpmsg_device *dev)
+static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
 {
 	return -ENXIO;
 }
 
-static inline void unregister_rpmsg_device(struct rpmsg_device *dev)
+static inline int rpmsg_unregister_device(struct device *parent,
+					  struct rpmsg_channel_info *chinfo)
 {
 	/* This shouldn't be possible */
 	WARN_ON(1);
+
+	return -ENXIO;
 }
 
 static inline int __register_rpmsg_driver(struct rpmsg_driver *drv,
-- 
2.25.1

[PATCH v2 9/9] rpmsg: Turn name service into a stand alone driver

[Mathieu Poirier]

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Make the RPMSG name service announcement a stand alone driver so that it
can be reused by other subsystems.  It is also the first step in making the
functionatlity transport independent, i.e that is not tied to virtIO.

Co-developed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/Kconfig            |   8 +++
 drivers/rpmsg/Makefile           |   1 +
 drivers/rpmsg/rpmsg_ns.c         | 110 +++++++++++++++++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c |  84 +++++------------------
 include/linux/rpmsg_ns.h         |  17 +++++
 5 files changed, 154 insertions(+), 66 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ns.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index f96716893c2a..c3fc75e6514b 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 associated 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
new file mode 100644
index 000000000000..65be20583c8f
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -0,0 +1,110 @@
+// 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 <linux/rpmsg.h>
+#include <linux/rpmsg_byteorder.h>
+#include <linux/rpmsg_ns.h>
+
+#include "rpmsg_internal.h"
+
+/* 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.parent;
+	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 = rpmsg32_to_cpu(rpdev, msg->addr);
+
+	dev_info(dev, "%sing channel %s addr 0x%x\n",
+		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
+		 "destroy" : "creat", msg->name, chinfo.dst);
+
+	if (rpmsg32_to_cpu(rpdev, 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_endpoint *ns_ept;
+	struct rpmsg_channel_info ns_chinfo = {
+		.src = RPMSG_NS_ADDR,
+		.dst = RPMSG_NS_ADDR,
+		.name = "name_service",
+	};
+
+	/*
+	 * Create the NS announcement 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;
+
+	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");
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 1488b9ddc18d..a5284dc40ac5 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -49,7 +49,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
@@ -68,7 +67,6 @@ struct virtproc_info {
 	struct mutex endpoints_lock;
 	wait_queue_head_t sendq;
 	atomic_t sleepers;
-	struct rpmsg_endpoint *ns_ept;
 };
 
 /**
@@ -794,68 +792,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 = rpmsg32_to_cpu(rpdev, msg->addr);
-
-	dev_info(dev, "%sing channel %s addr 0x%x\n",
-		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
-		 "destroy" : "creat", msg->name, chinfo.dst);
-
-	if (rpmsg32_to_cpu(rpdev, 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;
@@ -931,14 +875,25 @@ 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)
+			goto free_coherent;
 	}
 
 	/*
@@ -991,9 +946,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);
diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
index 3d836b8580b2..9e529ef40ce5 100644
--- a/include/linux/rpmsg_ns.h
+++ b/include/linux/rpmsg_ns.h
@@ -59,4 +59,21 @@ enum rpmsg_ns_flags {
 /* Address 53 is reserved for advertising remote services */
 #define RPMSG_NS_ADDR			(53)
 
+/**
+ * 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
-- 
2.25.1

Re: [PATCH v2 0/9] rpmsg: Make RPMSG name service modular

[Guennadi Liakhovetski]

On Tue, Oct 13, 2020 at 05:25:10PM -0600, Mathieu Poirier wrote:
> Good afternoon,
> 
> This set starts by making the RPMSG protocol transport agnostic by
> moving the headers it uses to generic types and using those in the
> current implementation.  From there it re-uses the work that Arnaud
> published[1] to make the name service modular. 
> 
> Tested on stm32mp157 with the RPMSG client sample application.  Applies
> cleanly on v5.9.

Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Thanks
Guennadi

> 
> Thanks,
> Mathieu
> 
> [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> 
> ------
> New for V2:
> - Created new RPMSG types (Guennadi).
> - Re-worked byte conversion functions(Guennadi).
> - Added a single ->is_little_endian() operation (Arnaud).
> - Fixed byte conversion before making name service modular.
> 
> Arnaud Pouliquen (4):
>   rpmsg: virtio: Rename rpmsg_create_channel
>   rpmsg: core: Add channel creation internal API
>   rpmsg: virtio: Add rpmsg channel device ops
>   rpmsg: Turn name service into a stand alone driver
> 
> Mathieu Poirier (5):
>   rpmsg: Move rpmsg_endpoint_ops to rpmsg.h
>   rpmsg: Introduce __rpmsg{16|32|64} types
>   rpmsg: virtio: Move from virtio to rpmsg byte conversion
>   rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file
>   rpmsg: Make rpmsg_{register|unregister}_device() public
> 
>  drivers/rpmsg/Kconfig            |   8 ++
>  drivers/rpmsg/Makefile           |   1 +
>  drivers/rpmsg/rpmsg_core.c       |  44 +++++++
>  drivers/rpmsg/rpmsg_internal.h   |  27 +---
>  drivers/rpmsg/rpmsg_ns.c         | 110 +++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 206 ++++++++++---------------------
>  include/linux/rpmsg.h            |  93 ++++++++++++--
>  include/linux/rpmsg_byteorder.h  |  67 ++++++++++
>  include/linux/rpmsg_ns.h         |  79 ++++++++++++
>  include/uapi/linux/rpmsg.h       |   3 +
>  include/uapi/linux/rpmsg_types.h |  11 ++
>  11 files changed, 481 insertions(+), 168 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
>  create mode 100644 include/linux/rpmsg_byteorder.h
>  create mode 100644 include/linux/rpmsg_ns.h
>  create mode 100644 include/uapi/linux/rpmsg_types.h
> 
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/9] rpmsg: Introduce __rpmsg{16|32|64} types

[Arnaud POULIQUEN]

Hi Mathieu,

On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Introduce __rpmsg{16|32|64} types along with byte order conversion
> functions based on an rpmsg_device operation as a foundation to
> make RPMSG modular and transport agnostic.
> 
> Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  include/linux/rpmsg.h            | 51 ++++++++++++++++++++++++
>  include/linux/rpmsg_byteorder.h  | 67 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/rpmsg_types.h | 11 ++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 include/linux/rpmsg_byteorder.h
>  create mode 100644 include/uapi/linux/rpmsg_types.h
> 
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 9fe1c54ae995..165e4c6d4cd3 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -17,6 +17,7 @@
>  #include <linux/kref.h>
>  #include <linux/mutex.h>
>  #include <linux/poll.h>
> +#include <linux/rpmsg_byteorder.h>
>  
>  #define RPMSG_ADDR_ANY		0xFFFFFFFF
>  
> @@ -40,6 +41,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @is_little_endian:	returns true if using little endian byte ordering
>   * @create_ept:		create backend-specific endpoint, required
>   * @announce_create:	announce presence of new channel, optional
>   * @announce_destroy:	announce destruction of channel, optional
> @@ -49,6 +51,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	bool (*is_little_endian)(struct rpmsg_device *rpdev);
>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>  					    rpmsg_rx_cb_t cb, void *priv,
>  					    struct rpmsg_channel_info chinfo);
> @@ -129,6 +132,54 @@ struct rpmsg_driver {
>  	int (*callback)(struct rpmsg_device *, void *, int, void *, u32);
>  };
>  
> +static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __rpmsg16_to_cpu(rpmsg_is_little_endian(), val);
> +	else
> +		return __rpmsg16_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
> +static inline __rpmsg16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __cpu_to_rpmsg16(rpmsg_is_little_endian(), val);
> +	else
> +		return __cpu_to_rpmsg16(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
> +static inline u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, __rpmsg32 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __rpmsg32_to_cpu(rpmsg_is_little_endian(), val);
> +	else
> +		return __rpmsg32_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
> +static inline __rpmsg32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __cpu_to_rpmsg32(rpmsg_is_little_endian(), val);
> +	else
> +		return __cpu_to_rpmsg32(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
> +static inline u64 rpmsg64_to_cpu(struct rpmsg_device *rpdev, __rpmsg64 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __rpmsg64_to_cpu(rpmsg_is_little_endian(), val);
> +	else
> +		return __rpmsg64_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
> +static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
> +{
> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> +		return __cpu_to_rpmsg64(rpmsg_is_little_endian(), val);
> +	else
> +		return __cpu_to_rpmsg64(rpdev->ops->is_little_endian(rpdev), val);
> +}
> +
>  #if IS_ENABLED(CONFIG_RPMSG)
>  
>  int register_rpmsg_device(struct rpmsg_device *dev);
> diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg_byteorder.h
> new file mode 100644
> index 000000000000..c0f565dbad6d
> --- /dev/null
> +++ b/include/linux/rpmsg_byteorder.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Follows implementation found in linux/virtio_byteorder.h
> + */
> +#ifndef _LINUX_RPMSG_BYTEORDER_H
> +#define _LINUX_RPMSG_BYTEORDER_H
> +#include <linux/types.h>
> +#include <uapi/linux/rpmsg_types.h>
> +
> +static inline bool rpmsg_is_little_endian(void)
> +{
> +#ifdef __LITTLE_ENDIAN
> +	return true;
> +#else
> +	return false;
> +#endif
> +}

A suggestion:

static inline bool rpmsg_is_little_endian(void)
#if defined(__BYTE_ORDER)
#  if __BYTE_ORDER == __BIG_ENDIAN
	return true;
#  elif __BYTE_ORDER == __LITTLE_ENDIAN
	return false;
#  else
#    warning "unknown endianess, set to little by default"
	return true;
#  endif
#endif
}

Otherwise

Reviewed-by:  Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> +
> +static inline u16 __rpmsg16_to_cpu(bool little_endian, __rpmsg16 val)
> +{
> +	if (little_endian)
> +		return le16_to_cpu((__force __le16)val);
> +	else
> +		return be16_to_cpu((__force __be16)val);
> +}
> +
> +static inline __rpmsg16 __cpu_to_rpmsg16(bool little_endian, u16 val)
> +{
> +	if (little_endian)
> +		return (__force __rpmsg16)cpu_to_le16(val);
> +	else
> +		return (__force __rpmsg16)cpu_to_be16(val);
> +}
> +
> +static inline u32 __rpmsg32_to_cpu(bool little_endian, __rpmsg32 val)
> +{
> +	if (little_endian)
> +		return le32_to_cpu((__force __le32)val);
> +	else
> +		return be32_to_cpu((__force __be32)val);
> +}
> +
> +static inline __rpmsg32 __cpu_to_rpmsg32(bool little_endian, u32 val)
> +{
> +	if (little_endian)
> +		return (__force __rpmsg32)cpu_to_le32(val);
> +	else
> +		return (__force __rpmsg32)cpu_to_be32(val);
> +}
> +
> +static inline u64 __rpmsg64_to_cpu(bool little_endian, __rpmsg64 val)
> +{
> +	if (little_endian)
> +		return le64_to_cpu((__force __le64)val);
> +	else
> +		return be64_to_cpu((__force __be64)val);
> +}
> +
> +static inline __rpmsg64 __cpu_to_rpmsg64(bool little_endian, u64 val)
> +{
> +	if (little_endian)
> +		return (__force __rpmsg64)cpu_to_le64(val);
> +	else
> +		return (__force __rpmsg64)cpu_to_be64(val);
> +}
> +
> +#endif /* _LINUX_RPMSG_BYTEORDER_H */
> diff --git a/include/uapi/linux/rpmsg_types.h b/include/uapi/linux/rpmsg_types.h
> new file mode 100644
> index 000000000000..36e3b9404391
> --- /dev/null
> +++ b/include/uapi/linux/rpmsg_types.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_RPMSG_TYPES_H
> +#define _UAPI_LINUX_RPMSG_TYPES_H
> +
> +#include <linux/types.h>
> +
> +typedef __u16 __bitwise __rpmsg16;
> +typedef __u32 __bitwise __rpmsg32;
> +typedef __u64 __bitwise __rpmsg64;
> +
> +#endif /* _UAPI_LINUX_RPMSG_TYPES_H */
> 

Re: [PATCH v2 3/9] rpmsg: virtio: Move from virtio to rpmsg byte conversion

[Arnaud POULIQUEN]

On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Use rpmsg byte conversion functions in order for the RPMSG
> headers and generic functions to be used by external entities.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 60 +++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 9006fc7f73d0..793fe924671f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,11 +19,11 @@
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg_byteorder.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/virtio.h>
> -#include <linux/virtio_byteorder.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
>  #include <linux/wait.h>
> @@ -85,11 +85,11 @@ struct virtproc_info {
>   * Every message sent(/received) on the rpmsg bus begins with this header.
>   */
>  struct rpmsg_hdr {
> -	__virtio32 src;
> -	__virtio32 dst;
> -	__virtio32 reserved;
> -	__virtio16 len;
> -	__virtio16 flags;
> +	__rpmsg32 src;
> +	__rpmsg32 dst;
> +	__rpmsg32 reserved;
> +	__rpmsg16 len;
> +	__rpmsg16 flags;
>  	u8 data[];
>  } __packed;
>  
> @@ -107,8 +107,8 @@ struct rpmsg_hdr {
>   */
>  struct rpmsg_ns_msg {
>  	char name[RPMSG_NAME_SIZE];
> -	__virtio32 addr;
> -	__virtio32 flags;
> +	__rpmsg32 addr;
> +	__rpmsg32 flags;
>  } __packed;
>  
>  /**
> @@ -280,6 +280,14 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
>  	return NULL;
>  }
>  
> +static bool virtio_rpmsg_is_little_endian(struct rpmsg_device *rpdev)
> +{
> +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> +	struct virtproc_info *vrp = vch->vrp;
> +
> +	return virtio_is_little_endian(vrp->vdev);
> +}

Regarding this i wonder if the endianess could not be a rpmsg_device field that would be set on 
__rpmsg_create_channel?
I don't think that the endianess could change, so perhaps no need to call ops for each conversion
using interface implemented in rpmsg.h...
But perhaps I missed something?
 
> +
>  static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
>  						      rpmsg_rx_cb_t cb,
>  						      void *priv,
> @@ -336,8 +344,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
>  		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);
> +		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> +		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_CREATE);
>  
>  		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>  		if (err)
> @@ -360,8 +368,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>  		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);
> +		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> +		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_DESTROY);
>  
>  		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>  		if (err)
> @@ -372,6 +380,7 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>  }
>  
>  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
> +	.is_little_endian = virtio_rpmsg_is_little_endian,
>  	.create_ept = virtio_rpmsg_create_ept,
>  	.announce_create = virtio_rpmsg_announce_create,
>  	.announce_destroy = virtio_rpmsg_announce_destroy,
> @@ -613,10 +622,10 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>  		}
>  	}
>  
> -	msg->len = cpu_to_virtio16(vrp->vdev, len);
> +	msg->len = cpu_to_rpmsg16(rpdev, len);
>  	msg->flags = 0;
> -	msg->src = cpu_to_virtio32(vrp->vdev, src);
> -	msg->dst = cpu_to_virtio32(vrp->vdev, dst);
> +	msg->src = cpu_to_rpmsg32(rpdev, src);
> +	msg->dst = cpu_to_rpmsg32(rpdev, dst);
>  	msg->reserved = 0;
>  	memcpy(msg->data, data, len);
>  
> @@ -705,14 +714,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  {
>  	struct rpmsg_endpoint *ept;
>  	struct scatterlist sg;
> -	unsigned int msg_len = virtio16_to_cpu(vrp->vdev, msg->len);
> +	bool little_endian = rpmsg_is_little_endian();
> +	unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
>  	int err;
>  
>  	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
> -		virtio32_to_cpu(vrp->vdev, msg->src),
> -		virtio32_to_cpu(vrp->vdev, msg->dst), msg_len,
> -		virtio16_to_cpu(vrp->vdev, msg->flags),
> -		virtio32_to_cpu(vrp->vdev, msg->reserved));
> +		__rpmsg32_to_cpu(little_endian, msg->src),
> +		__rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
> +		__rpmsg16_to_cpu(little_endian, msg->flags),
> +		__rpmsg32_to_cpu(little_endian, msg->reserved));

Nitpicking: sometime rpmsgXX_to_cpu is used, sometime __rpmsgXX_to_cpu, 
Perhaps only one API should be used... But i don't see any blocking point to use both...:)

Thanks,
Arnaud

>  #if defined(CONFIG_DYNAMIC_DEBUG)
>  	dynamic_hex_dump("rpmsg_virtio RX: ", DUMP_PREFIX_NONE, 16, 1,
>  			 msg, sizeof(*msg) + msg_len, true);
> @@ -731,7 +741,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  	/* use the dst addr to fetch the callback of the appropriate user */
>  	mutex_lock(&vrp->endpoints_lock);
>  
> -	ept = idr_find(&vrp->endpoints, virtio32_to_cpu(vrp->vdev, msg->dst));
> +	ept = idr_find(&vrp->endpoints, __rpmsg32_to_cpu(little_endian, msg->dst));
>  
>  	/* let's make sure no one deallocates ept while we use it */
>  	if (ept)
> @@ -745,7 +755,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  
>  		if (ept->cb)
>  			ept->cb(ept->rpdev, msg->data, msg_len, ept->priv,
> -				virtio32_to_cpu(vrp->vdev, msg->src));
> +				__rpmsg32_to_cpu(little_endian, msg->src));
>  
>  		mutex_unlock(&ept->cb_lock);
>  
> @@ -853,13 +863,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 = virtio32_to_cpu(vrp->vdev, msg->addr);
> +	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
>  
>  	dev_info(dev, "%sing channel %s addr 0x%x\n",
> -		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> +		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
>  		 "destroy" : "creat", msg->name, chinfo.dst);
>  
> -	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> +	if (rpmsg32_to_cpu(rpdev, 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);
> 

Re: [PATCH v2 4/9] rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file

[Arnaud POULIQUEN]

Hi Mathieu,

On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
> so that they can be used by other entities.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
>  include/linux/rpmsg_ns.h         | 62 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/rpmsg.h       |  3 ++
>  3 files changed, 67 insertions(+), 56 deletions(-)
>  create mode 100644 include/linux/rpmsg_ns.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 793fe924671f..85f2acc4ed9f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,7 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/rpmsg.h>
> -#include <linux/rpmsg_byteorder.h>
> +#include <linux/rpmsg_ns.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> @@ -27,6 +27,7 @@
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
>  #include <linux/wait.h>
> +#include <uapi/linux/rpmsg.h>
>  
>  #include "rpmsg_internal.h"
>  
> @@ -70,58 +71,6 @@ struct virtproc_info {
>  	struct rpmsg_endpoint *ns_ept;
>  };
>  
> -/* The feature bitmap for virtio rpmsg */
> -#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> -
> -/**
> - * struct rpmsg_hdr - common header for all rpmsg messages
> - * @src: source address
> - * @dst: destination address
> - * @reserved: reserved for future use
> - * @len: length of payload (in bytes)
> - * @flags: message flags
> - * @data: @len bytes of message payload data
> - *
> - * Every message sent(/received) on the rpmsg bus begins with this header.
> - */
> -struct rpmsg_hdr {
> -	__rpmsg32 src;
> -	__rpmsg32 dst;
> -	__rpmsg32 reserved;
> -	__rpmsg16 len;
> -	__rpmsg16 flags;
> -	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];
> -	__rpmsg32 addr;
> -	__rpmsg32 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
>   */
> @@ -162,9 +111,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_ns.h b/include/linux/rpmsg_ns.h
> new file mode 100644
> index 000000000000..3d836b8580b2
> --- /dev/null
> +++ b/include/linux/rpmsg_ns.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_RPMSG_NS_H
> +#define _LINUX_RPMSG_NS_H
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +#include <linux/rpmsg_byteorder.h>
> +
> +/**
> + * struct rpmsg_hdr - common header for all rpmsg messages
> + * @src: source address
> + * @dst: destination address
> + * @reserved: reserved for future use
> + * @len: length of payload (in bytes)
> + * @flags: message flags
> + * @data: @len bytes of message payload data
> + *
> + * Every message sent(/received) on the rpmsg bus begins with this header.
> + */
> +struct rpmsg_hdr {
> +	__rpmsg32 src;
> +	__rpmsg32 dst;
> +	__rpmsg32 reserved;
> +	__rpmsg16 len;
> +	__rpmsg16 flags;
> +	u8 data[];
> +} __packed;

This structure is not related to the rpmsg ns service but to the rpmsg bus.
If this structure has to be exposed to rpmsg client should be in rpmsg.h, but 
Is there a need to expose it for now?
I suppose that it is for vhost...As the need will depends on the implementation, 
I would suggest leaving it internally and expose only if needed, in the
related series.

> +
> +/**
> + * 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];
> +	__rpmsg32 addr;
> +	__rpmsg32 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,
> +};
> +
> +/* Address 53 is reserved for advertising remote services */
> +#define RPMSG_NS_ADDR			(53)

What about my proposal [1] to put this in rpmsg.h, to create a list of
reserved Address

[1] https://lkml.org/lkml/2020/7/31/442

> +
> +#endif
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index e14c6dab4223..d669c04ef289 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
>  #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
>  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
>  
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> +

Same suggestion here,i would drop this from this series

Thanks,
Arnaud

>  #endif
> 

Re: [PATCH v2 8/9] rpmsg: Make rpmsg_{register|unregister}_device() public

[Arnaud POULIQUEN]

On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Make function rpmsg_register_device() and rpmsg_unregister_device()
> functions public so that they can be used by other clients.  While
> doing so get rid of two obsolete function, i.e register_rpmsg_device()
> and unregister_rpmsg_device(), to prevent confusion.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_internal.h |  4 ----
>  include/linux/rpmsg.h          | 12 ++++++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index b9b34b416b7b..70de053581bd 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -49,10 +49,6 @@ struct rpmsg_endpoint_ops {
>  			     poll_table *wait);
>  };
>  
> -int rpmsg_register_device(struct rpmsg_device *rpdev);
> -int rpmsg_unregister_device(struct device *parent,
> -			    struct rpmsg_channel_info *chinfo);
> -
>  struct device *rpmsg_find_device(struct device *parent,
>  				 struct rpmsg_channel_info *chinfo);
>  
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index eb70463a9f2e..0b3ec18ddbaa 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -188,8 +188,9 @@ static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
>  
>  #if IS_ENABLED(CONFIG_RPMSG)
>  
> -int register_rpmsg_device(struct rpmsg_device *dev);
> -void unregister_rpmsg_device(struct rpmsg_device *dev);
> +int rpmsg_register_device(struct rpmsg_device *rpdev);
> +int rpmsg_unregister_device(struct device *parent,
> +			    struct rpmsg_channel_info *chinfo);
>  int __register_rpmsg_driver(struct rpmsg_driver *drv, struct module *owner);
>  void unregister_rpmsg_driver(struct rpmsg_driver *drv);
>  void rpmsg_destroy_ept(struct rpmsg_endpoint *);
> @@ -212,15 +213,18 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>  
>  #else
>  
> -static inline int register_rpmsg_device(struct rpmsg_device *dev)
> +static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>  {
>  	return -ENXIO;
>  }
>  
> -static inline void unregister_rpmsg_device(struct rpmsg_device *dev)
> +static inline int rpmsg_unregister_device(struct device *parent,
> +					  struct rpmsg_channel_info *chinfo)
>  {
>  	/* This shouldn't be possible */
>  	WARN_ON(1);
> +
> +	return -ENXIO;
>  }
>  
>  static inline int __register_rpmsg_driver(struct rpmsg_driver *drv,
> 


Reviewed-by:  Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

Re: [PATCH v2 1/9] rpmsg: Move rpmsg_endpoint_ops to rpmsg.h

[Arnaud POULIQUEN]

On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Move structure rpmsg_endpoint_ops to header rpmsg.h so that it can
> be used by other entities.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_internal.h | 19 -------------------
>  include/linux/rpmsg.h          | 24 +++++++++++++++++++++---
>  2 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3fc83cd50e98..094cf968d2d3 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -18,25 +18,6 @@
>  #define to_rpmsg_device(d) container_of(d, struct rpmsg_device, dev)
>  #define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
>  
> -/**
> - * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> - * @create_ept:		create backend-specific endpoint, required
> - * @announce_create:	announce presence of new channel, optional
> - * @announce_destroy:	announce destruction of channel, optional
> - *
> - * Indirection table for the operations that a rpmsg backend should implement.
> - * @announce_create and @announce_destroy are optional as the backend might
> - * advertise new channels implicitly by creating the endpoints.
> - */
> -struct rpmsg_device_ops {
> -	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
> -					    rpmsg_rx_cb_t cb, void *priv,
> -					    struct rpmsg_channel_info chinfo);
> -
> -	int (*announce_create)(struct rpmsg_device *ept);
> -	int (*announce_destroy)(struct rpmsg_device *ept);
> -};
> -
>  /**
>   * struct rpmsg_endpoint_ops - indirection table for rpmsg_endpoint operations
>   * @destroy_ept:	see @rpmsg_destroy_ept(), required
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 9fe156d1c018..9fe1c54ae995 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -22,7 +22,6 @@
>  
>  struct rpmsg_device;
>  struct rpmsg_endpoint;
> -struct rpmsg_device_ops;
>  struct rpmsg_endpoint_ops;
>  
>  /**
> @@ -37,6 +36,27 @@ struct rpmsg_channel_info {
>  	u32 dst;
>  };
>  
> +typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> +
> +/**
> + * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @create_ept:		create backend-specific endpoint, required
> + * @announce_create:	announce presence of new channel, optional
> + * @announce_destroy:	announce destruction of channel, optional
> + *
> + * Indirection table for the operations that a rpmsg backend should implement.
> + * @announce_create and @announce_destroy are optional as the backend might
> + * advertise new channels implicitly by creating the endpoints.
> + */
> +struct rpmsg_device_ops {
> +	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
> +					    rpmsg_rx_cb_t cb, void *priv,
> +					    struct rpmsg_channel_info chinfo);
> +
> +	int (*announce_create)(struct rpmsg_device *ept);
> +	int (*announce_destroy)(struct rpmsg_device *ept);
> +};
> +
>  /**
>   * rpmsg_device - device that belong to the rpmsg bus
>   * @dev: the device struct
> @@ -59,8 +79,6 @@ struct rpmsg_device {
>  	const struct rpmsg_device_ops *ops;
>  };
>  
> -typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> -
>  /**
>   * struct rpmsg_endpoint - binds a local rpmsg address to its user
>   * @rpdev: rpmsg channel device
> 

Reviewed-by:  Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

Re: [PATCH v2 2/9] rpmsg: Introduce __rpmsg{16|32|64} types

[Mathieu Poirier]

Good day,

On Wed, Oct 14, 2020 at 06:31:49PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> > Introduce __rpmsg{16|32|64} types along with byte order conversion
> > functions based on an rpmsg_device operation as a foundation to
> > make RPMSG modular and transport agnostic.
> > 
> > Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  include/linux/rpmsg.h            | 51 ++++++++++++++++++++++++
> >  include/linux/rpmsg_byteorder.h  | 67 ++++++++++++++++++++++++++++++++
> >  include/uapi/linux/rpmsg_types.h | 11 ++++++
> >  3 files changed, 129 insertions(+)
> >  create mode 100644 include/linux/rpmsg_byteorder.h
> >  create mode 100644 include/uapi/linux/rpmsg_types.h
> > 
> > diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> > index 9fe1c54ae995..165e4c6d4cd3 100644
> > --- a/include/linux/rpmsg.h
> > +++ b/include/linux/rpmsg.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/kref.h>
> >  #include <linux/mutex.h>
> >  #include <linux/poll.h>
> > +#include <linux/rpmsg_byteorder.h>
> >  
> >  #define RPMSG_ADDR_ANY		0xFFFFFFFF
> >  
> > @@ -40,6 +41,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> >  
> >  /**
> >   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> > + * @is_little_endian:	returns true if using little endian byte ordering
> >   * @create_ept:		create backend-specific endpoint, required
> >   * @announce_create:	announce presence of new channel, optional
> >   * @announce_destroy:	announce destruction of channel, optional
> > @@ -49,6 +51,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> >   * advertise new channels implicitly by creating the endpoints.
> >   */
> >  struct rpmsg_device_ops {
> > +	bool (*is_little_endian)(struct rpmsg_device *rpdev);
> >  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
> >  					    rpmsg_rx_cb_t cb, void *priv,
> >  					    struct rpmsg_channel_info chinfo);
> > @@ -129,6 +132,54 @@ struct rpmsg_driver {
> >  	int (*callback)(struct rpmsg_device *, void *, int, void *, u32);
> >  };
> >  
> > +static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __rpmsg16_to_cpu(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __rpmsg16_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> > +static inline __rpmsg16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __cpu_to_rpmsg16(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __cpu_to_rpmsg16(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> > +static inline u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, __rpmsg32 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __rpmsg32_to_cpu(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __rpmsg32_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> > +static inline __rpmsg32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __cpu_to_rpmsg32(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __cpu_to_rpmsg32(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> > +static inline u64 rpmsg64_to_cpu(struct rpmsg_device *rpdev, __rpmsg64 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __rpmsg64_to_cpu(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __rpmsg64_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> > +static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
> > +{
> > +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
> > +		return __cpu_to_rpmsg64(rpmsg_is_little_endian(), val);
> > +	else
> > +		return __cpu_to_rpmsg64(rpdev->ops->is_little_endian(rpdev), val);
> > +}
> > +
> >  #if IS_ENABLED(CONFIG_RPMSG)
> >  
> >  int register_rpmsg_device(struct rpmsg_device *dev);
> > diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg_byteorder.h
> > new file mode 100644
> > index 000000000000..c0f565dbad6d
> > --- /dev/null
> > +++ b/include/linux/rpmsg_byteorder.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Follows implementation found in linux/virtio_byteorder.h
> > + */
> > +#ifndef _LINUX_RPMSG_BYTEORDER_H
> > +#define _LINUX_RPMSG_BYTEORDER_H
> > +#include <linux/types.h>
> > +#include <uapi/linux/rpmsg_types.h>
> > +
> > +static inline bool rpmsg_is_little_endian(void)
> > +{
> > +#ifdef __LITTLE_ENDIAN
> > +	return true;
> > +#else
> > +	return false;
> > +#endif
> > +}
> 
> A suggestion:
> 
> static inline bool rpmsg_is_little_endian(void)
> #if defined(__BYTE_ORDER)
> #  if __BYTE_ORDER == __BIG_ENDIAN
> 	return true;
> #  elif __BYTE_ORDER == __LITTLE_ENDIAN
> 	return false;
> #  else
> #    warning "unknown endianess, set to little by default"
> 	return true;
> #  endif
> #endif
> }

I found that __BYTE_ORDER is not defined stm32mp157 - have you used it before? 

> 
> Otherwise
> 
> Reviewed-by:  Arnaud Pouliquen <arnaud.pouliquen@st.com>
> 
> Thanks,
> Arnaud
> 
> > +
> > +static inline u16 __rpmsg16_to_cpu(bool little_endian, __rpmsg16 val)
> > +{
> > +	if (little_endian)
> > +		return le16_to_cpu((__force __le16)val);
> > +	else
> > +		return be16_to_cpu((__force __be16)val);
> > +}
> > +
> > +static inline __rpmsg16 __cpu_to_rpmsg16(bool little_endian, u16 val)
> > +{
> > +	if (little_endian)
> > +		return (__force __rpmsg16)cpu_to_le16(val);
> > +	else
> > +		return (__force __rpmsg16)cpu_to_be16(val);
> > +}
> > +
> > +static inline u32 __rpmsg32_to_cpu(bool little_endian, __rpmsg32 val)
> > +{
> > +	if (little_endian)
> > +		return le32_to_cpu((__force __le32)val);
> > +	else
> > +		return be32_to_cpu((__force __be32)val);
> > +}
> > +
> > +static inline __rpmsg32 __cpu_to_rpmsg32(bool little_endian, u32 val)
> > +{
> > +	if (little_endian)
> > +		return (__force __rpmsg32)cpu_to_le32(val);
> > +	else
> > +		return (__force __rpmsg32)cpu_to_be32(val);
> > +}
> > +
> > +static inline u64 __rpmsg64_to_cpu(bool little_endian, __rpmsg64 val)
> > +{
> > +	if (little_endian)
> > +		return le64_to_cpu((__force __le64)val);
> > +	else
> > +		return be64_to_cpu((__force __be64)val);
> > +}
> > +
> > +static inline __rpmsg64 __cpu_to_rpmsg64(bool little_endian, u64 val)
> > +{
> > +	if (little_endian)
> > +		return (__force __rpmsg64)cpu_to_le64(val);
> > +	else
> > +		return (__force __rpmsg64)cpu_to_be64(val);
> > +}
> > +
> > +#endif /* _LINUX_RPMSG_BYTEORDER_H */
> > diff --git a/include/uapi/linux/rpmsg_types.h b/include/uapi/linux/rpmsg_types.h
> > new file mode 100644
> > index 000000000000..36e3b9404391
> > --- /dev/null
> > +++ b/include/uapi/linux/rpmsg_types.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_LINUX_RPMSG_TYPES_H
> > +#define _UAPI_LINUX_RPMSG_TYPES_H
> > +
> > +#include <linux/types.h>
> > +
> > +typedef __u16 __bitwise __rpmsg16;
> > +typedef __u32 __bitwise __rpmsg32;
> > +typedef __u64 __bitwise __rpmsg64;
> > +
> > +#endif /* _UAPI_LINUX_RPMSG_TYPES_H */
> > 

Re: [PATCH v2 3/9] rpmsg: virtio: Move from virtio to rpmsg byte conversion

[Mathieu Poirier]

On Wed, Oct 14, 2020 at 07:04:32PM +0200, Arnaud POULIQUEN wrote:
> 
> 
> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> > Use rpmsg byte conversion functions in order for the RPMSG
> > headers and generic functions to be used by external entities.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 60 +++++++++++++++++++-------------
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 9006fc7f73d0..793fe924671f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,11 +19,11 @@
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >  #include <linux/rpmsg.h>
> > +#include <linux/rpmsg_byteorder.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> >  #include <linux/virtio.h>
> > -#include <linux/virtio_byteorder.h>
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/wait.h>
> > @@ -85,11 +85,11 @@ struct virtproc_info {
> >   * Every message sent(/received) on the rpmsg bus begins with this header.
> >   */
> >  struct rpmsg_hdr {
> > -	__virtio32 src;
> > -	__virtio32 dst;
> > -	__virtio32 reserved;
> > -	__virtio16 len;
> > -	__virtio16 flags;
> > +	__rpmsg32 src;
> > +	__rpmsg32 dst;
> > +	__rpmsg32 reserved;
> > +	__rpmsg16 len;
> > +	__rpmsg16 flags;
> >  	u8 data[];
> >  } __packed;
> >  
> > @@ -107,8 +107,8 @@ struct rpmsg_hdr {
> >   */
> >  struct rpmsg_ns_msg {
> >  	char name[RPMSG_NAME_SIZE];
> > -	__virtio32 addr;
> > -	__virtio32 flags;
> > +	__rpmsg32 addr;
> > +	__rpmsg32 flags;
> >  } __packed;
> >  
> >  /**
> > @@ -280,6 +280,14 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> >  	return NULL;
> >  }
> >  
> > +static bool virtio_rpmsg_is_little_endian(struct rpmsg_device *rpdev)
> > +{
> > +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> > +	struct virtproc_info *vrp = vch->vrp;
> > +
> > +	return virtio_is_little_endian(vrp->vdev);
> > +}
> 
> Regarding this i wonder if the endianess could not be a rpmsg_device field that would be set on 
> __rpmsg_create_channel?
> I don't think that the endianess could change, so perhaps no need to call ops for each conversion
> using interface implemented in rpmsg.h...

That's a valid point - I certainly don't expect the endianness to change
dynamically.

> But perhaps I missed something?
>  
> > +
> >  static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
> >  						      rpmsg_rx_cb_t cb,
> >  						      void *priv,
> > @@ -336,8 +344,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
> >  		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);
> > +		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> > +		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_CREATE);
> >  
> >  		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> >  		if (err)
> > @@ -360,8 +368,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> >  		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);
> > +		nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> > +		nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_DESTROY);
> >  
> >  		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> >  		if (err)
> > @@ -372,6 +380,7 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> >  }
> >  
> >  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
> > +	.is_little_endian = virtio_rpmsg_is_little_endian,
> >  	.create_ept = virtio_rpmsg_create_ept,
> >  	.announce_create = virtio_rpmsg_announce_create,
> >  	.announce_destroy = virtio_rpmsg_announce_destroy,
> > @@ -613,10 +622,10 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >  		}
> >  	}
> >  
> > -	msg->len = cpu_to_virtio16(vrp->vdev, len);
> > +	msg->len = cpu_to_rpmsg16(rpdev, len);
> >  	msg->flags = 0;
> > -	msg->src = cpu_to_virtio32(vrp->vdev, src);
> > -	msg->dst = cpu_to_virtio32(vrp->vdev, dst);
> > +	msg->src = cpu_to_rpmsg32(rpdev, src);
> > +	msg->dst = cpu_to_rpmsg32(rpdev, dst);
> >  	msg->reserved = 0;
> >  	memcpy(msg->data, data, len);
> >  
> > @@ -705,14 +714,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >  {
> >  	struct rpmsg_endpoint *ept;
> >  	struct scatterlist sg;
> > -	unsigned int msg_len = virtio16_to_cpu(vrp->vdev, msg->len);
> > +	bool little_endian = rpmsg_is_little_endian();
> > +	unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
> >  	int err;
> >  
> >  	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
> > -		virtio32_to_cpu(vrp->vdev, msg->src),
> > -		virtio32_to_cpu(vrp->vdev, msg->dst), msg_len,
> > -		virtio16_to_cpu(vrp->vdev, msg->flags),
> > -		virtio32_to_cpu(vrp->vdev, msg->reserved));
> > +		__rpmsg32_to_cpu(little_endian, msg->src),
> > +		__rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
> > +		__rpmsg16_to_cpu(little_endian, msg->flags),
> > +		__rpmsg32_to_cpu(little_endian, msg->reserved));
> 
> Nitpicking: sometime rpmsgXX_to_cpu is used, sometime __rpmsgXX_to_cpu, 
> Perhaps only one API should be used... But i don't see any blocking point to use both...:)

Here we have to use __rpmsgXX_to_CPU() because the rpmsg_device is not
available.  It could be fixed but would require a serious amount of refactoring
for little value.  Moreover since this rpmsg_recv_single() is specific to the
virtIO implementation the end result is the same.

> 
> Thanks,
> Arnaud
> 
> >  #if defined(CONFIG_DYNAMIC_DEBUG)
> >  	dynamic_hex_dump("rpmsg_virtio RX: ", DUMP_PREFIX_NONE, 16, 1,
> >  			 msg, sizeof(*msg) + msg_len, true);
> > @@ -731,7 +741,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >  	/* use the dst addr to fetch the callback of the appropriate user */
> >  	mutex_lock(&vrp->endpoints_lock);
> >  
> > -	ept = idr_find(&vrp->endpoints, virtio32_to_cpu(vrp->vdev, msg->dst));
> > +	ept = idr_find(&vrp->endpoints, __rpmsg32_to_cpu(little_endian, msg->dst));
> >  
> >  	/* let's make sure no one deallocates ept while we use it */
> >  	if (ept)
> > @@ -745,7 +755,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >  
> >  		if (ept->cb)
> >  			ept->cb(ept->rpdev, msg->data, msg_len, ept->priv,
> > -				virtio32_to_cpu(vrp->vdev, msg->src));
> > +				__rpmsg32_to_cpu(little_endian, msg->src));
> >  
> >  		mutex_unlock(&ept->cb_lock);
> >  
> > @@ -853,13 +863,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 = virtio32_to_cpu(vrp->vdev, msg->addr);
> > +	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> >  
> >  	dev_info(dev, "%sing channel %s addr 0x%x\n",
> > -		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> > +		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> >  		 "destroy" : "creat", msg->name, chinfo.dst);
> >  
> > -	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> > +	if (rpmsg32_to_cpu(rpdev, 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);
> > 

Re: [PATCH v2 4/9] rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file

[Mathieu Poirier]

On Thu, Oct 15, 2020 at 10:33:25AM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> > Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
> > so that they can be used by other entities.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
> >  include/linux/rpmsg_ns.h         | 62 ++++++++++++++++++++++++++++++++
> >  include/uapi/linux/rpmsg.h       |  3 ++
> >  3 files changed, 67 insertions(+), 56 deletions(-)
> >  create mode 100644 include/linux/rpmsg_ns.h
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 793fe924671f..85f2acc4ed9f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,7 +19,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >  #include <linux/rpmsg.h>
> > -#include <linux/rpmsg_byteorder.h>
> > +#include <linux/rpmsg_ns.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> > @@ -27,6 +27,7 @@
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/wait.h>
> > +#include <uapi/linux/rpmsg.h>
> >  
> >  #include "rpmsg_internal.h"
> >  
> > @@ -70,58 +71,6 @@ struct virtproc_info {
> >  	struct rpmsg_endpoint *ns_ept;
> >  };
> >  
> > -/* The feature bitmap for virtio rpmsg */
> > -#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> > -
> > -/**
> > - * struct rpmsg_hdr - common header for all rpmsg messages
> > - * @src: source address
> > - * @dst: destination address
> > - * @reserved: reserved for future use
> > - * @len: length of payload (in bytes)
> > - * @flags: message flags
> > - * @data: @len bytes of message payload data
> > - *
> > - * Every message sent(/received) on the rpmsg bus begins with this header.
> > - */
> > -struct rpmsg_hdr {
> > -	__rpmsg32 src;
> > -	__rpmsg32 dst;
> > -	__rpmsg32 reserved;
> > -	__rpmsg16 len;
> > -	__rpmsg16 flags;
> > -	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];
> > -	__rpmsg32 addr;
> > -	__rpmsg32 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
> >   */
> > @@ -162,9 +111,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_ns.h b/include/linux/rpmsg_ns.h
> > new file mode 100644
> > index 000000000000..3d836b8580b2
> > --- /dev/null
> > +++ b/include/linux/rpmsg_ns.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_RPMSG_NS_H
> > +#define _LINUX_RPMSG_NS_H
> > +
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/types.h>
> > +#include <linux/rpmsg_byteorder.h>
> > +
> > +/**
> > + * struct rpmsg_hdr - common header for all rpmsg messages
> > + * @src: source address
> > + * @dst: destination address
> > + * @reserved: reserved for future use
> > + * @len: length of payload (in bytes)
> > + * @flags: message flags
> > + * @data: @len bytes of message payload data
> > + *
> > + * Every message sent(/received) on the rpmsg bus begins with this header.
> > + */
> > +struct rpmsg_hdr {
> > +	__rpmsg32 src;
> > +	__rpmsg32 dst;
> > +	__rpmsg32 reserved;
> > +	__rpmsg16 len;
> > +	__rpmsg16 flags;
> > +	u8 data[];
> > +} __packed;
> 
> This structure is not related to the rpmsg ns service but to the rpmsg bus.
> If this structure has to be exposed to rpmsg client should be in rpmsg.h, but 
> Is there a need to expose it for now?
> I suppose that it is for vhost...As the need will depends on the implementation, 
> I would suggest leaving it internally and expose only if needed, in the
> related series.
>

I also thought about moving rpmsg_hdr to rpmsg.h but decided against because in
most cases using the name space service usually means that a message header will
be required.  I also thought it would be easier to use, i.e include one header
rather than two.  That too is a little thin because anyone using a name service
will also need to get access to rpmsg_device, which is in rpmsg.h.

I'm definitely not strongly opinionated on where it should go, or I can leave it
in virtio_rpmsg_bus.c too...  
 
> > +
> > +/**
> > + * 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];
> > +	__rpmsg32 addr;
> > +	__rpmsg32 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,
> > +};
> > +
> > +/* Address 53 is reserved for advertising remote services */
> > +#define RPMSG_NS_ADDR			(53)
> 
> What about my proposal [1] to put this in rpmsg.h, to create a list of
> reserved Address

That too is a grey area... Moving RPMSG_NS_ADDR to rpmsg.h means we have a name
service #define in rpmsg.h.  I think that one should stay in rpmsg_ns.h. 

> 
> [1] https://lkml.org/lkml/2020/7/31/442
> 
> > +
> > +#endif
> > diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> > index e14c6dab4223..d669c04ef289 100644
> > --- a/include/uapi/linux/rpmsg.h
> > +++ b/include/uapi/linux/rpmsg.h
> > @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
> >  #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
> >  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
> >  
> > +/* The feature bitmap for virtio rpmsg */
> > +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> > +
> 
> Same suggestion here,i would drop this from this series

Will do.

Thanks for the feedback,
Mathieu

> 
> Thanks,
> Arnaud
> 
> >  #endif
> > 

Re: [PATCH v2 2/9] rpmsg: Introduce __rpmsg{16|32|64} types

[Arnaud POULIQUEN]

On 10/15/20 9:32 PM, Mathieu Poirier wrote:
> Good day,
> 
> On Wed, Oct 14, 2020 at 06:31:49PM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
>>> Introduce __rpmsg{16|32|64} types along with byte order conversion
>>> functions based on an rpmsg_device operation as a foundation to
>>> make RPMSG modular and transport agnostic.
>>>
>>> Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>  include/linux/rpmsg.h            | 51 ++++++++++++++++++++++++
>>>  include/linux/rpmsg_byteorder.h  | 67 ++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/rpmsg_types.h | 11 ++++++
>>>  3 files changed, 129 insertions(+)
>>>  create mode 100644 include/linux/rpmsg_byteorder.h
>>>  create mode 100644 include/uapi/linux/rpmsg_types.h
>>>
>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>> index 9fe1c54ae995..165e4c6d4cd3 100644
>>> --- a/include/linux/rpmsg.h
>>> +++ b/include/linux/rpmsg.h
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/kref.h>
>>>  #include <linux/mutex.h>
>>>  #include <linux/poll.h>
>>> +#include <linux/rpmsg_byteorder.h>
>>>  
>>>  #define RPMSG_ADDR_ANY		0xFFFFFFFF
>>>  
>>> @@ -40,6 +41,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>>>  
>>>  /**
>>>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>>> + * @is_little_endian:	returns true if using little endian byte ordering
>>>   * @create_ept:		create backend-specific endpoint, required
>>>   * @announce_create:	announce presence of new channel, optional
>>>   * @announce_destroy:	announce destruction of channel, optional
>>> @@ -49,6 +51,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>>>   * advertise new channels implicitly by creating the endpoints.
>>>   */
>>>  struct rpmsg_device_ops {
>>> +	bool (*is_little_endian)(struct rpmsg_device *rpdev);
>>>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>>>  					    rpmsg_rx_cb_t cb, void *priv,
>>>  					    struct rpmsg_channel_info chinfo);
>>> @@ -129,6 +132,54 @@ struct rpmsg_driver {
>>>  	int (*callback)(struct rpmsg_device *, void *, int, void *, u32);
>>>  };
>>>  
>>> +static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __rpmsg16_to_cpu(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __rpmsg16_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>> +static inline __rpmsg16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __cpu_to_rpmsg16(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __cpu_to_rpmsg16(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>> +static inline u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, __rpmsg32 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __rpmsg32_to_cpu(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __rpmsg32_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>> +static inline __rpmsg32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __cpu_to_rpmsg32(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __cpu_to_rpmsg32(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>> +static inline u64 rpmsg64_to_cpu(struct rpmsg_device *rpdev, __rpmsg64 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __rpmsg64_to_cpu(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __rpmsg64_to_cpu(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>> +static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
>>> +{
>>> +	if (!rpdev || !rpdev->ops || !rpdev->ops->is_little_endian)
>>> +		return __cpu_to_rpmsg64(rpmsg_is_little_endian(), val);
>>> +	else
>>> +		return __cpu_to_rpmsg64(rpdev->ops->is_little_endian(rpdev), val);
>>> +}
>>> +
>>>  #if IS_ENABLED(CONFIG_RPMSG)
>>>  
>>>  int register_rpmsg_device(struct rpmsg_device *dev);
>>> diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg_byteorder.h
>>> new file mode 100644
>>> index 000000000000..c0f565dbad6d
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg_byteorder.h
>>> @@ -0,0 +1,67 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Follows implementation found in linux/virtio_byteorder.h
>>> + */
>>> +#ifndef _LINUX_RPMSG_BYTEORDER_H
>>> +#define _LINUX_RPMSG_BYTEORDER_H
>>> +#include <linux/types.h>
>>> +#include <uapi/linux/rpmsg_types.h>
>>> +
>>> +static inline bool rpmsg_is_little_endian(void)
>>> +{
>>> +#ifdef __LITTLE_ENDIAN
>>> +	return true;
>>> +#else
>>> +	return false;
>>> +#endif
>>> +}
>>
>> A suggestion:
>>
>> static inline bool rpmsg_is_little_endian(void)
>> #if defined(__BYTE_ORDER)
>> #  if __BYTE_ORDER == __BIG_ENDIAN
>> 	return true;
>> #  elif __BYTE_ORDER == __LITTLE_ENDIAN
>> 	return false;
>> #  else
>> #    warning "unknown endianess, set to little by default"
>> 	return true;
>> #  endif
>> #endif
>> }
> 
> I found that __BYTE_ORDER is not defined stm32mp157 - have you used it before? 

Good point! it seems not defined for some architectures...something strange is that
it is used in some generic code (e.g.[1]) but it works considering little by default... 

I saw this implementation in Linux kernel code, for instance [2].
that's said,  __LITTLE_ENDIAN or __BIG_ENDIAN seems always defined regarding [3].
So just forget this suggestion...

[1]https://elixir.bootlin.com/linux/latest/source/include/math-emu/double.h#L59
[2]https://elixir.bootlin.com/linux/latest/source/tools/lib/bpf/btf.c#L506
[3]https://elixir.bootlin.com/linux/latest/source/include/linux/kconfig.h#L12

> 
>>
>> Otherwise
>>
>> Reviewed-by:  Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>
>> Thanks,
>> Arnaud
>>
>>> +
>>> +static inline u16 __rpmsg16_to_cpu(bool little_endian, __rpmsg16 val)
>>> +{
>>> +	if (little_endian)
>>> +		return le16_to_cpu((__force __le16)val);
>>> +	else
>>> +		return be16_to_cpu((__force __be16)val);
>>> +}
>>> +
>>> +static inline __rpmsg16 __cpu_to_rpmsg16(bool little_endian, u16 val)
>>> +{
>>> +	if (little_endian)
>>> +		return (__force __rpmsg16)cpu_to_le16(val);
>>> +	else
>>> +		return (__force __rpmsg16)cpu_to_be16(val);
>>> +}
>>> +
>>> +static inline u32 __rpmsg32_to_cpu(bool little_endian, __rpmsg32 val)
>>> +{
>>> +	if (little_endian)
>>> +		return le32_to_cpu((__force __le32)val);
>>> +	else
>>> +		return be32_to_cpu((__force __be32)val);
>>> +}
>>> +
>>> +static inline __rpmsg32 __cpu_to_rpmsg32(bool little_endian, u32 val)
>>> +{
>>> +	if (little_endian)
>>> +		return (__force __rpmsg32)cpu_to_le32(val);
>>> +	else
>>> +		return (__force __rpmsg32)cpu_to_be32(val);
>>> +}
>>> +
>>> +static inline u64 __rpmsg64_to_cpu(bool little_endian, __rpmsg64 val)
>>> +{
>>> +	if (little_endian)
>>> +		return le64_to_cpu((__force __le64)val);
>>> +	else
>>> +		return be64_to_cpu((__force __be64)val);
>>> +}
>>> +
>>> +static inline __rpmsg64 __cpu_to_rpmsg64(bool little_endian, u64 val)
>>> +{
>>> +	if (little_endian)
>>> +		return (__force __rpmsg64)cpu_to_le64(val);
>>> +	else
>>> +		return (__force __rpmsg64)cpu_to_be64(val);
>>> +}
>>> +
>>> +#endif /* _LINUX_RPMSG_BYTEORDER_H */
>>> diff --git a/include/uapi/linux/rpmsg_types.h b/include/uapi/linux/rpmsg_types.h
>>> new file mode 100644
>>> index 000000000000..36e3b9404391
>>> --- /dev/null
>>> +++ b/include/uapi/linux/rpmsg_types.h
>>> @@ -0,0 +1,11 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +#ifndef _UAPI_LINUX_RPMSG_TYPES_H
>>> +#define _UAPI_LINUX_RPMSG_TYPES_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +typedef __u16 __bitwise __rpmsg16;
>>> +typedef __u32 __bitwise __rpmsg32;
>>> +typedef __u64 __bitwise __rpmsg64;
>>> +
>>> +#endif /* _UAPI_LINUX_RPMSG_TYPES_H */
>>>

Re: [PATCH v2 4/9] rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file

[Arnaud POULIQUEN]

On 10/15/20 10:19 PM, Mathieu Poirier wrote:
> On Thu, Oct 15, 2020 at 10:33:25AM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
>>> Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
>>> so that they can be used by other entities.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
>>>  include/linux/rpmsg_ns.h         | 62 ++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/rpmsg.h       |  3 ++
>>>  3 files changed, 67 insertions(+), 56 deletions(-)
>>>  create mode 100644 include/linux/rpmsg_ns.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 793fe924671f..85f2acc4ed9f 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -19,7 +19,7 @@
>>>  #include <linux/mutex.h>
>>>  #include <linux/of_device.h>
>>>  #include <linux/rpmsg.h>
>>> -#include <linux/rpmsg_byteorder.h>
>>> +#include <linux/rpmsg_ns.h>
>>>  #include <linux/scatterlist.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/sched.h>
>>> @@ -27,6 +27,7 @@
>>>  #include <linux/virtio_ids.h>
>>>  #include <linux/virtio_config.h>
>>>  #include <linux/wait.h>
>>> +#include <uapi/linux/rpmsg.h>
>>>  
>>>  #include "rpmsg_internal.h"
>>>  
>>> @@ -70,58 +71,6 @@ struct virtproc_info {
>>>  	struct rpmsg_endpoint *ns_ept;
>>>  };
>>>  
>>> -/* The feature bitmap for virtio rpmsg */
>>> -#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
>>> -
>>> -/**
>>> - * struct rpmsg_hdr - common header for all rpmsg messages
>>> - * @src: source address
>>> - * @dst: destination address
>>> - * @reserved: reserved for future use
>>> - * @len: length of payload (in bytes)
>>> - * @flags: message flags
>>> - * @data: @len bytes of message payload data
>>> - *
>>> - * Every message sent(/received) on the rpmsg bus begins with this header.
>>> - */
>>> -struct rpmsg_hdr {
>>> -	__rpmsg32 src;
>>> -	__rpmsg32 dst;
>>> -	__rpmsg32 reserved;
>>> -	__rpmsg16 len;
>>> -	__rpmsg16 flags;
>>> -	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];
>>> -	__rpmsg32 addr;
>>> -	__rpmsg32 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
>>>   */
>>> @@ -162,9 +111,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_ns.h b/include/linux/rpmsg_ns.h
>>> new file mode 100644
>>> index 000000000000..3d836b8580b2
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg_ns.h
>>> @@ -0,0 +1,62 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +#ifndef _LINUX_RPMSG_NS_H
>>> +#define _LINUX_RPMSG_NS_H
>>> +
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/types.h>
>>> +#include <linux/rpmsg_byteorder.h>
>>> +
>>> +/**
>>> + * struct rpmsg_hdr - common header for all rpmsg messages
>>> + * @src: source address
>>> + * @dst: destination address
>>> + * @reserved: reserved for future use
>>> + * @len: length of payload (in bytes)
>>> + * @flags: message flags
>>> + * @data: @len bytes of message payload data
>>> + *
>>> + * Every message sent(/received) on the rpmsg bus begins with this header.
>>> + */
>>> +struct rpmsg_hdr {
>>> +	__rpmsg32 src;
>>> +	__rpmsg32 dst;
>>> +	__rpmsg32 reserved;
>>> +	__rpmsg16 len;
>>> +	__rpmsg16 flags;
>>> +	u8 data[];
>>> +} __packed;
>>
>> This structure is not related to the rpmsg ns service but to the rpmsg bus.
>> If this structure has to be exposed to rpmsg client should be in rpmsg.h, but 
>> Is there a need to expose it for now?
>> I suppose that it is for vhost...As the need will depends on the implementation, 
>> I would suggest leaving it internally and expose only if needed, in the
>> related series.
>>
> 
> I also thought about moving rpmsg_hdr to rpmsg.h but decided against because in
> most cases using the name space service usually means that a message header will
> be required.  I also thought it would be easier to use, i.e include one header
> rather than two.  That too is a little thin because anyone using a name service
> will also need to get access to rpmsg_device, which is in rpmsg.h.
> 
> I'm definitely not strongly opinionated on where it should go, or I can leave it
> in virtio_rpmsg_bus.c too... 

The rpmsg_ns service does not use this structure. So seems to me not at the good place

The virtio_rpmsg.c does, and a vhost_rpmsg.c should do it as well.
I would be in favor of moving it to rpmsg_internal.h to expose it to
the rpmg_buses (e.g. rpmsg_vhost) as a generic message header.
Leaving it in virtio_rpmsg_bus.c also seems also a good alternative.

>  
>>> +
>>> +/**
>>> + * 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];
>>> +	__rpmsg32 addr;
>>> +	__rpmsg32 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,
>>> +};
>>> +
>>> +/* Address 53 is reserved for advertising remote services */
>>> +#define RPMSG_NS_ADDR			(53)
>>
>> What about my proposal [1] to put this in rpmsg.h, to create a list of
>> reserved Address
> 
> That too is a grey area... Moving RPMSG_NS_ADDR to rpmsg.h means we have a name
> service #define in rpmsg.h.  I think that one should stay in rpmsg_ns.h. 

That seems reasonable, very simple to change this in future if needed.

Regards
Arnaud

> 
>>
>> [1] https://lkml.org/lkml/2020/7/31/442
>>
>>> +
>>> +#endif
>>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>>> index e14c6dab4223..d669c04ef289 100644
>>> --- a/include/uapi/linux/rpmsg.h
>>> +++ b/include/uapi/linux/rpmsg.h
>>> @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
>>>  #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
>>>  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
>>>  
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
>>> +
>>
>> Same suggestion here,i would drop this from this series
> 
> Will do.
> 
> Thanks for the feedback,
> Mathieu
> 
>>
>> Thanks,
>> Arnaud
>>
>>>  #endif
>>>