The NS announcement is implemented by several backends, but could be
considered as part the RPMsg protocol.
In this case it should be managed as a reserved rpmsg service and so
implemented on top of the rpmsg protocol.
This series introduces the rpmsg_ns driver that handles the name service
announcement. The virtio backend is updated in consequence to use this
service.
Applies cleanly on Bjorn for-next branch (ca69dba7f13a)
Main updates from V1 to V2:
- Integrate Mathieu's comments:
- rework the rpmsg_create_channel and rpmsg_release_channel error management.
- merge patch "[9/9] rpmsg: ns: name service announcement endianness"
in other pathes.
V1: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=327257
Arnaud Pouliquen (8):
rpmsg: virtio: rename rpmsg_create_channel
rpmsg: core: add channel creation internal API
rpmsg: virtio: add rpmsg channel device ops
rpmsg: define the name service announcement as reserved address
rpmsg: introduce reserved rpmsg driver for ns announcement
rpmsg: virtio: use rpmsg ns device for the ns announcement
rpmsg: ns: add name service announcement service
rpmsg: virtio: use rpmsg_ns driver to manage ns announcement
drivers/rpmsg/Kconfig | 9 ++
drivers/rpmsg/Makefile | 1 +
drivers/rpmsg/rpmsg_core.c | 45 +++++++
drivers/rpmsg/rpmsg_internal.h | 32 +++++
drivers/rpmsg/rpmsg_ns.c | 174 +++++++++++++++++++++++++
drivers/rpmsg/virtio_rpmsg_bus.c | 213 +++++++++----------------------
include/linux/rpmsg.h | 9 ++
7 files changed, 332 insertions(+), 151 deletions(-)
create mode 100644 drivers/rpmsg/rpmsg_ns.c
--
2.17.1
As the RPMsg driver is in charge of the name service announcement,
create an API to send channel creation and destruction to the remote
processor.
Notice that the source address of the message sent is now RPMSG_NS_ADDR.
Legacy implementation was to send the message with source address
corresponding to the ept created.
RPMSG_NS_ADDR as source address make sense as we want to send a message
belonging to the NS announcement service and the created ept address is
already in the message payload.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_internal.h | 3 +++
drivers/rpmsg/rpmsg_ns.c | 39 ++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 641b48f6bf2a..d1549e5cb607 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -119,4 +119,7 @@ static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
return rpmsg_register_device(rpdev);
}
+int rpmsg_ns_announce_create(struct rpmsg_device *rpdev);
+int rpmsg_ns_announce_destroy(struct rpmsg_device *rpdev);
+
#endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 3c929b6976a6..2a34e36a592d 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -37,6 +37,45 @@ struct rpmsg_ns_msg {
__le32 flags;
} __packed;
+/**
+ * rpmsg_ns_announce_create() - announce to the remote processor
+ * the service creation or destruction
+ * @rpdev: the rpmsg channel
+ * @ns_flag: related to the @rpmsg_ns_flags enum
+ *
+ * This function can be called on a channel creation or destruction to inform
+ * the remote processor, using the reserved name service anouncement channel.
+ */
+static int rpmsg_ns_channel_announce(struct rpmsg_device *rpdev, int ns_flag)
+{
+ struct rpmsg_ns_msg nsm;
+
+ if (!rpdev->announce || !rpdev->ept)
+ return 0;
+
+ if (ns_flag != RPMSG_NS_CREATE && ns_flag != RPMSG_NS_DESTROY)
+ return -EINVAL;
+
+ strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
+ nsm.addr = cpu_to_le32(rpdev->ept->addr);
+ nsm.flags = cpu_to_le32(ns_flag);
+
+ return rpmsg_send_offchannel(rpdev->ept, RPMSG_NS_ADDR, RPMSG_NS_ADDR,
+ &nsm, sizeof(nsm));
+}
+
+int rpmsg_ns_announce_create(struct rpmsg_device *rpdev)
+{
+ return rpmsg_ns_channel_announce(rpdev, RPMSG_NS_CREATE);
+}
+EXPORT_SYMBOL(rpmsg_ns_announce_create);
+
+int rpmsg_ns_announce_destroy(struct rpmsg_device *rpdev)
+{
+ return rpmsg_ns_channel_announce(rpdev, RPMSG_NS_DESTROY);
+}
+EXPORT_SYMBOL(rpmsg_ns_announce_destroy);
+
/* Invoked when a name service announcement arrives */
static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
void *priv, u32 src)
--
2.17.1
The name service announcement should not be linked to the RPMsg virtio bus
but to the RPMsg protocol itself.
This patch proposes to break the dependency with the RPmsg virtio bus by
the introduction of the reserved RPMsg name service driver which will be in
charge of managing the RPMsg name service announcement.
This first patch only implements the probe and the RPMsg endpoint to
manage create and release channels remote requests.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/Kconfig | 8 ++
drivers/rpmsg/Makefile | 1 +
drivers/rpmsg/rpmsg_internal.h | 17 +++++
drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
4 files changed, 161 insertions(+)
create mode 100644 drivers/rpmsg/rpmsg_ns.c
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
return rpmsg_register_device(rpdev);
}
+/**
+ * rpmsg_ns_register_device() - register name service device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg name service device.
+ */
+static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+ strcpy(rpdev->id.name, "rpmsg_ns");
+ rpdev->driver_override = "rpmsg_ns";
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ return rpmsg_register_device(rpdev);
+}
+
#endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
new file mode 100644
index 000000000000..3c929b6976a6
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "rpmsg_internal.h"
+
+/**
+ * enum rpmsg_ns_flags - dynamic name service announcement flags
+ *
+ * @RPMSG_NS_CREATE: a new remote service was just created
+ * @RPMSG_NS_DESTROY: a known remote service was just destroyed
+ */
+enum rpmsg_ns_flags {
+ RPMSG_NS_CREATE = 0,
+ RPMSG_NS_DESTROY = 1,
+};
+
+/**
+ * struct rpmsg_ns_msg - dynamic name service announcement message
+ * @name: name of remote service that is published
+ * @addr: address of remote service that is published
+ * @flags: indicates whether service is created or destroyed
+ *
+ * This message is sent across to publish a new service, or announce
+ * about its removal. When we receive these messages, an appropriate
+ * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
+ * or ->remove() handler of the appropriate rpmsg driver will be invoked
+ * (if/as-soon-as one is registered).
+ */
+struct rpmsg_ns_msg {
+ char name[RPMSG_NAME_SIZE];
+ __le32 addr;
+ __le32 flags;
+} __packed;
+
+/* Invoked when a name service announcement arrives */
+static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
+ void *priv, u32 src)
+{
+ struct rpmsg_ns_msg *msg = data;
+ struct rpmsg_device *newch;
+ struct rpmsg_channel_info chinfo;
+ struct device *dev = &rpdev->dev;
+ unsigned int flags = le32_to_cpu(msg->flags);
+ 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 = le32_to_cpu(msg->addr);
+
+ dev_info(dev, "%sing channel %s addr 0x%x\n",
+ (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
+ msg->name, chinfo.dst);
+
+ if (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.17.1
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 9006fc7f73d0..736bc7b9dea8 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -390,8 +390,9 @@ static void virtio_rpmsg_release_device(struct device *dev)
* this function will be used to create both static and dynamic
* channels.
*/
-static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
- struct rpmsg_channel_info *chinfo)
+static struct rpmsg_device *
+__rpmsg_create_channel(struct virtproc_info *vrp,
+ struct rpmsg_channel_info *chinfo)
{
struct virtio_rpmsg_channel *vch;
struct rpmsg_device *rpdev;
@@ -864,7 +865,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
if (ret)
dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
} else {
- newch = rpmsg_create_channel(vrp, &chinfo);
+ newch = __rpmsg_create_channel(vrp, &chinfo);
if (!newch)
dev_err(dev, "rpmsg_create_channel failed\n");
}
--
2.17.1
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 a6361cad608b..97c15c1a0cc8 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..d5ab286d0e5e 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -20,6 +20,8 @@
/**
* struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @create_channel: create backend-specific channel, optional
+ * @release_channel: release backend-specific channel, optional
* @create_ept: create backend-specific endpoint, required
* @announce_create: announce presence of new channel, optional
* @announce_destroy: announce destruction of channel, optional
@@ -29,6 +31,11 @@
* advertise new channels implicitly by creating the endpoints.
*/
struct rpmsg_device_ops {
+ struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
+ struct rpmsg_channel_info *chinfo);
+ int (*release_channel)(struct rpmsg_device *rpdev,
+ struct rpmsg_channel_info *chinfo);
+
struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
rpmsg_rx_cb_t cb, void *priv,
struct rpmsg_channel_info chinfo);
@@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
struct device *rpmsg_find_device(struct device *parent,
struct rpmsg_channel_info *chinfo);
+struct rpmsg_device *
+ rpmsg_create_channel(struct rpmsg_device *rpdev,
+ struct rpmsg_channel_info *chinfo);
+int rpmsg_release_channel(struct rpmsg_device *rpdev,
+ struct rpmsg_channel_info *chinfo);
/**
* rpmsg_chrdev_register_device() - register chrdev device based on rpdev
* @rpdev: prepared rpdev to be used for creating endpoints
--
2.17.1
On Tue, Aug 25, 2020 at 06:49:04PM +0200, Arnaud Pouliquen wrote:
> The name service announcement should not be linked to the RPMsg virtio bus
> but to the RPMsg protocol itself.
>
> This patch proposes to break the dependency with the RPmsg virtio bus by
> the introduction of the reserved RPMsg name service driver which will be in
> charge of managing the RPMsg name service announcement.
>
> This first patch only implements the probe and the RPMsg endpoint to
> manage create and release channels remote requests.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/Kconfig | 8 ++
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/rpmsg_internal.h | 17 +++++
> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
> 4 files changed, 161 insertions(+)
> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> return rpmsg_register_device(rpdev);
> }
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, "rpmsg_ns");
> + rpdev->driver_override = "rpmsg_ns";
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +
> #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index 000000000000..3c929b6976a6
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + */
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "rpmsg_internal.h"
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> + RPMSG_NS_CREATE = 0,
> + RPMSG_NS_DESTROY = 1,
> +};
> +
> +/**
> + * struct rpmsg_ns_msg - dynamic name service announcement message
> + * @name: name of remote service that is published
> + * @addr: address of remote service that is published
> + * @flags: indicates whether service is created or destroyed
> + *
> + * This message is sent across to publish a new service, or announce
> + * about its removal. When we receive these messages, an appropriate
> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> + * (if/as-soon-as one is registered).
> + */
> +struct rpmsg_ns_msg {
> + char name[RPMSG_NAME_SIZE];
> + __le32 addr;
> + __le32 flags;
This is about to get moved to a header file [1] so that the virtualisation people
can use the same structures. As such we can't just assume their type is __le32
and we can't move them here either. I suggest to move this to
include/linux/rpmsg/virtio.h as Guennadi did.
[1]. https://patchwork.kernel.org/patch/11749285/
> +} __packed;
> +
> +/* Invoked when a name service announcement arrives */
> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> + void *priv, u32 src)
> +{
> + struct rpmsg_ns_msg *msg = data;
> + struct rpmsg_device *newch;
> + struct rpmsg_channel_info chinfo;
> + struct device *dev = &rpdev->dev;
> + unsigned int flags = le32_to_cpu(msg->flags);
I've been staring at this for a long time and I suspect you did too.
Can we assume that a name service is running on a virtio implementation? It
certainly has been the case so far, and doing so would make this patchset a lot
more simple. Otherwise we need to find a way to make this work without
losing flexibility, which will make things more complex.
What do you think?
> + 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 = le32_to_cpu(msg->addr);
> +
> + dev_info(dev, "%sing channel %s addr 0x%x\n",
> + (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
> + msg->name, chinfo.dst);
> +
> + if (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.17.1
>
Hi Mathieu,
On 9/4/20 1:00 AM, Mathieu Poirier wrote:
> On Tue, Aug 25, 2020 at 06:49:04PM +0200, Arnaud Pouliquen wrote:
>> The name service announcement should not be linked to the RPMsg virtio bus
>> but to the RPMsg protocol itself.
>>
>> This patch proposes to break the dependency with the RPmsg virtio bus by
>> the introduction of the reserved RPMsg name service driver which will be in
>> charge of managing the RPMsg name service announcement.
>>
>> This first patch only implements the probe and the RPMsg endpoint to
>> manage create and release channels remote requests.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/Kconfig | 8 ++
>> drivers/rpmsg/Makefile | 1 +
>> drivers/rpmsg/rpmsg_internal.h | 17 +++++
>> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
>> 4 files changed, 161 insertions(+)
>> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> return rpmsg_register_device(rpdev);
>> }
>>
>> +/**
>> + * rpmsg_ns_register_device() - register name service device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> + * basis for the rpmsg name service device.
>> + */
>> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> + strcpy(rpdev->id.name, "rpmsg_ns");
>> + rpdev->driver_override = "rpmsg_ns";
>> + rpdev->src = RPMSG_NS_ADDR;
>> + rpdev->dst = RPMSG_NS_ADDR;
>> +
>> + return rpmsg_register_device(rpdev);
>> +}
>> +
>> #endif
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> new file mode 100644
>> index 000000000000..3c929b6976a6
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -0,0 +1,135 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> + */
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include "rpmsg_internal.h"
>> +
>> +/**
>> + * enum rpmsg_ns_flags - dynamic name service announcement flags
>> + *
>> + * @RPMSG_NS_CREATE: a new remote service was just created
>> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>> + */
>> +enum rpmsg_ns_flags {
>> + RPMSG_NS_CREATE = 0,
>> + RPMSG_NS_DESTROY = 1,
>> +};
>> +
>> +/**
>> + * struct rpmsg_ns_msg - dynamic name service announcement message
>> + * @name: name of remote service that is published
>> + * @addr: address of remote service that is published
>> + * @flags: indicates whether service is created or destroyed
>> + *
>> + * This message is sent across to publish a new service, or announce
>> + * about its removal. When we receive these messages, an appropriate
>> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
>> + * (if/as-soon-as one is registered).
>> + */
>> +struct rpmsg_ns_msg {
>> + char name[RPMSG_NAME_SIZE];
>> + __le32 addr;
>> + __le32 flags;
>
> This is about to get moved to a header file [1] so that the virtualisation people
> can use the same structures. As such we can't just assume their type is __le32
> and we can't move them here either. I suggest to move this to
> include/linux/rpmsg/virtio.h as Guennadi did.
>
> [1]. https://patchwork.kernel.org/patch/11749285/
The objective of this patch is to treat the ns annoucement as a service so it should not
depend on the virtio.
From my POV we have to separate the header from the payload in term of endianness.
That make sense that the rpmsg_hdr is virtio struct dependent. But the rpmsg_ns_msg
describes the payload message. The payload message should be fixed by the service itself
and should not depend on the virualization.
Here i proposed to fix the payload in little endian for compatibility with the legacy.
But we also could decide to not fixe the endianess.
>
>> +} __packed;
>> +
>> +/* Invoked when a name service announcement arrives */
>> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> + void *priv, u32 src)
>> +{
>> + struct rpmsg_ns_msg *msg = data;
>> + struct rpmsg_device *newch;
>> + struct rpmsg_channel_info chinfo;
>> + struct device *dev = &rpdev->dev;
>> + unsigned int flags = le32_to_cpu(msg->flags);
>
> I've been staring at this for a long time and I suspect you did too.
>
> Can we assume that a name service is running on a virtio implementation? It
> certainly has been the case so far, and doing so would make this patchset a lot
> more simple. Otherwise we need to find a way to make this work without
> losing flexibility, which will make things more complex.
>
> What do you think?
I am not sure to understand your point...
I'm also intertesting in understanding more in details what do you have in mind in term of
"losing flexibility"?
The purpose of this patchset is to generalize the ns announcement so that it does
not depend on the implementation. The goal is to avoid that implementations
rewrite the service.
The vhost patchset is an exemple which is also virtio based. But there are some other
implementations (exemples below).
Anyway I think the main question associated to this patchset is:
Is the ns annoucement is a RPMsg service or a virtio RPMsg service?
From my point of view it should be a RPMsg service, that is enbled or
not, depending on the backend implementation.
2 other ns annoucement service exemples:
- the mtk_rpmsg driver[1].
- In ST we have a PoC to extend the rpmsg over a physical serial link [2]. Notice that, with
this implementation we can have the LE-BE inter-communication use case.
For this PoC we also defined a common file that does not depend on the virtio[3], so
an alternative to what Guennadi did in include/linux/rpmsg/virtio.h.
[1]https://elixir.bootlin.com/linux/v5.9-rc3/source/drivers/rpmsg/mtk_rpmsg.c#L45
[2]https://github.com/arnopo/linux/blob/ELCE_demos/drivers/rpmsg/uart_rpmsg_bus.c
[3]https://github.com/arnopo/linux/blob/ELCE_demos/drivers/rpmsg/rpmsg_bus_common.c
Thanks,
Arnaud
>
>> + 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 = le32_to_cpu(msg->addr);
>> +
>> + dev_info(dev, "%sing channel %s addr 0x%x\n",
>> + (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
>> + msg->name, chinfo.dst);
>> +
>> + if (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.17.1
>>
On Fri, 4 Sep 2020 at 02:28, Arnaud POULIQUEN <[email protected]> wrote:
>
> Hi Mathieu,
>
> On 9/4/20 1:00 AM, Mathieu Poirier wrote:
> > On Tue, Aug 25, 2020 at 06:49:04PM +0200, Arnaud Pouliquen wrote:
> >> The name service announcement should not be linked to the RPMsg virtio bus
> >> but to the RPMsg protocol itself.
> >>
> >> This patch proposes to break the dependency with the RPmsg virtio bus by
> >> the introduction of the reserved RPMsg name service driver which will be in
> >> charge of managing the RPMsg name service announcement.
> >>
> >> This first patch only implements the probe and the RPMsg endpoint to
> >> manage create and release channels remote requests.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> drivers/rpmsg/Kconfig | 8 ++
> >> drivers/rpmsg/Makefile | 1 +
> >> drivers/rpmsg/rpmsg_internal.h | 17 +++++
> >> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
> >> 4 files changed, 161 insertions(+)
> >> create mode 100644 drivers/rpmsg/rpmsg_ns.c
> >>
> >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> >> index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
> >> --- a/drivers/rpmsg/rpmsg_internal.h
> >> +++ b/drivers/rpmsg/rpmsg_internal.h
> >> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> >> return rpmsg_register_device(rpdev);
> >> }
> >>
> >> +/**
> >> + * rpmsg_ns_register_device() - register name service device based on rpdev
> >> + * @rpdev: prepared rpdev to be used for creating endpoints
> >> + *
> >> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> >> + * basis for the rpmsg name service device.
> >> + */
> >> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> + strcpy(rpdev->id.name, "rpmsg_ns");
> >> + rpdev->driver_override = "rpmsg_ns";
> >> + rpdev->src = RPMSG_NS_ADDR;
> >> + rpdev->dst = RPMSG_NS_ADDR;
> >> +
> >> + return rpmsg_register_device(rpdev);
> >> +}
> >> +
> >> #endif
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> new file mode 100644
> >> index 000000000000..3c929b6976a6
> >> --- /dev/null
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -0,0 +1,135 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> >> + */
> >> +#include <linux/device.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include "rpmsg_internal.h"
> >> +
> >> +/**
> >> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> >> + *
> >> + * @RPMSG_NS_CREATE: a new remote service was just created
> >> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> >> + */
> >> +enum rpmsg_ns_flags {
> >> + RPMSG_NS_CREATE = 0,
> >> + RPMSG_NS_DESTROY = 1,
> >> +};
> >> +
> >> +/**
> >> + * struct rpmsg_ns_msg - dynamic name service announcement message
> >> + * @name: name of remote service that is published
> >> + * @addr: address of remote service that is published
> >> + * @flags: indicates whether service is created or destroyed
> >> + *
> >> + * This message is sent across to publish a new service, or announce
> >> + * about its removal. When we receive these messages, an appropriate
> >> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> >> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> >> + * (if/as-soon-as one is registered).
> >> + */
> >> +struct rpmsg_ns_msg {
> >> + char name[RPMSG_NAME_SIZE];
> >> + __le32 addr;
> >> + __le32 flags;
> >
> > This is about to get moved to a header file [1] so that the virtualisation people
> > can use the same structures. As such we can't just assume their type is __le32
> > and we can't move them here either. I suggest to move this to
> > include/linux/rpmsg/virtio.h as Guennadi did.
> >
> > [1]. https://patchwork.kernel.org/patch/11749285/
>
> The objective of this patch is to treat the ns annoucement as a service so it should not
> depend on the virtio.
I agree that name service announcement and virtio are disjoint and
should remain as such. That being said I would like to avoid the
proliferation of "struct rpmsg_ns_msg" in the kernel. I am convinced
we can find a middle ground that can work for everyone.
> From my POV we have to separate the header from the payload in term of endianness.
>
> That make sense that the rpmsg_hdr is virtio struct dependent. But the rpmsg_ns_msg
> describes the payload message. The payload message should be fixed by the service itself
> and should not depend on the virualization.
> Here i proposed to fix the payload in little endian for compatibility with the legacy.
> But we also could decide to not fixe the endianess.
I think the only way forward is to take the time to really understand
how the rpmsg_ns_msg is used in [1]. I will do that tomorrow
(Wednesday) or Thursday and I suggest you do the same at one point.
>
> >
> >> +} __packed;
> >> +
> >> +/* Invoked when a name service announcement arrives */
> >> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >> + void *priv, u32 src)
> >> +{
> >> + struct rpmsg_ns_msg *msg = data;
> >> + struct rpmsg_device *newch;
> >> + struct rpmsg_channel_info chinfo;
> >> + struct device *dev = &rpdev->dev;
> >> + unsigned int flags = le32_to_cpu(msg->flags);
> >
> > I've been staring at this for a long time and I suspect you did too.
> >
> > Can we assume that a name service is running on a virtio implementation? It
> > certainly has been the case so far, and doing so would make this patchset a lot
> > more simple. Otherwise we need to find a way to make this work without
> > losing flexibility, which will make things more complex.
> >
> > What do you think?
>
> I am not sure to understand your point...
> I'm also intertesting in understanding more in details what do you have in mind in term of
> "losing flexibility"?
>
I was referring to the format of the rpmsg_ns_msg and the transport
layer, i.e virtio, Glink or otherwise.
> The purpose of this patchset is to generalize the ns announcement so that it does
> not depend on the implementation. The goal is to avoid that implementations
> rewrite the service.
>
> The vhost patchset is an exemple which is also virtio based. But there are some other
> implementations (exemples below).
>
> Anyway I think the main question associated to this patchset is:
> Is the ns annoucement is a RPMsg service or a virtio RPMsg service?
>
> From my point of view it should be a RPMsg service, that is enbled or
> not, depending on the backend implementation.
I agree.
As I said above I don't think we can move forward with this set and
[1] until we understand how name space services are used in the vhost
driver. That will help us define the rpmsg_ns_msg structure and how
to handle it in a generic way. Otherwise I think the rest of this set
works.
I will get back to you once I have gone through [1].
Thanks,
Mathieu
>
> 2 other ns annoucement service exemples:
> - the mtk_rpmsg driver[1].
> - In ST we have a PoC to extend the rpmsg over a physical serial link [2]. Notice that, with
> this implementation we can have the LE-BE inter-communication use case.
> For this PoC we also defined a common file that does not depend on the virtio[3], so
> an alternative to what Guennadi did in include/linux/rpmsg/virtio.h.
>
> [1]https://elixir.bootlin.com/linux/v5.9-rc3/source/drivers/rpmsg/mtk_rpmsg.c#L45
> [2]https://github.com/arnopo/linux/blob/ELCE_demos/drivers/rpmsg/uart_rpmsg_bus.c
> [3]https://github.com/arnopo/linux/blob/ELCE_demos/drivers/rpmsg/rpmsg_bus_common.c
>
> Thanks,
> Arnaud
>
> >
> >> + 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 = le32_to_cpu(msg->addr);
> >> +
> >> + dev_info(dev, "%sing channel %s addr 0x%x\n",
> >> + (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
> >> + msg->name, chinfo.dst);
> >> +
> >> + if (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.17.1
> >>
Hey Arnaud,
On Tue, Aug 25, 2020 at 06:49:04PM +0200, Arnaud Pouliquen wrote:
> The name service announcement should not be linked to the RPMsg virtio bus
> but to the RPMsg protocol itself.
>
> This patch proposes to break the dependency with the RPmsg virtio bus by
> the introduction of the reserved RPMsg name service driver which will be in
> charge of managing the RPMsg name service announcement.
>
> This first patch only implements the probe and the RPMsg endpoint to
> manage create and release channels remote requests.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/Kconfig | 8 ++
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/rpmsg_internal.h | 17 +++++
> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
> 4 files changed, 161 insertions(+)
> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> return rpmsg_register_device(rpdev);
> }
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, "rpmsg_ns");
> + rpdev->driver_override = "rpmsg_ns";
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +
> #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index 000000000000..3c929b6976a6
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + */
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "rpmsg_internal.h"
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> + RPMSG_NS_CREATE = 0,
> + RPMSG_NS_DESTROY = 1,
> +};
> +
> +/**
> + * struct rpmsg_ns_msg - dynamic name service announcement message
> + * @name: name of remote service that is published
> + * @addr: address of remote service that is published
> + * @flags: indicates whether service is created or destroyed
> + *
> + * This message is sent across to publish a new service, or announce
> + * about its removal. When we receive these messages, an appropriate
> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> + * (if/as-soon-as one is registered).
> + */
> +struct rpmsg_ns_msg {
> + char name[RPMSG_NAME_SIZE];
> + __le32 addr;
> + __le32 flags;
> +} __packed;
> +
> +/* Invoked when a name service announcement arrives */
> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> + void *priv, u32 src)
> +{
> + struct rpmsg_ns_msg *msg = data;
> + struct rpmsg_device *newch;
> + struct rpmsg_channel_info chinfo;
> + struct device *dev = &rpdev->dev;
> + unsigned int flags = le32_to_cpu(msg->flags);
> + 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 = le32_to_cpu(msg->addr);
As I said in an earlier comment I would like to avoid redefining structures that
already exist. With this patch [1] we made structure rpmsg_hdr and rpmsg_ns_msg
virtIO specific, which in hindsight, wasn't the best move forward. After taking
another look at this set and keeping Guennadi's work in mind it is probably best
to keep rpmsg_hdr and rpmsg_ns_msg generic and do the byte conversion based on
the transport layer used by the rpmsg_device. Taking the above as an example we
would have:
chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
Where rpmsg32_to_cpu() would be defined as follow:
u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
{
return rpdev->ops->transport32_to_cpu(rpdev, val);
}
And in the case of a virtIO based rpmsg_device like we currently have:
u32 virtio_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);
}
That would allow the virtualisation people (and anyone else) to use the RPMSG
common code without modification and split up RPMSG name service from the
transport layer, as you're doing in this patch.
We need to fix the current code before moving further with either
patchset (yours and Guennadi's). I started working on a patchset that does that
but I'll need to merge it with yours on Monday (I'm out of time for today).
Thanks,
Mathieu
[1]. 111d1089700c rpmsg: virtio: add endianness conversions
> +
> + dev_info(dev, "%sing channel %s addr 0x%x\n",
> + (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
> + msg->name, chinfo.dst);
> +
> + if (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.17.1
>
Hi mathieu,
On 9/19/20 1:10 AM, Mathieu Poirier wrote:
> Hey Arnaud,
>
> On Tue, Aug 25, 2020 at 06:49:04PM +0200, Arnaud Pouliquen wrote:
>> The name service announcement should not be linked to the RPMsg virtio bus
>> but to the RPMsg protocol itself.
>>
>> This patch proposes to break the dependency with the RPmsg virtio bus by
>> the introduction of the reserved RPMsg name service driver which will be in
>> charge of managing the RPMsg name service announcement.
>>
>> This first patch only implements the probe and the RPMsg endpoint to
>> manage create and release channels remote requests.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/Kconfig | 8 ++
>> drivers/rpmsg/Makefile | 1 +
>> drivers/rpmsg/rpmsg_internal.h | 17 +++++
>> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++
>> 4 files changed, 161 insertions(+)
>> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index f96716893c2a..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 d5ab286d0e5e..641b48f6bf2a 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> return rpmsg_register_device(rpdev);
>> }
>>
>> +/**
>> + * rpmsg_ns_register_device() - register name service device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> + * basis for the rpmsg name service device.
>> + */
>> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> + strcpy(rpdev->id.name, "rpmsg_ns");
>> + rpdev->driver_override = "rpmsg_ns";
>> + rpdev->src = RPMSG_NS_ADDR;
>> + rpdev->dst = RPMSG_NS_ADDR;
>> +
>> + return rpmsg_register_device(rpdev);
>> +}
>> +
>> #endif
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> new file mode 100644
>> index 000000000000..3c929b6976a6
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -0,0 +1,135 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> + */
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include "rpmsg_internal.h"
>> +
>> +/**
>> + * enum rpmsg_ns_flags - dynamic name service announcement flags
>> + *
>> + * @RPMSG_NS_CREATE: a new remote service was just created
>> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>> + */
>> +enum rpmsg_ns_flags {
>> + RPMSG_NS_CREATE = 0,
>> + RPMSG_NS_DESTROY = 1,
>> +};
>> +
>> +/**
>> + * struct rpmsg_ns_msg - dynamic name service announcement message
>> + * @name: name of remote service that is published
>> + * @addr: address of remote service that is published
>> + * @flags: indicates whether service is created or destroyed
>> + *
>> + * This message is sent across to publish a new service, or announce
>> + * about its removal. When we receive these messages, an appropriate
>> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
>> + * (if/as-soon-as one is registered).
>> + */
>> +struct rpmsg_ns_msg {
>> + char name[RPMSG_NAME_SIZE];
>> + __le32 addr;
>> + __le32 flags;
>> +} __packed;
>> +
>> +/* Invoked when a name service announcement arrives */
>> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> + void *priv, u32 src)
>> +{
>> + struct rpmsg_ns_msg *msg = data;
>> + struct rpmsg_device *newch;
>> + struct rpmsg_channel_info chinfo;
>> + struct device *dev = &rpdev->dev;
>> + unsigned int flags = le32_to_cpu(msg->flags);
>> + 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 = le32_to_cpu(msg->addr);
>
> As I said in an earlier comment I would like to avoid redefining structures that
> already exist. With this patch [1] we made structure rpmsg_hdr and rpmsg_ns_msg
> virtIO specific, which in hindsight, wasn't the best move forward. After taking
> another look at this set and keeping Guennadi's work in mind it is probably best
> to keep rpmsg_hdr and rpmsg_ns_msg generic and do the byte conversion based on
> the transport layer used by the rpmsg_device. Taking the above as an example we
> would have:
>
> chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
>
> Where rpmsg32_to_cpu() would be defined as follow:
>
> u32 rpmsg32_to_cpu(struct rpmsg_device *rpdev, u32 val)
> {
> return rpdev->ops->transport32_to_cpu(rpdev, val);
> }
>
> And in the case of a virtIO based rpmsg_device like we currently have:
>
> u32 virtio_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);
> }
>
> That would allow the virtualization people (and anyone else) to use the RPMSG
> common code without modification and split up RPMSG name service from the
> transport layer, as you're doing in this patch.
This looks to me an excellent idea! More flexible than patch [1].
I'm just wondering if it wouldn't be useful to extend API to 16 and 64 bits for
future evolutions.
>
> We need to fix the current code before moving further with either
> patchset (yours and Guennadi's). I started working on a patchset that does that
> but I'll need to merge it with yours on Monday (I'm out of time for today).
That makes sense.
Thanks,
Arnaud
>
> Thanks,
> Mathieu
>
> [1]. 111d1089700c rpmsg: virtio: add endianness conversions
>
>> +
>> + dev_info(dev, "%sing channel %s addr 0x%x\n",
>> + (flags & RPMSG_NS_DESTROY) ? "destroy" : "creat",
>> + msg->name, chinfo.dst);
>> +
>> + if (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.17.1
>>