2020-09-22 01:16:35

by Mathieu Poirier

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

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


2020-09-22 01:19:15

by Mathieu Poirier

[permalink] [raw]
Subject: [PATCH 03/10] rpmsg: virtio: Add rpmsg channel device ops

From: Arnaud Pouliquen <[email protected]>

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

Signed-off-by: Arnaud Pouliquen <[email protected]>
[Moved function declaration above struct virtio_rpmsg_ops]
Signed-off-by: Mathieu Poirier <[email protected]>
---
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

2020-09-22 01:19:15

by Mathieu Poirier

[permalink] [raw]
Subject: [PATCH 02/10] rpmsg: core: Add channel creation internal API

From: Arnaud Pouliquen <[email protected]>

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 <[email protected]>
---
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

2020-09-22 01:19:36

by Mathieu Poirier

[permalink] [raw]
Subject: [PATCH 01/10] rpmsg: virtio: rename rpmsg_create_channel

From: Arnaud Pouliquen <[email protected]>

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

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
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

2020-09-22 01:19:39

by Mathieu Poirier

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

From: Arnaud Pouliquen <[email protected]>

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 <[email protected]>
Signed-off-by: Mathieu Poirier <[email protected]>
---
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 <[email protected]>");
+MODULE_ALIAS("rpmsg_ns");
+MODULE_LICENSE("GPL v2");
--
2.25.1

2020-09-22 01:23:06

by Mathieu Poirier

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

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

Signed-off-by: Mathieu Poirier <[email protected]>
---
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

2020-09-22 01:23:06

by Mathieu Poirier

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

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 <[email protected]>
---
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

2020-09-22 01:23:27

by Mathieu Poirier

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

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 <[email protected]>
---
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

2020-09-22 01:23:30

by Mathieu Poirier

[permalink] [raw]
Subject: [PATCH 05/10] rpmsg: virtio: Move virtio RPMSG structures to private header

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 <[email protected]>
---
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

2020-09-22 01:23:30

by Mathieu Poirier

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

From: Arnaud Pouliquen <[email protected]>

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 <[email protected]>
---
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

2020-09-22 01:25:23

by Mathieu Poirier

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

From: Guennadi Liakhovetski <[email protected]>

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 <[email protected]>
[Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
Signed-off-by: Mathieu Poirier <[email protected]>
---
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

2020-09-22 02:18:31

by Randy Dunlap

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

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

2020-09-22 08:31:50

by Guennadi Liakhovetski

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

On Mon, Sep 21, 2020 at 06:09:51PM -0600, Mathieu Poirier wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> Rename the internal function as it is internal, and as
> the name will be used in rpmsg_core.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> 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
>

2020-09-22 08:36:51

by Guennadi Liakhovetski

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

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
>

2020-09-22 14:30:06

by Arnaud POULIQUEN

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



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 <[email protected]>
> ---
> 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,
>

2020-09-22 14:38:34

by Arnaud POULIQUEN

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



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 <[email protected]>
> ---
> 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
>

2020-09-22 14:38:35

by Arnaud POULIQUEN

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

Hi Mathieu,

On 9/22/20 2:09 AM, Mathieu Poirier wrote:
> From: Guennadi Liakhovetski <[email protected]>
>
> 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 <[email protected]>
> [Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> 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
>

2020-09-22 19:14:48

by Mathieu Poirier

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

Good day Guennadi,

On Tue, 22 Sep 2020 at 02:09, Guennadi Liakhovetski
<[email protected]> 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
> >

2020-09-22 19:26:55

by Mathieu Poirier

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

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 <[email protected]>
> >
> > Rename the internal function as it is internal, and as
> > the name will be used in rpmsg_core.
> >
> > Signed-off-by: Arnaud Pouliquen <[email protected]>
> > ---
> > 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
> >

2020-09-22 19:38:38

by Mathieu Poirier

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

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 <[email protected]>
> >
> > 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 <[email protected]>
> > [Renamed header file from linux/rpmsg/rpmsg_virtio.h to linux/rpmsg_ns.h]
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > 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
> >

2020-09-22 19:51:01

by Mathieu Poirier

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

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 <[email protected]>
> > ---
> > 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
> >

2020-09-23 01:32:20

by kernel test robot

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

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 <[email protected]>


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/[email protected]


Attachments:
(No filename) (4.14 kB)
.config.gz (36.85 kB)
Download all attachments

2020-09-23 02:51:57

by kernel test robot

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

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 <[email protected]>

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/[email protected]


Attachments:
(No filename) (3.16 kB)
.config.gz (64.45 kB)
Download all attachments

2020-09-23 03:03:46

by kernel test robot

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

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 <[email protected]>


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/[email protected]


Attachments:
(No filename) (13.89 kB)
.config.gz (36.85 kB)
Download all attachments

2020-09-23 11:59:30

by Dan Carpenter

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

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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>

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/[email protected]


Attachments:
(No filename) (2.94 kB)
.config.gz (40.15 kB)
Download all attachments

2020-09-24 06:55:51

by Guennadi Liakhovetski

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

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
> <[email protected]> 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
> > >

2020-09-24 18:20:36

by Mathieu Poirier

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

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
> > <[email protected]> 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
> > > >

2020-09-28 19:34:38

by Mathieu Poirier

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

Hey Guennadi,

On Mon, Sep 28, 2020 at 11:49:42AM +0200, Guennadi Liakhovetski wrote:
> (re-sending, mailing list delivery attempts last Friday failed)
>

I got your email on Friday but had to tend to other things.

> Hi Mathieu,
>
> On Thu, Sep 24, 2020 at 12:18:53PM -0600, Mathieu Poirier wrote:
> > On Thu, Sep 24, 2020 at 08:53:56AM +0200, Guennadi Liakhovetski wrote:
>
> [snip]
>
> > > 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.
>
> Are there really cases when an NS announcement is sent multiple times by a
> remote? But not for the same name-space, at least in virtio_rpmsg_bus.c
> there's a check for a duplicate announcement in rpmsg_create_channel().
>
> > > 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.
>
> I think the terminology is slightly incorrect above. It isn't a channel, it's
> an endpoint. A channel is a synonym of a device in RPMsg (from rpmsg.txt):
>
> "Every rpmsg device is a communication channel with a remote processor (thus
> rpmsg devices are called channels)."
>
> > An official rpmsg_device is not needed since
> > it is implicit.
>
> Agree.
>
> > 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.
>
> Oh, I think I'm getting it now. I think now I understand where the
> disagreement lies. If I understand correctly in your model each remote
> processor can provide multiple *devices* / *channels*. E.g. a remote

That is correct

> processor can provide a character device, a network device etc. Each of
> those devices / channels would send a namespace announcement to the
> main processor, which then would create a respective device and probe
> the respective driver - all with the same remote processor over the same
> RPMsg bus. I understand this concept and in fact I find it logical.
>

Ok

> However, since I have no experience with real life RPMsg implementations
> I am basing my understanding of RPMsg on the little and scarce and
> non-conclusive documentation that I can find online. E.g. on
>
> https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol
>
> which says:
>
> "Every remote core in RPMsg component is represented by RPMsg device that
> provides a communication channel between master and remote, hence RPMsg
> devices are also known as channels"
>
> So, according to that definition you cannot have a remote processor,
> doing both a character and a network devices. However, in kernel's
> rpmsg.txt an *almost exact* copy of that sentence is the quote, that
> I've already provided above, with a subtle but important difference:
>
> "Every rpmsg device is a communication channel with a remote processor
> (thus rpmsg devices are called channels)."

The documentation isn't easy to follow and personally got very confused when I
started getting involved with the remoteproc/rpmsg subsystems. I have the
intention of doing a good revamp but there is never enough time.

>
> It doesn't explicitly say, that there can be multiple devices per
> remote processor, but it doesn't specify, that there can be only one
> (TM) either, so, implicitly there can be many :-/ So, with this model,
> yes, I can understand, how a single instance of the RPMsg bus (in
> VirtIO case a pair of virtual queues) can have just *one* namespace
> service and serve *multiple* devices channels. In that case yes, the
> namespace service can be a separate device.

I will spin off a V2 of this set and we'll go from there. I also want to get a
look at Kishon's patchset that Arnaud and Vincent were referring to before
making up my mind about how to move foward with your vhost RPMSG API patchset.

This is certainly taking longer than expected but I'd rather take the time to
explore all aspects of the question rather than having to live with a solution
that is not adequate.

Thanks for the patience,
Mathieu

>
> Thanks
> Guennadi
>
> > 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
> > > > > >
>
> ----- End forwarded message -----

2020-09-30 05:37:42

by Guennadi Liakhovetski

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

Hi Mathieu,

On Mon, Sep 28, 2020 at 01:33:08PM -0600, Mathieu Poirier wrote:
> Hey Guennadi,
>
> On Mon, Sep 28, 2020 at 11:49:42AM +0200, Guennadi Liakhovetski wrote:
> > (re-sending, mailing list delivery attempts last Friday failed)
> >
>
> I got your email on Friday but had to tend to other things.

Sure, no problem, you got it directly because you were on CC, but it didn't
make it to the mailing lists - you can check archives, it isn't there :-/
But thanks to your reply now it's also on the lists.

> > Hi Mathieu,
> >
> > On Thu, Sep 24, 2020 at 12:18:53PM -0600, Mathieu Poirier wrote:
> > > On Thu, Sep 24, 2020 at 08:53:56AM +0200, Guennadi Liakhovetski wrote:
> >
> > [snip]
> >
> > > > 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.
> >
> > Are there really cases when an NS announcement is sent multiple times by a
> > remote? But not for the same name-space, at least in virtio_rpmsg_bus.c
> > there's a check for a duplicate announcement in rpmsg_create_channel().
> >
> > > > 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.
> >
> > I think the terminology is slightly incorrect above. It isn't a channel, it's
> > an endpoint. A channel is a synonym of a device in RPMsg (from rpmsg.txt):
> >
> > "Every rpmsg device is a communication channel with a remote processor (thus
> > rpmsg devices are called channels)."
> >
> > > An official rpmsg_device is not needed since
> > > it is implicit.
> >
> > Agree.
> >
> > > 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.
> >
> > Oh, I think I'm getting it now. I think now I understand where the
> > disagreement lies. If I understand correctly in your model each remote
> > processor can provide multiple *devices* / *channels*. E.g. a remote
>
> That is correct
>
> > processor can provide a character device, a network device etc. Each of
> > those devices / channels would send a namespace announcement to the
> > main processor, which then would create a respective device and probe
> > the respective driver - all with the same remote processor over the same
> > RPMsg bus. I understand this concept and in fact I find it logical.
> >
>
> Ok
>
> > However, since I have no experience with real life RPMsg implementations
> > I am basing my understanding of RPMsg on the little and scarce and
> > non-conclusive documentation that I can find online. E.g. on
> >
> > https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol
> >
> > which says:
> >
> > "Every remote core in RPMsg component is represented by RPMsg device that
> > provides a communication channel between master and remote, hence RPMsg
> > devices are also known as channels"
> >
> > So, according to that definition you cannot have a remote processor,
> > doing both a character and a network devices. However, in kernel's
> > rpmsg.txt an *almost exact* copy of that sentence is the quote, that
> > I've already provided above, with a subtle but important difference:
> >
> > "Every rpmsg device is a communication channel with a remote processor
> > (thus rpmsg devices are called channels)."
>
> The documentation isn't easy to follow and personally got very confused when I
> started getting involved with the remoteproc/rpmsg subsystems. I have the
> intention of doing a good revamp but there is never enough time.
>
> >
> > It doesn't explicitly say, that there can be multiple devices per
> > remote processor, but it doesn't specify, that there can be only one
> > (TM) either, so, implicitly there can be many :-/ So, with this model,
> > yes, I can understand, how a single instance of the RPMsg bus (in
> > VirtIO case a pair of virtual queues) can have just *one* namespace
> > service and serve *multiple* devices channels. In that case yes, the
> > namespace service can be a separate device.
>
> I will spin off a V2 of this set and we'll go from there. I also want to get a
> look at Kishon's patchset that Arnaud and Vincent were referring to before
> making up my mind about how to move foward with your vhost RPMSG API patchset.

I should have a look at the remaining patches from your v1, hopefully my
problem with sending emails to vger is either already resolved or gets resolved
very soon.

> This is certainly taking longer than expected but I'd rather take the time to
> explore all aspects of the question rather than having to live with a solution
> that is not adequate.

Yes, this is just the first step in my audio DSP virtualisation patch set,
once the kernel has a vhost side RPMsg support, I can continue with upstreaming
of the next step.

Thanks
Guennadi

> Thanks for the patience,
> Mathieu
>
> >
> > Thanks
> > Guennadi
> >
> > > 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
> > > > > > >
> >
> > ----- End forwarded message -----

2020-09-30 06:37:28

by Guennadi Liakhovetski

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

On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>

2020-09-30 06:55:31

by Guennadi Liakhovetski

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

On Mon, Sep 21, 2020 at 06:09:54PM -0600, Mathieu Poirier wrote:
> From: Guennadi Liakhovetski <[email protected]>
>
> 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 <[email protected]>
> [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 <[email protected]>
> ---
> 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
>

2020-09-30 07:07:25

by Guennadi Liakhovetski

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

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 <[email protected]>
> ---
> 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
>

2020-09-30 07:11:05

by Guennadi Liakhovetski

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

On Mon, Sep 21, 2020 at 06:09:56PM -0600, Mathieu Poirier wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> 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 <[email protected]>");
> +MODULE_ALIAS("rpmsg_ns");
> +MODULE_LICENSE("GPL v2");
> --
> 2.25.1
>

2020-09-30 07:14:54

by Guennadi Liakhovetski

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

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 <[email protected]>
> ---
> 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
>

2020-09-30 07:16:05

by Guennadi Liakhovetski

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

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 <[email protected]>
> ---
> 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
>

2020-10-01 14:48:20

by Arnaud POULIQUEN

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



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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>>

2020-10-01 16:16:41

by Arnaud POULIQUEN

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



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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Mathieu Poirier <[email protected]>
>> ---
>> 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 <[email protected]>");
>> +MODULE_ALIAS("rpmsg_ns");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.25.1
>>

2020-10-07 18:17:53

by Mathieu Poirier

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

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 <[email protected]>
> > ---
> > 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
> >

2020-10-07 19:27:52

by Mathieu Poirier

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

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 <[email protected]>
> > ---
> > 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
> >