[PATCH 00/10] rpmsg: Make RPMSG name service modular

[PATCH 00/10] rpmsg: Make RPMSG name service modular

[Mathieu Poirier]

Hi all,

After looking at Guennadi[1] and Arnaud's patchsets[2] it became
clear that we need to go back to a generic rpmsg_ns_msg structure
if we wanted to make progress.  To do that some of the work from
Arnaud had to be modified in a way that common name service
functionality was transport agnostic.

This patchset is based on Arnaud's work but also include a patch
from Guennadi and some input from me.  It should serve as a
foundation for the next revision of [1].

Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
did not test the modularisation.   

Comments and feedback would be greatly appreciated.

Thanks,
Mathieu 

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

Arnaud Pouliquen (5):
  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
  rpmsg: virtio: use rpmsg ns device for the ns announcement

Guennadi Liakhovetski (1):
  rpmsg: Move common structures and defines to headers

Mathieu Poirier (4):
  rpmsg: virtio: Move virtio RPMSG structures to private header
  rpmsg: core: Add RPMSG byte conversion operations
  rpmsg: virtio: Make endianness conversion virtIO specific
  rpmsg: ns: Make Name service module transport agnostic

 drivers/rpmsg/Kconfig            |   9 +
 drivers/rpmsg/Makefile           |   1 +
 drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
 drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
 drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
 include/linux/rpmsg_ns.h         |  83 +++++++++
 include/uapi/linux/rpmsg.h       |   3 +
 8 files changed, 487 insertions(+), 199 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ns.c
 create mode 100644 include/linux/rpmsg_ns.h

-- 
2.25.1

[PATCH 01/10] 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>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 7d7ed4e5cce7..e8d55c8b9cbf 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -395,8 +395,9 @@ static void virtio_rpmsg_release_device(struct device *dev)
  * this function will be used to create both static and dynamic
  * channels.
  */
-static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
-						 struct rpmsg_channel_info *chinfo)
+static struct rpmsg_device *
+__rpmsg_create_channel(struct virtproc_info *vrp,
+		       struct rpmsg_channel_info *chinfo)
 {
 	struct virtio_rpmsg_channel *vch;
 	struct rpmsg_device *rpdev;
@@ -869,7 +870,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 02/10] 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>
---
 drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
 2 files changed, 57 insertions(+)

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

[PATCH 03/10] 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>
[Moved function declaration above struct virtio_rpmsg_ops]
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e8d55c8b9cbf..e87cf0b79542 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -181,6 +181,9 @@ 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,
@@ -285,6 +288,25 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
 	return NULL;
 }
 
+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,
@@ -377,6 +399,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 }
 
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
+	.create_channel = virtio_rpmsg_create_channel,
+	.release_channel = virtio_rpmsg_release_channel,
 	.create_ept = virtio_rpmsg_create_ept,
 	.announce_create = virtio_rpmsg_announce_create,
 	.announce_destroy = virtio_rpmsg_announce_destroy,
-- 
2.25.1

[PATCH 04/10] rpmsg: Move common structures and defines to headers

[Mathieu Poirier]

From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

virtio_rpmsg_bus.c keeps RPMsg protocol structure declarations and
common defines like the ones, needed for name-space announcements,
internal. Move them to common headers instead.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
[Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
 include/linux/rpmsg_ns.h         | 83 ++++++++++++++++++++++++++++++++
 include/uapi/linux/rpmsg.h       |  3 ++
 3 files changed, 88 insertions(+), 76 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 e87cf0b79542..eaf3b2c012c8 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/rpmsg.h>
+#include <linux/rpmsg_ns.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -27,6 +28,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 +72,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 {
-	__virtio32 src;
-	__virtio32 dst;
-	__virtio32 reserved;
-	__virtio16 len;
-	__virtio16 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];
-	__virtio32 addr;
-	__virtio32 flags;
-} __packed;
-
-/**
- * enum rpmsg_ns_flags - dynamic name service announcement flags
- *
- * @RPMSG_NS_CREATE: a new remote service was just created
- * @RPMSG_NS_DESTROY: a known remote service was just destroyed
- */
-enum rpmsg_ns_flags {
-	RPMSG_NS_CREATE		= 0,
-	RPMSG_NS_DESTROY	= 1,
-};
-
 /**
  * struct virtio_rpmsg_channel - rpmsg channel descriptor
  * @rpdev: the rpmsg channel device
@@ -139,27 +89,6 @@ struct virtio_rpmsg_channel {
 #define to_virtio_rpmsg_channel(_rpdev) \
 	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
 
-/*
- * We're allocating buffers of 512 bytes each for communications. The
- * number of buffers will be computed from the number of buffers supported
- * by the vring, upto a maximum of 512 buffers (256 in each direction).
- *
- * Each buffer will have 16 bytes for the msg header and 496 bytes for
- * the payload.
- *
- * This will utilize a maximum total space of 256KB for the buffers.
- *
- * We might also want to add support for user-provided buffers in time.
- * This will allow bigger buffer size flexibility, and can also be used
- * to achieve zero-copy messaging.
- *
- * Note that these numbers are purely a decision of this driver - we
- * can change this without changing anything in the firmware of the remote
- * processor.
- */
-#define MAX_RPMSG_NUM_BUFS	(512)
-#define MAX_RPMSG_BUF_SIZE	(512)
-
 /*
  * Local addresses are dynamically allocated on-demand.
  * We do not dynamically assign addresses from the low 1024 range,
@@ -167,9 +96,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..aabc6c3c0d6d
--- /dev/null
+++ b/include/linux/rpmsg_ns.h
@@ -0,0 +1,83 @@
+/* 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/virtio_types.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 {
+	__virtio32 src;
+	__virtio32 dst;
+	__virtio32 reserved;
+	__virtio16 len;
+	__virtio16 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];
+	__virtio32 addr;
+	__virtio32 flags;
+} __packed;
+
+/**
+ * enum rpmsg_ns_flags - dynamic name service announcement flags
+ *
+ * @RPMSG_NS_CREATE: a new remote service was just created
+ * @RPMSG_NS_DESTROY: a known remote service was just destroyed
+ */
+enum rpmsg_ns_flags {
+	RPMSG_NS_CREATE		= 0,
+	RPMSG_NS_DESTROY	= 1,
+};
+
+/*
+ * We're allocating buffers of 512 bytes each for communications. The
+ * number of buffers will be computed from the number of buffers supported
+ * by the vring, upto a maximum of 512 buffers (256 in each direction).
+ *
+ * Each buffer will have 16 bytes for the msg header and 496 bytes for
+ * the payload.
+ *
+ * This will utilize a maximum total space of 256KB for the buffers.
+ *
+ * We might also want to add support for user-provided buffers in time.
+ * This will allow bigger buffer size flexibility, and can also be used
+ * to achieve zero-copy messaging.
+ *
+ * Note that these numbers are purely a decision of this driver - we
+ * can change this without changing anything in the firmware of the remote
+ * processor.
+ */
+#define MAX_RPMSG_NUM_BUFS	512
+#define MAX_RPMSG_BUF_SIZE	512
+
+/* 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 05/10] rpmsg: virtio: Move virtio RPMSG structures to private header

[Mathieu Poirier]

Move structure virtiproc_info and virtio_rpmsg_channel to rpmsg_internal.h
so that they can be used by rpmsg_ns.c

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_internal.h   | 62 ++++++++++++++++++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 57 -----------------------------
 2 files changed, 62 insertions(+), 57 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 587f723757d4..3ea9cec26fc0 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -12,12 +12,74 @@
 #ifndef __RPMSG_INTERNAL_H__
 #define __RPMSG_INTERNAL_H__
 
+#include <linux/idr.h>
+#include <linux/mutex.h>
 #include <linux/rpmsg.h>
+#include <linux/types.h>
+#include <linux/virtio.h>
+#include <linux/wait.h>
 #include <linux/poll.h>
 
 #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 virtproc_info - virtual remote processor state
+ * @vdev:	the virtio device
+ * @rvq:	rx virtqueue
+ * @svq:	tx virtqueue
+ * @rbufs:	kernel address of rx buffers
+ * @sbufs:	kernel address of tx buffers
+ * @num_bufs:	total number of buffers for rx and tx
+ * @buf_size:   size of one rx or tx buffer
+ * @last_sbuf:	index of last tx buffer used
+ * @bufs_dma:	dma base addr of the buffers
+ * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
+ *		sending a message might require waking up a dozing remote
+ *		processor, which involves sleeping, hence the mutex.
+ * @endpoints:	idr of local endpoints, allows fast retrieval
+ * @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
+ * remote processor).
+ */
+struct virtproc_info {
+	struct virtio_device *vdev;
+	struct virtqueue *rvq, *svq;
+	void *rbufs, *sbufs;
+	unsigned int num_bufs;
+	unsigned int buf_size;
+	int last_sbuf;
+	dma_addr_t bufs_dma;
+	struct mutex tx_lock;
+	struct idr endpoints;
+	struct mutex endpoints_lock;
+	wait_queue_head_t sendq;
+	atomic_t sleepers;
+	struct rpmsg_endpoint *ns_ept;
+};
+
+/**
+ * struct virtio_rpmsg_channel - rpmsg channel descriptor
+ * @rpdev: the rpmsg channel device
+ * @vrp: the virtio remote processor device this channel belongs to
+ *
+ * This structure stores the channel that links the rpmsg device to the virtio
+ * remote processor device.
+ */
+struct virtio_rpmsg_channel {
+	struct rpmsg_device rpdev;
+
+	struct virtproc_info *vrp;
+};
+
+#define to_virtio_rpmsg_channel(_rpdev) \
+	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
+
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
  * @create_channel:	create backend-specific channel, optional
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index eaf3b2c012c8..0635d86d490f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -32,63 +32,6 @@
 
 #include "rpmsg_internal.h"
 
-/**
- * struct virtproc_info - virtual remote processor state
- * @vdev:	the virtio device
- * @rvq:	rx virtqueue
- * @svq:	tx virtqueue
- * @rbufs:	kernel address of rx buffers
- * @sbufs:	kernel address of tx buffers
- * @num_bufs:	total number of buffers for rx and tx
- * @buf_size:   size of one rx or tx buffer
- * @last_sbuf:	index of last tx buffer used
- * @bufs_dma:	dma base addr of the buffers
- * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
- *		sending a message might require waking up a dozing remote
- *		processor, which involves sleeping, hence the mutex.
- * @endpoints:	idr of local endpoints, allows fast retrieval
- * @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
- * remote processor).
- */
-struct virtproc_info {
-	struct virtio_device *vdev;
-	struct virtqueue *rvq, *svq;
-	void *rbufs, *sbufs;
-	unsigned int num_bufs;
-	unsigned int buf_size;
-	int last_sbuf;
-	dma_addr_t bufs_dma;
-	struct mutex tx_lock;
-	struct idr endpoints;
-	struct mutex endpoints_lock;
-	wait_queue_head_t sendq;
-	atomic_t sleepers;
-	struct rpmsg_endpoint *ns_ept;
-};
-
-/**
- * struct virtio_rpmsg_channel - rpmsg channel descriptor
- * @rpdev: the rpmsg channel device
- * @vrp: the virtio remote processor device this channel belongs to
- *
- * This structure stores the channel that links the rpmsg device to the virtio
- * remote processor device.
- */
-struct virtio_rpmsg_channel {
-	struct rpmsg_device rpdev;
-
-	struct virtproc_info *vrp;
-};
-
-#define to_virtio_rpmsg_channel(_rpdev) \
-	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
-
 /*
  * Local addresses are dynamically allocated on-demand.
  * We do not dynamically assign addresses from the low 1024 range,
-- 
2.25.1

[PATCH 06/10] 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.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/Kconfig          |   8 +++
 drivers/rpmsg/Makefile         |   1 +
 drivers/rpmsg/rpmsg_internal.h |  18 ++++++
 drivers/rpmsg/rpmsg_ns.c       | 110 +++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+)
 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_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3ea9cec26fc0..04e6cb287e18 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -15,6 +15,7 @@
 #include <linux/idr.h>
 #include <linux/mutex.h>
 #include <linux/rpmsg.h>
+#include <linux/rpmsg_ns.h>
 #include <linux/types.h>
 #include <linux/virtio.h>
 #include <linux/wait.h>
@@ -164,4 +165,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
 	return rpmsg_register_device(rpdev);
 }
 
+/**
+ * rpmsg_ns_register_device() - register name service device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg name service device.
+ */
+static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+	strcpy(rpdev->id.name, "rpmsg_ns");
+	rpdev->driver_override = "rpmsg_ns";
+	rpdev->src = RPMSG_NS_ADDR;
+	rpdev->dst = RPMSG_NS_ADDR;
+
+	return rpmsg_register_device(rpdev);
+}
+
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
new file mode 100644
index 000000000000..b3318bf84433
--- /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_ns.h>
+#include <linux/virtio_config.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 virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+	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;
+	}
+
+	/* Don't trust the remote processor for null terminating the name */
+	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
+
+	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
+	chinfo.src = RPMSG_ADDR_ANY;
+	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
+
+	dev_info(dev, "%sing channel %s addr 0x%x\n",
+		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
+		 "destroy" : "creat", msg->name, chinfo.dst);
+
+	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
+		ret = rpmsg_release_channel(rpdev, &chinfo);
+		if (ret)
+			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
+	} else {
+		newch = rpmsg_create_channel(rpdev, &chinfo);
+		if (!newch)
+			dev_err(dev, "rpmsg_create_channel failed\n");
+	}
+
+	return 0;
+}
+
+static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_channel_info ns_chinfo;
+	struct rpmsg_endpoint *ns_ept;
+
+	ns_chinfo.src = RPMSG_NS_ADDR;
+	ns_chinfo.dst = RPMSG_NS_ADDR;
+	strcpy(ns_chinfo.name, "name_service");
+
+	/*
+	 * Create 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");
-- 
2.25.1

[PATCH 07/10] rpmsg: virtio: use rpmsg ns device for the ns announcement

[Mathieu Poirier]

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

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

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

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index c3fc75e6514b..1394114782d2 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -71,5 +71,6 @@ config RPMSG_VIRTIO
 	depends on HAS_DMA
 	select RPMSG
 	select VIRTIO
+	select RPMSG_NS
 
 endmenu
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 04e6cb287e18..2e65386f191e 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -42,7 +42,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
@@ -61,7 +60,6 @@ struct virtproc_info {
 	struct mutex endpoints_lock;
 	wait_queue_head_t sendq;
 	atomic_t sleepers;
-	struct rpmsg_endpoint *ns_ept;
 };
 
 /**
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 0635d86d490f..1c0be0ee790c 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -715,68 +715,14 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
 	wake_up_interruptible(&vrp->sendq);
 }
 
-/* invoked when a name service announcement arrives */
-static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
-		       void *priv, u32 src)
-{
-	struct rpmsg_ns_msg *msg = data;
-	struct rpmsg_device *newch;
-	struct rpmsg_channel_info chinfo;
-	struct virtproc_info *vrp = priv;
-	struct device *dev = &vrp->vdev->dev;
-	int ret;
-
-#if defined(CONFIG_DYNAMIC_DEBUG)
-	dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
-			 data, len, true);
-#endif
-
-	if (len != sizeof(*msg)) {
-		dev_err(dev, "malformed ns msg (%d)\n", len);
-		return -EINVAL;
-	}
-
-	/*
-	 * the name service ept does _not_ belong to a real rpmsg channel,
-	 * and is handled by the rpmsg bus itself.
-	 * for sanity reasons, make sure a valid rpdev has _not_ sneaked
-	 * in somehow.
-	 */
-	if (rpdev) {
-		dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
-		return -EINVAL;
-	}
-
-	/* don't trust the remote processor for null terminating the name */
-	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
-
-	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
-	chinfo.src = RPMSG_ADDR_ANY;
-	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
-
-	dev_info(dev, "%sing channel %s addr 0x%x\n",
-		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
-		 "destroy" : "creat", msg->name, chinfo.dst);
-
-	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
-		ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
-		if (ret)
-			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
-	} else {
-		newch = __rpmsg_create_channel(vrp, &chinfo);
-		if (!newch)
-			dev_err(dev, "rpmsg_create_channel failed\n");
-	}
-
-	return 0;
-}
-
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
 	static const char * const names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct virtproc_info *vrp;
+	struct virtio_rpmsg_channel *vch;
+	struct rpmsg_device *rpdev_ns;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
@@ -852,14 +798,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 	/* if supported by the remote processor, enable the name service */
 	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
-		/* a dedicated endpoint handles the name service msgs */
-		vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
-						vrp, RPMSG_NS_ADDR);
-		if (!vrp->ns_ept) {
-			dev_err(&vdev->dev, "failed to create the ns ept\n");
+		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+		if (!vch) {
 			err = -ENOMEM;
 			goto free_coherent;
 		}
+
+		/* Link the channel to our vrp */
+		vch->vrp = vrp;
+
+		/* Assign public information to the rpmsg_device */
+		rpdev_ns = &vch->rpdev;
+		rpdev_ns->ops = &virtio_rpmsg_ops;
+
+		rpdev_ns->dev.parent = &vrp->vdev->dev;
+		rpdev_ns->dev.release = virtio_rpmsg_release_device;
+
+		err = rpmsg_ns_register_device(rpdev_ns);
+		if (err) {
+			kfree(vch);
+			goto free_coherent;
+		}
 	}
 
 	/*
@@ -912,9 +871,6 @@ static void rpmsg_remove(struct virtio_device *vdev)
 	if (ret)
 		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
 
-	if (vrp->ns_ept)
-		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
-
 	idr_destroy(&vrp->endpoints);
 
 	vdev->config->del_vqs(vrp->vdev);
-- 
2.25.1

[PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Mathieu Poirier]

Add RPMSG device specific byte conversion operations as a first
step to separate the RPMSG name space service from the virtIO
transport layer.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_core.c     | 51 ++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h | 12 ++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 50a835eaf1ba..66ad5b5f1e87 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,57 @@
 
 #include "rpmsg_internal.h"
 
+/**
+ * rpmsg{16|32}_to_cpu()
+ * cpu_to_rpmsg[16|32}() - rpmsg device specific byte conversion functions to
+ *			   perform byte conversion between rpmsg device and the
+ *			   transport layer it is operating on.
+ */
+
+u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->transport16_to_cpu)
+		return -EPERM;
+
+	return rpdev->ops->transport16_to_cpu(rpdev, val);
+}
+EXPORT_SYMBOL(rpmsg16_to_cpu);
+
+u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->cpu_to_transport16)
+		return -EPERM;
+
+	return rpdev->ops->cpu_to_transport16(rpdev, val);
+}
+EXPORT_SYMBOL(cpu_to_rpmsg16);
+
+u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->transport32_to_cpu)
+		return -EPERM;
+
+	return rpdev->ops->transport32_to_cpu(rpdev, val);
+}
+EXPORT_SYMBOL(rpmsg32_to_cpu);
+
+u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->cpu_to_transport32)
+		return -EPERM;
+
+	return rpdev->ops->cpu_to_transport32(rpdev, val);
+}
+EXPORT_SYMBOL(cpu_to_rpmsg32);
+
 /**
  * rpmsg_create_channel() - create a new rpmsg channel
  * using its name and address info.
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 2e65386f191e..2f0ad1a52698 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -81,6 +81,8 @@ struct virtio_rpmsg_channel {
 
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @transport{16|32}_to_cpu: byte conversion from rpmsg device to transport layer
+ * @cpu_to_transport{16|32}: byte conversion from transport layer to rpmsg device
  * @create_channel:	create backend-specific channel, optional
  * @release_channel:	release backend-specific channel, optional
  * @create_ept:		create backend-specific endpoint, required
@@ -92,6 +94,10 @@ struct virtio_rpmsg_channel {
  * advertise new channels implicitly by creating the endpoints.
  */
 struct rpmsg_device_ops {
+	u16 (*transport16_to_cpu)(struct rpmsg_device *rpdev, u16 val);
+	u16 (*cpu_to_transport16)(struct rpmsg_device *rpdev, u16 val);
+	u32 (*transport32_to_cpu)(struct rpmsg_device *rpdev, u32 val);
+	u32 (*cpu_to_transport32)(struct rpmsg_device *rpdev, u32 val);
 	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
 					     struct rpmsg_channel_info *chinfo);
 	int (*release_channel)(struct rpmsg_device *rpdev,
@@ -148,6 +154,12 @@ 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);
+
+u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val);
+u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val);
+u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val);
+u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val);
+
 /**
  * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
  * @rpdev:	prepared rpdev to be used for creating endpoints
-- 
2.25.1

[PATCH 09/10] rpmsg: virtio: Make endianness conversion virtIO specific

[Mathieu Poirier]

Introduce rpmsg operations to make byte conversion specific to the
virtIO transport layer, therefore avoiding the protocol code from
being aware of the virtIO transport specification.

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

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 1c0be0ee790c..15cc383a85cc 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -157,6 +157,38 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
 	return NULL;
 }
 
+static u16 virtio_rpmsg_transport16_to_cpu(struct rpmsg_device *rpdev, u16 val)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return virtio16_to_cpu(vrp->vdev, val);
+}
+
+static u16 virtio_rpmsg_cpu_to_transport16(struct rpmsg_device *rpdev, u16 val)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return cpu_to_virtio16(vrp->vdev, val);
+}
+
+static u32 virtio_rpmsg_transport32_to_cpu(struct rpmsg_device *rpdev, u32 val)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return virtio32_to_cpu(vrp->vdev, val);
+}
+
+static u32 virtio_rpmsg_cpu_to_transport32(struct rpmsg_device *rpdev, u32 val)
+{
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	struct virtproc_info *vrp = vch->vrp;
+
+	return cpu_to_virtio32(vrp->vdev, val);
+}
+
 static struct rpmsg_device *
 virtio_rpmsg_create_channel(struct rpmsg_device *rpdev,
 			    struct rpmsg_channel_info *chinfo)
@@ -268,6 +300,10 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 }
 
 static const struct rpmsg_device_ops virtio_rpmsg_ops = {
+	.transport16_to_cpu = virtio_rpmsg_transport16_to_cpu,
+	.cpu_to_transport16 = virtio_rpmsg_cpu_to_transport16,
+	.transport32_to_cpu = virtio_rpmsg_transport32_to_cpu,
+	.cpu_to_transport32 = virtio_rpmsg_cpu_to_transport32,
 	.create_channel = virtio_rpmsg_create_channel,
 	.release_channel = virtio_rpmsg_release_channel,
 	.create_ept = virtio_rpmsg_create_ept,
-- 
2.25.1

[PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic

[Mathieu Poirier]

Make name service module transport agnostic by using the rpmsg
device specific byte conversion routine.

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

diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index b3318bf84433..1df3aaadfe10 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -18,9 +18,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 	struct rpmsg_ns_msg *msg = data;
 	struct rpmsg_device *newch;
 	struct rpmsg_channel_info chinfo;
-	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
-	struct virtproc_info *vrp = vch->vrp;
-	struct device *dev = &vrp->vdev->dev;
+	struct device *dev = &rpdev->dev;
 	int ret;
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
@@ -38,13 +36,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_release_channel(rpdev, &chinfo);
 		if (ret)
 			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
index aabc6c3c0d6d..9f3030b2145b 100644
--- a/include/linux/rpmsg_ns.h
+++ b/include/linux/rpmsg_ns.h
@@ -41,8 +41,8 @@ struct rpmsg_hdr {
  */
 struct rpmsg_ns_msg {
 	char name[RPMSG_NAME_SIZE];
-	__virtio32 addr;
-	__virtio32 flags;
+	u32 addr;
+	u32 flags;
 } __packed;
 
 /**
-- 
2.25.1

Re: [PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Randy Dunlap]

On 9/21/20 5:09 PM, Mathieu Poirier wrote:
> +/**

Hi,
This block is not in kernel-doc format, so the comment block should not
begin with /**.

> + * rpmsg{16|32}_to_cpu()
> + * cpu_to_rpmsg[16|32}() - rpmsg device specific byte conversion functions to
> + *			   perform byte conversion between rpmsg device and the
> + *			   transport layer it is operating on.
> + */
> +
> +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)
> +{


thanks.
-- 
~Randy

Re: [PATCH 01/10] rpmsg: virtio: rename rpmsg_create_channel

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:51PM -0600, Mathieu Poirier wrote:
> 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>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 7d7ed4e5cce7..e8d55c8b9cbf 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -395,8 +395,9 @@ static void virtio_rpmsg_release_device(struct device *dev)
>   * this function will be used to create both static and dynamic
>   * channels.
>   */
> -static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
> -						 struct rpmsg_channel_info *chinfo)
> +static struct rpmsg_device *
> +__rpmsg_create_channel(struct virtproc_info *vrp,
> +		       struct rpmsg_channel_info *chinfo)

Nitpick: we now have 100 characters, so there's no *need* any more to split that 
line, now it's more a matter of consistent style and personal preference. Most 
functions in that file have function type and name on the same line, but a few 
also make the split like here... So, we can choose our poison here I guess.

Thanks
Guennadi

>  {
>  	struct virtio_rpmsg_channel *vch;
>  	struct rpmsg_device *rpdev;
> @@ -869,7 +870,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
> 

Re: [PATCH 00/10] rpmsg: Make RPMSG name service modular

[Guennadi Liakhovetski]

Hi Mathieu,

Thanks for the patches. I'm trying to understand the concept of 
this approach and I'm probably failing at that. It seems to me 
that this patch set is making the NS announcement service to a 
separate RPMsg device and I don't understand the reasoning for 
doing this. As far as I understand namespace announcements 
belong to RPMsg devices / channels, they create a dedicated 
endpoint on them with a fixed pre-defined address. But they 
don't form a separate RPMsg device. I think the current 
virtio_rpmsg_bus.c has that correctly: for each rpmsg device / 
channel multiple endpoints can be created, where the NS 
service is one of them. It's just an endpoing of an rpmsg 
device, not a complete separate device. Have I misunderstood 
anything?

Thanks
Guennadi

On Mon, Sep 21, 2020 at 06:09:50PM -0600, Mathieu Poirier wrote:
> Hi all,
> 
> After looking at Guennadi[1] and Arnaud's patchsets[2] it became
> clear that we need to go back to a generic rpmsg_ns_msg structure
> if we wanted to make progress.  To do that some of the work from
> Arnaud had to be modified in a way that common name service
> functionality was transport agnostic.
> 
> This patchset is based on Arnaud's work but also include a patch
> from Guennadi and some input from me.  It should serve as a
> foundation for the next revision of [1].
> 
> Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
> did not test the modularisation.   
> 
> Comments and feedback would be greatly appreciated.
> 
> Thanks,
> Mathieu 
> 
> [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=346593
> [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> 
> Arnaud Pouliquen (5):
>   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
>   rpmsg: virtio: use rpmsg ns device for the ns announcement
> 
> Guennadi Liakhovetski (1):
>   rpmsg: Move common structures and defines to headers
> 
> Mathieu Poirier (4):
>   rpmsg: virtio: Move virtio RPMSG structures to private header
>   rpmsg: core: Add RPMSG byte conversion operations
>   rpmsg: virtio: Make endianness conversion virtIO specific
>   rpmsg: ns: Make Name service module transport agnostic
> 
>  drivers/rpmsg/Kconfig            |   9 +
>  drivers/rpmsg/Makefile           |   1 +
>  drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
>  drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
>  drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
>  include/linux/rpmsg_ns.h         |  83 +++++++++
>  include/uapi/linux/rpmsg.h       |   3 +
>  8 files changed, 487 insertions(+), 199 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ns.c
>  create mode 100644 include/linux/rpmsg_ns.h
> 
> -- 
> 2.25.1
> 

Re: [PATCH 04/10] rpmsg: Move common structures and defines to headers

[Arnaud POULIQUEN]

Hi Mathieu,

On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> 
> virtio_rpmsg_bus.c keeps RPMsg protocol structure declarations and
> common defines like the ones, needed for name-space announcements,
> internal. Move them to common headers instead.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> [Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
>  include/linux/rpmsg_ns.h         | 83 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/rpmsg.h       |  3 ++
>  3 files changed, 88 insertions(+), 76 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 e87cf0b79542..eaf3b2c012c8 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,6 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg_ns.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> @@ -27,6 +28,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 +72,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 {
> -	__virtio32 src;
> -	__virtio32 dst;
> -	__virtio32 reserved;
> -	__virtio16 len;
> -	__virtio16 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];
> -	__virtio32 addr;
> -	__virtio32 flags;
> -} __packed;
> -
> -/**
> - * enum rpmsg_ns_flags - dynamic name service announcement flags
> - *
> - * @RPMSG_NS_CREATE: a new remote service was just created
> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> - */
> -enum rpmsg_ns_flags {
> -	RPMSG_NS_CREATE		= 0,
> -	RPMSG_NS_DESTROY	= 1,
> -};
> -
>  /**
>   * struct virtio_rpmsg_channel - rpmsg channel descriptor
>   * @rpdev: the rpmsg channel device
> @@ -139,27 +89,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>  	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>  
> -/*
> - * We're allocating buffers of 512 bytes each for communications. The
> - * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> - *
> - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> - * the payload.
> - *
> - * This will utilize a maximum total space of 256KB for the buffers.
> - *
> - * We might also want to add support for user-provided buffers in time.
> - * This will allow bigger buffer size flexibility, and can also be used
> - * to achieve zero-copy messaging.
> - *
> - * Note that these numbers are purely a decision of this driver - we
> - * can change this without changing anything in the firmware of the remote
> - * processor.
> - */
> -#define MAX_RPMSG_NUM_BUFS	(512)
> -#define MAX_RPMSG_BUF_SIZE	(512)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> @@ -167,9 +96,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..aabc6c3c0d6d
> --- /dev/null
> +++ b/include/linux/rpmsg_ns.h
> @@ -0,0 +1,83 @@
> +/* 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/virtio_types.h>

That means that this file is correlated with the virtio, right?
there is a risk that It cannot be used by some platforms which not use virtio...

> +
> +/**
> + * 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 {
> +	__virtio32 src;
> +	__virtio32 dst;
> +	__virtio32 reserved;
> +	__virtio16 len;
> +	__virtio16 flags;
> +	u8 data[];
> +} __packed;
This header is related to the virtio implementation, and represent the header of
the RPMsgs in virtio implementation not only the ns annoucement.

What about splitting it in 2 files?
1) rpmsg_ns.h 
  definitions related to the ns announcement

2) rpmsg_virtio.h
- definitions related to the RPMsg virtio implementation
- This file could include the rpmsg_ns.h 

Thanks,
Arnaud

> +
> +/**
> + * struct rpmsg_ns_msg - dynamic name service announcement message
> + * @name: name of remote service that is published
> + * @addr: address of remote service that is published
> + * @flags: indicates whether service is created or destroyed
> + *
> + * This message is sent across to publish a new service, or announce
> + * about its removal. When we receive these messages, an appropriate
> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> + * (if/as-soon-as one is registered).
> + */
> +struct rpmsg_ns_msg {
> +	char name[RPMSG_NAME_SIZE];
> +	__virtio32 addr;
> +	__virtio32 flags;
> +} __packed;
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> +	RPMSG_NS_CREATE		= 0,
> +	RPMSG_NS_DESTROY	= 1,
> +};
> +
> +/*
> + * We're allocating buffers of 512 bytes each for communications. The
> + * number of buffers will be computed from the number of buffers supported
> + * by the vring, upto a maximum of 512 buffers (256 in each direction).
> + *
> + * Each buffer will have 16 bytes for the msg header and 496 bytes for
> + * the payload.
> + *
> + * This will utilize a maximum total space of 256KB for the buffers.
> + *
> + * We might also want to add support for user-provided buffers in time.
> + * This will allow bigger buffer size flexibility, and can also be used
> + * to achieve zero-copy messaging.
> + *
> + * Note that these numbers are purely a decision of this driver - we
> + * can change this without changing anything in the firmware of the remote
> + * processor.
> + */
> +#define MAX_RPMSG_NUM_BUFS	512
> +#define MAX_RPMSG_BUF_SIZE	512
> +
> +/* 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
> 

Re: [PATCH 05/10] rpmsg: virtio: Move virtio RPMSG structures to private header

[Arnaud POULIQUEN]

On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> Move structure virtiproc_info and virtio_rpmsg_channel to rpmsg_internal.h
> so that they can be used by rpmsg_ns.c
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_internal.h   | 62 ++++++++++++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 57 -----------------------------
>  2 files changed, 62 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 587f723757d4..3ea9cec26fc0 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -12,12 +12,74 @@
>  #ifndef __RPMSG_INTERNAL_H__
>  #define __RPMSG_INTERNAL_H__
>  
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
>  #include <linux/rpmsg.h>
> +#include <linux/types.h>
> +#include <linux/virtio.h>

This also creates a dependency with virtio
This file is included by several drivers...

Regards,
Arnaud

> +#include <linux/wait.h>
>  #include <linux/poll.h>
>  
>  #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 virtproc_info - virtual remote processor state
> + * @vdev:	the virtio device
> + * @rvq:	rx virtqueue
> + * @svq:	tx virtqueue
> + * @rbufs:	kernel address of rx buffers
> + * @sbufs:	kernel address of tx buffers
> + * @num_bufs:	total number of buffers for rx and tx
> + * @buf_size:   size of one rx or tx buffer
> + * @last_sbuf:	index of last tx buffer used
> + * @bufs_dma:	dma base addr of the buffers
> + * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> + *		sending a message might require waking up a dozing remote
> + *		processor, which involves sleeping, hence the mutex.
> + * @endpoints:	idr of local endpoints, allows fast retrieval
> + * @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
> + * remote processor).
> + */
> +struct virtproc_info {
> +	struct virtio_device *vdev;
> +	struct virtqueue *rvq, *svq;
> +	void *rbufs, *sbufs;
> +	unsigned int num_bufs;
> +	unsigned int buf_size;
> +	int last_sbuf;
> +	dma_addr_t bufs_dma;
> +	struct mutex tx_lock;
> +	struct idr endpoints;
> +	struct mutex endpoints_lock;
> +	wait_queue_head_t sendq;
> +	atomic_t sleepers;
> +	struct rpmsg_endpoint *ns_ept;
> +};
> +
> +/**
> + * struct virtio_rpmsg_channel - rpmsg channel descriptor
> + * @rpdev: the rpmsg channel device
> + * @vrp: the virtio remote processor device this channel belongs to
> + *
> + * This structure stores the channel that links the rpmsg device to the virtio
> + * remote processor device.
> + */
> +struct virtio_rpmsg_channel {
> +	struct rpmsg_device rpdev;
> +
> +	struct virtproc_info *vrp;
> +};
> +
> +#define to_virtio_rpmsg_channel(_rpdev) \
> +	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> +
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>   * @create_channel:	create backend-specific channel, optional
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index eaf3b2c012c8..0635d86d490f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -32,63 +32,6 @@
>  
>  #include "rpmsg_internal.h"
>  
> -/**
> - * struct virtproc_info - virtual remote processor state
> - * @vdev:	the virtio device
> - * @rvq:	rx virtqueue
> - * @svq:	tx virtqueue
> - * @rbufs:	kernel address of rx buffers
> - * @sbufs:	kernel address of tx buffers
> - * @num_bufs:	total number of buffers for rx and tx
> - * @buf_size:   size of one rx or tx buffer
> - * @last_sbuf:	index of last tx buffer used
> - * @bufs_dma:	dma base addr of the buffers
> - * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> - *		sending a message might require waking up a dozing remote
> - *		processor, which involves sleeping, hence the mutex.
> - * @endpoints:	idr of local endpoints, allows fast retrieval
> - * @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
> - * remote processor).
> - */
> -struct virtproc_info {
> -	struct virtio_device *vdev;
> -	struct virtqueue *rvq, *svq;
> -	void *rbufs, *sbufs;
> -	unsigned int num_bufs;
> -	unsigned int buf_size;
> -	int last_sbuf;
> -	dma_addr_t bufs_dma;
> -	struct mutex tx_lock;
> -	struct idr endpoints;
> -	struct mutex endpoints_lock;
> -	wait_queue_head_t sendq;
> -	atomic_t sleepers;
> -	struct rpmsg_endpoint *ns_ept;
> -};
> -
> -/**
> - * struct virtio_rpmsg_channel - rpmsg channel descriptor
> - * @rpdev: the rpmsg channel device
> - * @vrp: the virtio remote processor device this channel belongs to
> - *
> - * This structure stores the channel that links the rpmsg device to the virtio
> - * remote processor device.
> - */
> -struct virtio_rpmsg_channel {
> -	struct rpmsg_device rpdev;
> -
> -	struct virtproc_info *vrp;
> -};
> -
> -#define to_virtio_rpmsg_channel(_rpdev) \
> -	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> 

Re: [PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Arnaud POULIQUEN]

On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> Add RPMSG device specific byte conversion operations as a first
> step to separate the RPMSG name space service from the virtIO
> transport layer.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 51 ++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h | 12 ++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 50a835eaf1ba..66ad5b5f1e87 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,57 @@
>  
>  #include "rpmsg_internal.h"
>  
> +/**
> + * rpmsg{16|32}_to_cpu()
> + * cpu_to_rpmsg[16|32}() - rpmsg device specific byte conversion functions to
> + *			   perform byte conversion between rpmsg device and the
> + *			   transport layer it is operating on.
> + */
> +
> +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->transport16_to_cpu)
> +		return -EPERM;
> +
> +	return rpdev->ops->transport16_to_cpu(rpdev, val);
> +}
> +EXPORT_SYMBOL(rpmsg16_to_cpu);
> +
> +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport16)
> +		return -EPERM;
> +
> +	return rpdev->ops->cpu_to_transport16(rpdev, val);
> +}
> +EXPORT_SYMBOL(cpu_to_rpmsg16);
> +
> +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->transport32_to_cpu)
> +		return -EPERM;
> +
> +	return rpdev->ops->transport32_to_cpu(rpdev, val);
> +}
> +EXPORT_SYMBOL(rpmsg32_to_cpu);
> +
> +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport32)
> +		return -EPERM;

Alternative could be to choice the processor endianness ( it was the case
before the virtio patch to set the endianness

> +
> +	return rpdev->ops->cpu_to_transport32(rpdev, val);
> +}
> +EXPORT_SYMBOL(cpu_to_rpmsg32);
> +
>  /**
>   * rpmsg_create_channel() - create a new rpmsg channel
>   * using its name and address info.
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 2e65386f191e..2f0ad1a52698 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -81,6 +81,8 @@ struct virtio_rpmsg_channel {
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @transport{16|32}_to_cpu: byte conversion from rpmsg device to transport layer
> + * @cpu_to_transport{16|32}: byte conversion from transport layer to rpmsg device
>   * @create_channel:	create backend-specific channel, optional
>   * @release_channel:	release backend-specific channel, optional
>   * @create_ept:		create backend-specific endpoint, required
> @@ -92,6 +94,10 @@ struct virtio_rpmsg_channel {
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	u16 (*transport16_to_cpu)(struct rpmsg_device *rpdev, u16 val);
> +	u16 (*cpu_to_transport16)(struct rpmsg_device *rpdev, u16 val);
> +	u32 (*transport32_to_cpu)(struct rpmsg_device *rpdev, u32 val);
> +	u32 (*cpu_to_transport32)(struct rpmsg_device *rpdev, u32 val);

This trigg me a suggestion. Perhaps it would be simpler to have only on ops
to get the endianness.

Regards
Arnaud

>  	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
>  					     struct rpmsg_channel_info *chinfo);
>  	int (*release_channel)(struct rpmsg_device *rpdev,
> @@ -148,6 +154,12 @@ 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);
> +
> +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val);
> +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val);
> +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val);
> +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val);
> +
>  /**
>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>   * @rpdev:	prepared rpdev to be used for creating endpoints
> 

Re: [PATCH 00/10] rpmsg: Make RPMSG name service modular

[Mathieu Poirier]

Good day Guennadi,

On Tue, 22 Sep 2020 at 02:09, Guennadi Liakhovetski
<guennadi.liakhovetski@linux.intel.com> wrote:
>
> Hi Mathieu,
>
> Thanks for the patches. I'm trying to understand the concept of
> this approach and I'm probably failing at that. It seems to me
> that this patch set is making the NS announcement service to a
> separate RPMsg device and I don't understand the reasoning for
> doing this. As far as I understand namespace announcements
> belong to RPMsg devices / channels, they create a dedicated
> endpoint on them with a fixed pre-defined address. But they
> don't form a separate RPMsg device. I think the current
> virtio_rpmsg_bus.c has that correctly: for each rpmsg device /
> channel multiple endpoints can be created, where the NS
> service is one of them. It's just an endpoing of an rpmsg
> device, not a complete separate device. Have I misunderstood
> anything?

This patchset does not introduce any new features - the end result in
terms of functionality is exactly the same.  It is also a carbon copy
of the work introduced by Arnaud (hence reusing his patches), with the
exception that the code is presented in a slightly different order to
allow for a complete dissociation of RPMSG name service from the
virtIO transport mechanic.

To make that happen rpmsg device specific byte conversion operations
had to be introduced in struct rpmsg_device_ops and the explicit
creation of an rpmsg_device associated with the name service (that
wasn't needed when name service was welded to virtIO).  But
associating a rpmsg_device to the name service doesn't change anything
- RPMSG devices are created the same way when name service messages
are received from the host or the remote processor.

To prove my theory I ran the rpmsg_client_sample.c and it just worked,
no changes to client code needed.

Let's keep talking, it's the only way we'll get through this.

Mathieu

>
> Thanks
> Guennadi
>
> On Mon, Sep 21, 2020 at 06:09:50PM -0600, Mathieu Poirier wrote:
> > Hi all,
> >
> > After looking at Guennadi[1] and Arnaud's patchsets[2] it became
> > clear that we need to go back to a generic rpmsg_ns_msg structure
> > if we wanted to make progress.  To do that some of the work from
> > Arnaud had to be modified in a way that common name service
> > functionality was transport agnostic.
> >
> > This patchset is based on Arnaud's work but also include a patch
> > from Guennadi and some input from me.  It should serve as a
> > foundation for the next revision of [1].
> >
> > Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
> > did not test the modularisation.
> >
> > Comments and feedback would be greatly appreciated.
> >
> > Thanks,
> > Mathieu
> >
> > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=346593
> > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> >
> > Arnaud Pouliquen (5):
> >   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
> >   rpmsg: virtio: use rpmsg ns device for the ns announcement
> >
> > Guennadi Liakhovetski (1):
> >   rpmsg: Move common structures and defines to headers
> >
> > Mathieu Poirier (4):
> >   rpmsg: virtio: Move virtio RPMSG structures to private header
> >   rpmsg: core: Add RPMSG byte conversion operations
> >   rpmsg: virtio: Make endianness conversion virtIO specific
> >   rpmsg: ns: Make Name service module transport agnostic
> >
> >  drivers/rpmsg/Kconfig            |   9 +
> >  drivers/rpmsg/Makefile           |   1 +
> >  drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
> >  drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
> >  drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
> >  include/linux/rpmsg_ns.h         |  83 +++++++++
> >  include/uapi/linux/rpmsg.h       |   3 +
> >  8 files changed, 487 insertions(+), 199 deletions(-)
> >  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> >  create mode 100644 include/linux/rpmsg_ns.h
> >
> > --
> > 2.25.1
> >

Re: [PATCH 01/10] rpmsg: virtio: rename rpmsg_create_channel

[Mathieu Poirier]

On Tue, Sep 22, 2020 at 09:06:03AM +0200, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:51PM -0600, Mathieu Poirier wrote:
> > 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>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 7d7ed4e5cce7..e8d55c8b9cbf 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -395,8 +395,9 @@ static void virtio_rpmsg_release_device(struct device *dev)
> >   * this function will be used to create both static and dynamic
> >   * channels.
> >   */
> > -static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
> > -						 struct rpmsg_channel_info *chinfo)
> > +static struct rpmsg_device *
> > +__rpmsg_create_channel(struct virtproc_info *vrp,
> > +		       struct rpmsg_channel_info *chinfo)
> 
> Nitpick: we now have 100 characters, so there's no *need* any more to split that 
> line, now it's more a matter of consistent style and personal preference. Most 
> functions in that file have function type and name on the same line, but a few 
> also make the split like here...
>
 So, we can choose our poison here I guess.
>

I agree - there is really no _better_ way of doing this.  I'll let Bjorn make
the final call but I'm pretty sure he doesn't have a strong opinion either.

> Thanks
> Guennadi
> 
> >  {
> >  	struct virtio_rpmsg_channel *vch;
> >  	struct rpmsg_device *rpdev;
> > @@ -869,7 +870,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
> > 

Re: [PATCH 04/10] rpmsg: Move common structures and defines to headers

[Mathieu Poirier]

On Tue, Sep 22, 2020 at 04:26:23PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> > From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > 
> > virtio_rpmsg_bus.c keeps RPMsg protocol structure declarations and
> > common defines like the ones, needed for name-space announcements,
> > internal. Move them to common headers instead.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > [Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
> >  include/linux/rpmsg_ns.h         | 83 ++++++++++++++++++++++++++++++++
> >  include/uapi/linux/rpmsg.h       |  3 ++
> >  3 files changed, 88 insertions(+), 76 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 e87cf0b79542..eaf3b2c012c8 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >  #include <linux/rpmsg.h>
> > +#include <linux/rpmsg_ns.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> > @@ -27,6 +28,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 +72,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 {
> > -	__virtio32 src;
> > -	__virtio32 dst;
> > -	__virtio32 reserved;
> > -	__virtio16 len;
> > -	__virtio16 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];
> > -	__virtio32 addr;
> > -	__virtio32 flags;
> > -} __packed;
> > -
> > -/**
> > - * enum rpmsg_ns_flags - dynamic name service announcement flags
> > - *
> > - * @RPMSG_NS_CREATE: a new remote service was just created
> > - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> > - */
> > -enum rpmsg_ns_flags {
> > -	RPMSG_NS_CREATE		= 0,
> > -	RPMSG_NS_DESTROY	= 1,
> > -};
> > -
> >  /**
> >   * struct virtio_rpmsg_channel - rpmsg channel descriptor
> >   * @rpdev: the rpmsg channel device
> > @@ -139,27 +89,6 @@ struct virtio_rpmsg_channel {
> >  #define to_virtio_rpmsg_channel(_rpdev) \
> >  	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> >  
> > -/*
> > - * We're allocating buffers of 512 bytes each for communications. The
> > - * number of buffers will be computed from the number of buffers supported
> > - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> > - *
> > - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> > - * the payload.
> > - *
> > - * This will utilize a maximum total space of 256KB for the buffers.
> > - *
> > - * We might also want to add support for user-provided buffers in time.
> > - * This will allow bigger buffer size flexibility, and can also be used
> > - * to achieve zero-copy messaging.
> > - *
> > - * Note that these numbers are purely a decision of this driver - we
> > - * can change this without changing anything in the firmware of the remote
> > - * processor.
> > - */
> > -#define MAX_RPMSG_NUM_BUFS	(512)
> > -#define MAX_RPMSG_BUF_SIZE	(512)
> > -
> >  /*
> >   * Local addresses are dynamically allocated on-demand.
> >   * We do not dynamically assign addresses from the low 1024 range,
> > @@ -167,9 +96,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..aabc6c3c0d6d
> > --- /dev/null
> > +++ b/include/linux/rpmsg_ns.h
> > @@ -0,0 +1,83 @@
> > +/* 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/virtio_types.h>
> 
> That means that this file is correlated with the virtio, right?
> there is a risk that It cannot be used by some platforms which not use virtio...

An astute observation...

> 
> > +
> > +/**
> > + * 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 {
> > +	__virtio32 src;
> > +	__virtio32 dst;
> > +	__virtio32 reserved;
> > +	__virtio16 len;
> > +	__virtio16 flags;
> > +	u8 data[];
> > +} __packed;
> This header is related to the virtio implementation, and represent the header of
> the RPMsgs in virtio implementation not only the ns annoucement.
> 
> What about splitting it in 2 files?
> 1) rpmsg_ns.h 
>   definitions related to the ns announcement
> 
> 2) rpmsg_virtio.h
> - definitions related to the RPMsg virtio implementation
> - This file could include the rpmsg_ns.h

Yes, that make sense.

> 
> Thanks,
> Arnaud
> 
> > +
> > +/**
> > + * struct rpmsg_ns_msg - dynamic name service announcement message
> > + * @name: name of remote service that is published
> > + * @addr: address of remote service that is published
> > + * @flags: indicates whether service is created or destroyed
> > + *
> > + * This message is sent across to publish a new service, or announce
> > + * about its removal. When we receive these messages, an appropriate
> > + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> > + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> > + * (if/as-soon-as one is registered).
> > + */
> > +struct rpmsg_ns_msg {
> > +	char name[RPMSG_NAME_SIZE];
> > +	__virtio32 addr;
> > +	__virtio32 flags;
> > +} __packed;
> > +
> > +/**
> > + * enum rpmsg_ns_flags - dynamic name service announcement flags
> > + *
> > + * @RPMSG_NS_CREATE: a new remote service was just created
> > + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> > + */
> > +enum rpmsg_ns_flags {
> > +	RPMSG_NS_CREATE		= 0,
> > +	RPMSG_NS_DESTROY	= 1,
> > +};
> > +
> > +/*
> > + * We're allocating buffers of 512 bytes each for communications. The
> > + * number of buffers will be computed from the number of buffers supported
> > + * by the vring, upto a maximum of 512 buffers (256 in each direction).
> > + *
> > + * Each buffer will have 16 bytes for the msg header and 496 bytes for
> > + * the payload.
> > + *
> > + * This will utilize a maximum total space of 256KB for the buffers.
> > + *
> > + * We might also want to add support for user-provided buffers in time.
> > + * This will allow bigger buffer size flexibility, and can also be used
> > + * to achieve zero-copy messaging.
> > + *
> > + * Note that these numbers are purely a decision of this driver - we
> > + * can change this without changing anything in the firmware of the remote
> > + * processor.
> > + */
> > +#define MAX_RPMSG_NUM_BUFS	512
> > +#define MAX_RPMSG_BUF_SIZE	512
> > +
> > +/* 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
> > 

Re: [PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Mathieu Poirier]

On Tue, Sep 22, 2020 at 04:34:53PM +0200, Arnaud POULIQUEN wrote:
> 
> 
> On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> > Add RPMSG device specific byte conversion operations as a first
> > step to separate the RPMSG name space service from the virtIO
> > transport layer.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/rpmsg_core.c     | 51 ++++++++++++++++++++++++++++++++++
> >  drivers/rpmsg/rpmsg_internal.h | 12 ++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> > index 50a835eaf1ba..66ad5b5f1e87 100644
> > --- a/drivers/rpmsg/rpmsg_core.c
> > +++ b/drivers/rpmsg/rpmsg_core.c
> > @@ -20,6 +20,57 @@
> >  
> >  #include "rpmsg_internal.h"
> >  
> > +/**
> > + * rpmsg{16|32}_to_cpu()
> > + * cpu_to_rpmsg[16|32}() - rpmsg device specific byte conversion functions to
> > + *			   perform byte conversion between rpmsg device and the
> > + *			   transport layer it is operating on.
> > + */
> > +
> > +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)
> > +{
> > +	if (WARN_ON(!rpdev))
> > +		return -EINVAL;
> > +	if (!rpdev->ops || !rpdev->ops->transport16_to_cpu)
> > +		return -EPERM;
> > +
> > +	return rpdev->ops->transport16_to_cpu(rpdev, val);
> > +}
> > +EXPORT_SYMBOL(rpmsg16_to_cpu);
> > +
> > +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
> > +{
> > +	if (WARN_ON(!rpdev))
> > +		return -EINVAL;
> > +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport16)
> > +		return -EPERM;
> > +
> > +	return rpdev->ops->cpu_to_transport16(rpdev, val);
> > +}
> > +EXPORT_SYMBOL(cpu_to_rpmsg16);
> > +
> > +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
> > +{
> > +	if (WARN_ON(!rpdev))
> > +		return -EINVAL;
> > +	if (!rpdev->ops || !rpdev->ops->transport32_to_cpu)
> > +		return -EPERM;
> > +
> > +	return rpdev->ops->transport32_to_cpu(rpdev, val);
> > +}
> > +EXPORT_SYMBOL(rpmsg32_to_cpu);
> > +
> > +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
> > +{
> > +	if (WARN_ON(!rpdev))
> > +		return -EINVAL;
> > +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport32)
> > +		return -EPERM;
> 
> Alternative could be to choice the processor endianness ( it was the case
> before the virtio patch to set the endianness

That's a good idea.

> 
> > +
> > +	return rpdev->ops->cpu_to_transport32(rpdev, val);
> > +}
> > +EXPORT_SYMBOL(cpu_to_rpmsg32);
> > +
> >  /**
> >   * rpmsg_create_channel() - create a new rpmsg channel
> >   * using its name and address info.
> > diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> > index 2e65386f191e..2f0ad1a52698 100644
> > --- a/drivers/rpmsg/rpmsg_internal.h
> > +++ b/drivers/rpmsg/rpmsg_internal.h
> > @@ -81,6 +81,8 @@ struct virtio_rpmsg_channel {
> >  
> >  /**
> >   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> > + * @transport{16|32}_to_cpu: byte conversion from rpmsg device to transport layer
> > + * @cpu_to_transport{16|32}: byte conversion from transport layer to rpmsg device
> >   * @create_channel:	create backend-specific channel, optional
> >   * @release_channel:	release backend-specific channel, optional
> >   * @create_ept:		create backend-specific endpoint, required
> > @@ -92,6 +94,10 @@ struct virtio_rpmsg_channel {
> >   * advertise new channels implicitly by creating the endpoints.
> >   */
> >  struct rpmsg_device_ops {
> > +	u16 (*transport16_to_cpu)(struct rpmsg_device *rpdev, u16 val);
> > +	u16 (*cpu_to_transport16)(struct rpmsg_device *rpdev, u16 val);
> > +	u32 (*transport32_to_cpu)(struct rpmsg_device *rpdev, u32 val);
> > +	u32 (*cpu_to_transport32)(struct rpmsg_device *rpdev, u32 val);
> 
> This trigg me a suggestion. Perhaps it would be simpler to have only on ops
> to get the endianness.
> 

Another good idea, I'll look into it.

Thanks for the comments,
Mathieu

> Regards
> Arnaud
> 
> >  	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
> >  					     struct rpmsg_channel_info *chinfo);
> >  	int (*release_channel)(struct rpmsg_device *rpdev,
> > @@ -148,6 +154,12 @@ 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);
> > +
> > +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val);
> > +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val);
> > +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val);
> > +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val);
> > +
> >  /**
> >   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> >   * @rpdev:	prepared rpdev to be used for creating endpoints
> > 

Re: [PATCH 09/10] rpmsg: virtio: Make endianness conversion virtIO specific

[kernel test robot]

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

Hi Mathieu,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
base:    b10b8ad862118bf42c28a98b0f067619aadcfb23
config: i386-randconfig-s001-20200921 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-201-g24bdaac6-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

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


sparse warnings: (new ones prefixed by >>)

>> drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted __virtio16 [usertype] val @@     got unsigned short [usertype] val @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse:     expected restricted __virtio16 [usertype] val
>> drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse:     got unsigned short [usertype] val
>> drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse: sparse: incorrect type in return expression (different base types) @@     expected unsigned short @@     got restricted __virtio16 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse:     expected unsigned short
>> drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse:     got restricted __virtio16
>> drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted __virtio32 [usertype] val @@     got unsigned int [usertype] val @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse:     expected restricted __virtio32 [usertype] val
>> drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse:     got unsigned int [usertype] val
>> drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse: sparse: incorrect type in return expression (different base types) @@     expected unsigned int @@     got restricted __virtio32 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse:     expected unsigned int
>> drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse:     got restricted __virtio32

# https://github.com/0day-ci/linux/commit/dd032e0b67fcd6197fed6b6a35c14fa07934a9b4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
git checkout dd032e0b67fcd6197fed6b6a35c14fa07934a9b4
vim +165 drivers/rpmsg/virtio_rpmsg_bus.c

   159	
   160	static u16 virtio_rpmsg_transport16_to_cpu(struct rpmsg_device *rpdev, u16 val)
   161	{
   162		struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
   163		struct virtproc_info *vrp = vch->vrp;
   164	
 > 165		return virtio16_to_cpu(vrp->vdev, val);
   166	}
   167	
   168	static u16 virtio_rpmsg_cpu_to_transport16(struct rpmsg_device *rpdev, u16 val)
   169	{
   170		struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
   171		struct virtproc_info *vrp = vch->vrp;
   172	
 > 173		return cpu_to_virtio16(vrp->vdev, val);
   174	}
   175	
   176	static u32 virtio_rpmsg_transport32_to_cpu(struct rpmsg_device *rpdev, u32 val)
   177	{
   178		struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
   179		struct virtproc_info *vrp = vch->vrp;
   180	
 > 181		return virtio32_to_cpu(vrp->vdev, val);
   182	}
   183	
   184	static u32 virtio_rpmsg_cpu_to_transport32(struct rpmsg_device *rpdev, u32 val)
   185	{
   186		struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
   187		struct virtproc_info *vrp = vch->vrp;
   188	
 > 189		return cpu_to_virtio32(vrp->vdev, val);
   190	}
   191	

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

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

Re: [PATCH 06/10] rpmsg: Turn name service into a stand alone driver

[kernel test robot]

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

Hi Mathieu,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
base:    b10b8ad862118bf42c28a98b0f067619aadcfb23
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

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

All errors (new ones prefixed by >>):

>> drivers/rpmsg/mtk_rpmsg.c:45:8: error: redefinition of 'struct rpmsg_ns_msg'
      45 | struct rpmsg_ns_msg {
         |        ^~~~~~~~~~~~
   In file included from drivers/rpmsg/rpmsg_internal.h:18,
                    from drivers/rpmsg/mtk_rpmsg.c:14:
   include/linux/rpmsg_ns.h:42:8: note: originally defined here
      42 | struct rpmsg_ns_msg {
         |        ^~~~~~~~~~~~

# https://github.com/0day-ci/linux/commit/16d58dca9b736143347676e7d7f0aabbf8e746c0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
git checkout 16d58dca9b736143347676e7d7f0aabbf8e746c0
vim +45 drivers/rpmsg/mtk_rpmsg.c

7017996951fde84 Pi-Hsun Shih 2019-11-12  34  
7017996951fde84 Pi-Hsun Shih 2019-11-12  35  /**
7017996951fde84 Pi-Hsun Shih 2019-11-12  36   * struct rpmsg_ns_msg - dynamic name service announcement message
7017996951fde84 Pi-Hsun Shih 2019-11-12  37   * @name: name of remote service that is published
7017996951fde84 Pi-Hsun Shih 2019-11-12  38   * @addr: address of remote service that is published
7017996951fde84 Pi-Hsun Shih 2019-11-12  39   *
7017996951fde84 Pi-Hsun Shih 2019-11-12  40   * This message is sent across to publish a new service. When we receive these
7017996951fde84 Pi-Hsun Shih 2019-11-12  41   * messages, an appropriate rpmsg channel (i.e device) is created. In turn, the
7017996951fde84 Pi-Hsun Shih 2019-11-12  42   * ->probe() handler of the appropriate rpmsg driver will be invoked
7017996951fde84 Pi-Hsun Shih 2019-11-12  43   *  (if/as-soon-as one is registered).
7017996951fde84 Pi-Hsun Shih 2019-11-12  44   */
7017996951fde84 Pi-Hsun Shih 2019-11-12 @45  struct rpmsg_ns_msg {
7017996951fde84 Pi-Hsun Shih 2019-11-12  46  	char name[RPMSG_NAME_SIZE];
7017996951fde84 Pi-Hsun Shih 2019-11-12  47  	u32 addr;
7017996951fde84 Pi-Hsun Shih 2019-11-12  48  } __packed;
7017996951fde84 Pi-Hsun Shih 2019-11-12  49  

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

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

Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic

[kernel test robot]

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

Hi Mathieu,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
base:    b10b8ad862118bf42c28a98b0f067619aadcfb23
config: i386-randconfig-s001-20200921 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-201-g24bdaac6-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

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


sparse warnings: (new ones prefixed by >>)

   drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted __virtio16 [usertype] val @@     got unsigned short [usertype] val @@
   drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse:     expected restricted __virtio16 [usertype] val
   drivers/rpmsg/virtio_rpmsg_bus.c:165:43: sparse:     got unsigned short [usertype] val
   drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse: sparse: incorrect type in return expression (different base types) @@     expected unsigned short @@     got restricted __virtio16 @@
   drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse:     expected unsigned short
   drivers/rpmsg/virtio_rpmsg_bus.c:173:31: sparse:     got restricted __virtio16
   drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted __virtio32 [usertype] val @@     got unsigned int [usertype] val @@
   drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse:     expected restricted __virtio32 [usertype] val
   drivers/rpmsg/virtio_rpmsg_bus.c:181:43: sparse:     got unsigned int [usertype] val
   drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse: sparse: incorrect type in return expression (different base types) @@     expected unsigned int @@     got restricted __virtio32 @@
   drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse:     expected unsigned int
   drivers/rpmsg/virtio_rpmsg_bus.c:189:31: sparse:     got restricted __virtio32
>> drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] addr @@     got restricted __virtio32 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse:     expected unsigned int [addressable] [usertype] addr
   drivers/rpmsg/virtio_rpmsg_bus.c:267:26: sparse:     got restricted __virtio32
>> drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] flags @@     got restricted __virtio32 @@
>> drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse:     expected unsigned int [addressable] [usertype] flags
   drivers/rpmsg/virtio_rpmsg_bus.c:268:27: sparse:     got restricted __virtio32
   drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] addr @@     got restricted __virtio32 @@
   drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse:     expected unsigned int [addressable] [usertype] addr
   drivers/rpmsg/virtio_rpmsg_bus.c:291:26: sparse:     got restricted __virtio32
   drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] flags @@     got restricted __virtio32 @@
   drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse:     expected unsigned int [addressable] [usertype] flags
   drivers/rpmsg/virtio_rpmsg_bus.c:292:27: sparse:     got restricted __virtio32

# https://github.com/0day-ci/linux/commit/ab159ea48198df2ab06ff9fe97e63cca354bff20
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
git checkout ab159ea48198df2ab06ff9fe97e63cca354bff20
vim +267 drivers/rpmsg/virtio_rpmsg_bus.c

dd032e0b67fcd61 Mathieu Poirier       2020-09-21  167  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  168  static u16 virtio_rpmsg_cpu_to_transport16(struct rpmsg_device *rpdev, u16 val)
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  169  {
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  170  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  171  	struct virtproc_info *vrp = vch->vrp;
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  172  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21 @173  	return cpu_to_virtio16(vrp->vdev, val);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  174  }
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  175  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  176  static u32 virtio_rpmsg_transport32_to_cpu(struct rpmsg_device *rpdev, u32 val)
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  177  {
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  178  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  179  	struct virtproc_info *vrp = vch->vrp;
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  180  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21 @181  	return virtio32_to_cpu(vrp->vdev, val);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  182  }
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  183  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  184  static u32 virtio_rpmsg_cpu_to_transport32(struct rpmsg_device *rpdev, u32 val)
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  185  {
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  186  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  187  	struct virtproc_info *vrp = vch->vrp;
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  188  
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  189  	return cpu_to_virtio32(vrp->vdev, val);
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  190  }
dd032e0b67fcd61 Mathieu Poirier       2020-09-21  191  
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  192  static struct rpmsg_device *
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  193  virtio_rpmsg_create_channel(struct rpmsg_device *rpdev,
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  194  			    struct rpmsg_channel_info *chinfo)
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  195  {
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  196  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  197  	struct virtproc_info *vrp = vch->vrp;
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  198  
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  199  	return __rpmsg_create_channel(vrp, chinfo);
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  200  }
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  201  
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  202  static int virtio_rpmsg_release_channel(struct rpmsg_device *rpdev,
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  203  					struct rpmsg_channel_info *chinfo)
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  204  {
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  205  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  206  	struct virtproc_info *vrp = vch->vrp;
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  207  
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  208  	return rpmsg_unregister_device(&vrp->vdev->dev, chinfo);
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  209  }
644f6e9ac5ebdd8 Arnaud Pouliquen      2020-09-21  210  
36b72c7dca71871 Bjorn Andersson       2016-09-01  211  static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
36b72c7dca71871 Bjorn Andersson       2016-09-01  212  						      rpmsg_rx_cb_t cb,
36b72c7dca71871 Bjorn Andersson       2016-09-01  213  						      void *priv,
36b72c7dca71871 Bjorn Andersson       2016-09-01  214  						      struct rpmsg_channel_info chinfo)
36b72c7dca71871 Bjorn Andersson       2016-09-01  215  {
3bf950ff23337fc Bjorn Andersson       2016-09-01  216  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
3bf950ff23337fc Bjorn Andersson       2016-09-01  217  
3bf950ff23337fc Bjorn Andersson       2016-09-01  218  	return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
36b72c7dca71871 Bjorn Andersson       2016-09-01  219  }
36b72c7dca71871 Bjorn Andersson       2016-09-01  220  
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  221  /**
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  222   * __rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  223   * @vrp: virtproc which owns this ept
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  224   * @ept: endpoing to destroy
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  225   *
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  226   * An internal function which destroy an ept without assuming it is
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  227   * bound to an rpmsg channel. This is needed for handling the internal
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  228   * name service endpoint, which isn't bound to an rpmsg channel.
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  229   * See also __rpmsg_create_ept().
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  230   */
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  231  static void
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  232  __rpmsg_destroy_ept(struct virtproc_info *vrp, struct rpmsg_endpoint *ept)
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  233  {
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  234  	/* make sure new inbound messages can't find this ept anymore */
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  235  	mutex_lock(&vrp->endpoints_lock);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  236  	idr_remove(&vrp->endpoints, ept->addr);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  237  	mutex_unlock(&vrp->endpoints_lock);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  238  
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  239  	/* make sure in-flight inbound messages won't invoke cb anymore */
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  240  	mutex_lock(&ept->cb_lock);
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  241  	ept->cb = NULL;
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  242  	mutex_unlock(&ept->cb_lock);
15fd943af50dbc5 Ohad Ben-Cohen        2012-06-07  243  
5a081caa0414b9b Ohad Ben-Cohen        2012-06-06  244  	kref_put(&ept->refcount, __ept_release);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  245  }
fa2d7795b2e8595 Ohad Ben-Cohen        2012-02-09  246  
8a228ecfe086b84 Bjorn Andersson       2016-09-01  247  static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
8a228ecfe086b84 Bjorn Andersson       2016-09-01  248  {
3bf950ff23337fc Bjorn Andersson       2016-09-01  249  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(ept->rpdev);
3bf950ff23337fc Bjorn Andersson       2016-09-01  250  
3bf950ff23337fc Bjorn Andersson       2016-09-01  251  	__rpmsg_destroy_ept(vch->vrp, ept);
8a228ecfe086b84 Bjorn Andersson       2016-09-01  252  }
8a228ecfe086b84 Bjorn Andersson       2016-09-01  253  
36b72c7dca71871 Bjorn Andersson       2016-09-01  254  static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
36b72c7dca71871 Bjorn Andersson       2016-09-01  255  {
3bf950ff23337fc Bjorn Andersson       2016-09-01  256  	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
3bf950ff23337fc Bjorn Andersson       2016-09-01  257  	struct virtproc_info *vrp = vch->vrp;
36b72c7dca71871 Bjorn Andersson       2016-09-01  258  	struct device *dev = &rpdev->dev;
36b72c7dca71871 Bjorn Andersson       2016-09-01  259  	int err = 0;
36b72c7dca71871 Bjorn Andersson       2016-09-01  260  
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  261  	/* need to tell remote processor's name service about this channel ? */
b2599ebffb2d32e Henri Roosen          2017-06-02  262  	if (rpdev->announce && rpdev->ept &&
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  263  	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  264  		struct rpmsg_ns_msg nsm;
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  265  
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  266  		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
111d1089700cdb7 Guennadi Liakhovetski 2020-07-21 @267  		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
111d1089700cdb7 Guennadi Liakhovetski 2020-07-21 @268  		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  269  
2a48d7322dc88f1 Bjorn Andersson       2016-09-01  270  		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  271  		if (err)
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  272  			dev_err(dev, "failed to announce service %d\n", err);
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  273  	}
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  274  
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  275  	return err;
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  276  }
bcabbccabffe732 Ohad Ben-Cohen        2011-10-20  277  

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

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

Re: [PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Dan Carpenter]

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

Hi Mathieu,

url:    https://github.com/0day-ci/linux/commits/Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
base:    b10b8ad862118bf42c28a98b0f067619aadcfb23
config: i386-randconfig-m021-20200923 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

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

smatch warnings:
drivers/rpmsg/rpmsg_core.c:33 rpmsg16_to_cpu() warn: signedness bug returning '(-22)'
drivers/rpmsg/rpmsg_core.c:44 cpu_to_rpmsg16() warn: signedness bug returning '(-22)'

# https://github.com/0day-ci/linux/commit/547ad00c50065bf914ac4090882d0ac692f5452d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/rpmsg-Make-RPMSG-name-service-modular/20200922-081745
git checkout 547ad00c50065bf914ac4090882d0ac692f5452d
vim +33 drivers/rpmsg/rpmsg_core.c

547ad00c50065bf Mathieu Poirier 2020-09-21  30  u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)
                                                ^^^

547ad00c50065bf Mathieu Poirier 2020-09-21  31  {
547ad00c50065bf Mathieu Poirier 2020-09-21  32  	if (WARN_ON(!rpdev))
547ad00c50065bf Mathieu Poirier 2020-09-21 @33  		return -EINVAL;
                                                                ^^^^^^^^^^^^^^
All the negative returns get truncated to a high u16 value.

547ad00c50065bf Mathieu Poirier 2020-09-21  34  	if (!rpdev->ops || !rpdev->ops->transport16_to_cpu)
547ad00c50065bf Mathieu Poirier 2020-09-21  35  		return -EPERM;
                                                                ^^^^^^^^^^^^^^
547ad00c50065bf Mathieu Poirier 2020-09-21  36  
547ad00c50065bf Mathieu Poirier 2020-09-21  37  	return rpdev->ops->transport16_to_cpu(rpdev, val);
547ad00c50065bf Mathieu Poirier 2020-09-21  38  }
547ad00c50065bf Mathieu Poirier 2020-09-21  39  EXPORT_SYMBOL(rpmsg16_to_cpu);
547ad00c50065bf Mathieu Poirier 2020-09-21  40  
547ad00c50065bf Mathieu Poirier 2020-09-21  41  u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
547ad00c50065bf Mathieu Poirier 2020-09-21  42  {
547ad00c50065bf Mathieu Poirier 2020-09-21  43  	if (WARN_ON(!rpdev))
547ad00c50065bf Mathieu Poirier 2020-09-21 @44  		return -EINVAL;
                                                                ^^^^^^^^^^^^^^
547ad00c50065bf Mathieu Poirier 2020-09-21  45  	if (!rpdev->ops || !rpdev->ops->cpu_to_transport16)
547ad00c50065bf Mathieu Poirier 2020-09-21  46  		return -EPERM;
                                                                ^^^^^^^^^^^^^
547ad00c50065bf Mathieu Poirier 2020-09-21  47  
547ad00c50065bf Mathieu Poirier 2020-09-21  48  	return rpdev->ops->cpu_to_transport16(rpdev, val);
547ad00c50065bf Mathieu Poirier 2020-09-21  49  }

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

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

Re: [PATCH 00/10] rpmsg: Make RPMSG name service modular

[Guennadi Liakhovetski]

Hi Mathieu,

Sorry for a delayed response, after I'd sent that my message I've 
subscribed to remoteproc and it seems during that transition some 
messages were only delivered from the list and not directly to me 
or something similar has happened.

On Tue, Sep 22, 2020 at 01:12:41PM -0600, Mathieu Poirier wrote:
> Good day Guennadi,
> 
> On Tue, 22 Sep 2020 at 02:09, Guennadi Liakhovetski
> <guennadi.liakhovetski@linux.intel.com> wrote:
> >
> > Hi Mathieu,
> >
> > Thanks for the patches. I'm trying to understand the concept of
> > this approach and I'm probably failing at that. It seems to me
> > that this patch set is making the NS announcement service to a
> > separate RPMsg device and I don't understand the reasoning for
> > doing this. As far as I understand namespace announcements
> > belong to RPMsg devices / channels, they create a dedicated
> > endpoint on them with a fixed pre-defined address. But they
> > don't form a separate RPMsg device. I think the current
> > virtio_rpmsg_bus.c has that correctly: for each rpmsg device /
> > channel multiple endpoints can be created, where the NS
> > service is one of them. It's just an endpoing of an rpmsg
> > device, not a complete separate device. Have I misunderstood
> > anything?
> 
> This patchset does not introduce any new features - the end result in
> terms of functionality is exactly the same.  It is also a carbon copy
> of the work introduced by Arnaud (hence reusing his patches), with the
> exception that the code is presented in a slightly different order to
> allow for a complete dissociation of RPMSG name service from the
> virtIO transport mechanic.
> 
> To make that happen rpmsg device specific byte conversion operations
> had to be introduced in struct rpmsg_device_ops and the explicit
> creation of an rpmsg_device associated with the name service (that
> wasn't needed when name service was welded to virtIO).  But
> associating a rpmsg_device to the name service doesn't change anything
> - RPMSG devices are created the same way when name service messages
> are received from the host or the remote processor.

Yes, the current rpmsg-virtio code does create *one* rpmsg device when 
an NS announcement arrives. Whereas with this patch set the first rpmsg 
device would be created to probe the NS service driver and the next one 
would still be created following the code borrowed from rpmsg-virtio 
when an NS announcement arrives. And I don't see how those two devices 
now make sense, sorry. I understand one device per channel, but two, of 
which one is for a certain endpoing only, whereas other endpoints don't 
create their devices, don't seem very logical to me.

Thanks
Guennadi

> To prove my theory I ran the rpmsg_client_sample.c and it just worked,
> no changes to client code needed.
> 
> Let's keep talking, it's the only way we'll get through this.
> 
> Mathieu
> 
> >
> > Thanks
> > Guennadi
> >
> > On Mon, Sep 21, 2020 at 06:09:50PM -0600, Mathieu Poirier wrote:
> > > Hi all,
> > >
> > > After looking at Guennadi[1] and Arnaud's patchsets[2] it became
> > > clear that we need to go back to a generic rpmsg_ns_msg structure
> > > if we wanted to make progress.  To do that some of the work from
> > > Arnaud had to be modified in a way that common name service
> > > functionality was transport agnostic.
> > >
> > > This patchset is based on Arnaud's work but also include a patch
> > > from Guennadi and some input from me.  It should serve as a
> > > foundation for the next revision of [1].
> > >
> > > Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
> > > did not test the modularisation.
> > >
> > > Comments and feedback would be greatly appreciated.
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=346593
> > > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> > >
> > > Arnaud Pouliquen (5):
> > >   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
> > >   rpmsg: virtio: use rpmsg ns device for the ns announcement
> > >
> > > Guennadi Liakhovetski (1):
> > >   rpmsg: Move common structures and defines to headers
> > >
> > > Mathieu Poirier (4):
> > >   rpmsg: virtio: Move virtio RPMSG structures to private header
> > >   rpmsg: core: Add RPMSG byte conversion operations
> > >   rpmsg: virtio: Make endianness conversion virtIO specific
> > >   rpmsg: ns: Make Name service module transport agnostic
> > >
> > >  drivers/rpmsg/Kconfig            |   9 +
> > >  drivers/rpmsg/Makefile           |   1 +
> > >  drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
> > >  drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
> > >  drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
> > >  drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
> > >  include/linux/rpmsg_ns.h         |  83 +++++++++
> > >  include/uapi/linux/rpmsg.h       |   3 +
> > >  8 files changed, 487 insertions(+), 199 deletions(-)
> > >  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> > >  create mode 100644 include/linux/rpmsg_ns.h
> > >
> > > --
> > > 2.25.1
> > >

Re: [PATCH 00/10] rpmsg: Make RPMSG name service modular

[Mathieu Poirier]

On Thu, Sep 24, 2020 at 08:53:56AM +0200, Guennadi Liakhovetski wrote:
> Hi Mathieu,
> 
> Sorry for a delayed response, after I'd sent that my message I've 
> subscribed to remoteproc and it seems during that transition some 
> messages were only delivered from the list and not directly to me 
> or something similar has happened.
>

Ok
 
> On Tue, Sep 22, 2020 at 01:12:41PM -0600, Mathieu Poirier wrote:
> > Good day Guennadi,
> > 
> > On Tue, 22 Sep 2020 at 02:09, Guennadi Liakhovetski
> > <guennadi.liakhovetski@linux.intel.com> wrote:
> > >
> > > Hi Mathieu,
> > >
> > > Thanks for the patches. I'm trying to understand the concept of
> > > this approach and I'm probably failing at that. It seems to me
> > > that this patch set is making the NS announcement service to a
> > > separate RPMsg device and I don't understand the reasoning for
> > > doing this. As far as I understand namespace announcements
> > > belong to RPMsg devices / channels, they create a dedicated
> > > endpoint on them with a fixed pre-defined address. But they
> > > don't form a separate RPMsg device. I think the current
> > > virtio_rpmsg_bus.c has that correctly: for each rpmsg device /
> > > channel multiple endpoints can be created, where the NS
> > > service is one of them. It's just an endpoing of an rpmsg
> > > device, not a complete separate device. Have I misunderstood
> > > anything?
> > 
> > This patchset does not introduce any new features - the end result in
> > terms of functionality is exactly the same.  It is also a carbon copy
> > of the work introduced by Arnaud (hence reusing his patches), with the
> > exception that the code is presented in a slightly different order to
> > allow for a complete dissociation of RPMSG name service from the
> > virtIO transport mechanic.
> > 
> > To make that happen rpmsg device specific byte conversion operations
> > had to be introduced in struct rpmsg_device_ops and the explicit
> > creation of an rpmsg_device associated with the name service (that
> > wasn't needed when name service was welded to virtIO).  But
> > associating a rpmsg_device to the name service doesn't change anything
> > - RPMSG devices are created the same way when name service messages
> > are received from the host or the remote processor.
> 
> Yes, the current rpmsg-virtio code does create *one* rpmsg device when 
> an NS announcement arrives.

Currently an rpmsg_device is created each time a NS announcement is received.  

> Whereas with this patch set the first rpmsg 
> device would be created to probe the NS service driver and the next one 
> would still be created following the code borrowed from rpmsg-virtio 
> when an NS announcement arrives. And I don't see how those two devices 
> now make sense, sorry. I understand one device per channel, but two, of 
> which one is for a certain endpoing only, whereas other endpoints don't 
> create their devices, don't seem very logical to me.

In the current implementation the NS service channel is created automatically
when instantiating an rproc_vdev.  An official rpmsg_device is not needed since
it is implicit.  With this set (and as you noted above) an rpmsg_device to
represent the NS service is registered, the same way other services such as
rpmsg_chrdev are.  After that nothing else changes and no other rpmgs_device
are created until NS request come in.  When an NS request does come in an
rpmsg_device is created, and that for each request that is received.

> 
> Thanks
> Guennadi
> 
> > To prove my theory I ran the rpmsg_client_sample.c and it just worked,
> > no changes to client code needed.
> > 
> > Let's keep talking, it's the only way we'll get through this.
> > 
> > Mathieu
> > 
> > >
> > > Thanks
> > > Guennadi
> > >
> > > On Mon, Sep 21, 2020 at 06:09:50PM -0600, Mathieu Poirier wrote:
> > > > Hi all,
> > > >
> > > > After looking at Guennadi[1] and Arnaud's patchsets[2] it became
> > > > clear that we need to go back to a generic rpmsg_ns_msg structure
> > > > if we wanted to make progress.  To do that some of the work from
> > > > Arnaud had to be modified in a way that common name service
> > > > functionality was transport agnostic.
> > > >
> > > > This patchset is based on Arnaud's work but also include a patch
> > > > from Guennadi and some input from me.  It should serve as a
> > > > foundation for the next revision of [1].
> > > >
> > > > Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
> > > > did not test the modularisation.
> > > >
> > > > Comments and feedback would be greatly appreciated.
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=346593
> > > > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> > > >
> > > > Arnaud Pouliquen (5):
> > > >   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
> > > >   rpmsg: virtio: use rpmsg ns device for the ns announcement
> > > >
> > > > Guennadi Liakhovetski (1):
> > > >   rpmsg: Move common structures and defines to headers
> > > >
> > > > Mathieu Poirier (4):
> > > >   rpmsg: virtio: Move virtio RPMSG structures to private header
> > > >   rpmsg: core: Add RPMSG byte conversion operations
> > > >   rpmsg: virtio: Make endianness conversion virtIO specific
> > > >   rpmsg: ns: Make Name service module transport agnostic
> > > >
> > > >  drivers/rpmsg/Kconfig            |   9 +
> > > >  drivers/rpmsg/Makefile           |   1 +
> > > >  drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
> > > >  drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
> > > >  drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
> > > >  drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
> > > >  include/linux/rpmsg_ns.h         |  83 +++++++++
> > > >  include/uapi/linux/rpmsg.h       |   3 +
> > > >  8 files changed, 487 insertions(+), 199 deletions(-)
> > > >  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> > > >  create mode 100644 include/linux/rpmsg_ns.h
> > > >
> > > > --
> > > > 2.25.1
> > > >

Re: [PATCH 02/10] rpmsg: core: Add channel creation internal API

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
> 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>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 91de940896e3..50a835eaf1ba 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,51 @@
>  
>  #include "rpmsg_internal.h"
>  
> +/**
> + * rpmsg_create_channel() - create a new rpmsg channel
> + * using its name and address info.
> + * @rpdev: rpmsg driver

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,

You might call this nitpicking, but we already have two indentation styles for 
functions:

return_type function(type1 arg1,
			...)

(my personal preference, it also has sub-variants - depending on the aligning 
of the second line and any following lines, and one more moving "type1 arg1" 
to the second line)

return_type
function(...

and now you're also introducing the third style - with "function" indented... 
Maybe we don't need more of those, particularly since now with 100 chars per 
line in most cases the very first style can be used.

> +			     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 driver

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 -EPERM;

ENOSYS or EOPNOTSUPP? I'm never sure which one is appropriate for 
this kind of errors.

> +	}
> +
> +	return rpdev->ops->release_channel(rpdev, chinfo);
> +}
> +EXPORT_SYMBOL(rpmsg_release_channel);
> +
>  /**
>   * rpmsg_create_ept() - create a new rpmsg_endpoint
>   * @rpdev: rpmsg channel device
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3fc83cd50e98..587f723757d4 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -20,6 +20,8 @@
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @create_channel:	create backend-specific channel, optional
> + * @release_channel:	release backend-specific channel, optional

Are they really optional? You return errors if they aren't available.

>   * @create_ept:		create backend-specific endpoint, required
>   * @announce_create:	announce presence of new channel, optional
>   * @announce_destroy:	announce destruction of channel, optional
> @@ -29,6 +31,11 @@
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
> +					     struct rpmsg_channel_info *chinfo);
> +	int (*release_channel)(struct rpmsg_device *rpdev,
> +			       struct rpmsg_channel_info *chinfo);
> +
>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>  					    rpmsg_rx_cb_t cb, void *priv,
>  					    struct rpmsg_channel_info chinfo);
> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>  struct device *rpmsg_find_device(struct device *parent,
>  				 struct rpmsg_channel_info *chinfo);
>  
> +struct rpmsg_device *
> +rpmsg_create_channel(struct rpmsg_device *rpdev,
> +		     struct rpmsg_channel_info *chinfo);
> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
> +			  struct rpmsg_channel_info *chinfo);
>  /**
>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>   * @rpdev:	prepared rpdev to be used for creating endpoints
> -- 
> 2.25.1
> 

Re: [PATCH 04/10] rpmsg: Move common structures and defines to headers

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:54PM -0600, Mathieu Poirier wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> 
> virtio_rpmsg_bus.c keeps RPMsg protocol structure declarations and
> common defines like the ones, needed for name-space announcements,
> internal. Move them to common headers instead.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> [Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]

No, this isn't only for the name-service, the message header is common 
for all RPMsg messages.

Thanks
Guennadi

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
>  include/linux/rpmsg_ns.h         | 83 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/rpmsg.h       |  3 ++
>  3 files changed, 88 insertions(+), 76 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 e87cf0b79542..eaf3b2c012c8 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,6 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg_ns.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> @@ -27,6 +28,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 +72,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 {
> -	__virtio32 src;
> -	__virtio32 dst;
> -	__virtio32 reserved;
> -	__virtio16 len;
> -	__virtio16 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];
> -	__virtio32 addr;
> -	__virtio32 flags;
> -} __packed;
> -
> -/**
> - * enum rpmsg_ns_flags - dynamic name service announcement flags
> - *
> - * @RPMSG_NS_CREATE: a new remote service was just created
> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> - */
> -enum rpmsg_ns_flags {
> -	RPMSG_NS_CREATE		= 0,
> -	RPMSG_NS_DESTROY	= 1,
> -};
> -
>  /**
>   * struct virtio_rpmsg_channel - rpmsg channel descriptor
>   * @rpdev: the rpmsg channel device
> @@ -139,27 +89,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>  	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>  
> -/*
> - * We're allocating buffers of 512 bytes each for communications. The
> - * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> - *
> - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> - * the payload.
> - *
> - * This will utilize a maximum total space of 256KB for the buffers.
> - *
> - * We might also want to add support for user-provided buffers in time.
> - * This will allow bigger buffer size flexibility, and can also be used
> - * to achieve zero-copy messaging.
> - *
> - * Note that these numbers are purely a decision of this driver - we
> - * can change this without changing anything in the firmware of the remote
> - * processor.
> - */
> -#define MAX_RPMSG_NUM_BUFS	(512)
> -#define MAX_RPMSG_BUF_SIZE	(512)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> @@ -167,9 +96,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..aabc6c3c0d6d
> --- /dev/null
> +++ b/include/linux/rpmsg_ns.h
> @@ -0,0 +1,83 @@
> +/* 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/virtio_types.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 {
> +	__virtio32 src;
> +	__virtio32 dst;
> +	__virtio32 reserved;
> +	__virtio16 len;
> +	__virtio16 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];
> +	__virtio32 addr;
> +	__virtio32 flags;
> +} __packed;
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> +	RPMSG_NS_CREATE		= 0,
> +	RPMSG_NS_DESTROY	= 1,
> +};
> +
> +/*
> + * We're allocating buffers of 512 bytes each for communications. The
> + * number of buffers will be computed from the number of buffers supported
> + * by the vring, upto a maximum of 512 buffers (256 in each direction).
> + *
> + * Each buffer will have 16 bytes for the msg header and 496 bytes for
> + * the payload.
> + *
> + * This will utilize a maximum total space of 256KB for the buffers.
> + *
> + * We might also want to add support for user-provided buffers in time.
> + * This will allow bigger buffer size flexibility, and can also be used
> + * to achieve zero-copy messaging.
> + *
> + * Note that these numbers are purely a decision of this driver - we
> + * can change this without changing anything in the firmware of the remote
> + * processor.
> + */
> +#define MAX_RPMSG_NUM_BUFS	512
> +#define MAX_RPMSG_BUF_SIZE	512
> +
> +/* 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
> 

Re: [PATCH 05/10] rpmsg: virtio: Move virtio RPMSG structures to private header

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:55PM -0600, Mathieu Poirier wrote:
> Move structure virtiproc_info and virtio_rpmsg_channel to rpmsg_internal.h
> so that they can be used by rpmsg_ns.c
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_internal.h   | 62 ++++++++++++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 57 -----------------------------
>  2 files changed, 62 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 587f723757d4..3ea9cec26fc0 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -12,12 +12,74 @@
>  #ifndef __RPMSG_INTERNAL_H__
>  #define __RPMSG_INTERNAL_H__
>  
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
>  #include <linux/rpmsg.h>
> +#include <linux/types.h>
> +#include <linux/virtio.h>

I think it would be better to not add any VirtIO dependencies here even 
temporarily.

> +#include <linux/wait.h>
>  #include <linux/poll.h>
>  
>  #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 virtproc_info - virtual remote processor state

This struct shouldn't be here, let's first prepare the NS callback by 
removing any VirtIO dependencies and only then move it to the generic 
driver.

Thanks
Guennadi

> + * @vdev:	the virtio device
> + * @rvq:	rx virtqueue
> + * @svq:	tx virtqueue
> + * @rbufs:	kernel address of rx buffers
> + * @sbufs:	kernel address of tx buffers
> + * @num_bufs:	total number of buffers for rx and tx
> + * @buf_size:   size of one rx or tx buffer
> + * @last_sbuf:	index of last tx buffer used
> + * @bufs_dma:	dma base addr of the buffers
> + * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> + *		sending a message might require waking up a dozing remote
> + *		processor, which involves sleeping, hence the mutex.
> + * @endpoints:	idr of local endpoints, allows fast retrieval
> + * @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
> + * remote processor).
> + */
> +struct virtproc_info {
> +	struct virtio_device *vdev;
> +	struct virtqueue *rvq, *svq;
> +	void *rbufs, *sbufs;
> +	unsigned int num_bufs;
> +	unsigned int buf_size;
> +	int last_sbuf;
> +	dma_addr_t bufs_dma;
> +	struct mutex tx_lock;
> +	struct idr endpoints;
> +	struct mutex endpoints_lock;
> +	wait_queue_head_t sendq;
> +	atomic_t sleepers;
> +	struct rpmsg_endpoint *ns_ept;
> +};
> +
> +/**
> + * struct virtio_rpmsg_channel - rpmsg channel descriptor
> + * @rpdev: the rpmsg channel device
> + * @vrp: the virtio remote processor device this channel belongs to
> + *
> + * This structure stores the channel that links the rpmsg device to the virtio
> + * remote processor device.
> + */
> +struct virtio_rpmsg_channel {
> +	struct rpmsg_device rpdev;
> +
> +	struct virtproc_info *vrp;
> +};
> +
> +#define to_virtio_rpmsg_channel(_rpdev) \
> +	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> +
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>   * @create_channel:	create backend-specific channel, optional
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index eaf3b2c012c8..0635d86d490f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -32,63 +32,6 @@
>  
>  #include "rpmsg_internal.h"
>  
> -/**
> - * struct virtproc_info - virtual remote processor state
> - * @vdev:	the virtio device
> - * @rvq:	rx virtqueue
> - * @svq:	tx virtqueue
> - * @rbufs:	kernel address of rx buffers
> - * @sbufs:	kernel address of tx buffers
> - * @num_bufs:	total number of buffers for rx and tx
> - * @buf_size:   size of one rx or tx buffer
> - * @last_sbuf:	index of last tx buffer used
> - * @bufs_dma:	dma base addr of the buffers
> - * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> - *		sending a message might require waking up a dozing remote
> - *		processor, which involves sleeping, hence the mutex.
> - * @endpoints:	idr of local endpoints, allows fast retrieval
> - * @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
> - * remote processor).
> - */
> -struct virtproc_info {
> -	struct virtio_device *vdev;
> -	struct virtqueue *rvq, *svq;
> -	void *rbufs, *sbufs;
> -	unsigned int num_bufs;
> -	unsigned int buf_size;
> -	int last_sbuf;
> -	dma_addr_t bufs_dma;
> -	struct mutex tx_lock;
> -	struct idr endpoints;
> -	struct mutex endpoints_lock;
> -	wait_queue_head_t sendq;
> -	atomic_t sleepers;
> -	struct rpmsg_endpoint *ns_ept;
> -};
> -
> -/**
> - * struct virtio_rpmsg_channel - rpmsg channel descriptor
> - * @rpdev: the rpmsg channel device
> - * @vrp: the virtio remote processor device this channel belongs to
> - *
> - * This structure stores the channel that links the rpmsg device to the virtio
> - * remote processor device.
> - */
> -struct virtio_rpmsg_channel {
> -	struct rpmsg_device rpdev;
> -
> -	struct virtproc_info *vrp;
> -};
> -
> -#define to_virtio_rpmsg_channel(_rpdev) \
> -	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> -- 
> 2.25.1
> 

Re: [PATCH 06/10] rpmsg: Turn name service into a stand alone driver

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:56PM -0600, Mathieu Poirier wrote:
> 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.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/Kconfig          |   8 +++
>  drivers/rpmsg/Makefile         |   1 +
>  drivers/rpmsg/rpmsg_internal.h |  18 ++++++
>  drivers/rpmsg/rpmsg_ns.c       | 110 +++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+)
>  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_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3ea9cec26fc0..04e6cb287e18 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -15,6 +15,7 @@
>  #include <linux/idr.h>
>  #include <linux/mutex.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg_ns.h>
>  #include <linux/types.h>
>  #include <linux/virtio.h>
>  #include <linux/wait.h>
> @@ -164,4 +165,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>  	return rpmsg_register_device(rpdev);
>  }
>  
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> +	strcpy(rpdev->id.name, "rpmsg_ns");
> +	rpdev->driver_override = "rpmsg_ns";
> +	rpdev->src = RPMSG_NS_ADDR;
> +	rpdev->dst = RPMSG_NS_ADDR;
> +
> +	return rpmsg_register_device(rpdev);
> +}
> +
>  #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index 000000000000..b3318bf84433
> --- /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_ns.h>
> +#include <linux/virtio_config.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 virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);

Let's not do this, non-VirtIO RPMsg drivers shouldn't need this struct.

> +	struct virtproc_info *vrp = vch->vrp;
> +	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;
> +	}
> +
> +	/* Don't trust the remote processor for null terminating the name */
> +	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> +
> +	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> +	chinfo.src = RPMSG_ADDR_ANY;
> +	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
> +
> +	dev_info(dev, "%sing channel %s addr 0x%x\n",
> +		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> +		 "destroy" : "creat", msg->name, chinfo.dst);
> +
> +	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> +		ret = rpmsg_release_channel(rpdev, &chinfo);
> +		if (ret)
> +			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> +	} else {
> +		newch = rpmsg_create_channel(rpdev, &chinfo);
> +		if (!newch)
> +			dev_err(dev, "rpmsg_create_channel failed\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_channel_info ns_chinfo;

+	struct rpmsg_channel_info ns_chinfo = {
+		.src = RPMSG_NS_ADDR,
+		.dst = RPMSG_NS_ADDR,
+		.name = "name_service",
+	};

Thanks
Guennadi

> +	struct rpmsg_endpoint *ns_ept;
> +
> +	ns_chinfo.src = RPMSG_NS_ADDR;
> +	ns_chinfo.dst = RPMSG_NS_ADDR;
> +	strcpy(ns_chinfo.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");
> -- 
> 2.25.1
> 

Re: [PATCH 08/10] rpmsg: core: Add RPMSG byte conversion operations

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:09:58PM -0600, Mathieu Poirier wrote:
> Add RPMSG device specific byte conversion operations as a first
> step to separate the RPMSG name space service from the virtIO
> transport layer.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 51 ++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h | 12 ++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 50a835eaf1ba..66ad5b5f1e87 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,57 @@
>  
>  #include "rpmsg_internal.h"
>  
> +/**
> + * rpmsg{16|32}_to_cpu()
> + * cpu_to_rpmsg[16|32}() - rpmsg device specific byte conversion functions to
> + *			   perform byte conversion between rpmsg device and the
> + *			   transport layer it is operating on.
> + */
> +
> +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val)

All the endianness conversions are great as long as they compile to 
NOPs where no conversion is needed. Can we come up with a solution 
to keep it that way?

Thanks
Guennadi

> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->transport16_to_cpu)
> +		return -EPERM;
> +
> +	return rpdev->ops->transport16_to_cpu(rpdev, val);
> +}
> +EXPORT_SYMBOL(rpmsg16_to_cpu);
> +
> +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport16)
> +		return -EPERM;
> +
> +	return rpdev->ops->cpu_to_transport16(rpdev, val);
> +}
> +EXPORT_SYMBOL(cpu_to_rpmsg16);
> +
> +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->transport32_to_cpu)
> +		return -EPERM;
> +
> +	return rpdev->ops->transport32_to_cpu(rpdev, val);
> +}
> +EXPORT_SYMBOL(rpmsg32_to_cpu);
> +
> +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->cpu_to_transport32)
> +		return -EPERM;
> +
> +	return rpdev->ops->cpu_to_transport32(rpdev, val);
> +}
> +EXPORT_SYMBOL(cpu_to_rpmsg32);
> +
>  /**
>   * rpmsg_create_channel() - create a new rpmsg channel
>   * using its name and address info.
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 2e65386f191e..2f0ad1a52698 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -81,6 +81,8 @@ struct virtio_rpmsg_channel {
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @transport{16|32}_to_cpu: byte conversion from rpmsg device to transport layer
> + * @cpu_to_transport{16|32}: byte conversion from transport layer to rpmsg device
>   * @create_channel:	create backend-specific channel, optional
>   * @release_channel:	release backend-specific channel, optional
>   * @create_ept:		create backend-specific endpoint, required
> @@ -92,6 +94,10 @@ struct virtio_rpmsg_channel {
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	u16 (*transport16_to_cpu)(struct rpmsg_device *rpdev, u16 val);
> +	u16 (*cpu_to_transport16)(struct rpmsg_device *rpdev, u16 val);
> +	u32 (*transport32_to_cpu)(struct rpmsg_device *rpdev, u32 val);
> +	u32 (*cpu_to_transport32)(struct rpmsg_device *rpdev, u32 val);
>  	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
>  					     struct rpmsg_channel_info *chinfo);
>  	int (*release_channel)(struct rpmsg_device *rpdev,
> @@ -148,6 +154,12 @@ 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);
> +
> +u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, u16 val);
> +u16 cpu_to_rpmsg16(struct rpmsg_device *rpdev, u16 val);
> +u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val);
> +u32 cpu_to_rpmsg32(struct rpmsg_device *rpdev, u32 val);
> +
>  /**
>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>   * @rpdev:	prepared rpdev to be used for creating endpoints
> -- 
> 2.25.1
> 

Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic

[Guennadi Liakhovetski]

On Mon, Sep 21, 2020 at 06:10:00PM -0600, Mathieu Poirier wrote:
> Make name service module transport agnostic by using the rpmsg
> device specific byte conversion routine.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_ns.c | 10 ++++------
>  include/linux/rpmsg_ns.h |  4 ++--
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index b3318bf84433..1df3aaadfe10 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -18,9 +18,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>  	struct rpmsg_ns_msg *msg = data;
>  	struct rpmsg_device *newch;
>  	struct rpmsg_channel_info chinfo;
> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> -	struct virtproc_info *vrp = vch->vrp;
> -	struct device *dev = &vrp->vdev->dev;
> +	struct device *dev = &rpdev->dev;
>  	int ret;
>  
>  #if defined(CONFIG_DYNAMIC_DEBUG)
> @@ -38,13 +36,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_release_channel(rpdev, &chinfo);
>  		if (ret)
>  			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> index aabc6c3c0d6d..9f3030b2145b 100644
> --- a/include/linux/rpmsg_ns.h
> +++ b/include/linux/rpmsg_ns.h
> @@ -41,8 +41,8 @@ struct rpmsg_hdr {
>   */
>  struct rpmsg_ns_msg {
>  	char name[RPMSG_NAME_SIZE];
> -	__virtio32 addr;
> -	__virtio32 flags;
> +	u32 addr;
> +	u32 flags;

These aren't (necessarily) native endianness, right? Don't they deserve a 
separate type? __rpmsg32?

Thanks
Guennadi

>  } __packed;
>  
>  /**
> -- 
> 2.25.1
> 

Re: [PATCH 02/10] rpmsg: core: Add channel creation internal API

[Arnaud POULIQUEN]

On 9/30/20 8:35 AM, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
>> 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>
>> ---
>>  drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 91de940896e3..50a835eaf1ba 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -20,6 +20,51 @@
>>  
>>  #include "rpmsg_internal.h"
>>  
>> +/**
>> + * rpmsg_create_channel() - create a new rpmsg channel
>> + * using its name and address info.
>> + * @rpdev: rpmsg driver
> 
> 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,
> 
> You might call this nitpicking, but we already have two indentation styles for 
> functions:
> 
> return_type function(type1 arg1,
> 			...)
> 
> (my personal preference, it also has sub-variants - depending on the aligning 
> of the second line and any following lines, and one more moving "type1 arg1" 
> to the second line)
> 
> return_type
> function(...
> 
> and now you're also introducing the third style - with "function" indented... 
> Maybe we don't need more of those, particularly since now with 100 chars per 
> line in most cases the very first style can be used.

Right, bad coding style.

> 
>> +			     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 driver
> 
> 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 -EPERM;
> 
> ENOSYS or EOPNOTSUPP? I'm never sure which one is appropriate for 
> this kind of errors.

For coherency with the other RPMsg API functions, could be 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 3fc83cd50e98..587f723757d4 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -20,6 +20,8 @@
>>  
>>  /**
>>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>> + * @create_channel:	create backend-specific channel, optional
>> + * @release_channel:	release backend-specific channel, optional
> 
> Are they really optional? You return errors if they aren't available.

I think they are optional, the back-end might not support the operation. 
In this case, activating an RPMsg client that uses the interface should result in an
error because the service is not compatible with the back-end implementation.

Regards,
Arnaud

> 
>>   * @create_ept:		create backend-specific endpoint, required
>>   * @announce_create:	announce presence of new channel, optional
>>   * @announce_destroy:	announce destruction of channel, optional
>> @@ -29,6 +31,11 @@
>>   * advertise new channels implicitly by creating the endpoints.
>>   */
>>  struct rpmsg_device_ops {
>> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
>> +					     struct rpmsg_channel_info *chinfo);
>> +	int (*release_channel)(struct rpmsg_device *rpdev,
>> +			       struct rpmsg_channel_info *chinfo);
>> +
>>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>>  					    rpmsg_rx_cb_t cb, void *priv,
>>  					    struct rpmsg_channel_info chinfo);
>> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>>  struct device *rpmsg_find_device(struct device *parent,
>>  				 struct rpmsg_channel_info *chinfo);
>>  
>> +struct rpmsg_device *
>> +rpmsg_create_channel(struct rpmsg_device *rpdev,
>> +		     struct rpmsg_channel_info *chinfo);
>> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
>> +			  struct rpmsg_channel_info *chinfo);
>>  /**
>>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>>   * @rpdev:	prepared rpdev to be used for creating endpoints
>> -- 
>> 2.25.1
>>

Re: [PATCH 06/10] rpmsg: Turn name service into a stand alone driver

[Arnaud POULIQUEN]

On 9/30/20 9:09 AM, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:56PM -0600, Mathieu Poirier wrote:
>> 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.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  drivers/rpmsg/Kconfig          |   8 +++
>>  drivers/rpmsg/Makefile         |   1 +
>>  drivers/rpmsg/rpmsg_internal.h |  18 ++++++
>>  drivers/rpmsg/rpmsg_ns.c       | 110 +++++++++++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+)
>>  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_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index 3ea9cec26fc0..04e6cb287e18 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/idr.h>
>>  #include <linux/mutex.h>
>>  #include <linux/rpmsg.h>
>> +#include <linux/rpmsg_ns.h>
>>  #include <linux/types.h>
>>  #include <linux/virtio.h>
>>  #include <linux/wait.h>
>> @@ -164,4 +165,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>>  	return rpmsg_register_device(rpdev);
>>  }
>>  
>> +/**
>> + * rpmsg_ns_register_device() - register name service device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> + * basis for the rpmsg name service device.
>> + */
>> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> +	strcpy(rpdev->id.name, "rpmsg_ns");
>> +	rpdev->driver_override = "rpmsg_ns";
>> +	rpdev->src = RPMSG_NS_ADDR;
>> +	rpdev->dst = RPMSG_NS_ADDR;
>> +
>> +	return rpmsg_register_device(rpdev);
>> +}
>> +
>>  #endif
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> new file mode 100644
>> index 000000000000..b3318bf84433
>> --- /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_ns.h>
>> +#include <linux/virtio_config.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 virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> 
> Let's not do this, non-VirtIO RPMsg drivers shouldn't need this struct.

Is this patch could be merged with the patch 10/10 to avoid this
temporay implementation?

Regards,
Arnaud

> 
>> +	struct virtproc_info *vrp = vch->vrp;
>> +	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;
>> +	}
>> +
>> +	/* Don't trust the remote processor for null terminating the name */
>> +	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
>> +
>> +	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
>> +	chinfo.src = RPMSG_ADDR_ANY;
>> +	chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
>> +
>> +	dev_info(dev, "%sing channel %s addr 0x%x\n",
>> +		 virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
>> +		 "destroy" : "creat", msg->name, chinfo.dst);
>> +
>> +	if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
>> +		ret = rpmsg_release_channel(rpdev, &chinfo);
>> +		if (ret)
>> +			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
>> +	} else {
>> +		newch = rpmsg_create_channel(rpdev, &chinfo);
>> +		if (!newch)
>> +			dev_err(dev, "rpmsg_create_channel failed\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_channel_info ns_chinfo;
> 
> +	struct rpmsg_channel_info ns_chinfo = {
> +		.src = RPMSG_NS_ADDR,
> +		.dst = RPMSG_NS_ADDR,
> +		.name = "name_service",
> +	};
> 
> Thanks
> Guennadi
> 
>> +	struct rpmsg_endpoint *ns_ept;
>> +
>> +	ns_chinfo.src = RPMSG_NS_ADDR;
>> +	ns_chinfo.dst = RPMSG_NS_ADDR;
>> +	strcpy(ns_chinfo.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");
>> -- 
>> 2.25.1
>>

Re: [PATCH 05/10] rpmsg: virtio: Move virtio RPMSG structures to private header

[Mathieu Poirier]

Hi Guennadi,

Apologies for the late reply, I've been away.

On Wed, Sep 30, 2020 at 09:03:45AM +0200, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:55PM -0600, Mathieu Poirier wrote:
> > Move structure virtiproc_info and virtio_rpmsg_channel to rpmsg_internal.h
> > so that they can be used by rpmsg_ns.c
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/rpmsg_internal.h   | 62 ++++++++++++++++++++++++++++++++
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 57 -----------------------------
> >  2 files changed, 62 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> > index 587f723757d4..3ea9cec26fc0 100644
> > --- a/drivers/rpmsg/rpmsg_internal.h
> > +++ b/drivers/rpmsg/rpmsg_internal.h
> > @@ -12,12 +12,74 @@
> >  #ifndef __RPMSG_INTERNAL_H__
> >  #define __RPMSG_INTERNAL_H__
> >  
> > +#include <linux/idr.h>
> > +#include <linux/mutex.h>
> >  #include <linux/rpmsg.h>
> > +#include <linux/types.h>
> > +#include <linux/virtio.h>
> 
> I think it would be better to not add any VirtIO dependencies here even 
> temporarily.
> 
> > +#include <linux/wait.h>
> >  #include <linux/poll.h>
> >  
> >  #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 virtproc_info - virtual remote processor state
> 
> This struct shouldn't be here, let's first prepare the NS callback by 
> removing any VirtIO dependencies and only then move it to the generic 
> driver.

I agree... I will proceed differently in the next revision.

> 
> Thanks
> Guennadi
> 
> > + * @vdev:	the virtio device
> > + * @rvq:	rx virtqueue
> > + * @svq:	tx virtqueue
> > + * @rbufs:	kernel address of rx buffers
> > + * @sbufs:	kernel address of tx buffers
> > + * @num_bufs:	total number of buffers for rx and tx
> > + * @buf_size:   size of one rx or tx buffer
> > + * @last_sbuf:	index of last tx buffer used
> > + * @bufs_dma:	dma base addr of the buffers
> > + * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> > + *		sending a message might require waking up a dozing remote
> > + *		processor, which involves sleeping, hence the mutex.
> > + * @endpoints:	idr of local endpoints, allows fast retrieval
> > + * @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
> > + * remote processor).
> > + */
> > +struct virtproc_info {
> > +	struct virtio_device *vdev;
> > +	struct virtqueue *rvq, *svq;
> > +	void *rbufs, *sbufs;
> > +	unsigned int num_bufs;
> > +	unsigned int buf_size;
> > +	int last_sbuf;
> > +	dma_addr_t bufs_dma;
> > +	struct mutex tx_lock;
> > +	struct idr endpoints;
> > +	struct mutex endpoints_lock;
> > +	wait_queue_head_t sendq;
> > +	atomic_t sleepers;
> > +	struct rpmsg_endpoint *ns_ept;
> > +};
> > +
> > +/**
> > + * struct virtio_rpmsg_channel - rpmsg channel descriptor
> > + * @rpdev: the rpmsg channel device
> > + * @vrp: the virtio remote processor device this channel belongs to
> > + *
> > + * This structure stores the channel that links the rpmsg device to the virtio
> > + * remote processor device.
> > + */
> > +struct virtio_rpmsg_channel {
> > +	struct rpmsg_device rpdev;
> > +
> > +	struct virtproc_info *vrp;
> > +};
> > +
> > +#define to_virtio_rpmsg_channel(_rpdev) \
> > +	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> > +
> >  /**
> >   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> >   * @create_channel:	create backend-specific channel, optional
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index eaf3b2c012c8..0635d86d490f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -32,63 +32,6 @@
> >  
> >  #include "rpmsg_internal.h"
> >  
> > -/**
> > - * struct virtproc_info - virtual remote processor state
> > - * @vdev:	the virtio device
> > - * @rvq:	rx virtqueue
> > - * @svq:	tx virtqueue
> > - * @rbufs:	kernel address of rx buffers
> > - * @sbufs:	kernel address of tx buffers
> > - * @num_bufs:	total number of buffers for rx and tx
> > - * @buf_size:   size of one rx or tx buffer
> > - * @last_sbuf:	index of last tx buffer used
> > - * @bufs_dma:	dma base addr of the buffers
> > - * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> > - *		sending a message might require waking up a dozing remote
> > - *		processor, which involves sleeping, hence the mutex.
> > - * @endpoints:	idr of local endpoints, allows fast retrieval
> > - * @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
> > - * remote processor).
> > - */
> > -struct virtproc_info {
> > -	struct virtio_device *vdev;
> > -	struct virtqueue *rvq, *svq;
> > -	void *rbufs, *sbufs;
> > -	unsigned int num_bufs;
> > -	unsigned int buf_size;
> > -	int last_sbuf;
> > -	dma_addr_t bufs_dma;
> > -	struct mutex tx_lock;
> > -	struct idr endpoints;
> > -	struct mutex endpoints_lock;
> > -	wait_queue_head_t sendq;
> > -	atomic_t sleepers;
> > -	struct rpmsg_endpoint *ns_ept;
> > -};
> > -
> > -/**
> > - * struct virtio_rpmsg_channel - rpmsg channel descriptor
> > - * @rpdev: the rpmsg channel device
> > - * @vrp: the virtio remote processor device this channel belongs to
> > - *
> > - * This structure stores the channel that links the rpmsg device to the virtio
> > - * remote processor device.
> > - */
> > -struct virtio_rpmsg_channel {
> > -	struct rpmsg_device rpdev;
> > -
> > -	struct virtproc_info *vrp;
> > -};
> > -
> > -#define to_virtio_rpmsg_channel(_rpdev) \
> > -	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> > -
> >  /*
> >   * Local addresses are dynamically allocated on-demand.
> >   * We do not dynamically assign addresses from the low 1024 range,
> > -- 
> > 2.25.1
> > 

Re: [PATCH 10/10] rpmsg: ns: Make Name service module transport agnostic

[Mathieu Poirier]

On Wed, Sep 30, 2020 at 09:13:27AM +0200, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:10:00PM -0600, Mathieu Poirier wrote:
> > Make name service module transport agnostic by using the rpmsg
> > device specific byte conversion routine.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/rpmsg/rpmsg_ns.c | 10 ++++------
> >  include/linux/rpmsg_ns.h |  4 ++--
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> > index b3318bf84433..1df3aaadfe10 100644
> > --- a/drivers/rpmsg/rpmsg_ns.c
> > +++ b/drivers/rpmsg/rpmsg_ns.c
> > @@ -18,9 +18,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >  	struct rpmsg_ns_msg *msg = data;
> >  	struct rpmsg_device *newch;
> >  	struct rpmsg_channel_info chinfo;
> > -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> > -	struct virtproc_info *vrp = vch->vrp;
> > -	struct device *dev = &vrp->vdev->dev;
> > +	struct device *dev = &rpdev->dev;
> >  	int ret;
> >  
> >  #if defined(CONFIG_DYNAMIC_DEBUG)
> > @@ -38,13 +36,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_release_channel(rpdev, &chinfo);
> >  		if (ret)
> >  			dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> > diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> > index aabc6c3c0d6d..9f3030b2145b 100644
> > --- a/include/linux/rpmsg_ns.h
> > +++ b/include/linux/rpmsg_ns.h
> > @@ -41,8 +41,8 @@ struct rpmsg_hdr {
> >   */
> >  struct rpmsg_ns_msg {
> >  	char name[RPMSG_NAME_SIZE];
> > -	__virtio32 addr;
> > -	__virtio32 flags;
> > +	u32 addr;
> > +	u32 flags;
> 
> These aren't (necessarily) native endianness, right? Don't they deserve a 
> separate type? __rpmsg32?
> 

I kept them as u32 type to be as generic as possible and let the endianness
conversion callback do the work.  Making them a __rpmsg32 is a good idea, it
would force a proper implementation. 

> Thanks
> Guennadi
> 
> >  } __packed;
> >  
> >  /**
> > -- 
> > 2.25.1
> >