LinuxLists.cc - [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Mathieu, Arnaud,

On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.

Sorry, I just realised that my testing was incomplete. I haven't tested
automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
it probes and it's working, but if it isn't loaded and instead the rpmsg
bus driver is probed (e.g. virtio_rpmsg_bus), calling
rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
rpmsg_ns to be loaded.

Thanks
Guennadi

> Co-developed-by: Mathieu Poirier <[email protected]>
> Signed-off-by: Mathieu Poirier <[email protected]>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/Kconfig | 8 +++
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/rpmsg_ns.c | 108 +++++++++++++++++++++++++++++++
> drivers/rpmsg/virtio_rpmsg_bus.c | 86 ++++++------------------
> include/linux/rpmsg/ns.h | 18 ++++++
> 5 files changed, 154 insertions(+), 67 deletions(-)
> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index f96716893c2a..c3fc75e6514b 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -15,6 +15,14 @@ config RPMSG_CHAR
> in /dev. They make it possible for user-space programs to send and
> receive rpmsg packets.
>
> +config RPMSG_NS
> + tristate "RPMSG name service announcement"
> + depends on RPMSG
> + help
> + Say Y here to enable the support of the name service announcement
> + channel that probes the associated RPMsg device on remote endpoint
> + service announcement.
> +
> config RPMSG_MTK_SCP
> tristate "MediaTek SCP"
> depends on MTK_SCP
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index ffe932ef6050..8d452656f0ee 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_RPMSG) += rpmsg_core.o
> obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o
> +obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o
> obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o
> qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o
> obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index 000000000000..5bda7cb44618
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -0,0 +1,108 @@
> +// 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 "rpmsg_internal.h"
> +
> +/* invoked when a name service announcement arrives */
> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> + void *priv, u32 src)
> +{
> + struct rpmsg_ns_msg *msg = data;
> + struct rpmsg_device *newch;
> + struct rpmsg_channel_info chinfo;
> + struct device *dev = rpdev->dev.parent;
> + int ret;
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> + data, len, true);
> +#endif
> +
> + if (len != sizeof(*msg)) {
> + dev_err(dev, "malformed ns msg (%d)\n", len);
> + return -EINVAL;
> + }
> +
> + /* don't trust the remote processor for null terminating the name */
> + msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> +
> + strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> + chinfo.src = RPMSG_ADDR_ANY;
> + chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> +
> + dev_info(dev, "%sing channel %s addr 0x%x\n",
> + rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> + "destroy" : "creat", msg->name, chinfo.dst);
> +
> + if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) {
> + ret = rpmsg_release_channel(rpdev, &chinfo);
> + if (ret)
> + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> + } else {
> + newch = rpmsg_create_channel(rpdev, &chinfo);
> + if (!newch)
> + dev_err(dev, "rpmsg_create_channel failed\n");
> + }
> +
> + return 0;
> +}
> +
> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> +{
> + struct rpmsg_endpoint *ns_ept;
> + struct rpmsg_channel_info ns_chinfo = {
> + .src = RPMSG_NS_ADDR,
> + .dst = RPMSG_NS_ADDR,
> + .name = "name_service",
> + };
> +
> + /*
> + * Create the NS announcement service endpoint associated to the RPMsg
> + * device. The endpoint will be automatically destroyed when the RPMsg
> + * device will be deleted.
> + */
> + ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo);
> + if (!ns_ept) {
> + dev_err(&rpdev->dev, "failed to create the ns ept\n");
> + return -ENOMEM;
> + }
> + rpdev->ept = ns_ept;
> +
> + return 0;
> +}
> +
> +static struct rpmsg_driver rpmsg_ns_driver = {
> + .drv.name = "rpmsg_ns",
> + .probe = rpmsg_ns_probe,
> +};
> +
> +static int rpmsg_ns_init(void)
> +{
> + int ret;
> +
> + ret = register_rpmsg_driver(&rpmsg_ns_driver);
> + if (ret < 0)
> + pr_err("%s: Failed to register rpmsg driver\n", __func__);
> +
> + return ret;
> +}
> +postcore_initcall(rpmsg_ns_init);
> +
> +static void rpmsg_ns_exit(void)
> +{
> + unregister_rpmsg_driver(&rpmsg_ns_driver);
> +}
> +module_exit(rpmsg_ns_exit);
> +
> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> +MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> +MODULE_ALIAS("rpmsg_ns");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 6ec299f7f790..338f16c6563d 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -49,7 +49,6 @@
> * @endpoints_lock: lock of the endpoints set
> * @sendq: wait queue of sending contexts waiting for a tx buffers
> * @sleepers: number of senders that are waiting for a tx buffer
> - * @ns_ept: the bus's name service endpoint
> *
> * This structure stores the rpmsg state of a given virtio remote processor
> * device (there might be several virtio proc devices for each physical
> @@ -68,7 +67,6 @@ struct virtproc_info {
> struct mutex endpoints_lock;
> wait_queue_head_t sendq;
> atomic_t sleepers;
> - struct rpmsg_endpoint *ns_ept;
> };
>
> /* The feature bitmap for virtio rpmsg */
> @@ -815,69 +813,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;
> - bool little_endian = virtio_is_little_endian(vrp->vdev);
> - int ret;
> -
> -#if defined(CONFIG_DYNAMIC_DEBUG)
> - dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> - data, len, true);
> -#endif
> -
> - if (len != sizeof(*msg)) {
> - dev_err(dev, "malformed ns msg (%d)\n", len);
> - return -EINVAL;
> - }
> -
> - /*
> - * the name service ept does _not_ belong to a real rpmsg channel,
> - * and is handled by the rpmsg bus itself.
> - * for sanity reasons, make sure a valid rpdev has _not_ sneaked
> - * in somehow.
> - */
> - if (rpdev) {
> - dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
> - return -EINVAL;
> - }
> -
> - /* don't trust the remote processor for null terminating the name */
> - msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> -
> - strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> - chinfo.src = RPMSG_ADDR_ANY;
> - chinfo.dst = __rpmsg32_to_cpu(little_endian, msg->addr);
> -
> - dev_info(dev, "%sing channel %s addr 0x%x\n",
> - __rpmsg32_to_cpu(little_endian, msg->flags) & RPMSG_NS_DESTROY ?
> - "destroy" : "creat", msg->name, chinfo.dst);
> -
> - if (__rpmsg32_to_cpu(little_endian, 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;
> @@ -953,14 +896,26 @@ 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->little_endian = virtio_is_little_endian(vrp->vdev);
> +
> + rpdev_ns->dev.parent = &vrp->vdev->dev;
> + rpdev_ns->dev.release = virtio_rpmsg_release_device;
> +
> + err = rpmsg_ns_register_device(rpdev_ns);
> + if (err)
> + goto free_coherent;
> }
>
> /*
> @@ -1013,9 +968,6 @@ static void rpmsg_remove(struct virtio_device *vdev)
> if (ret)
> dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
>
> - if (vrp->ns_ept)
> - __rpmsg_destroy_ept(vrp, vrp->ns_ept);
> -
> idr_destroy(&vrp->endpoints);
>
> vdev->config->del_vqs(vrp->vdev);
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index 73ecc91dc26f..e267dd5f909b 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -4,6 +4,7 @@
> #define _LINUX_RPMSG_NS_H
>
> #include <linux/mod_devicetable.h>
> +#include <linux/rpmsg.h>
> #include <linux/rpmsg/byteorder.h>
> #include <linux/types.h>
>
> @@ -39,4 +40,21 @@ enum rpmsg_ns_flags {
> /* Address 53 is reserved for advertising remote services */
> #define RPMSG_NS_ADDR (53)
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, "rpmsg_ns");
> + rpdev->driver_override = "rpmsg_ns";
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +
> #endif
> --
> 2.25.1
>

2020-11-06 14:04:05

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> Hi Mathieu, Arnaud,
>
> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
>
> Sorry, I just realised that my testing was incomplete. I haven't tested
> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> it probes and it's working, but if it isn't loaded and instead the rpmsg
> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> rpmsg_ns to be loaded.

A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
but that alone doesn't fix the problem completely - the module does load then
but not quickly enough, the NS announcement from the host / remote arrives
before rpmsg_ns has properly registered. I think the best solution would be
to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
the module name, so you could rename them to just core.c and ns.c.

Thanks
Guennadi

2020-11-06 17:56:23

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> > Hi Mathieu, Arnaud,
> >
> > On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> >
> > Sorry, I just realised that my testing was incomplete. I haven't tested
> > automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> > it probes and it's working, but if it isn't loaded and instead the rpmsg
> > bus driver is probed (e.g. virtio_rpmsg_bus), calling
> > rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> > rpmsg_ns to be loaded.
>
> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> but that alone doesn't fix the problem completely - the module does load then
> but not quickly enough, the NS announcement from the host / remote arrives
> before rpmsg_ns has properly registered. I think the best solution would be
> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> the module name, so you could rename them to just core.c and ns.c.

I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
before the kernel has finished loading the name space driver. There has to be
a way to prevent that from happening - I will investigate further.

Thanks for reporting this,
Mathieu

>
> Thanks
> Guennadi

2020-11-09 08:50:46

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Guennadi, Mathieu,

On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
>>> Hi Mathieu, Arnaud,
>>>
>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
>>>
>>> Sorry, I just realised that my testing was incomplete. I haven't tested
>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
>>> rpmsg_ns to be loaded.
>>
>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
>> but that alone doesn't fix the problem completely - the module does load then
>> but not quickly enough, the NS announcement from the host / remote arrives
>> before rpmsg_ns has properly registered. I think the best solution would be
>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
>> the module name, so you could rename them to just core.c and ns.c.
>
> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> before the kernel has finished loading the name space driver. There has to be
> a way to prevent that from happening - I will investigate further.

Right, no dependency is set so the rpmsg_ns driver is never probed...
And name service announcement messages are dropped if the service is not present.

if rpmsg_virtio_bus is built-in
-> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that rpmsg_ns is also built-in
if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
-> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c

Thanks,
Arnaud

>
> Thanks for reporting this,
> Mathieu
>
>>
>> Thanks
>> Guennadi

2020-11-09 10:25:56

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Arnaud,

On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi, Mathieu,
>
> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> > On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> >> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> >>> Hi Mathieu, Arnaud,
> >>>
> >>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> >>>
> >>> Sorry, I just realised that my testing was incomplete. I haven't tested
> >>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> >>> it probes and it's working, but if it isn't loaded and instead the rpmsg
> >>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> >>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> >>> rpmsg_ns to be loaded.
> >>
> >> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> >> but that alone doesn't fix the problem completely - the module does load then
> >> but not quickly enough, the NS announcement from the host / remote arrives
> >> before rpmsg_ns has properly registered. I think the best solution would be
> >> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> >> the module name, so you could rename them to just core.c and ns.c.
> >
> > I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> > before the kernel has finished loading the name space driver. There has to be
> > a way to prevent that from happening - I will investigate further.
>
> Right, no dependency is set so the rpmsg_ns driver is never probed...
> And name service announcement messages are dropped if the service is not present.

The mentioned change

-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:rpmsg_ns");

is actually a compulsory fix, without it the driver doesn't even get loaded when
a device id registered, using rpmsg_ns_register_device(). So this has to be done
as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
still doesn't fix the problem relyably because of timing. I've merged both the
RPMsg core and NS into a single module, which fixed the issue for me. I'm
appending a patch to this email, but since it's a "fixup" please, feel free to
roll it into the original work. But thinking about it, even linking modules
together doesn't guarantee the order. I think rpmsg_ns_register_device() should
actually actively wait for NS device probing to finish - successfully or not.
I can add a complete() / wait_for_completion() pair to the process if you like.

Thanks
Guennadi

> if rpmsg_virtio_bus is built-in
> -> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that rpmsg_ns is also built-in
> if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
> -> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c
>
> Thanks,
> Arnaud

From: Guennadi Liakhovetski <[email protected]>
Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver

Link ns.c with core.c together to guarantee immediate probing.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
---
drivers/rpmsg/Makefile | 2 +-
drivers/rpmsg/{rpmsg_core.c => core.c} | 13 ++++++++++---
drivers/rpmsg/{rpmsg_ns.c => ns.c} | 13 +++----------
include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 6 +++++-
4 files changed, 19 insertions(+), 15 deletions(-)
rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (87%)
rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (95%)

diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..5aa79e167372 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
+rpmsg_core-objs := core.o ns.o
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_core.c b/drivers/rpmsg/core.c
similarity index 99%
rename from drivers/rpmsg/rpmsg_core.c
rename to drivers/rpmsg/core.c
index 6381c1e00741..0c622cced804 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/core.c
@@ -14,6 +14,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/rpmsg.h>
+#include <linux/rpmsg/ns.h>
#include <linux/of_device.h>
#include <linux/pm_domain.h>
#include <linux/slab.h>
@@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
}
EXPORT_SYMBOL(unregister_rpmsg_driver);

-
static int __init rpmsg_init(void)
{
int ret;

ret = bus_register(&rpmsg_bus);
- if (ret)
+ if (ret) {
pr_err("failed to register rpmsg bus: %d\n", ret);
+ return ret;
+ }
+
+ ret = rpmsg_ns_init();
+ if (ret)
+ bus_unregister(&rpmsg_bus);

return ret;
}
postcore_initcall(rpmsg_init);

-static void __exit rpmsg_fini(void)
+static void rpmsg_fini(void)
{
+ rpmsg_ns_exit();
bus_unregister(&rpmsg_bus);
}
module_exit(rpmsg_fini);
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
similarity index 87%
rename from drivers/rpmsg/rpmsg_ns.c
rename to drivers/rpmsg/ns.c
index 8e26824ca328..859c587b8300 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/ns.c
@@ -7,7 +7,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/rpmsg.h>
-#include <linux/rpmsg_ns.h>
+#include <linux/rpmsg/ns.h>

#include "rpmsg_internal.h"

@@ -84,7 +84,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
.probe = rpmsg_ns_probe,
};

-static int rpmsg_ns_init(void)
+int rpmsg_ns_init(void)
{
int ret;

@@ -94,15 +94,8 @@ static int rpmsg_ns_init(void)

return ret;
}
-postcore_initcall(rpmsg_ns_init);

-static void rpmsg_ns_exit(void)
+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");
diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
similarity index 95%
rename from include/linux/rpmsg_ns.h
rename to include/linux/rpmsg/ns.h
index 42786bb759b5..2838788c8448 100644
--- a/include/linux/rpmsg_ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -4,8 +4,9 @@
#define _LINUX_RPMSG_NS_H

#include <linux/mod_devicetable.h>
-#include <linux/types.h>
+#include <linux/rpmsg.h>
#include <linux/rpmsg_byteorder.h>
+#include <linux/types.h>

/**
* struct rpmsg_ns_msg - dynamic name service announcement message
@@ -56,4 +57,7 @@ static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
return rpmsg_register_device(rpdev);
}

+int rpmsg_ns_init(void);
+void rpmsg_ns_exit(void);
+
#endif
--
2.28.0

2020-11-09 17:59:50

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
> Hi Arnaud,
>
> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
> > Hi Guennadi, Mathieu,
> >
> > On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> > > On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> > >> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> > >>> Hi Mathieu, Arnaud,
> > >>>
> > >>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> > >>>
> > >>> Sorry, I just realised that my testing was incomplete. I haven't tested
> > >>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> > >>> it probes and it's working, but if it isn't loaded and instead the rpmsg
> > >>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> > >>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> > >>> rpmsg_ns to be loaded.
> > >>
> > >> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> > >> but that alone doesn't fix the problem completely - the module does load then
> > >> but not quickly enough, the NS announcement from the host / remote arrives
> > >> before rpmsg_ns has properly registered. I think the best solution would be
> > >> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> > >> the module name, so you could rename them to just core.c and ns.c.
> > >
> > > I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> > > before the kernel has finished loading the name space driver. There has to be
> > > a way to prevent that from happening - I will investigate further.
> >
> > Right, no dependency is set so the rpmsg_ns driver is never probed...
> > And name service announcement messages are dropped if the service is not present.
>
> The mentioned change
>
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");

Yes, I'm good with that part.

>
> is actually a compulsory fix, without it the driver doesn't even get loaded when
> a device id registered, using rpmsg_ns_register_device(). So this has to be done
> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
> still doesn't fix the problem relyably because of timing. I've merged both the
> RPMsg core and NS into a single module, which fixed the issue for me. I'm
> appending a patch to this email, but since it's a "fixup" please, feel free to
> roll it into the original work. But thinking about it, even linking modules
> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
> actually actively wait for NS device probing to finish - successfully or not.
> I can add a complete() / wait_for_completion() pair to the process if you like.
>

Working with a completion is the kind of thing I had in mind. But I would still
like to keep the drivers separate and that's the part I need to think about.

> Thanks
> Guennadi
>
> > if rpmsg_virtio_bus is built-in
> > -> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that rpmsg_ns is also built-in
> > if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
> > -> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c
> >
> > Thanks,
> > Arnaud
>
> From: Guennadi Liakhovetski <[email protected]>
> Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver
>
> Link ns.c with core.c together to guarantee immediate probing.
>
> Signed-off-by: Guennadi Liakhovetski <[email protected]>
> ---
> drivers/rpmsg/Makefile | 2 +-
> drivers/rpmsg/{rpmsg_core.c => core.c} | 13 ++++++++++---
> drivers/rpmsg/{rpmsg_ns.c => ns.c} | 13 +++----------
> include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 6 +++++-
> 4 files changed, 19 insertions(+), 15 deletions(-)
> rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
> rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (87%)
> rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (95%)
>
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..5aa79e167372 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> +rpmsg_core-objs := core.o ns.o
> 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_core.c b/drivers/rpmsg/core.c
> similarity index 99%
> rename from drivers/rpmsg/rpmsg_core.c
> rename to drivers/rpmsg/core.c
> index 6381c1e00741..0c622cced804 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/core.c
> @@ -14,6 +14,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/rpmsg.h>
> +#include <linux/rpmsg/ns.h>
> #include <linux/of_device.h>
> #include <linux/pm_domain.h>
> #include <linux/slab.h>
> @@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
> }
> EXPORT_SYMBOL(unregister_rpmsg_driver);
>
> -
> static int __init rpmsg_init(void)
> {
> int ret;
>
> ret = bus_register(&rpmsg_bus);
> - if (ret)
> + if (ret) {
> pr_err("failed to register rpmsg bus: %d\n", ret);
> + return ret;
> + }
> +
> + ret = rpmsg_ns_init();
> + if (ret)
> + bus_unregister(&rpmsg_bus);
>
> return ret;
> }
> postcore_initcall(rpmsg_init);
>
> -static void __exit rpmsg_fini(void)
> +static void rpmsg_fini(void)
> {
> + rpmsg_ns_exit();
> bus_unregister(&rpmsg_bus);
> }
> module_exit(rpmsg_fini);
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
> similarity index 87%
> rename from drivers/rpmsg/rpmsg_ns.c
> rename to drivers/rpmsg/ns.c
> index 8e26824ca328..859c587b8300 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/ns.c
> @@ -7,7 +7,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/rpmsg.h>
> -#include <linux/rpmsg_ns.h>
> +#include <linux/rpmsg/ns.h>
>
> #include "rpmsg_internal.h"
>
> @@ -84,7 +84,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
> .probe = rpmsg_ns_probe,
> };
>
> -static int rpmsg_ns_init(void)
> +int rpmsg_ns_init(void)
> {
> int ret;
>
> @@ -94,15 +94,8 @@ static int rpmsg_ns_init(void)
>
> return ret;
> }
> -postcore_initcall(rpmsg_ns_init);
>
> -static void rpmsg_ns_exit(void)
> +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");
> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
> similarity index 95%
> rename from include/linux/rpmsg_ns.h
> rename to include/linux/rpmsg/ns.h
> index 42786bb759b5..2838788c8448 100644
> --- a/include/linux/rpmsg_ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -4,8 +4,9 @@
> #define _LINUX_RPMSG_NS_H
>
> #include <linux/mod_devicetable.h>
> -#include <linux/types.h>
> +#include <linux/rpmsg.h>
> #include <linux/rpmsg_byteorder.h>
> +#include <linux/types.h>
>
> /**
> * struct rpmsg_ns_msg - dynamic name service announcement message
> @@ -56,4 +57,7 @@ static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> return rpmsg_register_device(rpdev);
> }
>
> +int rpmsg_ns_init(void);
> +void rpmsg_ns_exit(void);
> +
> #endif
> --
> 2.28.0
>

2020-11-10 18:24:00

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Mathieu, Guennadi,

On 11/9/20 6:55 PM, Mathieu Poirier wrote:
> On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
>> Hi Arnaud,
>>
>> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
>>> Hi Guennadi, Mathieu,
>>>
>>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
>>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
>>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
>>>>>> Hi Mathieu, Arnaud,
>>>>>>
>>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
>>>>>>
>>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
>>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
>>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
>>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
>>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
>>>>>> rpmsg_ns to be loaded.
>>>>>
>>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
>>>>> but that alone doesn't fix the problem completely - the module does load then
>>>>> but not quickly enough, the NS announcement from the host / remote arrives
>>>>> before rpmsg_ns has properly registered. I think the best solution would be
>>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
>>>>> the module name, so you could rename them to just core.c and ns.c.
>>>>
>>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
>>>> before the kernel has finished loading the name space driver. There has to be
>>>> a way to prevent that from happening - I will investigate further.
>>>
>>> Right, no dependency is set so the rpmsg_ns driver is never probed...
>>> And name service announcement messages are dropped if the service is not present.
>>
>> The mentioned change
>>
>> -MODULE_ALIAS("rpmsg_ns");
>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
>
> Yes, I'm good with that part.
>
>>
>> is actually a compulsory fix, without it the driver doesn't even get loaded when
>> a device id registered, using rpmsg_ns_register_device(). So this has to be done
>> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
>> still doesn't fix the problem relyably because of timing. I've merged both the
>> RPMsg core and NS into a single module, which fixed the issue for me. I'm
>> appending a patch to this email, but since it's a "fixup" please, feel free to
>> roll it into the original work. But thinking about it, even linking modules
>> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
>> actually actively wait for NS device probing to finish - successfully or not.
>> I can add a complete() / wait_for_completion() pair to the process if you like.
>>
>
> Working with a completion is the kind of thing I had in mind. But I would still
> like to keep the drivers separate and that's the part I need to think about.

I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
What makes the fix not simple is that the virtio forces the virtio status to ready
after the probe of the virtio unit [1].
Set this status tiggs the remote processor first messages.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253

Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)

Based on my observations, I can see two alternatives.
- rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a generic name service.
- we implement a completion as proposed by Mathieu.

I tried this second solution based on the component bind mechanism.
I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
services in the future.

Regards,
Arnaud

From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
From: Arnaud Pouliquen <[email protected]>
Date: Tue, 10 Nov 2020 18:39:29 +0100
Subject: [PATCH] rpmsg_ns: add synchronization based on component mechanism

Implement the component bind mechanism to ensure that the rpmsg virtio bus
driver are probed before treating the first RPMsg.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_ns.c | 26 ++++++++++++-
drivers/rpmsg/virtio_rpmsg_bus.c | 65 ++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..057e5d1d29a0 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -2,6 +2,7 @@
/*
* Copyright (C) STMicroelectronics 2020 - All Rights Reserved
*/
+#include <linux/component.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -55,6 +56,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
return 0;
}

+static int rpmsg_ns_bind(struct device *dev, struct device *master, void *data)
+{
+ dev_info(dev, "rpmsg ns bound\n");
+
+ return 0;
+}
+
+static void rpmsg_ns_unbind(struct device *dev, struct device *master,
+ void *data)
+{
+ dev_info(dev, "rpmsg ns unbound\n");
+}
+
+static const struct component_ops rpmsg_ns_ops = {
+ .bind = rpmsg_ns_bind,
+ .unbind = rpmsg_ns_unbind,
+};
+
static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
{
struct rpmsg_endpoint *ns_ept;
@@ -63,6 +82,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
.dst = RPMSG_NS_ADDR,
.name = "name_service",
};
+ int ret;

/*
* Create the NS announcement service endpoint associated to the RPMsg
@@ -76,7 +96,9 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}
rpdev->ept = ns_ept;

- return 0;
+ ret = component_add(&rpdev->dev, &rpmsg_ns_ops);
+
+ return ret;
}

static struct rpmsg_driver rpmsg_ns_driver = {
@@ -104,5 +126,5 @@ module_exit(rpmsg_ns_exit);

MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:rpmsg_ns");
MODULE_LICENSE("GPL v2");
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 30ef4a5de4ed..c28aac1295fa 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -11,6 +11,7 @@

#define pr_fmt(fmt) "%s: " fmt, __func__

+#include <linux/component.h>
#include <linux/dma-mapping.h>
#include <linux/idr.h>
#include <linux/jiffies.h>
@@ -67,11 +68,16 @@ struct virtproc_info {
struct mutex endpoints_lock;
wait_queue_head_t sendq;
atomic_t sleepers;
+ struct component_match *match;
+ struct completion completed;
+ int bind_status;
};

/* The feature bitmap for virtio rpmsg */
#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */

+#define BIND_TIMEOUT_MS 1000
+
/**
* struct rpmsg_hdr - common header for all rpmsg messages
* @src: source address
@@ -768,6 +774,17 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
unsigned int len, msgs_received = 0;
int err;

+ /* Wait for all children to be bound */
+ if (vrp->bind_status) {
+ dev_dbg(dev, "cwait bind\n");
+ if (!wait_for_completion_timeout(&vrp->completed,
+ msecs_to_jiffies(BIND_TIMEOUT_MS)))
+ dev_err(dev, "child device(s) binding timeout\n");
+
+ if (vrp->bind_status)
+ dev_err(dev, "failed to bind RPMsg sub device(s)\n");
+ }
+
msg = virtqueue_get_buf(rvq, &len);
if (!msg) {
dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
@@ -808,6 +825,39 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
wake_up_interruptible(&vrp->sendq);
}

+static int virtio_rpmsg_compare(struct device *dev, void *data)
+{
+ return dev == data;
+}
+
+static void virtio_rpmsg_unbind(struct device *dev)
+{
+ /* undbind all child components */
+ component_unbind_all(dev, NULL);
+}
+
+static int virtio_rpmsg_bind(struct device *dev)
+{
+ struct virtio_device *vdev = dev_to_virtio(dev);
+ struct virtproc_info *vrp = vdev->priv;
+
+ dev_dbg(dev, "Bind virtio rpmsg sub devices\n");
+
+ vdev = container_of(dev, struct virtio_device, dev);
+ vrp->bind_status = component_bind_all(dev, NULL);
+ if (vrp->bind_status)
+ dev_err(dev, "bind virtio rpmsg failed\n");
+
+ complete(&vrp->completed);
+
+ return vrp->bind_status;
+}
+
+static const struct component_master_ops virtio_rpmsg_cmp_ops = {
+ .bind = virtio_rpmsg_bind,
+ .unbind = virtio_rpmsg_unbind,
+};
+
static int rpmsg_probe(struct virtio_device *vdev)
{
vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -892,6 +942,7 @@ 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)) {
vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+
if (!vch) {
err = -ENOMEM;
goto free_coherent;
@@ -911,6 +962,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
err = rpmsg_ns_register_device(rpdev_ns);
if (err)
goto free_coherent;
+ /* register a component associated to the virtio platform */
+ component_match_add_release(&vdev->dev, &vrp->match,
+ NULL, virtio_rpmsg_compare,
+ &rpdev_ns->dev);
+
+ vrp->bind_status = -ENXIO;
+ init_completion(&vrp->completed);
+ err = component_master_add_with_match(&vdev->dev,
+ &virtio_rpmsg_cmp_ops,
+ vrp->match);
+ if (err) {
+ dev_err(&vdev->dev, "failed to bind virtio rpmsg\n");
+ goto free_coherent;
+ }
}

/*
--
2.17.1

2020-11-11 00:41:22

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN <[email protected]> wrote:
>
> Hi Mathieu, Guennadi,
>
> On 11/9/20 6:55 PM, Mathieu Poirier wrote:
> > On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
> >> Hi Arnaud,
> >>
> >> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
> >>> Hi Guennadi, Mathieu,
> >>>
> >>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> >>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> >>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> >>>>>> Hi Mathieu, Arnaud,
> >>>>>>
> >>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> >>>>>>
> >>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
> >>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> >>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
> >>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> >>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> >>>>>> rpmsg_ns to be loaded.
> >>>>>
> >>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> >>>>> but that alone doesn't fix the problem completely - the module does load then
> >>>>> but not quickly enough, the NS announcement from the host / remote arrives
> >>>>> before rpmsg_ns has properly registered. I think the best solution would be
> >>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> >>>>> the module name, so you could rename them to just core.c and ns.c.
> >>>>
> >>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> >>>> before the kernel has finished loading the name space driver. There has to be
> >>>> a way to prevent that from happening - I will investigate further.
> >>>
> >>> Right, no dependency is set so the rpmsg_ns driver is never probed...
> >>> And name service announcement messages are dropped if the service is not present.
> >>
> >> The mentioned change
> >>
> >> -MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >
> > Yes, I'm good with that part.
> >
> >>
> >> is actually a compulsory fix, without it the driver doesn't even get loaded when
> >> a device id registered, using rpmsg_ns_register_device(). So this has to be done
> >> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
> >> still doesn't fix the problem relyably because of timing. I've merged both the
> >> RPMsg core and NS into a single module, which fixed the issue for me. I'm
> >> appending a patch to this email, but since it's a "fixup" please, feel free to
> >> roll it into the original work. But thinking about it, even linking modules
> >> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
> >> actually actively wait for NS device probing to finish - successfully or not.
> >> I can add a complete() / wait_for_completion() pair to the process if you like.
> >>
> >
> > Working with a completion is the kind of thing I had in mind. But I would still
> > like to keep the drivers separate and that's the part I need to think about.
>
> I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
> What makes the fix not simple is that the virtio forces the virtio status to ready
> after the probe of the virtio unit [1].
> Set this status tiggs the remote processor first messages.
>
> [1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253
>
> Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
> delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)
>
> Based on my observations, I can see two alternatives.
> - rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a generic name service.

That option joins Guennadi's vision - I think he just expressed it in
a different way. The more I think about it, the more I find that
option appealing. With the code separation already achieved in this
patchset it wouldn't be hard to implement.

> - we implement a completion as proposed by Mathieu.
>
> I tried this second solution based on the component bind mechanism.
> I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
> Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
> services in the future.
>

Wasn't familiar with the "component" infrastructure - I suppose you
stumbled on it while working on sound drivers. I have to spend more
time looking at it. But if you have time and want to spin off a new
revision that implements the library concept, I'll invest time on that
instead.

> Regards,
> Arnaud
>
> From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
> From: Arnaud Pouliquen <[email protected]>
> Date: Tue, 10 Nov 2020 18:39:29 +0100
> Subject: [PATCH] rpmsg_ns: add synchronization based on component mechanism
>
> Implement the component bind mechanism to ensure that the rpmsg virtio bus
> driver are probed before treating the first RPMsg.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/rpmsg_ns.c | 26 ++++++++++++-
> drivers/rpmsg/virtio_rpmsg_bus.c | 65 ++++++++++++++++++++++++++++++++
> 2 files changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..057e5d1d29a0 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -2,6 +2,7 @@
> /*
> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> */
> +#include <linux/component.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -55,6 +56,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> return 0;
> }
>
> +static int rpmsg_ns_bind(struct device *dev, struct device *master, void *data)
> +{
> + dev_info(dev, "rpmsg ns bound\n");
> +
> + return 0;
> +}
> +
> +static void rpmsg_ns_unbind(struct device *dev, struct device *master,
> + void *data)
> +{
> + dev_info(dev, "rpmsg ns unbound\n");
> +}
> +
> +static const struct component_ops rpmsg_ns_ops = {
> + .bind = rpmsg_ns_bind,
> + .unbind = rpmsg_ns_unbind,
> +};
> +
> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> {
> struct rpmsg_endpoint *ns_ept;
> @@ -63,6 +82,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> .dst = RPMSG_NS_ADDR,
> .name = "name_service",
> };
> + int ret;
>
> /*
> * Create the NS announcement service endpoint associated to the RPMsg
> @@ -76,7 +96,9 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> }
> rpdev->ept = ns_ept;
>
> - return 0;
> + ret = component_add(&rpdev->dev, &rpmsg_ns_ops);
> +
> + return ret;
> }
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> @@ -104,5 +126,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 30ef4a5de4ed..c28aac1295fa 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -11,6 +11,7 @@
>
> #define pr_fmt(fmt) "%s: " fmt, __func__
>
> +#include <linux/component.h>
> #include <linux/dma-mapping.h>
> #include <linux/idr.h>
> #include <linux/jiffies.h>
> @@ -67,11 +68,16 @@ struct virtproc_info {
> struct mutex endpoints_lock;
> wait_queue_head_t sendq;
> atomic_t sleepers;
> + struct component_match *match;
> + struct completion completed;
> + int bind_status;
> };
>
> /* The feature bitmap for virtio rpmsg */
> #define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
>
> +#define BIND_TIMEOUT_MS 1000
> +
> /**
> * struct rpmsg_hdr - common header for all rpmsg messages
> * @src: source address
> @@ -768,6 +774,17 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
> unsigned int len, msgs_received = 0;
> int err;
>
> + /* Wait for all children to be bound */
> + if (vrp->bind_status) {
> + dev_dbg(dev, "cwait bind\n");
> + if (!wait_for_completion_timeout(&vrp->completed,
> + msecs_to_jiffies(BIND_TIMEOUT_MS)))
> + dev_err(dev, "child device(s) binding timeout\n");
> +
> + if (vrp->bind_status)
> + dev_err(dev, "failed to bind RPMsg sub device(s)\n");
> + }
> +
> msg = virtqueue_get_buf(rvq, &len);
> if (!msg) {
> dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
> @@ -808,6 +825,39 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
> wake_up_interruptible(&vrp->sendq);
> }
>
> +static int virtio_rpmsg_compare(struct device *dev, void *data)
> +{
> + return dev == data;
> +}
> +
> +static void virtio_rpmsg_unbind(struct device *dev)
> +{
> + /* undbind all child components */
> + component_unbind_all(dev, NULL);
> +}
> +
> +static int virtio_rpmsg_bind(struct device *dev)
> +{
> + struct virtio_device *vdev = dev_to_virtio(dev);
> + struct virtproc_info *vrp = vdev->priv;
> +
> + dev_dbg(dev, "Bind virtio rpmsg sub devices\n");
> +
> + vdev = container_of(dev, struct virtio_device, dev);
> + vrp->bind_status = component_bind_all(dev, NULL);
> + if (vrp->bind_status)
> + dev_err(dev, "bind virtio rpmsg failed\n");
> +
> + complete(&vrp->completed);
> +
> + return vrp->bind_status;
> +}
> +
> +static const struct component_master_ops virtio_rpmsg_cmp_ops = {
> + .bind = virtio_rpmsg_bind,
> + .unbind = virtio_rpmsg_unbind,
> +};
> +
> static int rpmsg_probe(struct virtio_device *vdev)
> {
> vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> @@ -892,6 +942,7 @@ 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)) {
> vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> +
> if (!vch) {
> err = -ENOMEM;
> goto free_coherent;
> @@ -911,6 +962,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
> err = rpmsg_ns_register_device(rpdev_ns);
> if (err)
> goto free_coherent;
> + /* register a component associated to the virtio platform */
> + component_match_add_release(&vdev->dev, &vrp->match,
> + NULL, virtio_rpmsg_compare,
> + &rpdev_ns->dev);
> +
> + vrp->bind_status = -ENXIO;
> + init_completion(&vrp->completed);
> + err = component_master_add_with_match(&vdev->dev,
> + &virtio_rpmsg_cmp_ops,
> + vrp->match);
> + if (err) {
> + dev_err(&vdev->dev, "failed to bind virtio rpmsg\n");
> + goto free_coherent;
> + }
> }
>
> /*
> --
> 2.17.1
>
>
>
>

2020-11-11 14:51:51

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Arnaud,

On Tue, Nov 10, 2020 at 07:18:45PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu, Guennadi,
>
> On 11/9/20 6:55 PM, Mathieu Poirier wrote:
> > On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
> >> Hi Arnaud,
> >>
> >> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
> >>> Hi Guennadi, Mathieu,
> >>>
> >>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> >>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> >>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> >>>>>> Hi Mathieu, Arnaud,
> >>>>>>
> >>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> >>>>>>
> >>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
> >>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> >>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
> >>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> >>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> >>>>>> rpmsg_ns to be loaded.
> >>>>>
> >>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> >>>>> but that alone doesn't fix the problem completely - the module does load then
> >>>>> but not quickly enough, the NS announcement from the host / remote arrives
> >>>>> before rpmsg_ns has properly registered. I think the best solution would be
> >>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> >>>>> the module name, so you could rename them to just core.c and ns.c.
> >>>>
> >>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> >>>> before the kernel has finished loading the name space driver. There has to be
> >>>> a way to prevent that from happening - I will investigate further.
> >>>
> >>> Right, no dependency is set so the rpmsg_ns driver is never probed...
> >>> And name service announcement messages are dropped if the service is not present.
> >>
> >> The mentioned change
> >>
> >> -MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >
> > Yes, I'm good with that part.
> >
> >>
> >> is actually a compulsory fix, without it the driver doesn't even get loaded when
> >> a device id registered, using rpmsg_ns_register_device(). So this has to be done
> >> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
> >> still doesn't fix the problem relyably because of timing. I've merged both the
> >> RPMsg core and NS into a single module, which fixed the issue for me. I'm
> >> appending a patch to this email, but since it's a "fixup" please, feel free to
> >> roll it into the original work. But thinking about it, even linking modules
> >> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
> >> actually actively wait for NS device probing to finish - successfully or not.
> >> I can add a complete() / wait_for_completion() pair to the process if you like.
> >>
> >
> > Working with a completion is the kind of thing I had in mind. But I would still
> > like to keep the drivers separate and that's the part I need to think about.
>
> I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
> What makes the fix not simple is that the virtio forces the virtio status to ready
> after the probe of the virtio unit [1].
> Set this status tiggs the remote processor first messages.
>
> [1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253
>
> Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
> delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)

Right, as I mentioned in the email, that specific patch version only makes the
race window smaller, but doesn't close it completely. However, I think, we could
use a completion to close it fully, we discussed it with Mathieu and I think he
is working on a solution ATM.

> Based on my observations, I can see two alternatives.
> - rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a
> generic name service.

Right, this is basically the current state of the virtio_rpmsg_bus.c driver. You'd
just need to make __rpmsg_create_ept() and rpmsg_ns_cb() global and generic.

> - we implement a completion as proposed by Mathieu.

Exactly, this is the second option. And I think if we link NS with the rpmsg core
together (or use a different method to make sure it's loaded early), we can use
a completion to close the raice completely. And since the waiting and the completing
take place in the same NS driver, I think we can keep it quite simple. An updated
version of my earlier patch is below. Note, that it also fixes a memory leak in the
proposed NS implementation:

@@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
return 0;

free_coherent:
+ kfree(vch);
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:

Thanks
Guennadi

> I tried this second solution based on the component bind mechanism.
> I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
> Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
> services in the future.
>
> Regards,
> Arnaud

From: Guennadi Liakhovetski <[email protected]>
Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver

Link ns.c with core.c together to guarantee immediate probing.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
---
drivers/rpmsg/Makefile | 2 +-
drivers/rpmsg/{rpmsg_core.c => core.c} | 13 +++--
drivers/rpmsg/{rpmsg_ns.c => ns.c} | 49 ++++++++++++++-----
drivers/rpmsg/virtio_rpmsg_bus.c | 5 +-
include/linux/rpmsg.h | 4 +-
.../{rpmsg_byteorder.h => rpmsg/byteorder.h} | 0
include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 16 +++---
7 files changed, 61 insertions(+), 28 deletions(-)
rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (76%)
rename include/linux/{rpmsg_byteorder.h => rpmsg/byteorder.h} (100%)
rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (82%)

diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..5aa79e167372 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
+rpmsg_core-objs := core.o ns.o
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_core.c b/drivers/rpmsg/core.c
similarity index 99%
rename from drivers/rpmsg/rpmsg_core.c
rename to drivers/rpmsg/core.c
index 6381c1e00741..0c622cced804 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/core.c
@@ -14,6 +14,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/rpmsg.h>
+#include <linux/rpmsg/ns.h>
#include <linux/of_device.h>
#include <linux/pm_domain.h>
#include <linux/slab.h>
@@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
}
EXPORT_SYMBOL(unregister_rpmsg_driver);

-
static int __init rpmsg_init(void)
{
int ret;

ret = bus_register(&rpmsg_bus);
- if (ret)
+ if (ret) {
pr_err("failed to register rpmsg bus: %d\n", ret);
+ return ret;
+ }
+
+ ret = rpmsg_ns_init();
+ if (ret)
+ bus_unregister(&rpmsg_bus);

return ret;
}
postcore_initcall(rpmsg_init);

-static void __exit rpmsg_fini(void)
+static void rpmsg_fini(void)
{
+ rpmsg_ns_exit();
bus_unregister(&rpmsg_bus);
}
module_exit(rpmsg_fini);
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
similarity index 76%
rename from drivers/rpmsg/rpmsg_ns.c
rename to drivers/rpmsg/ns.c
index 8e26824ca328..86c011bfb62f 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/ns.c
@@ -2,15 +2,47 @@
/*
* Copyright (C) STMicroelectronics 2020 - All Rights Reserved
*/
+#include <linux/completion.h>
#include <linux/device.h>
+#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/slab.h>
#include <linux/rpmsg.h>
-#include <linux/rpmsg_ns.h>
+#include <linux/rpmsg/ns.h>
+#include <linux/slab.h>

#include "rpmsg_internal.h"

+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+ int ret;
+
+ strcpy(rpdev->id.name, "rpmsg_ns");
+ rpdev->driver_override = "rpmsg_ns";
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ ret = rpmsg_register_device(rpdev);
+ if (ret < 0)
+ return ret;
+
+ if (!wait_for_completion_timeout(&rpdev->ns_ready,
+ msecs_to_jiffies(1))) {
+ struct rpmsg_channel_info info = {
+ .name = "rpmsg_ns",
+ .src = rpdev->src,
+ .dst = rpdev->dst,
+ };
+
+ rpmsg_unregister_device(rpdev->dev.parent, &info);
+
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(rpmsg_ns_register_device);
+
/* invoked when a name service announcement arrives */
static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
void *priv, u32 src)
@@ -76,6 +108,8 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}
rpdev->ept = ns_ept;

+ complete(&rpdev->ns_ready);
+
return 0;
}

@@ -84,7 +118,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
.probe = rpmsg_ns_probe,
};

-static int rpmsg_ns_init(void)
+int rpmsg_ns_init(void)
{
int ret;

@@ -94,15 +128,8 @@ static int rpmsg_ns_init(void)

return ret;
}
-postcore_initcall(rpmsg_ns_init);

-static void rpmsg_ns_exit(void)
+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");
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 10a16be986fc..fdf00cc5f57f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,8 +19,8 @@
#include <linux/mutex.h>
#include <linux/of_device.h>
#include <linux/rpmsg.h>
-#include <linux/rpmsg_byteorder.h>
-#include <linux/rpmsg_ns.h>
+#include <linux/rpmsg/byteorder.h>
+#include <linux/rpmsg/ns.h>
#include <linux/rpmsg/virtio.h>
#include <linux/scatterlist.h>
#include <linux/slab.h>
@@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
return 0;

free_coherent:
+ kfree(vch);
dma_free_coherent(vdev->dev.parent, total_buf_space,
bufs_va, vrp->bufs_dma);
vqs_del:
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 8ee1b1dab657..71fd15ada5c0 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -10,6 +10,7 @@
#ifndef _LINUX_RPMSG_H
#define _LINUX_RPMSG_H

+#include <linux/completion.h>
#include <linux/types.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -17,7 +18,7 @@
#include <linux/kref.h>
#include <linux/mutex.h>
#include <linux/poll.h>
-#include <linux/rpmsg_byteorder.h>
+#include <linux/rpmsg/byteorder.h>

#define RPMSG_ADDR_ANY 0xFFFFFFFF

@@ -58,6 +59,7 @@ struct rpmsg_device {
struct rpmsg_endpoint *ept;
bool announce;
bool little_endian;
+ struct completion ns_ready;

const struct rpmsg_device_ops *ops;
};
diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg/byteorder.h
similarity index 100%
rename from include/linux/rpmsg_byteorder.h
rename to include/linux/rpmsg/byteorder.h
diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
similarity index 82%
rename from include/linux/rpmsg_ns.h
rename to include/linux/rpmsg/ns.h
index 42786bb759b5..2499db0c8c3d 100644
--- a/include/linux/rpmsg_ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -4,8 +4,9 @@
#define _LINUX_RPMSG_NS_H

#include <linux/mod_devicetable.h>
+#include <linux/rpmsg.h>
+#include <linux/rpmsg/byteorder.h>
#include <linux/types.h>
-#include <linux/rpmsg_byteorder.h>

/**
* struct rpmsg_ns_msg - dynamic name service announcement message
@@ -46,14 +47,9 @@ enum rpmsg_ns_flags {
* 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);
-}
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
+
+int rpmsg_ns_init(void);
+void rpmsg_ns_exit(void);

#endif
--
2.28.0

2020-11-12 09:07:49

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On 11/11/20 1:37 AM, Mathieu Poirier wrote:
> On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN <[email protected]> wrote:
>>
>> Hi Mathieu, Guennadi,
>>
>> On 11/9/20 6:55 PM, Mathieu Poirier wrote:
>>> On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
>>>> Hi Arnaud,
>>>>
>>>> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
>>>>> Hi Guennadi, Mathieu,
>>>>>
>>>>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
>>>>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
>>>>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
>>>>>>>> Hi Mathieu, Arnaud,
>>>>>>>>
>>>>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
>>>>>>>>
>>>>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
>>>>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
>>>>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
>>>>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
>>>>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
>>>>>>>> rpmsg_ns to be loaded.
>>>>>>>
>>>>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
>>>>>>> but that alone doesn't fix the problem completely - the module does load then
>>>>>>> but not quickly enough, the NS announcement from the host / remote arrives
>>>>>>> before rpmsg_ns has properly registered. I think the best solution would be
>>>>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
>>>>>>> the module name, so you could rename them to just core.c and ns.c.
>>>>>>
>>>>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
>>>>>> before the kernel has finished loading the name space driver. There has to be
>>>>>> a way to prevent that from happening - I will investigate further.
>>>>>
>>>>> Right, no dependency is set so the rpmsg_ns driver is never probed...
>>>>> And name service announcement messages are dropped if the service is not present.
>>>>
>>>> The mentioned change
>>>>
>>>> -MODULE_ALIAS("rpmsg_ns");
>>>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
>>>
>>> Yes, I'm good with that part.
>>>
>>>>
>>>> is actually a compulsory fix, without it the driver doesn't even get loaded when
>>>> a device id registered, using rpmsg_ns_register_device(). So this has to be done
>>>> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
>>>> still doesn't fix the problem relyably because of timing. I've merged both the
>>>> RPMsg core and NS into a single module, which fixed the issue for me. I'm
>>>> appending a patch to this email, but since it's a "fixup" please, feel free to
>>>> roll it into the original work. But thinking about it, even linking modules
>>>> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
>>>> actually actively wait for NS device probing to finish - successfully or not.
>>>> I can add a complete() / wait_for_completion() pair to the process if you like.
>>>>
>>>
>>> Working with a completion is the kind of thing I had in mind. But I would still
>>> like to keep the drivers separate and that's the part I need to think about.
>>
>> I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
>> What makes the fix not simple is that the virtio forces the virtio status to ready
>> after the probe of the virtio unit [1].
>> Set this status tiggs the remote processor first messages.
>>
>> [1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253
>>
>> Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
>> delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)
>>
>> Based on my observations, I can see two alternatives.
>> - rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a generic name service.
>
> That option joins Guennadi's vision - I think he just expressed it in
> a different way. The more I think about it, the more I find that
> option appealing. With the code separation already achieved in this
> patchset it wouldn't be hard to implement.

Right, similar to Guennadi's version, if we want to keep it simpler this is
probably the preferred option.
From my point of view the main requierement is that the ns announcement service
is generic.

>
>> - we implement a completion as proposed by Mathieu.
>>
>> I tried this second solution based on the component bind mechanism.
>> I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
>> Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
>> services in the future.
>>
>
> Wasn't familiar with the "component" infrastructure - I suppose you
> stumbled on it while working on sound drivers. I have to spend more
> time looking at it.

Used in DRM framework mainly, i implemented this in my RFC[1] concerning the
refactoring of the rproc_virtio in a platform driver. The idea was to ensure
that all rproc sub-devices are registered before starting the remote processor.

[1]https://lkml.org/lkml/2020/4/16/1817

The principle it to attach child components to a master component, this
relationship allows to synchronize all using component_master_add_with_match
and component_bind_all after the drivers probing step.
The drawback of this solution is that it make code more complex.

> But if you have time and want to spin off a new
> revision that implements the library concept, I'll invest time on that
> instead.

Time is always a major issue :)
No time this week, but i will try to send patches next week.

Regards,
Arnaud

>
>> Regards,
>> Arnaud
>>
>> From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
>> From: Arnaud Pouliquen <[email protected]>
>> Date: Tue, 10 Nov 2020 18:39:29 +0100
>> Subject: [PATCH] rpmsg_ns: add synchronization based on component mechanism
>>
>> Implement the component bind mechanism to ensure that the rpmsg virtio bus
>> driver are probed before treating the first RPMsg.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/rpmsg_ns.c | 26 ++++++++++++-
>> drivers/rpmsg/virtio_rpmsg_bus.c | 65 ++++++++++++++++++++++++++++++++
>> 2 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..057e5d1d29a0 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -2,6 +2,7 @@
>> /*
>> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> */
>> +#include <linux/component.h>
>> #include <linux/device.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> @@ -55,6 +56,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> return 0;
>> }
>>
>> +static int rpmsg_ns_bind(struct device *dev, struct device *master, void *data)
>> +{
>> + dev_info(dev, "rpmsg ns bound\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void rpmsg_ns_unbind(struct device *dev, struct device *master,
>> + void *data)
>> +{
>> + dev_info(dev, "rpmsg ns unbound\n");
>> +}
>> +
>> +static const struct component_ops rpmsg_ns_ops = {
>> + .bind = rpmsg_ns_bind,
>> + .unbind = rpmsg_ns_unbind,
>> +};
>> +
>> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> {
>> struct rpmsg_endpoint *ns_ept;
>> @@ -63,6 +82,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> .dst = RPMSG_NS_ADDR,
>> .name = "name_service",
>> };
>> + int ret;
>>
>> /*
>> * Create the NS announcement service endpoint associated to the RPMsg
>> @@ -76,7 +96,9 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> }
>> rpdev->ept = ns_ept;
>>
>> - return 0;
>> + ret = component_add(&rpdev->dev, &rpmsg_ns_ops);
>> +
>> + return ret;
>> }
>>
>> static struct rpmsg_driver rpmsg_ns_driver = {
>> @@ -104,5 +126,5 @@ module_exit(rpmsg_ns_exit);
>>
>> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
>> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
>> -MODULE_ALIAS("rpmsg_ns");
>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
>> MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 30ef4a5de4ed..c28aac1295fa 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -11,6 +11,7 @@
>>
>> #define pr_fmt(fmt) "%s: " fmt, __func__
>>
>> +#include <linux/component.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/idr.h>
>> #include <linux/jiffies.h>
>> @@ -67,11 +68,16 @@ struct virtproc_info {
>> struct mutex endpoints_lock;
>> wait_queue_head_t sendq;
>> atomic_t sleepers;
>> + struct component_match *match;
>> + struct completion completed;
>> + int bind_status;
>> };
>>
>> /* The feature bitmap for virtio rpmsg */
>> #define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
>>
>> +#define BIND_TIMEOUT_MS 1000
>> +
>> /**
>> * struct rpmsg_hdr - common header for all rpmsg messages
>> * @src: source address
>> @@ -768,6 +774,17 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
>> unsigned int len, msgs_received = 0;
>> int err;
>>
>> + /* Wait for all children to be bound */
>> + if (vrp->bind_status) {
>> + dev_dbg(dev, "cwait bind\n");
>> + if (!wait_for_completion_timeout(&vrp->completed,
>> + msecs_to_jiffies(BIND_TIMEOUT_MS)))
>> + dev_err(dev, "child device(s) binding timeout\n");
>> +
>> + if (vrp->bind_status)
>> + dev_err(dev, "failed to bind RPMsg sub device(s)\n");
>> + }
>> +
>> msg = virtqueue_get_buf(rvq, &len);
>> if (!msg) {
>> dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
>> @@ -808,6 +825,39 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>> wake_up_interruptible(&vrp->sendq);
>> }
>>
>> +static int virtio_rpmsg_compare(struct device *dev, void *data)
>> +{
>> + return dev == data;
>> +}
>> +
>> +static void virtio_rpmsg_unbind(struct device *dev)
>> +{
>> + /* undbind all child components */
>> + component_unbind_all(dev, NULL);
>> +}
>> +
>> +static int virtio_rpmsg_bind(struct device *dev)
>> +{
>> + struct virtio_device *vdev = dev_to_virtio(dev);
>> + struct virtproc_info *vrp = vdev->priv;
>> +
>> + dev_dbg(dev, "Bind virtio rpmsg sub devices\n");
>> +
>> + vdev = container_of(dev, struct virtio_device, dev);
>> + vrp->bind_status = component_bind_all(dev, NULL);
>> + if (vrp->bind_status)
>> + dev_err(dev, "bind virtio rpmsg failed\n");
>> +
>> + complete(&vrp->completed);
>> +
>> + return vrp->bind_status;
>> +}
>> +
>> +static const struct component_master_ops virtio_rpmsg_cmp_ops = {
>> + .bind = virtio_rpmsg_bind,
>> + .unbind = virtio_rpmsg_unbind,
>> +};
>> +
>> static int rpmsg_probe(struct virtio_device *vdev)
>> {
>> vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>> @@ -892,6 +942,7 @@ 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)) {
>> vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> +
>> if (!vch) {
>> err = -ENOMEM;
>> goto free_coherent;
>> @@ -911,6 +962,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> err = rpmsg_ns_register_device(rpdev_ns);
>> if (err)
>> goto free_coherent;
>> + /* register a component associated to the virtio platform */
>> + component_match_add_release(&vdev->dev, &vrp->match,
>> + NULL, virtio_rpmsg_compare,
>> + &rpdev_ns->dev);
>> +
>> + vrp->bind_status = -ENXIO;
>> + init_completion(&vrp->completed);
>> + err = component_master_add_with_match(&vdev->dev,
>> + &virtio_rpmsg_cmp_ops,
>> + vrp->match);
>> + if (err) {
>> + dev_err(&vdev->dev, "failed to bind virtio rpmsg\n");
>> + goto free_coherent;
>> + }
>> }
>>
>> /*
>> --
>> 2.17.1
>>
>>
>>
>>

2020-11-12 10:19:58

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Guennadi,

On 11/11/20 3:49 PM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
>
> On Tue, Nov 10, 2020 at 07:18:45PM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu, Guennadi,
>>
>> On 11/9/20 6:55 PM, Mathieu Poirier wrote:
>>> On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
>>>> Hi Arnaud,
>>>>
>>>> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
>>>>> Hi Guennadi, Mathieu,
>>>>>
>>>>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
>>>>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
>>>>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
>>>>>>>> Hi Mathieu, Arnaud,
>>>>>>>>
>>>>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
>>>>>>>>
>>>>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
>>>>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
>>>>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
>>>>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
>>>>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
>>>>>>>> rpmsg_ns to be loaded.
>>>>>>>
>>>>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
>>>>>>> but that alone doesn't fix the problem completely - the module does load then
>>>>>>> but not quickly enough, the NS announcement from the host / remote arrives
>>>>>>> before rpmsg_ns has properly registered. I think the best solution would be
>>>>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
>>>>>>> the module name, so you could rename them to just core.c and ns.c.
>>>>>>
>>>>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
>>>>>> before the kernel has finished loading the name space driver. There has to be
>>>>>> a way to prevent that from happening - I will investigate further.
>>>>>
>>>>> Right, no dependency is set so the rpmsg_ns driver is never probed...
>>>>> And name service announcement messages are dropped if the service is not present.
>>>>
>>>> The mentioned change
>>>>
>>>> -MODULE_ALIAS("rpmsg_ns");
>>>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
>>>
>>> Yes, I'm good with that part.
>>>
>>>>
>>>> is actually a compulsory fix, without it the driver doesn't even get loaded when
>>>> a device id registered, using rpmsg_ns_register_device(). So this has to be done
>>>> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
>>>> still doesn't fix the problem relyably because of timing. I've merged both the
>>>> RPMsg core and NS into a single module, which fixed the issue for me. I'm
>>>> appending a patch to this email, but since it's a "fixup" please, feel free to
>>>> roll it into the original work. But thinking about it, even linking modules
>>>> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
>>>> actually actively wait for NS device probing to finish - successfully or not.
>>>> I can add a complete() / wait_for_completion() pair to the process if you like.
>>>>
>>>
>>> Working with a completion is the kind of thing I had in mind. But I would still
>>> like to keep the drivers separate and that's the part I need to think about.
>>
>> I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
>> What makes the fix not simple is that the virtio forces the virtio status to ready
>> after the probe of the virtio unit [1].
>> Set this status tiggs the remote processor first messages.
>>
>> [1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253
>>
>> Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
>> delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)
>
> Right, as I mentioned in the email, that specific patch version only makes the
> race window smaller, but doesn't close it completely. However, I think, we could
> use a completion to close it fully, we discussed it with Mathieu and I think he
> is working on a solution ATM.
>
>> Based on my observations, I can see two alternatives.
>> - rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a
>> generic name service.
>
> Right, this is basically the current state of the virtio_rpmsg_bus.c driver. You'd
> just need to make __rpmsg_create_ept() and rpmsg_ns_cb() global and generic.

Yes this is what i have in mind, having something generic that can be called by
the RPMSg bus.

>
>> - we implement a completion as proposed by Mathieu.
>
> Exactly, this is the second option. And I think if we link NS with the rpmsg core
> together (or use a different method to make sure it's loaded early), we can use
> a completion to close the raice completely. And since the waiting and the completing
> take place in the same NS driver, I think we can keep it quite simple. An updated
> version of my earlier patch is below.

Please find comments in your patch.

>Note, that it also fixes a memory leak in the
> proposed NS implementation:
>
> @@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> return 0;
>
> free_coherent:
> + kfree(vch);
> dma_free_coherent(vdev->dev.parent, total_buf_space,
> bufs_va, vrp->bufs_dma);
> vqs_del:

Right, thanks for pointing that out!

>
> Thanks
> Guennadi
>
>> I tried this second solution based on the component bind mechanism.
>> I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
>> Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
>> services in the future.
>>
>> Regards,
>> Arnaud
>
> From: Guennadi Liakhovetski <[email protected]>
> Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver
>
> Link ns.c with core.c together to guarantee immediate probing.
>
> Signed-off-by: Guennadi Liakhovetski <[email protected]>
> ---
> drivers/rpmsg/Makefile | 2 +-
> drivers/rpmsg/{rpmsg_core.c => core.c} | 13 +++--
> drivers/rpmsg/{rpmsg_ns.c => ns.c} | 49 ++++++++++++++-----
> drivers/rpmsg/virtio_rpmsg_bus.c | 5 +-
> include/linux/rpmsg.h | 4 +-
> .../{rpmsg_byteorder.h => rpmsg/byteorder.h} | 0
> include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 16 +++---
> 7 files changed, 61 insertions(+), 28 deletions(-)
> rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
> rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (76%)
> rename include/linux/{rpmsg_byteorder.h => rpmsg/byteorder.h} (100%)
> rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (82%)
>
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..5aa79e167372 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> +rpmsg_core-objs := core.o ns.o
> 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_core.c b/drivers/rpmsg/core.c
> similarity index 99%
> rename from drivers/rpmsg/rpmsg_core.c
> rename to drivers/rpmsg/core.c
> index 6381c1e00741..0c622cced804 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/core.c
> @@ -14,6 +14,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/rpmsg.h>
> +#include <linux/rpmsg/ns.h>
> #include <linux/of_device.h>
> #include <linux/pm_domain.h>
> #include <linux/slab.h>
> @@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
> }
> EXPORT_SYMBOL(unregister_rpmsg_driver);
>
> -
> static int __init rpmsg_init(void)
> {
> int ret;
>
> ret = bus_register(&rpmsg_bus);
> - if (ret)
> + if (ret) {
> pr_err("failed to register rpmsg bus: %d\n", ret);
> + return ret;
> + }
> +
> + ret = rpmsg_ns_init();
> + if (ret)
> + bus_unregister(&rpmsg_bus);
>
> return ret;
> }
> postcore_initcall(rpmsg_init);
>
> -static void __exit rpmsg_fini(void)
> +static void rpmsg_fini(void)
> {
> + rpmsg_ns_exit();
> bus_unregister(&rpmsg_bus);
> }
> module_exit(rpmsg_fini);

The drawback of this solution is that it makes the anoucement service ns
mandatory, but it is optional because it depends on the RPMsg backend bus.
RPMsg NS should be generic but optional.
What about calling this in rpmsg_virtio?

> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
> similarity index 76%
> rename from drivers/rpmsg/rpmsg_ns.c
> rename to drivers/rpmsg/ns.c
> index 8e26824ca328..86c011bfb62f 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/ns.c
> @@ -2,15 +2,47 @@
> /*
> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> */
> +#include <linux/completion.h>
> #include <linux/device.h>
> +#include <linux/export.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/slab.h>
> #include <linux/rpmsg.h>
> -#include <linux/rpmsg_ns.h>
> +#include <linux/rpmsg/ns.h>
> +#include <linux/slab.h>
>
> #include "rpmsg_internal.h"
>
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + int ret;
> +
> + strcpy(rpdev->id.name, "rpmsg_ns");
> + rpdev->driver_override = "rpmsg_ns";
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + ret = rpmsg_register_device(rpdev);
> + if (ret < 0)
> + return ret;
> +
> + if (!wait_for_completion_timeout(&rpdev->ns_ready,
> + msecs_to_jiffies(1))) {

Does this work if called in rproc_virtio_probe? i tried a similar implementation
but it always falls in timeout because rpmsg_ns_probe never called, probably due
to the serial probing.The rpmsg_ns probe always occurs after the end of the
virtio probe.

For me the wait completion can not be called during the virtio probe. That's why
i implemented it in rpmsg_recv_done to ensure that the service is available
before first message treatment.

Thanks,
Arnaud

> + struct rpmsg_channel_info info = {
> + .name = "rpmsg_ns",
> + .src = rpdev->src,
> + .dst = rpdev->dst,
> + };
> +
> + rpmsg_unregister_device(rpdev->dev.parent, &info);
> +
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(rpmsg_ns_register_device);
> +
> /* invoked when a name service announcement arrives */
> static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> void *priv, u32 src)
> @@ -76,6 +108,8 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> }
> rpdev->ept = ns_ept;
>
> + complete(&rpdev->ns_ready);
> +
> return 0;
> }
>
> @@ -84,7 +118,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
> .probe = rpmsg_ns_probe,
> };
>
> -static int rpmsg_ns_init(void)
> +int rpmsg_ns_init(void)
> {
> int ret;
>
> @@ -94,15 +128,8 @@ static int rpmsg_ns_init(void)
>
> return ret;
> }
> -postcore_initcall(rpmsg_ns_init);
>
> -static void rpmsg_ns_exit(void)
> +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");
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 10a16be986fc..fdf00cc5f57f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,8 +19,8 @@
> #include <linux/mutex.h>
> #include <linux/of_device.h>
> #include <linux/rpmsg.h>
> -#include <linux/rpmsg_byteorder.h>
> -#include <linux/rpmsg_ns.h>
> +#include <linux/rpmsg/byteorder.h>
> +#include <linux/rpmsg/ns.h>
> #include <linux/rpmsg/virtio.h>
> #include <linux/scatterlist.h>
> #include <linux/slab.h>
> @@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> return 0;
>
> free_coherent:
> + kfree(vch);
> dma_free_coherent(vdev->dev.parent, total_buf_space,
> bufs_va, vrp->bufs_dma);
> vqs_del:
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 8ee1b1dab657..71fd15ada5c0 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -10,6 +10,7 @@
> #ifndef _LINUX_RPMSG_H
> #define _LINUX_RPMSG_H
>
> +#include <linux/completion.h>
> #include <linux/types.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -17,7 +18,7 @@
> #include <linux/kref.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> -#include <linux/rpmsg_byteorder.h>
> +#include <linux/rpmsg/byteorder.h>
>
> #define RPMSG_ADDR_ANY 0xFFFFFFFF
>
> @@ -58,6 +59,7 @@ struct rpmsg_device {
> struct rpmsg_endpoint *ept;
> bool announce;
> bool little_endian;
> + struct completion ns_ready;
>
> const struct rpmsg_device_ops *ops;
> };
> diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg/byteorder.h
> similarity index 100%
> rename from include/linux/rpmsg_byteorder.h
> rename to include/linux/rpmsg/byteorder.h
> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
> similarity index 82%
> rename from include/linux/rpmsg_ns.h
> rename to include/linux/rpmsg/ns.h
> index 42786bb759b5..2499db0c8c3d 100644
> --- a/include/linux/rpmsg_ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -4,8 +4,9 @@
> #define _LINUX_RPMSG_NS_H
>
> #include <linux/mod_devicetable.h>
> +#include <linux/rpmsg.h>
> +#include <linux/rpmsg/byteorder.h>
> #include <linux/types.h>
> -#include <linux/rpmsg_byteorder.h>
>
> /**
> * struct rpmsg_ns_msg - dynamic name service announcement message
> @@ -46,14 +47,9 @@ enum rpmsg_ns_flags {
> * 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);
> -}
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
> +
> +int rpmsg_ns_init(void);
> +void rpmsg_ns_exit(void);
>
> #endif
>

2020-11-12 11:56:31

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Arnaud,

On Thu, Nov 12, 2020 at 11:17:54AM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi,
>
> On 11/11/20 3:49 PM, Guennadi Liakhovetski wrote:
> > Hi Arnaud,

[snip]

> > From: Guennadi Liakhovetski <[email protected]>
> > Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver
> >
> > Link ns.c with core.c together to guarantee immediate probing.
> >
> > Signed-off-by: Guennadi Liakhovetski <[email protected]>
> > ---
> > drivers/rpmsg/Makefile | 2 +-
> > drivers/rpmsg/{rpmsg_core.c => core.c} | 13 +++--
> > drivers/rpmsg/{rpmsg_ns.c => ns.c} | 49 ++++++++++++++-----
> > drivers/rpmsg/virtio_rpmsg_bus.c | 5 +-
> > include/linux/rpmsg.h | 4 +-
> > .../{rpmsg_byteorder.h => rpmsg/byteorder.h} | 0
> > include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 16 +++---
> > 7 files changed, 61 insertions(+), 28 deletions(-)
> > rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
> > rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (76%)
> > rename include/linux/{rpmsg_byteorder.h => rpmsg/byteorder.h} (100%)
> > rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (82%)
> >
> > diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> > index 8d452656f0ee..5aa79e167372 100644
> > --- a/drivers/rpmsg/Makefile
> > +++ b/drivers/rpmsg/Makefile
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> > +rpmsg_core-objs := core.o ns.o
> > 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_core.c b/drivers/rpmsg/core.c
> > similarity index 99%
> > rename from drivers/rpmsg/rpmsg_core.c
> > rename to drivers/rpmsg/core.c
> > index 6381c1e00741..0c622cced804 100644
> > --- a/drivers/rpmsg/rpmsg_core.c
> > +++ b/drivers/rpmsg/core.c
> > @@ -14,6 +14,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/rpmsg.h>
> > +#include <linux/rpmsg/ns.h>
> > #include <linux/of_device.h>
> > #include <linux/pm_domain.h>
> > #include <linux/slab.h>
> > @@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
> > }
> > EXPORT_SYMBOL(unregister_rpmsg_driver);
> >
> > -
> > static int __init rpmsg_init(void)
> > {
> > int ret;
> >
> > ret = bus_register(&rpmsg_bus);
> > - if (ret)
> > + if (ret) {
> > pr_err("failed to register rpmsg bus: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = rpmsg_ns_init();
> > + if (ret)
> > + bus_unregister(&rpmsg_bus);
> >
> > return ret;
> > }
> > postcore_initcall(rpmsg_init);
> >
> > -static void __exit rpmsg_fini(void)
> > +static void rpmsg_fini(void)
> > {
> > + rpmsg_ns_exit();
> > bus_unregister(&rpmsg_bus);
> > }
> > module_exit(rpmsg_fini);
>
> The drawback of this solution is that it makes the anoucement service ns
> mandatory, but it is optional because it depends on the RPMsg backend bus.
> RPMsg NS should be generic but optional.
> What about calling this in rpmsg_virtio?

This just registers a driver. If the backend doesn't register a suitable
device by calling rpmsg_ns_register_device(); nothing happens. But if
you're concerned about wasted memory, we can make it conditional on a
configuration option.

> > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
> > similarity index 76%
> > rename from drivers/rpmsg/rpmsg_ns.c
> > rename to drivers/rpmsg/ns.c
> > index 8e26824ca328..86c011bfb62f 100644
> > --- a/drivers/rpmsg/rpmsg_ns.c
> > +++ b/drivers/rpmsg/ns.c
> > @@ -2,15 +2,47 @@
> > /*
> > * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> > */
> > +#include <linux/completion.h>
> > #include <linux/device.h>
> > +#include <linux/export.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > -#include <linux/slab.h>
> > #include <linux/rpmsg.h>
> > -#include <linux/rpmsg_ns.h>
> > +#include <linux/rpmsg/ns.h>
> > +#include <linux/slab.h>
> >
> > #include "rpmsg_internal.h"
> >
> > +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> > +{
> > + int ret;
> > +
> > + strcpy(rpdev->id.name, "rpmsg_ns");
> > + rpdev->driver_override = "rpmsg_ns";
> > + rpdev->src = RPMSG_NS_ADDR;
> > + rpdev->dst = RPMSG_NS_ADDR;
> > +
> > + ret = rpmsg_register_device(rpdev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!wait_for_completion_timeout(&rpdev->ns_ready,
> > + msecs_to_jiffies(1))) {
>
> Does this work if called in rproc_virtio_probe? i tried a similar implementation
> but it always falls in timeout because rpmsg_ns_probe never called, probably due
> to the serial probing.The rpmsg_ns probe always occurs after the end of the
> virtio probe.

It works, yes. As you see, rpmsg_register_device() is called first, that can
already result in the .probe() being called and the completion being signalled
before we actually start a wait on it. That works well. BTW, the version here is
missing a call to

+ init_completion(&rpdev->ns_ready);

right above the call to rpmsg_register_device(). But yes, it works.

> For me the wait completion can not be called during the virtio probe. That's why
> i implemented it in rpmsg_recv_done to ensure that the service is available
> before first message treatment.

Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
we begin communicating with the remote / host, e.g. by calling
virtio_device_ready() in case of the VirtIO backend, right?

Thanks
Guennadi

> Thanks,
> Arnaud
>
> > + struct rpmsg_channel_info info = {
> > + .name = "rpmsg_ns",
> > + .src = rpdev->src,
> > + .dst = rpdev->dst,
> > + };
> > +
> > + rpmsg_unregister_device(rpdev->dev.parent, &info);
> > +
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(rpmsg_ns_register_device);
> > +
> > /* invoked when a name service announcement arrives */
> > static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> > void *priv, u32 src)
> > @@ -76,6 +108,8 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> > }
> > rpdev->ept = ns_ept;
> >
> > + complete(&rpdev->ns_ready);
> > +
> > return 0;
> > }
> >
> > @@ -84,7 +118,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
> > .probe = rpmsg_ns_probe,
> > };
> >
> > -static int rpmsg_ns_init(void)
> > +int rpmsg_ns_init(void)
> > {
> > int ret;
> >
> > @@ -94,15 +128,8 @@ static int rpmsg_ns_init(void)
> >
> > return ret;
> > }
> > -postcore_initcall(rpmsg_ns_init);
> >
> > -static void rpmsg_ns_exit(void)
> > +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");
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 10a16be986fc..fdf00cc5f57f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,8 +19,8 @@
> > #include <linux/mutex.h>
> > #include <linux/of_device.h>
> > #include <linux/rpmsg.h>
> > -#include <linux/rpmsg_byteorder.h>
> > -#include <linux/rpmsg_ns.h>
> > +#include <linux/rpmsg/byteorder.h>
> > +#include <linux/rpmsg/ns.h>
> > #include <linux/rpmsg/virtio.h>
> > #include <linux/scatterlist.h>
> > #include <linux/slab.h>
> > @@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > return 0;
> >
> > free_coherent:
> > + kfree(vch);
> > dma_free_coherent(vdev->dev.parent, total_buf_space,
> > bufs_va, vrp->bufs_dma);
> > vqs_del:
> > diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> > index 8ee1b1dab657..71fd15ada5c0 100644
> > --- a/include/linux/rpmsg.h
> > +++ b/include/linux/rpmsg.h
> > @@ -10,6 +10,7 @@
> > #ifndef _LINUX_RPMSG_H
> > #define _LINUX_RPMSG_H
> >
> > +#include <linux/completion.h>
> > #include <linux/types.h>
> > #include <linux/device.h>
> > #include <linux/err.h>
> > @@ -17,7 +18,7 @@
> > #include <linux/kref.h>
> > #include <linux/mutex.h>
> > #include <linux/poll.h>
> > -#include <linux/rpmsg_byteorder.h>
> > +#include <linux/rpmsg/byteorder.h>
> >
> > #define RPMSG_ADDR_ANY 0xFFFFFFFF
> >
> > @@ -58,6 +59,7 @@ struct rpmsg_device {
> > struct rpmsg_endpoint *ept;
> > bool announce;
> > bool little_endian;
> > + struct completion ns_ready;
> >
> > const struct rpmsg_device_ops *ops;
> > };
> > diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg/byteorder.h
> > similarity index 100%
> > rename from include/linux/rpmsg_byteorder.h
> > rename to include/linux/rpmsg/byteorder.h
> > diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
> > similarity index 82%
> > rename from include/linux/rpmsg_ns.h
> > rename to include/linux/rpmsg/ns.h
> > index 42786bb759b5..2499db0c8c3d 100644
> > --- a/include/linux/rpmsg_ns.h
> > +++ b/include/linux/rpmsg/ns.h
> > @@ -4,8 +4,9 @@
> > #define _LINUX_RPMSG_NS_H
> >
> > #include <linux/mod_devicetable.h>
> > +#include <linux/rpmsg.h>
> > +#include <linux/rpmsg/byteorder.h>
> > #include <linux/types.h>
> > -#include <linux/rpmsg_byteorder.h>
> >
> > /**
> > * struct rpmsg_ns_msg - dynamic name service announcement message
> > @@ -46,14 +47,9 @@ enum rpmsg_ns_flags {
> > * 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);
> > -}
> > +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
> > +
> > +int rpmsg_ns_init(void);
> > +void rpmsg_ns_exit(void);
> >
> > #endif
> >

2020-11-12 13:33:22

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On 11/12/20 12:51 PM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
>
> On Thu, Nov 12, 2020 at 11:17:54AM +0100, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>> On 11/11/20 3:49 PM, Guennadi Liakhovetski wrote:
>>> Hi Arnaud,
>
> [snip]
>
>>> From: Guennadi Liakhovetski <[email protected]>
>>> Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver
>>>
>>> Link ns.c with core.c together to guarantee immediate probing.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <[email protected]>
>>> ---
>>> drivers/rpmsg/Makefile | 2 +-
>>> drivers/rpmsg/{rpmsg_core.c => core.c} | 13 +++--
>>> drivers/rpmsg/{rpmsg_ns.c => ns.c} | 49 ++++++++++++++-----
>>> drivers/rpmsg/virtio_rpmsg_bus.c | 5 +-
>>> include/linux/rpmsg.h | 4 +-
>>> .../{rpmsg_byteorder.h => rpmsg/byteorder.h} | 0
>>> include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 16 +++---
>>> 7 files changed, 61 insertions(+), 28 deletions(-)
>>> rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
>>> rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (76%)
>>> rename include/linux/{rpmsg_byteorder.h => rpmsg/byteorder.h} (100%)
>>> rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (82%)
>>>
>>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>>> index 8d452656f0ee..5aa79e167372 100644
>>> --- a/drivers/rpmsg/Makefile
>>> +++ b/drivers/rpmsg/Makefile
>>> @@ -1,7 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> +rpmsg_core-objs := core.o ns.o
>>> 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_core.c b/drivers/rpmsg/core.c
>>> similarity index 99%
>>> rename from drivers/rpmsg/rpmsg_core.c
>>> rename to drivers/rpmsg/core.c
>>> index 6381c1e00741..0c622cced804 100644
>>> --- a/drivers/rpmsg/rpmsg_core.c
>>> +++ b/drivers/rpmsg/core.c
>>> @@ -14,6 +14,7 @@
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/rpmsg.h>
>>> +#include <linux/rpmsg/ns.h>
>>> #include <linux/of_device.h>
>>> #include <linux/pm_domain.h>
>>> #include <linux/slab.h>
>>> @@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
>>> }
>>> EXPORT_SYMBOL(unregister_rpmsg_driver);
>>>
>>> -
>>> static int __init rpmsg_init(void)
>>> {
>>> int ret;
>>>
>>> ret = bus_register(&rpmsg_bus);
>>> - if (ret)
>>> + if (ret) {
>>> pr_err("failed to register rpmsg bus: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = rpmsg_ns_init();
>>> + if (ret)
>>> + bus_unregister(&rpmsg_bus);
>>>
>>> return ret;
>>> }
>>> postcore_initcall(rpmsg_init);
>>>
>>> -static void __exit rpmsg_fini(void)
>>> +static void rpmsg_fini(void)
>>> {
>>> + rpmsg_ns_exit();
>>> bus_unregister(&rpmsg_bus);
>>> }
>>> module_exit(rpmsg_fini);
>>
>> The drawback of this solution is that it makes the anoucement service ns
>> mandatory, but it is optional because it depends on the RPMsg backend bus.
>> RPMsg NS should be generic but optional.
>> What about calling this in rpmsg_virtio?
>
> This just registers a driver. If the backend doesn't register a suitable
> device by calling rpmsg_ns_register_device(); nothing happens. But if
> you're concerned about wasted memory, we can make it conditional on a
> configuration option.
I'm not worried about memory, but I'm trying to understand why this can't be
done in the background rather than the kernel. Doing this in the kernel can be
confusing enough to backend such as GLINK bus that does not use this service.

I saw also this alternative to keep module independent, but i did not test it yet.

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2274

>
>>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
>>> similarity index 76%
>>> rename from drivers/rpmsg/rpmsg_ns.c
>>> rename to drivers/rpmsg/ns.c
>>> index 8e26824ca328..86c011bfb62f 100644
>>> --- a/drivers/rpmsg/rpmsg_ns.c
>>> +++ b/drivers/rpmsg/ns.c
>>> @@ -2,15 +2,47 @@
>>> /*
>>> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>>> */
>>> +#include <linux/completion.h>
>>> #include <linux/device.h>
>>> +#include <linux/export.h>
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> -#include <linux/slab.h>
>>> #include <linux/rpmsg.h>
>>> -#include <linux/rpmsg_ns.h>
>>> +#include <linux/rpmsg/ns.h>
>>> +#include <linux/slab.h>
>>>
>>> #include "rpmsg_internal.h"
>>>
>>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>>> +{
>>> + int ret;
>>> +
>>> + strcpy(rpdev->id.name, "rpmsg_ns");
>>> + rpdev->driver_override = "rpmsg_ns";
>>> + rpdev->src = RPMSG_NS_ADDR;
>>> + rpdev->dst = RPMSG_NS_ADDR;
>>> +
>>> + ret = rpmsg_register_device(rpdev);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (!wait_for_completion_timeout(&rpdev->ns_ready,
>>> + msecs_to_jiffies(1))) {
>>
>> Does this work if called in rproc_virtio_probe? i tried a similar implementation
>> but it always falls in timeout because rpmsg_ns_probe never called, probably due
>> to the serial probing.The rpmsg_ns probe always occurs after the end of the
>> virtio probe.
>
> It works, yes. As you see, rpmsg_register_device() is called first, that can
> already result in the .probe() being called and the completion being signalled
> before we actually start a wait on it. That works well. BTW, the version here is
> missing a call to
>
> + init_completion(&rpdev->ns_ready);
>
> right above the call to rpmsg_register_device(). But yes, it works.
>
>> For me the wait completion can not be called during the virtio probe. That's why
>> i implemented it in rpmsg_recv_done to ensure that the service is available
>> before first message treatment.
>
> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
> we begin communicating with the remote / host, e.g. by calling
> virtio_device_ready() in case of the VirtIO backend, right?

How the module driver are probed during device registration is not cristal clear
for me here...
Your approach looks to me a good compromize, I definitively need to apply and
test you patch to well understood the associated scheduling...

Thanks,
Arnaud

>
> Thanks
> Guennadi
>
>> Thanks,
>> Arnaud
>>
>>> + struct rpmsg_channel_info info = {
>>> + .name = "rpmsg_ns",
>>> + .src = rpdev->src,
>>> + .dst = rpdev->dst,
>>> + };
>>> +
>>> + rpmsg_unregister_device(rpdev->dev.parent, &info);
>>> +
>>> + return -ETIMEDOUT;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(rpmsg_ns_register_device);
>>> +
>>> /* invoked when a name service announcement arrives */
>>> static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>>> void *priv, u32 src)
>>> @@ -76,6 +108,8 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>>> }
>>> rpdev->ept = ns_ept;
>>>
>>> + complete(&rpdev->ns_ready);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -84,7 +118,7 @@ static struct rpmsg_driver rpmsg_ns_driver = {
>>> .probe = rpmsg_ns_probe,
>>> };
>>>
>>> -static int rpmsg_ns_init(void)
>>> +int rpmsg_ns_init(void)
>>> {
>>> int ret;
>>>
>>> @@ -94,15 +128,8 @@ static int rpmsg_ns_init(void)
>>>
>>> return ret;
>>> }
>>> -postcore_initcall(rpmsg_ns_init);
>>>
>>> -static void rpmsg_ns_exit(void)
>>> +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");
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 10a16be986fc..fdf00cc5f57f 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -19,8 +19,8 @@
>>> #include <linux/mutex.h>
>>> #include <linux/of_device.h>
>>> #include <linux/rpmsg.h>
>>> -#include <linux/rpmsg_byteorder.h>
>>> -#include <linux/rpmsg_ns.h>
>>> +#include <linux/rpmsg/byteorder.h>
>>> +#include <linux/rpmsg/ns.h>
>>> #include <linux/rpmsg/virtio.h>
>>> #include <linux/scatterlist.h>
>>> #include <linux/slab.h>
>>> @@ -920,6 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> return 0;
>>>
>>> free_coherent:
>>> + kfree(vch);
>>> dma_free_coherent(vdev->dev.parent, total_buf_space,
>>> bufs_va, vrp->bufs_dma);
>>> vqs_del:
>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>> index 8ee1b1dab657..71fd15ada5c0 100644
>>> --- a/include/linux/rpmsg.h
>>> +++ b/include/linux/rpmsg.h
>>> @@ -10,6 +10,7 @@
>>> #ifndef _LINUX_RPMSG_H
>>> #define _LINUX_RPMSG_H
>>>
>>> +#include <linux/completion.h>
>>> #include <linux/types.h>
>>> #include <linux/device.h>
>>> #include <linux/err.h>
>>> @@ -17,7 +18,7 @@
>>> #include <linux/kref.h>
>>> #include <linux/mutex.h>
>>> #include <linux/poll.h>
>>> -#include <linux/rpmsg_byteorder.h>
>>> +#include <linux/rpmsg/byteorder.h>
>>>
>>> #define RPMSG_ADDR_ANY 0xFFFFFFFF
>>>
>>> @@ -58,6 +59,7 @@ struct rpmsg_device {
>>> struct rpmsg_endpoint *ept;
>>> bool announce;
>>> bool little_endian;
>>> + struct completion ns_ready;
>>>
>>> const struct rpmsg_device_ops *ops;
>>> };
>>> diff --git a/include/linux/rpmsg_byteorder.h b/include/linux/rpmsg/byteorder.h
>>> similarity index 100%
>>> rename from include/linux/rpmsg_byteorder.h
>>> rename to include/linux/rpmsg/byteorder.h
>>> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg/ns.h
>>> similarity index 82%
>>> rename from include/linux/rpmsg_ns.h
>>> rename to include/linux/rpmsg/ns.h
>>> index 42786bb759b5..2499db0c8c3d 100644
>>> --- a/include/linux/rpmsg_ns.h
>>> +++ b/include/linux/rpmsg/ns.h
>>> @@ -4,8 +4,9 @@
>>> #define _LINUX_RPMSG_NS_H
>>>
>>> #include <linux/mod_devicetable.h>
>>> +#include <linux/rpmsg.h>
>>> +#include <linux/rpmsg/byteorder.h>
>>> #include <linux/types.h>
>>> -#include <linux/rpmsg_byteorder.h>
>>>
>>> /**
>>> * struct rpmsg_ns_msg - dynamic name service announcement message
>>> @@ -46,14 +47,9 @@ enum rpmsg_ns_flags {
>>> * 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);
>>> -}
>>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
>>> +
>>> +int rpmsg_ns_init(void);
>>> +void rpmsg_ns_exit(void);
>>>
>>> #endif
>>>

2020-11-14 17:55:46

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Thu, Nov 12, 2020 at 10:04:17AM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/11/20 1:37 AM, Mathieu Poirier wrote:
> > On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN <[email protected]> wrote:
> >>
> >> Hi Mathieu, Guennadi,
> >>
> >> On 11/9/20 6:55 PM, Mathieu Poirier wrote:
> >>> On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
> >>>> Hi Arnaud,
> >>>>
> >>>> On Mon, Nov 09, 2020 at 09:48:37AM +0100, Arnaud POULIQUEN wrote:
> >>>>> Hi Guennadi, Mathieu,
> >>>>>
> >>>>> On 11/6/20 6:53 PM, Mathieu Poirier wrote:
> >>>>>> On Fri, Nov 06, 2020 at 03:00:28PM +0100, Guennadi Liakhovetski wrote:
> >>>>>>> On Fri, Nov 06, 2020 at 02:15:45PM +0100, Guennadi Liakhovetski wrote:
> >>>>>>>> Hi Mathieu, Arnaud,
> >>>>>>>>
> >>>>>>>> On Thu, Nov 05, 2020 at 03:50:28PM -0700, 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.
> >>>>>>>>
> >>>>>>>> Sorry, I just realised that my testing was incomplete. I haven't tested
> >>>>>>>> automatic module loading and indeed it doesn't work. If rpmsg_ns is loaded
> >>>>>>>> it probes and it's working, but if it isn't loaded and instead the rpmsg
> >>>>>>>> bus driver is probed (e.g. virtio_rpmsg_bus), calling
> >>>>>>>> rpmsg_ns_register_device() to create a new rpmsg_ns device doesn't cause
> >>>>>>>> rpmsg_ns to be loaded.
> >>>>>>>
> >>>>>>> A simple fix for that is using MODULE_ALIAS("rpmsg:rpmsg_ns"); in rpmsg_ns.c
> >>>>>>> but that alone doesn't fix the problem completely - the module does load then
> >>>>>>> but not quickly enough, the NS announcement from the host / remote arrives
> >>>>>>> before rpmsg_ns has properly registered. I think the best solution would be
> >>>>>>> to link rpmsg_ns.c together with rpmsg_core.c. You'll probably want to keep
> >>>>>>> the module name, so you could rename them to just core.c and ns.c.
> >>>>>>
> >>>>>> I'm pretty sure it is because virtio_device_ready() in rpmsg_probe() is called
> >>>>>> before the kernel has finished loading the name space driver. There has to be
> >>>>>> a way to prevent that from happening - I will investigate further.
> >>>>>
> >>>>> Right, no dependency is set so the rpmsg_ns driver is never probed...
> >>>>> And name service announcement messages are dropped if the service is not present.
> >>>>
> >>>> The mentioned change
> >>>>
> >>>> -MODULE_ALIAS("rpmsg_ns");
> >>>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >>>
> >>> Yes, I'm good with that part.
> >>>
> >>>>
> >>>> is actually a compulsory fix, without it the driver doesn't even get loaded when
> >>>> a device id registered, using rpmsg_ns_register_device(). So this has to be done
> >>>> as a minimum *if* we keep RPNsg NS as a separate kernel module. However, that
> >>>> still doesn't fix the problem relyably because of timing. I've merged both the
> >>>> RPMsg core and NS into a single module, which fixed the issue for me. I'm
> >>>> appending a patch to this email, but since it's a "fixup" please, feel free to
> >>>> roll it into the original work. But thinking about it, even linking modules
> >>>> together doesn't guarantee the order. I think rpmsg_ns_register_device() should
> >>>> actually actively wait for NS device probing to finish - successfully or not.
> >>>> I can add a complete() / wait_for_completion() pair to the process if you like.
> >>>>
> >>>
> >>> Working with a completion is the kind of thing I had in mind. But I would still
> >>> like to keep the drivers separate and that's the part I need to think about.
> >>
> >> I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
> >> What makes the fix not simple is that the virtio forces the virtio status to ready
> >> after the probe of the virtio unit [1].
> >> Set this status tiggs the remote processor first messages.
> >>
> >> [1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253
> >>
> >> Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
> >> delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)
> >>
> >> Based on my observations, I can see two alternatives.
> >> - rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a generic name service.
> >
> > That option joins Guennadi's vision - I think he just expressed it in
> > a different way. The more I think about it, the more I find that
> > option appealing. With the code separation already achieved in this
> > patchset it wouldn't be hard to implement.
>
> Right, similar to Guennadi's version, if we want to keep it simpler this is
> probably the preferred option.
> From my point of view the main requierement is that the ns announcement service
> is generic.
>
>
> >
> >> - we implement a completion as proposed by Mathieu.
> >>
> >> I tried this second solution based on the component bind mechanism.
> >> I added the patch at the end of the mail (the patch is a POC, so not ready for upstream).
> >> Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
> >> services in the future.
> >>
> >
> > Wasn't familiar with the "component" infrastructure - I suppose you
> > stumbled on it while working on sound drivers. I have to spend more
> > time looking at it.
>
> Used in DRM framework mainly, i implemented this in my RFC[1] concerning the
> refactoring of the rproc_virtio in a platform driver. The idea was to ensure
> that all rproc sub-devices are registered before starting the remote processor.
>
> [1]https://lkml.org/lkml/2020/4/16/1817
>
> The principle it to attach child components to a master component, this
> relationship allows to synchronize all using component_master_add_with_match
> and component_bind_all after the drivers probing step.
> The drawback of this solution is that it make code more complex.
>
> > But if you have time and want to spin off a new
> > revision that implements the library concept, I'll invest time on that
> > instead.
>
> Time is always a major issue :)
> No time this week, but i will try to send patches next week.

I agree, I'm running short on it too. I thought what you pointed out with
find_module() [1] was quite interesting. I tried it on my side but used
request_module() rather than request_module_nowait() - because we do want to
wait for the namespace driver to be loaded before moving forward. Unfortunatly
the system hung on me... I still don't know why, I will have to investigate
further.

Regarding your patch on components, I am now up to speed with the concept.
That too is interesting but likely an overkill for the current
situation. Right now we have a single driver to deal with and I think we should
keep things as simple as possible.

I'll talk to you guys on Monday,
Mathieu

[1]. https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2274

>
> Regards,
> Arnaud
>
> >
> >> Regards,
> >> Arnaud
> >>
> >> From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
> >> From: Arnaud Pouliquen <[email protected]>
> >> Date: Tue, 10 Nov 2020 18:39:29 +0100
> >> Subject: [PATCH] rpmsg_ns: add synchronization based on component mechanism
> >>
> >> Implement the component bind mechanism to ensure that the rpmsg virtio bus
> >> driver are probed before treating the first RPMsg.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> drivers/rpmsg/rpmsg_ns.c | 26 ++++++++++++-
> >> drivers/rpmsg/virtio_rpmsg_bus.c | 65 ++++++++++++++++++++++++++++++++
> >> 2 files changed, 89 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..057e5d1d29a0 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -2,6 +2,7 @@
> >> /*
> >> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> >> */
> >> +#include <linux/component.h>
> >> #include <linux/device.h>
> >> #include <linux/kernel.h>
> >> #include <linux/module.h>
> >> @@ -55,6 +56,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >> return 0;
> >> }
> >>
> >> +static int rpmsg_ns_bind(struct device *dev, struct device *master, void *data)
> >> +{
> >> + dev_info(dev, "rpmsg ns bound\n");
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void rpmsg_ns_unbind(struct device *dev, struct device *master,
> >> + void *data)
> >> +{
> >> + dev_info(dev, "rpmsg ns unbound\n");
> >> +}
> >> +
> >> +static const struct component_ops rpmsg_ns_ops = {
> >> + .bind = rpmsg_ns_bind,
> >> + .unbind = rpmsg_ns_unbind,
> >> +};
> >> +
> >> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> >> {
> >> struct rpmsg_endpoint *ns_ept;
> >> @@ -63,6 +82,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> >> .dst = RPMSG_NS_ADDR,
> >> .name = "name_service",
> >> };
> >> + int ret;
> >>
> >> /*
> >> * Create the NS announcement service endpoint associated to the RPMsg
> >> @@ -76,7 +96,9 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> >> }
> >> rpdev->ept = ns_ept;
> >>
> >> - return 0;
> >> + ret = component_add(&rpdev->dev, &rpmsg_ns_ops);
> >> +
> >> + return ret;
> >> }
> >>
> >> static struct rpmsg_driver rpmsg_ns_driver = {
> >> @@ -104,5 +126,5 @@ module_exit(rpmsg_ns_exit);
> >>
> >> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> >> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> >> -MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >> MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index 30ef4a5de4ed..c28aac1295fa 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -11,6 +11,7 @@
> >>
> >> #define pr_fmt(fmt) "%s: " fmt, __func__
> >>
> >> +#include <linux/component.h>
> >> #include <linux/dma-mapping.h>
> >> #include <linux/idr.h>
> >> #include <linux/jiffies.h>
> >> @@ -67,11 +68,16 @@ struct virtproc_info {
> >> struct mutex endpoints_lock;
> >> wait_queue_head_t sendq;
> >> atomic_t sleepers;
> >> + struct component_match *match;
> >> + struct completion completed;
> >> + int bind_status;
> >> };
> >>
> >> /* The feature bitmap for virtio rpmsg */
> >> #define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> >>
> >> +#define BIND_TIMEOUT_MS 1000
> >> +
> >> /**
> >> * struct rpmsg_hdr - common header for all rpmsg messages
> >> * @src: source address
> >> @@ -768,6 +774,17 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
> >> unsigned int len, msgs_received = 0;
> >> int err;
> >>
> >> + /* Wait for all children to be bound */
> >> + if (vrp->bind_status) {
> >> + dev_dbg(dev, "cwait bind\n");
> >> + if (!wait_for_completion_timeout(&vrp->completed,
> >> + msecs_to_jiffies(BIND_TIMEOUT_MS)))
> >> + dev_err(dev, "child device(s) binding timeout\n");
> >> +
> >> + if (vrp->bind_status)
> >> + dev_err(dev, "failed to bind RPMsg sub device(s)\n");
> >> + }
> >> +
> >> msg = virtqueue_get_buf(rvq, &len);
> >> if (!msg) {
> >> dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
> >> @@ -808,6 +825,39 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
> >> wake_up_interruptible(&vrp->sendq);
> >> }
> >>
> >> +static int virtio_rpmsg_compare(struct device *dev, void *data)
> >> +{
> >> + return dev == data;
> >> +}
> >> +
> >> +static void virtio_rpmsg_unbind(struct device *dev)
> >> +{
> >> + /* undbind all child components */
> >> + component_unbind_all(dev, NULL);
> >> +}
> >> +
> >> +static int virtio_rpmsg_bind(struct device *dev)
> >> +{
> >> + struct virtio_device *vdev = dev_to_virtio(dev);
> >> + struct virtproc_info *vrp = vdev->priv;
> >> +
> >> + dev_dbg(dev, "Bind virtio rpmsg sub devices\n");
> >> +
> >> + vdev = container_of(dev, struct virtio_device, dev);
> >> + vrp->bind_status = component_bind_all(dev, NULL);
> >> + if (vrp->bind_status)
> >> + dev_err(dev, "bind virtio rpmsg failed\n");
> >> +
> >> + complete(&vrp->completed);
> >> +
> >> + return vrp->bind_status;
> >> +}
> >> +
> >> +static const struct component_master_ops virtio_rpmsg_cmp_ops = {
> >> + .bind = virtio_rpmsg_bind,
> >> + .unbind = virtio_rpmsg_unbind,
> >> +};
> >> +
> >> static int rpmsg_probe(struct virtio_device *vdev)
> >> {
> >> vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> >> @@ -892,6 +942,7 @@ 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)) {
> >> vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> >> +
> >> if (!vch) {
> >> err = -ENOMEM;
> >> goto free_coherent;
> >> @@ -911,6 +962,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >> err = rpmsg_ns_register_device(rpdev_ns);
> >> if (err)
> >> goto free_coherent;
> >> + /* register a component associated to the virtio platform */
> >> + component_match_add_release(&vdev->dev, &vrp->match,
> >> + NULL, virtio_rpmsg_compare,
> >> + &rpdev_ns->dev);
> >> +
> >> + vrp->bind_status = -ENXIO;
> >> + init_completion(&vrp->completed);
> >> + err = component_master_add_with_match(&vdev->dev,
> >> + &virtio_rpmsg_cmp_ops,
> >> + vrp->match);
> >> + if (err) {
> >> + dev_err(&vdev->dev, "failed to bind virtio rpmsg\n");
> >> + goto free_coherent;
> >> + }
> >> }
> >>
> >> /*
> >> --
> >> 2.17.1
> >>
> >>
> >>
> >>

2020-11-16 14:47:01

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Guennadi, Mathieu,

On 11/12/20 2:27 PM, Arnaud POULIQUEN wrote:
>
>
> On 11/12/20 12:51 PM, Guennadi Liakhovetski wrote:
>> Hi Arnaud,
>>
>> On Thu, Nov 12, 2020 at 11:17:54AM +0100, Arnaud POULIQUEN wrote:
>>> Hi Guennadi,
>>>
>>> On 11/11/20 3:49 PM, Guennadi Liakhovetski wrote:
>>>> Hi Arnaud,
>>
>> [snip]
>>
>>>> From: Guennadi Liakhovetski <[email protected]>
>>>> Subject: [PATCH] fixup! rpmsg: Turn name service into a stand alone driver
>>>>
>>>> Link ns.c with core.c together to guarantee immediate probing.
>>>>
>>>> Signed-off-by: Guennadi Liakhovetski <[email protected]>
>>>> ---
>>>> drivers/rpmsg/Makefile | 2 +-
>>>> drivers/rpmsg/{rpmsg_core.c => core.c} | 13 +++--
>>>> drivers/rpmsg/{rpmsg_ns.c => ns.c} | 49 ++++++++++++++-----
>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 5 +-
>>>> include/linux/rpmsg.h | 4 +-
>>>> .../{rpmsg_byteorder.h => rpmsg/byteorder.h} | 0
>>>> include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 16 +++---
>>>> 7 files changed, 61 insertions(+), 28 deletions(-)
>>>> rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
>>>> rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (76%)
>>>> rename include/linux/{rpmsg_byteorder.h => rpmsg/byteorder.h} (100%)
>>>> rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (82%)
>>>>
>>>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>>>> index 8d452656f0ee..5aa79e167372 100644
>>>> --- a/drivers/rpmsg/Makefile
>>>> +++ b/drivers/rpmsg/Makefile
>>>> @@ -1,7 +1,7 @@
>>>> # SPDX-License-Identifier: GPL-2.0
>>>> +rpmsg_core-objs := core.o ns.o
>>>> 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_core.c b/drivers/rpmsg/core.c
>>>> similarity index 99%
>>>> rename from drivers/rpmsg/rpmsg_core.c
>>>> rename to drivers/rpmsg/core.c
>>>> index 6381c1e00741..0c622cced804 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/core.c
>>>> @@ -14,6 +14,7 @@
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> #include <linux/rpmsg.h>
>>>> +#include <linux/rpmsg/ns.h>
>>>> #include <linux/of_device.h>
>>>> #include <linux/pm_domain.h>
>>>> #include <linux/slab.h>
>>>> @@ -625,21 +626,27 @@ void unregister_rpmsg_driver(struct rpmsg_driver *rpdrv)
>>>> }
>>>> EXPORT_SYMBOL(unregister_rpmsg_driver);
>>>>
>>>> -
>>>> static int __init rpmsg_init(void)
>>>> {
>>>> int ret;
>>>>
>>>> ret = bus_register(&rpmsg_bus);
>>>> - if (ret)
>>>> + if (ret) {
>>>> pr_err("failed to register rpmsg bus: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = rpmsg_ns_init();
>>>> + if (ret)
>>>> + bus_unregister(&rpmsg_bus);
>>>>
>>>> return ret;
>>>> }
>>>> postcore_initcall(rpmsg_init);
>>>>
>>>> -static void __exit rpmsg_fini(void)
>>>> +static void rpmsg_fini(void)
>>>> {
>>>> + rpmsg_ns_exit();
>>>> bus_unregister(&rpmsg_bus);
>>>> }
>>>> module_exit(rpmsg_fini);
>>>
>>> The drawback of this solution is that it makes the anoucement service ns
>>> mandatory, but it is optional because it depends on the RPMsg backend bus.
>>> RPMsg NS should be generic but optional.
>>> What about calling this in rpmsg_virtio?
>>
>> This just registers a driver. If the backend doesn't register a suitable
>> device by calling rpmsg_ns_register_device(); nothing happens. But if
>> you're concerned about wasted memory, we can make it conditional on a
>> configuration option.
> I'm not worried about memory, but I'm trying to understand why this can't be
> done in the background rather than the kernel. Doing this in the kernel can be
> confusing enough to backend such as GLINK bus that does not use this service.
>
> I saw also this alternative to keep module independent, but i did not test it yet.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2274
>

[...]
I tried the find_module API, it's quite simple and seems to work well. just need
to be protected by #ifdef MODULE

I also added a select RMPS_NS in the RPMSG_VIRTIO for compatibility with the legacy.

look to me that this patch is a simple fix that should work also for the vhost...
that said, the question is can we use this API here?

I attached the patch at the end of the mail.

>>
>> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
>> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
>> we begin communicating with the remote / host, e.g. by calling
>> virtio_device_ready() in case of the VirtIO backend, right?
>
> How the module driver are probed during device registration is not cristal clear
> for me here...
> Your approach looks to me a good compromize, I definitively need to apply and
> test you patch to well understood the associated scheduling...

I looked in code, trying to understand behavior on device registration.

my understanding is: if driver is already registered (basic built-in or module
previously loaded) the driver is probed on device registration. No asynchronous
probing through a work queue or other.

So it seems, (but i'm not enough expert to be 100% sure) that ensuring that the
rmpsg_ns module is loaded (and so driver registered) before the device register
is enough, no completion mechanism is needed here.

Regards,
Arnaud

>
> Thanks,
> Arnaud
>
>>
>> Thanks
>> Guennadi
>>
>>> Thanks,
>>> Arnaud
>>>

[...]

From 2629298ef1b7eea7a3a7df245abba03914c09e6b Mon Sep 17 00:00:00 2001
From: Arnaud Pouliquen <[email protected]>
Date: Mon, 16 Nov 2020 15:07:14 +0100
Subject: [PATCH] rpmsg: specify dependency between virtio rpmsg and virtio_ns

The rpmsg_ns service has to be registered before the first
message reception. When used as module, this imply and
dependency of the rpmsg virtio on the rpmsg_ns module.
this patch solve the dependency issue.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/Kconfig | 1 +
drivers/rpmsg/rpmsg_ns.c | 2 +-
drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
3 files changed, 24 insertions(+), 1 deletion(-)

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_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..f19f3cbd2526 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -104,5 +104,5 @@ module_exit(rpmsg_ns_exit);

MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:rpmsg_ns");
MODULE_LICENSE("GPL v2");
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 338f16c6563d..f032e6c3f9a9 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -1001,6 +1001,28 @@ static int __init rpmsg_init(void)
{
int ret;

+#ifdef MODULE
+ static const char name[] = "rpmsg_ns";
+ struct module *ns;
+
+ /*
+ * Make sur that the rpmsg ns module is loaded in case of module.
+ * This ensures that the rpmsg_ns driver is probed immediately when the
+ * associated device is registered during the rpmsg virtio probe.
+ */
+ mutex_lock(&module_mutex);
+ ns = find_module(name);
+ mutex_unlock(&module_mutex);
+
+ if (!ns) {
+ ret = request_module(name);
+ if (ret) {
+ pr_err("can not find %s module (err %d)\n", name, ret);
+ return ret;
+ }
+ }
+#endif
+
ret = register_virtio_driver(&virtio_ipc_driver);
if (ret)
pr_err("failed to register virtio driver: %d\n", ret);
--
2.17.1

2020-11-16 15:15:28

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Arnaud,

On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi, Mathieu,

[snip]

> I tried the find_module API, it's quite simple and seems to work well. just need
> to be protected by #ifdef MODULE
>
> I also added a select RMPS_NS in the RPMSG_VIRTIO for compatibility with the legacy.
>
> look to me that this patch is a simple fix that should work also for the vhost...
> that said, the question is can we use this API here?
>
> I attached the patch at the end of the mail.

Thanks for the patch. Yes, this would guarantee, that the NS module is loaded. But
does it also guarantee, that the NS probing completes faster than an NS announcement
arrives? We have a race here:

rpmsg_ns_register_device() -----------------\
| |
virtio_device_ready() |
| |
remote sends NS announcement rpmsg_register_device()
| |
| rpmsg_ns_probe()
| |
| rpmsg_create_ept()
rpmsg_ns_cb()

In practice we *should* be fine - maybe the whole probing (the right branch) happens
synchronously on the same core as where rpmsg_ns_register_device() was called, or
even if not, the probing runs locally and the NS announcement either talks to a
remote core or a VM. So, it *should* be safe, but unless we can make guarantee, that
the probing is synchronous, I wouldn't rely on this. So, without a completion this
still seems incomplete to me.

Thanks
Guennadi

> >> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
> >> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
> >> we begin communicating with the remote / host, e.g. by calling
> >> virtio_device_ready() in case of the VirtIO backend, right?
> >
> > How the module driver are probed during device registration is not cristal clear
> > for me here...
> > Your approach looks to me a good compromize, I definitively need to apply and
> > test you patch to well understood the associated scheduling...
>
> I looked in code, trying to understand behavior on device registration.
>
> my understanding is: if driver is already registered (basic built-in or module
> previously loaded) the driver is probed on device registration. No asynchronous
> probing through a work queue or other.
>
> So it seems, (but i'm not enough expert to be 100% sure) that ensuring that the
> rmpsg_ns module is loaded (and so driver registered) before the device register
> is enough, no completion mechanism is needed here.
>
> Regards,
> Arnaud
>
> >
> > Thanks,
> > Arnaud
> >
> >>
> >> Thanks
> >> Guennadi
> >>
> >>> Thanks,
> >>> Arnaud
> >>>
>
> [...]
>
> From 2629298ef1b7eea7a3a7df245abba03914c09e6b Mon Sep 17 00:00:00 2001
> From: Arnaud Pouliquen <[email protected]>
> Date: Mon, 16 Nov 2020 15:07:14 +0100
> Subject: [PATCH] rpmsg: specify dependency between virtio rpmsg and virtio_ns
>
> The rpmsg_ns service has to be registered before the first
> message reception. When used as module, this imply and
> dependency of the rpmsg virtio on the rpmsg_ns module.
> this patch solve the dependency issue.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/Kconfig | 1 +
> drivers/rpmsg/rpmsg_ns.c | 2 +-
> drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..f19f3cbd2526 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -104,5 +104,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 338f16c6563d..f032e6c3f9a9 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -1001,6 +1001,28 @@ static int __init rpmsg_init(void)
> {
> int ret;
>
> +#ifdef MODULE
> + static const char name[] = "rpmsg_ns";
> + struct module *ns;
> +
> + /*
> + * Make sur that the rpmsg ns module is loaded in case of module.
> + * This ensures that the rpmsg_ns driver is probed immediately when the
> + * associated device is registered during the rpmsg virtio probe.
> + */
> + mutex_lock(&module_mutex);
> + ns = find_module(name);
> + mutex_unlock(&module_mutex);
> +
> + if (!ns) {
> + ret = request_module(name);
> + if (ret) {
> + pr_err("can not find %s module (err %d)\n", name, ret);
> + return ret;
> + }
> + }
> +#endif
> +
> ret = register_virtio_driver(&virtio_ipc_driver);
> if (ret)
> pr_err("failed to register virtio driver: %d\n", ret);
> --
> 2.17.1

2020-11-16 15:56:45

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Guennadi,

On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
>
> On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote:
>> Hi Guennadi, Mathieu,
>
> [snip]
>
>> I tried the find_module API, it's quite simple and seems to work well. just need
>> to be protected by #ifdef MODULE
>>
>> I also added a select RMPS_NS in the RPMSG_VIRTIO for compatibility with the legacy.
>>
>> look to me that this patch is a simple fix that should work also for the vhost...
>> that said, the question is can we use this API here?
>>
>> I attached the patch at the end of the mail.
>
> Thanks for the patch. Yes, this would guarantee, that the NS module is loaded. But
> does it also guarantee, that the NS probing completes faster than an NS announcement
> arrives? We have a race here:
>
> rpmsg_ns_register_device() -----------------\
> | |
> virtio_device_ready() |
> | |
> remote sends NS announcement rpmsg_register_device()
> | |
> | rpmsg_ns_probe()
> | |
> | rpmsg_create_ept()
> rpmsg_ns_cb()
>
> In practice we *should* be fine - maybe the whole probing (the right branch) happens
> synchronously on the same core as where rpmsg_ns_register_device() was called, or
> even if not, the probing runs locally and the NS announcement either talks to a
> remote core or a VM. So, it *should* be safe, but unless we can make guarantee, that
> the probing is synchronous, I wouldn't rely on this. So, without a completion this
> still seems incomplete to me.

Thanks for this description!
I tested on a dual core, and I expected what you are describing above but in
fact I observed following:

rpmsg_ns_register_device()
|
rpmsg_register_device()
|
rpmsg_ns_probe()
|
rpmsg_create_ept()
|
virtio_device_ready()
|
remote sends NS announcement
rpmsg_ns_cb()

Here is the associated call stack generated in rpmsg_ns_probe using "warn_on"

[ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54
[ 11.504106] Hardware name: STM32 (Device Tree Support)
[ 11.509271] [<c011062c>] (unwind_backtrace) from [<c010c528>]
(show_stack+0x10/0x14)
[ 11.516992] [<c010c528>] (show_stack) from [<c0ad1360>] (dump_stack+0xb8/0xcc)
[ 11.524204] [<c0ad1360>] (dump_stack) from [<c0120478>] (__warn+0xd8/0xf0)
[ 11.531066] [<c0120478>] (__warn) from [<c0acd2a0>] (warn_slowpath_fmt+0x64/0xc4)
[ 11.538547] [<c0acd2a0>] (warn_slowpath_fmt) from [<bf01505c>]
(rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
[ 11.547681] [<bf01505c>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
(rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
[ 11.557933] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067ae44>]
(really_probe+0x208/0x4f0)
[ 11.567050] [<c067ae44>] (really_probe) from [<c067b2f4>]
(driver_probe_device+0x78/0x16c)
[ 11.575305] [<c067b2f4>] (driver_probe_device) from [<c0678e44>]
(bus_for_each_drv+0x84/0xd0)
[ 11.583822] [<c0678e44>] (bus_for_each_drv) from [<c067ab9c>]
(__device_attach+0xf0/0x188)
[ 11.592075] [<c067ab9c>] (__device_attach) from [<c0679c0c>]
(bus_probe_device+0x84/0x8c)
[ 11.600248] [<c0679c0c>] (bus_probe_device) from [<c0676194>]
(device_add+0x3b0/0x7b0)
[ 11.608165] [<c0676194>] (device_add) from [<bf0003dc>]
(rpmsg_register_device+0x54/0x88 [rpmsg_core])
[ 11.617486] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
[<bf00ab84>] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus])
[ 11.628696] [<bf00ab84>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cb748>]
(virtio_dev_probe+0x1f4/0x2c4)
[ 11.638352] [<c05cb748>] (virtio_dev_probe) from [<c067ae44>]
(really_probe+0x208/0x4f0)
[ 11.646406] [<c067ae44>] (really_probe) from [<c067b2f4>]
(driver_probe_device+0x78/0x16c)
[ 11.654658] [<c067b2f4>] (driver_probe_device) from [<c067b648>]
(device_driver_attach+0x58/0x60)
[ 11.663522] [<c067b648>] (device_driver_attach) from [<c067b704>]
(__driver_attach+0xb4/0x154)
[ 11.672134] [<c067b704>] (__driver_attach) from [<c0678d64>]
(bus_for_each_dev+0x78/0xc0)
[ 11.680300] [<c0678d64>] (bus_for_each_dev) from [<c0679ebc>]
(bus_add_driver+0x170/0x20c)
[ 11.688599] [<c0679ebc>] (bus_add_driver) from [<c067c22c>]
(driver_register+0x74/0x108)
[ 11.696662] [<c067c22c>] (driver_register) from [<bf01006c>]
(rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus])
[ 11.706136] [<bf01006c>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
(do_one_initcall+0x58/0x2bc)
[ 11.713616] usb33: supplied by vdd_usb
[ 11.715500] [<c0102090>] (do_one_initcall) from [<c01b6608>]
(do_init_module+0x60/0x248)
[ 11.715525] [<c01b6608>] (do_init_module) from [<c01b86fc>]
(load_module+0x12e8/0x1638)
[ 11.715546] [<c01b86fc>] (load_module) from [<c01b8cd4>]
(sys_finit_module+0xd4/0x130)
[ 11.715575] [<c01b8cd4>] (sys_finit_module) from [<c0100060>]
(ret_fast_syscall+0x0/0x54)

Having said that, does this guarantee the probe, a good question!
Maybe you or Mathieu have the answer, not me...
So if we can't conclude, adding completion would be OK for me.

Thanks,
Arnaud

>
> Thanks
> Guennadi
>
>>>> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
>>>> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
>>>> we begin communicating with the remote / host, e.g. by calling
>>>> virtio_device_ready() in case of the VirtIO backend, right?
>>>
>>> How the module driver are probed during device registration is not cristal clear
>>> for me here...
>>> Your approach looks to me a good compromize, I definitively need to apply and
>>> test you patch to well understood the associated scheduling...
>>
>> I looked in code, trying to understand behavior on device registration.
>>
>> my understanding is: if driver is already registered (basic built-in or module
>> previously loaded) the driver is probed on device registration. No asynchronous
>> probing through a work queue or other.
>>
>> So it seems, (but i'm not enough expert to be 100% sure) that ensuring that the
>> rmpsg_ns module is loaded (and so driver registered) before the device register
>> is enough, no completion mechanism is needed here.
>>
>> Regards,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> Thanks
>>>> Guennadi
>>>>
>>>>> Thanks,
>>>>> Arnaud
>>>>>
>>
>> [...]
>>
>> From 2629298ef1b7eea7a3a7df245abba03914c09e6b Mon Sep 17 00:00:00 2001
>> From: Arnaud Pouliquen <[email protected]>
>> Date: Mon, 16 Nov 2020 15:07:14 +0100
>> Subject: [PATCH] rpmsg: specify dependency between virtio rpmsg and virtio_ns
>>
>> The rpmsg_ns service has to be registered before the first
>> message reception. When used as module, this imply and
>> dependency of the rpmsg virtio on the rpmsg_ns module.
>> this patch solve the dependency issue.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/Kconfig | 1 +
>> drivers/rpmsg/rpmsg_ns.c | 2 +-
>> drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..f19f3cbd2526 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -104,5 +104,5 @@ module_exit(rpmsg_ns_exit);
>>
>> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
>> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
>> -MODULE_ALIAS("rpmsg_ns");
>> +MODULE_ALIAS("rpmsg:rpmsg_ns");
>> MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 338f16c6563d..f032e6c3f9a9 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -1001,6 +1001,28 @@ static int __init rpmsg_init(void)
>> {
>> int ret;
>>
>> +#ifdef MODULE
>> + static const char name[] = "rpmsg_ns";
>> + struct module *ns;
>> +
>> + /*
>> + * Make sur that the rpmsg ns module is loaded in case of module.
>> + * This ensures that the rpmsg_ns driver is probed immediately when the
>> + * associated device is registered during the rpmsg virtio probe.
>> + */
>> + mutex_lock(&module_mutex);
>> + ns = find_module(name);
>> + mutex_unlock(&module_mutex);
>> +
>> + if (!ns) {
>> + ret = request_module(name);
>> + if (ret) {
>> + pr_err("can not find %s module (err %d)\n", name, ret);
>> + return ret;
>> + }
>> + }
>> +#endif
>> +
>> ret = register_virtio_driver(&virtio_ipc_driver);
>> if (ret)
>> pr_err("failed to register virtio driver: %d\n", ret);
>> --
>> 2.17.1

2020-11-16 16:23:38

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi,
>
> On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:
> > Hi Arnaud,
> >
> > On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote:
> >> Hi Guennadi, Mathieu,
> >
> > [snip]
> >
> >> I tried the find_module API, it's quite simple and seems to work well. just need
> >> to be protected by #ifdef MODULE
> >>
> >> I also added a select RMPS_NS in the RPMSG_VIRTIO for compatibility with the legacy.
> >>
> >> look to me that this patch is a simple fix that should work also for the vhost...
> >> that said, the question is can we use this API here?
> >>
> >> I attached the patch at the end of the mail.
> >
> > Thanks for the patch. Yes, this would guarantee, that the NS module is loaded. But
> > does it also guarantee, that the NS probing completes faster than an NS announcement
> > arrives? We have a race here:
> >
> > rpmsg_ns_register_device() -----------------\
> > | |
> > virtio_device_ready() |
> > | |
> > remote sends NS announcement rpmsg_register_device()
> > | |
> > | rpmsg_ns_probe()
> > | |
> > | rpmsg_create_ept()
> > rpmsg_ns_cb()
> >
> > In practice we *should* be fine - maybe the whole probing (the right branch) happens
> > synchronously on the same core as where rpmsg_ns_register_device() was called, or
> > even if not, the probing runs locally and the NS announcement either talks to a
> > remote core or a VM. So, it *should* be safe, but unless we can make guarantee, that
> > the probing is synchronous, I wouldn't rely on this. So, without a completion this
> > still seems incomplete to me.
>
> Thanks for this description!
> I tested on a dual core, and I expected what you are describing above but in
> fact I observed following:
>
> rpmsg_ns_register_device()
> |
> rpmsg_register_device()
> |
> rpmsg_ns_probe()
> |
> rpmsg_create_ept()
> |
> virtio_device_ready()
> |
> remote sends NS announcement
> rpmsg_ns_cb()
>
> Here is the associated call stack generated in rpmsg_ns_probe using "warn_on"
>
> [ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54
> [ 11.504106] Hardware name: STM32 (Device Tree Support)
> [ 11.509271] [<c011062c>] (unwind_backtrace) from [<c010c528>]
> (show_stack+0x10/0x14)
> [ 11.516992] [<c010c528>] (show_stack) from [<c0ad1360>] (dump_stack+0xb8/0xcc)
> [ 11.524204] [<c0ad1360>] (dump_stack) from [<c0120478>] (__warn+0xd8/0xf0)
> [ 11.531066] [<c0120478>] (__warn) from [<c0acd2a0>] (warn_slowpath_fmt+0x64/0xc4)
> [ 11.538547] [<c0acd2a0>] (warn_slowpath_fmt) from [<bf01505c>]
> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.547681] [<bf01505c>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.557933] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067ae44>]
> (really_probe+0x208/0x4f0)
> [ 11.567050] [<c067ae44>] (really_probe) from [<c067b2f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.575305] [<c067b2f4>] (driver_probe_device) from [<c0678e44>]
> (bus_for_each_drv+0x84/0xd0)
> [ 11.583822] [<c0678e44>] (bus_for_each_drv) from [<c067ab9c>]
> (__device_attach+0xf0/0x188)
> [ 11.592075] [<c067ab9c>] (__device_attach) from [<c0679c0c>]
> (bus_probe_device+0x84/0x8c)
> [ 11.600248] [<c0679c0c>] (bus_probe_device) from [<c0676194>]
> (device_add+0x3b0/0x7b0)
> [ 11.608165] [<c0676194>] (device_add) from [<bf0003dc>]
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.617486] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
> [<bf00ab84>] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus])
> [ 11.628696] [<bf00ab84>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cb748>]
> (virtio_dev_probe+0x1f4/0x2c4)

Right, yes, that's also what I expected to happen, but I wasn't sure whether
we can *rely* on this. It does indeed look like we can (by only looking at
your trace, without scrutinising the code). So, if everybody is happy with
this "empiric proof" we can use either this or the one-module approach
without a completion, I'd be fine either way.

Thanks
Guennadi

> [ 11.638352] [<c05cb748>] (virtio_dev_probe) from [<c067ae44>]
> (really_probe+0x208/0x4f0)
> [ 11.646406] [<c067ae44>] (really_probe) from [<c067b2f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.654658] [<c067b2f4>] (driver_probe_device) from [<c067b648>]
> (device_driver_attach+0x58/0x60)
> [ 11.663522] [<c067b648>] (device_driver_attach) from [<c067b704>]
> (__driver_attach+0xb4/0x154)
> [ 11.672134] [<c067b704>] (__driver_attach) from [<c0678d64>]
> (bus_for_each_dev+0x78/0xc0)
> [ 11.680300] [<c0678d64>] (bus_for_each_dev) from [<c0679ebc>]
> (bus_add_driver+0x170/0x20c)
> [ 11.688599] [<c0679ebc>] (bus_add_driver) from [<c067c22c>]
> (driver_register+0x74/0x108)
> [ 11.696662] [<c067c22c>] (driver_register) from [<bf01006c>]
> (rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus])
> [ 11.706136] [<bf01006c>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
> (do_one_initcall+0x58/0x2bc)
> [ 11.713616] usb33: supplied by vdd_usb
> [ 11.715500] [<c0102090>] (do_one_initcall) from [<c01b6608>]
> (do_init_module+0x60/0x248)
> [ 11.715525] [<c01b6608>] (do_init_module) from [<c01b86fc>]
> (load_module+0x12e8/0x1638)
> [ 11.715546] [<c01b86fc>] (load_module) from [<c01b8cd4>]
> (sys_finit_module+0xd4/0x130)
> [ 11.715575] [<c01b8cd4>] (sys_finit_module) from [<c0100060>]
> (ret_fast_syscall+0x0/0x54)
>
> Having said that, does this guarantee the probe, a good question!
> Maybe you or Mathieu have the answer, not me...
> So if we can't conclude, adding completion would be OK for me.
>
> Thanks,
> Arnaud
>
> >
> > Thanks
> > Guennadi
> >
> >>>> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
> >>>> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
> >>>> we begin communicating with the remote / host, e.g. by calling
> >>>> virtio_device_ready() in case of the VirtIO backend, right?
> >>>
> >>> How the module driver are probed during device registration is not cristal clear
> >>> for me here...
> >>> Your approach looks to me a good compromize, I definitively need to apply and
> >>> test you patch to well understood the associated scheduling...
> >>
> >> I looked in code, trying to understand behavior on device registration.
> >>
> >> my understanding is: if driver is already registered (basic built-in or module
> >> previously loaded) the driver is probed on device registration. No asynchronous
> >> probing through a work queue or other.
> >>
> >> So it seems, (but i'm not enough expert to be 100% sure) that ensuring that the
> >> rmpsg_ns module is loaded (and so driver registered) before the device register
> >> is enough, no completion mechanism is needed here.
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>>
> >>>> Thanks
> >>>> Guennadi
> >>>>
> >>>>> Thanks,
> >>>>> Arnaud
> >>>>>
> >>
> >> [...]
> >>
> >> From 2629298ef1b7eea7a3a7df245abba03914c09e6b Mon Sep 17 00:00:00 2001
> >> From: Arnaud Pouliquen <[email protected]>
> >> Date: Mon, 16 Nov 2020 15:07:14 +0100
> >> Subject: [PATCH] rpmsg: specify dependency between virtio rpmsg and virtio_ns
> >>
> >> The rpmsg_ns service has to be registered before the first
> >> message reception. When used as module, this imply and
> >> dependency of the rpmsg virtio on the rpmsg_ns module.
> >> this patch solve the dependency issue.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> drivers/rpmsg/Kconfig | 1 +
> >> drivers/rpmsg/rpmsg_ns.c | 2 +-
> >> drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
> >> 3 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..f19f3cbd2526 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -104,5 +104,5 @@ module_exit(rpmsg_ns_exit);
> >>
> >> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> >> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> >> -MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >> MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index 338f16c6563d..f032e6c3f9a9 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -1001,6 +1001,28 @@ static int __init rpmsg_init(void)
> >> {
> >> int ret;
> >>
> >> +#ifdef MODULE
> >> + static const char name[] = "rpmsg_ns";
> >> + struct module *ns;
> >> +
> >> + /*
> >> + * Make sur that the rpmsg ns module is loaded in case of module.
> >> + * This ensures that the rpmsg_ns driver is probed immediately when the
> >> + * associated device is registered during the rpmsg virtio probe.
> >> + */
> >> + mutex_lock(&module_mutex);
> >> + ns = find_module(name);
> >> + mutex_unlock(&module_mutex);
> >> +
> >> + if (!ns) {
> >> + ret = request_module(name);
> >> + if (ret) {
> >> + pr_err("can not find %s module (err %d)\n", name, ret);
> >> + return ret;
> >> + }
> >> + }
> >> +#endif
> >> +
> >> ret = register_virtio_driver(&virtio_ipc_driver);
> >> if (ret)
> >> pr_err("failed to register virtio driver: %d\n", ret);
> >> --
> >> 2.17.1

2020-11-16 23:04:36

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi,
>
> On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:
> > Hi Arnaud,
> >
> > On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote:
> >> Hi Guennadi, Mathieu,
> >
> > [snip]
> >
> >> I tried the find_module API, it's quite simple and seems to work well. just need
> >> to be protected by #ifdef MODULE
> >>
> >> I also added a select RMPS_NS in the RPMSG_VIRTIO for compatibility with the legacy.
> >>
> >> look to me that this patch is a simple fix that should work also for the vhost...
> >> that said, the question is can we use this API here?
> >>
> >> I attached the patch at the end of the mail.
> >
> > Thanks for the patch. Yes, this would guarantee, that the NS module is loaded. But
> > does it also guarantee, that the NS probing completes faster than an NS announcement
> > arrives? We have a race here:
> >
> > rpmsg_ns_register_device() -----------------\
> > | |
> > virtio_device_ready() |
> > | |
> > remote sends NS announcement rpmsg_register_device()
> > | |
> > | rpmsg_ns_probe()
> > | |
> > | rpmsg_create_ept()
> > rpmsg_ns_cb()
> >
> > In practice we *should* be fine - maybe the whole probing (the right branch) happens
> > synchronously on the same core as where rpmsg_ns_register_device() was called, or
> > even if not, the probing runs locally and the NS announcement either talks to a
> > remote core or a VM. So, it *should* be safe, but unless we can make guarantee, that
> > the probing is synchronous, I wouldn't rely on this. So, without a completion this
> > still seems incomplete to me.
>
> Thanks for this description!
> I tested on a dual core, and I expected what you are describing above but in
> fact I observed following:
>
> rpmsg_ns_register_device()
> |
> rpmsg_register_device()
> |
> rpmsg_ns_probe()
> |
> rpmsg_create_ept()
> |
> virtio_device_ready()
> |
> remote sends NS announcement
> rpmsg_ns_cb()
>
> Here is the associated call stack generated in rpmsg_ns_probe using "warn_on"
>
> [ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54
> [ 11.504106] Hardware name: STM32 (Device Tree Support)
> [ 11.509271] [<c011062c>] (unwind_backtrace) from [<c010c528>]
> (show_stack+0x10/0x14)
> [ 11.516992] [<c010c528>] (show_stack) from [<c0ad1360>] (dump_stack+0xb8/0xcc)
> [ 11.524204] [<c0ad1360>] (dump_stack) from [<c0120478>] (__warn+0xd8/0xf0)
> [ 11.531066] [<c0120478>] (__warn) from [<c0acd2a0>] (warn_slowpath_fmt+0x64/0xc4)
> [ 11.538547] [<c0acd2a0>] (warn_slowpath_fmt) from [<bf01505c>]
> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.547681] [<bf01505c>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.557933] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067ae44>]
> (really_probe+0x208/0x4f0)
> [ 11.567050] [<c067ae44>] (really_probe) from [<c067b2f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.575305] [<c067b2f4>] (driver_probe_device) from [<c0678e44>]
> (bus_for_each_drv+0x84/0xd0)
> [ 11.583822] [<c0678e44>] (bus_for_each_drv) from [<c067ab9c>]
> (__device_attach+0xf0/0x188)
> [ 11.592075] [<c067ab9c>] (__device_attach) from [<c0679c0c>]
> (bus_probe_device+0x84/0x8c)
> [ 11.600248] [<c0679c0c>] (bus_probe_device) from [<c0676194>]
> (device_add+0x3b0/0x7b0)
> [ 11.608165] [<c0676194>] (device_add) from [<bf0003dc>]
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.617486] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
> [<bf00ab84>] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus])
> [ 11.628696] [<bf00ab84>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cb748>]
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.638352] [<c05cb748>] (virtio_dev_probe) from [<c067ae44>]
> (really_probe+0x208/0x4f0)
> [ 11.646406] [<c067ae44>] (really_probe) from [<c067b2f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.654658] [<c067b2f4>] (driver_probe_device) from [<c067b648>]
> (device_driver_attach+0x58/0x60)
> [ 11.663522] [<c067b648>] (device_driver_attach) from [<c067b704>]
> (__driver_attach+0xb4/0x154)
> [ 11.672134] [<c067b704>] (__driver_attach) from [<c0678d64>]
> (bus_for_each_dev+0x78/0xc0)
> [ 11.680300] [<c0678d64>] (bus_for_each_dev) from [<c0679ebc>]
> (bus_add_driver+0x170/0x20c)
> [ 11.688599] [<c0679ebc>] (bus_add_driver) from [<c067c22c>]
> (driver_register+0x74/0x108)
> [ 11.696662] [<c067c22c>] (driver_register) from [<bf01006c>]
> (rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus])
> [ 11.706136] [<bf01006c>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
> (do_one_initcall+0x58/0x2bc)
> [ 11.713616] usb33: supplied by vdd_usb
> [ 11.715500] [<c0102090>] (do_one_initcall) from [<c01b6608>]
> (do_init_module+0x60/0x248)
> [ 11.715525] [<c01b6608>] (do_init_module) from [<c01b86fc>]
> (load_module+0x12e8/0x1638)
> [ 11.715546] [<c01b86fc>] (load_module) from [<c01b8cd4>]
> (sys_finit_module+0xd4/0x130)
> [ 11.715575] [<c01b8cd4>] (sys_finit_module) from [<c0100060>]
> (ret_fast_syscall+0x0/0x54)
>
> Having said that, does this guarantee the probe, a good question!
> Maybe you or Mathieu have the answer, not me...

I did a lot of probing, went deep in the bowels of the user mode helper
subsystem and looked at sys_load_module(). Especially at do_init_module() where
function do_one_initcall()[1] is called on mod->init, which happens to be
rpmsg_ns_init() where the name space driver is registered. I am confident we
can rely on this mechanism.

More comments below...

[1]. https://elixir.bootlin.com/linux/v5.10-rc3/source/kernel/module.c#L3604

> So if we can't conclude, adding completion would be OK for me.
>
> Thanks,
> Arnaud
>
> >
> > Thanks
> > Guennadi
> >
> >>>> Why can it not be called in rpmsg_ns_probe()? The only purpose of this completion
> >>>> is to make sure that rpmsg_create_ept() for the NS endpoint has completed before
> >>>> we begin communicating with the remote / host, e.g. by calling
> >>>> virtio_device_ready() in case of the VirtIO backend, right?
> >>>
> >>> How the module driver are probed during device registration is not cristal clear
> >>> for me here...
> >>> Your approach looks to me a good compromize, I definitively need to apply and
> >>> test you patch to well understood the associated scheduling...
> >>
> >> I looked in code, trying to understand behavior on device registration.
> >>
> >> my understanding is: if driver is already registered (basic built-in or module
> >> previously loaded) the driver is probed on device registration. No asynchronous
> >> probing through a work queue or other.
> >>
> >> So it seems, (but i'm not enough expert to be 100% sure) that ensuring that the
> >> rmpsg_ns module is loaded (and so driver registered) before the device register
> >> is enough, no completion mechanism is needed here.
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>>
> >>>> Thanks
> >>>> Guennadi
> >>>>
> >>>>> Thanks,
> >>>>> Arnaud
> >>>>>
> >>
> >> [...]
> >>
> >> From 2629298ef1b7eea7a3a7df245abba03914c09e6b Mon Sep 17 00:00:00 2001
> >> From: Arnaud Pouliquen <[email protected]>
> >> Date: Mon, 16 Nov 2020 15:07:14 +0100
> >> Subject: [PATCH] rpmsg: specify dependency between virtio rpmsg and virtio_ns
> >>
> >> The rpmsg_ns service has to be registered before the first
> >> message reception. When used as module, this imply and
> >> dependency of the rpmsg virtio on the rpmsg_ns module.
> >> this patch solve the dependency issue.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> drivers/rpmsg/Kconfig | 1 +
> >> drivers/rpmsg/rpmsg_ns.c | 2 +-
> >> drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++++++++++++
> >> 3 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..f19f3cbd2526 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -104,5 +104,5 @@ module_exit(rpmsg_ns_exit);
> >>
> >> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> >> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> >> -MODULE_ALIAS("rpmsg_ns");
> >> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> >> MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index 338f16c6563d..f032e6c3f9a9 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -1001,6 +1001,28 @@ static int __init rpmsg_init(void)
> >> {
> >> int ret;
> >>
> >> +#ifdef MODULE
> >> + static const char name[] = "rpmsg_ns";
> >> + struct module *ns;
> >> +
> >> + /*
> >> + * Make sur that the rpmsg ns module is loaded in case of module.
> >> + * This ensures that the rpmsg_ns driver is probed immediately when the
> >> + * associated device is registered during the rpmsg virtio probe.
> >> + */
> >> + mutex_lock(&module_mutex);
> >> + ns = find_module(name);
> >> + mutex_unlock(&module_mutex);
> >> +
> >> + if (!ns) {
> >> + ret = request_module(name);
> >> + if (ret) {
> >> + pr_err("can not find %s module (err %d)\n", name, ret);
> >> + return ret;
> >> + }
> >> + }
> >> +#endif

We came up with almost exactly the same thing:

diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..ab86b603c54e 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -81,6 +81,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)

static struct rpmsg_driver rpmsg_ns_driver = {
.drv.name = "rpmsg_ns",
+ .drv.mod_name = "rpmsg_ns",
.probe = rpmsg_ns_probe,
};

@@ -104,5 +105,5 @@ module_exit(rpmsg_ns_exit);

MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:rpmsg_ns");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
index e267dd5f909b..28251fd1b2e0 100644
--- a/include/linux/rpmsg/ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -3,6 +3,7 @@
#ifndef _LINUX_RPMSG_NS_H
#define _LINUX_RPMSG_NS_H

+#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/rpmsg.h>
#include <linux/rpmsg/byteorder.h>
@@ -49,11 +50,27 @@ enum rpmsg_ns_flags {
*/
static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
{
+ int ret;
+ struct module *rpmsg_ns;
+ const char name[] = "rpmsg_ns";
+
strcpy(rpdev->id.name, "rpmsg_ns");
rpdev->driver_override = "rpmsg_ns";
rpdev->src = RPMSG_NS_ADDR;
rpdev->dst = RPMSG_NS_ADDR;

+#ifdef CONFIG_MODULES
+ mutex_lock(&module_mutex);
+ rpmsg_ns = find_module(name);
+ mutex_unlock(&module_mutex);
+
+ if (!rpmsg_ns) {
+ ret = request_module(name);
+ if (ret)
+ pr_err("Can not find module %s (err %d)\n", name, ret);
+ }
+#endif
+

I think it is better to be in rpmsg_ns_register_devices() because it makes the
solution stand by itself, i.e no need to call the registration code from another
driver. Everything is self contained.

Also note the drv.mod_name = "rpmsg_ns" part. I took a look at find_module()
and that is what is supposed to be used.

That works on my side - please test on your setup.

> >> +
> >> ret = register_virtio_driver(&virtio_ipc_driver);
> >> if (ret)
> >> pr_err("failed to register virtio driver: %d\n", ret);
> >> --
> >> 2.17.1

2020-11-17 06:47:45

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Mathieu,

On Mon, Nov 16, 2020 at 03:40:03PM -0700, Mathieu Poirier wrote:
> On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:

[snip]

> > Having said that, does this guarantee the probe, a good question!
> > Maybe you or Mathieu have the answer, not me...
>
> I did a lot of probing, went deep in the bowels of the user mode helper
> subsystem and looked at sys_load_module(). Especially at do_init_module() where
> function do_one_initcall()[1] is called on mod->init, which happens to be
> rpmsg_ns_init() where the name space driver is registered. I am confident we
> can rely on this mechanism.

Thanks for investigating and confirming that! So, we can be confident, that
if the module is already loaded at the time when the NS device is registered,
the probing happens synchronously. Now, as for how to actually load the
module, I'd really propose to move rpmsg_ns_register_device() into the .c
file and then the problem will be resolved automatically: as a symbol
dependence the module will be loaded whenever another module, calling
rpmsg_ns_register_device() is loaded.

Thanks
Guennadi

2020-11-17 11:45:40

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

Hi Mathieu,

On 11/16/20 11:40 PM, Mathieu Poirier wrote:
> On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>> On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:

[snip]

> I did a lot of probing, went deep in the bowels of the user mode helper
> subsystem and looked at sys_load_module(). Especially at do_init_module() where
> function do_one_initcall()[1] is called on mod->init, which happens to be
> rpmsg_ns_init() where the name space driver is registered. I am confident we
> can rely on this mechanism.
>
> More comments below...
>
> [1]. https://elixir.bootlin.com/linux/v5.10-rc3/source/kernel/module.c#L3604
>
>> So if we can't conclude, adding completion would be OK for me.
>>
>> Thanks,
>> Arnaud
>>

[snip]

>
> We came up with almost exactly the same thing:>
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..ab86b603c54e 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -81,6 +81,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> .drv.name = "rpmsg_ns",
> + .drv.mod_name = "rpmsg_ns",

This does not work for me, in built-in the kernel freezes on start
i just have the " Starting kernel ..." print

Are you sure that is useful? it working for me without this.

> .probe = rpmsg_ns_probe,
> };
>
> @@ -104,5 +105,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index e267dd5f909b..28251fd1b2e0 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -3,6 +3,7 @@
> #ifndef _LINUX_RPMSG_NS_H
> #define _LINUX_RPMSG_NS_H
>
> +#include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/rpmsg.h>
> #include <linux/rpmsg/byteorder.h>
> @@ -49,11 +50,27 @@ enum rpmsg_ns_flags {
> */
> static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
Agree with the Guennadi's pertinent remarks, better to put this in rpmsg_ns.c

> {
> + int ret;
> + struct module *rpmsg_ns;
> + const char name[] = "rpmsg_ns";
> +
> strcpy(rpdev->id.name, "rpmsg_ns");
> rpdev->driver_override = "rpmsg_ns";
> rpdev->src = RPMSG_NS_ADDR;
> rpdev->dst = RPMSG_NS_ADDR;
>
> +#ifdef CONFIG_MODULES

This piece of code is executed also if rppmsg_ns is built-in

> + mutex_lock(&module_mutex);
> + rpmsg_ns = find_module(name);
> + mutex_unlock(&module_mutex);
> +
> + if (!rpmsg_ns) {
> + ret = request_module(name);
> + if (ret)
> + pr_err("Can not find module %s (err %d)\n", name, ret);

As consequence for built-in this error is printed.
To avoid this
- #ifdef CONFIG_MODULES
+ #ifdef MODULES

Then for me here we should return the error.

> + }
> +#endif
> +
>
> I think it is better to be in rpmsg_ns_register_devices() because it makes the
> solution stand by itself, i.e no need to call the registration code from another
> driver. Everything is self contained.

That's a very good idea!

In addition I would keep dependency between virtio and rpmsg_ns in kconfig. This
would ensure that rpmsg ns is built for the virtio bus. Then the device will be
probed on demand.

>
> Also note the drv.mod_name = "rpmsg_ns" part. I took a look at find_module()
> and that is what is supposed to be used.
>
> That works on my side - please test on your setup.

Please find update of your patch integrating my remarks:
- suppress drv.mod_name,
- migrate rpmsg_ns_register_device in rpmsg_ns.c,
- use KBUILD_MODNAME for the module name,
- add select RPMSG_NS for RPMSG_VIRTIO config.

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_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..80c2cc23bada 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}

+/**
+ * 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.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+#ifdef MODULES
+ int ret;
+ struct module *rpmsg_ns;
+
+ mutex_lock(&module_mutex);
+ rpmsg_ns = find_module(KBUILD_MODNAME);
+ mutex_unlock(&module_mutex);
+
+ if (!rpmsg_ns) {
+ ret = request_module(KBUILD_MODNAME);
+ if (ret)
+ pr_err("Can not find module %s (err %d)\n", KBUILD_MODNAME, ret);
+ }
+#endif
+
+ strcpy(rpdev->id.name, KBUILD_MODNAME);
+ rpdev->driver_override = KBUILD_MODNAME;
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_ns_register_device);
+
static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
{
struct rpmsg_endpoint *ns_ept;
@@ -80,7 +113,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}

static struct rpmsg_driver rpmsg_ns_driver = {
- .drv.name = "rpmsg_ns",
+ .drv.name = KBUILD_MODNAME,
.probe = rpmsg_ns_probe,
};

@@ -104,5 +137,5 @@ module_exit(rpmsg_ns_exit);

MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
index bdc1ea278814..68eac2b42075 100644
--- a/include/linux/rpmsg/ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
/* Address 53 is reserved for advertising remote services */
#define RPMSG_NS_ADDR (53)

-/**
- * rpmsg_ns_register_device() - register name service device based on rpdev
- * @rpdev: prepared rpdev to be used for creating endpoints
- *
- * This function wraps rpmsg_register_device() preparing the rpdev for use as
- * basis for the rpmsg name service device.
- */
-static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
-{
- strcpy(rpdev->id.name, "rpmsg_ns");
- rpdev->driver_override = "rpmsg_ns";
- rpdev->src = RPMSG_NS_ADDR;
- rpdev->dst = RPMSG_NS_ADDR;
-
- return rpmsg_register_device(rpdev);
-}
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev);

#endif

Regards,
Arnaud

2020-11-17 16:08:27

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:

[snip]

> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..80c2cc23bada 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * 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.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> +#ifdef MODULES
> + int ret;
> + struct module *rpmsg_ns;
> +
> + mutex_lock(&module_mutex);
> + rpmsg_ns = find_module(KBUILD_MODNAME);
> + mutex_unlock(&module_mutex);
> +
> + if (!rpmsg_ns) {
> + ret = request_module(KBUILD_MODNAME);

Is this code requesting the module in which it is located?.. I must be missing
something...

Thanks
Guennadi

2020-11-17 16:46:12

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
>
> [snip]
>
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..80c2cc23bada 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
>> *data, int len,
>> return 0;
>> }
>>
>> +/**
>> + * 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.
>> + */
>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> +#ifdef MODULES
>> + int ret;
>> + struct module *rpmsg_ns;
>> +
>> + mutex_lock(&module_mutex);
>> + rpmsg_ns = find_module(KBUILD_MODNAME);
>> + mutex_unlock(&module_mutex);
>> +
>> + if (!rpmsg_ns) {
>> + ret = request_module(KBUILD_MODNAME);
>
> Is this code requesting the module in which it is located?.. I must be missing
> something...

Right this is stupid...Thanks to highlight this!

That being said, your remark is very interesting: we need to load the module to
access to this function. This means that calling this function ensures that the
module is loaded. In this case no need to add the piece of code to find
module... here is the call stack associated (associated patch is available below):

(rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
[ 11.858748] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
(rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
[ 11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
(really_probe+0x208/0x4f0)
[ 11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
(driver_probe_device+0x78/0x16c)
[ 11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
(bus_for_each_drv+0x84/0xd0)
[ 11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
(__device_attach+0xf0/0x188)
[ 11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
(bus_probe_device+0x84/0x8c)
[ 11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
(device_add+0x3b0/0x7b0)
[ 11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
(rpmsg_register_device+0x54/0x88 [rpmsg_core])
[ 11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
[<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
[ 11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
(virtio_dev_probe+0x1f4/0x2c4)
[ 11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
(really_probe+0x208/0x4f0)
[ 11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
(driver_probe_device+0x78/0x16c)
[ 11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
(device_driver_attach+0x58/0x60)
[ 11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
(__driver_attach+0xb4/0x154)
[ 11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
(bus_for_each_dev+0x78/0xc0)
[ 11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
(bus_add_driver+0x170/0x20c)
[ 11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
(driver_register+0x74/0x108)
[ 12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
(rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
[ 12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
(do_one_initcall+0x58/0x2bc)
[

This would make the patch very simple. I tested following patch on my platform,
applying it, i do not reproduce the initial issue.

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_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..5867281188de 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}

+/**
+ * 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.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+ strcpy(rpdev->id.name, KBUILD_MODNAME);
+ rpdev->driver_override = KBUILD_MODNAME;
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_ns_register_device);
+
static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
{
struct rpmsg_endpoint *ns_ept;
@@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}

static struct rpmsg_driver rpmsg_ns_driver = {
- .drv.name = "rpmsg_ns",
+ .drv.name = KBUILD_MODNAME,
.probe = rpmsg_ns_probe,
};

@@ -104,5 +122,5 @@ module_exit(rpmsg_ns_exit);

MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
index bdc1ea278814..68eac2b42075 100644
--- a/include/linux/rpmsg/ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
/* Address 53 is reserved for advertising remote services */
#define RPMSG_NS_ADDR (53)

-/**
- * rpmsg_ns_register_device() - register name service device based on rpdev
- * @rpdev: prepared rpdev to be used for creating endpoints
- *
- * This function wraps rpmsg_register_device() preparing the rpdev for use as
- * basis for the rpmsg name service device.
- */
-static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
-{
- strcpy(rpdev->id.name, "rpmsg_ns");
- rpdev->driver_override = "rpmsg_ns";
- rpdev->src = RPMSG_NS_ADDR;
- rpdev->dst = RPMSG_NS_ADDR;
-
- return rpmsg_register_device(rpdev);
-}
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev);

#endif

Thanks,
Arnaud

>
> Thanks
> Guennadi
>

2020-11-17 17:06:02

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> > On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
> >
> > [snip]
> >
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..80c2cc23bada 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> >> *data, int len,
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * 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.
> >> + */
> >> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> +#ifdef MODULES
> >> + int ret;
> >> + struct module *rpmsg_ns;
> >> +
> >> + mutex_lock(&module_mutex);
> >> + rpmsg_ns = find_module(KBUILD_MODNAME);
> >> + mutex_unlock(&module_mutex);
> >> +
> >> + if (!rpmsg_ns) {
> >> + ret = request_module(KBUILD_MODNAME);
> >
> > Is this code requesting the module in which it is located?.. I must be missing
> > something...
>
> Right this is stupid...Thanks to highlight this!
>
> That being said, your remark is very interesting: we need to load the module to
> access to this function. This means that calling this function ensures that the
> module is loaded. In this case no need to add the piece of code to find
> module... here is the call stack associated (associated patch is available below):

Yes, as I wrote 10 hours ago:

> Now, as for how to actually load the
> module, I'd really propose to move rpmsg_ns_register_device() into the .c
> file and then the problem will be resolved automatically: as a symbol
> dependence the module will be loaded whenever another module, calling
> rpmsg_ns_register_device() is loaded.

Thanks
Guennadi

> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.858748] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
> (bus_for_each_drv+0x84/0xd0)
> [ 11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
> (__device_attach+0xf0/0x188)
> [ 11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
> (bus_probe_device+0x84/0x8c)
> [ 11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
> (device_add+0x3b0/0x7b0)
> [ 11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
> [<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
> [ 11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
> (device_driver_attach+0x58/0x60)
> [ 11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
> (__driver_attach+0xb4/0x154)
> [ 11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
> (bus_for_each_dev+0x78/0xc0)
> [ 11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
> (bus_add_driver+0x170/0x20c)
> [ 11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
> (driver_register+0x74/0x108)
> [ 12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
> [ 12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
> (do_one_initcall+0x58/0x2bc)
> [
>
> This would make the patch very simple. I tested following patch on my platform,
> applying it, i do not reproduce the initial issue.
>
>
> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..5867281188de 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * 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.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, KBUILD_MODNAME);
> + rpdev->driver_override = KBUILD_MODNAME;
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +EXPORT_SYMBOL(rpmsg_ns_register_device);
> +
> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> {
> struct rpmsg_endpoint *ns_ept;
> @@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> }
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> - .drv.name = "rpmsg_ns",
> + .drv.name = KBUILD_MODNAME,
> .probe = rpmsg_ns_probe,
> };
>
> @@ -104,5 +122,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index bdc1ea278814..68eac2b42075 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
> /* Address 53 is reserved for advertising remote services */
> #define RPMSG_NS_ADDR (53)
>
> -/**
> - * rpmsg_ns_register_device() - register name service device based on rpdev
> - * @rpdev: prepared rpdev to be used for creating endpoints
> - *
> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
> - * basis for the rpmsg name service device.
> - */
> -static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> -{
> - strcpy(rpdev->id.name, "rpmsg_ns");
> - rpdev->driver_override = "rpmsg_ns";
> - rpdev->src = RPMSG_NS_ADDR;
> - rpdev->dst = RPMSG_NS_ADDR;
> -
> - return rpmsg_register_device(rpdev);
> -}
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
>
> #endif
>
> Thanks,
> Arnaud
>
> >
> > Thanks
> > Guennadi
> >

2020-11-17 17:34:04

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On 11/17/20 5:58 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
>>> On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>>>> index 5bda7cb44618..80c2cc23bada 100644
>>>> --- a/drivers/rpmsg/rpmsg_ns.c
>>>> +++ b/drivers/rpmsg/rpmsg_ns.c
>>>> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
>>>> *data, int len,
>>>> return 0;
>>>> }
>>>>
>>>> +/**
>>>> + * 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.
>>>> + */
>>>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>>>> +{
>>>> +#ifdef MODULES
>>>> + int ret;
>>>> + struct module *rpmsg_ns;
>>>> +
>>>> + mutex_lock(&module_mutex);
>>>> + rpmsg_ns = find_module(KBUILD_MODNAME);
>>>> + mutex_unlock(&module_mutex);
>>>> +
>>>> + if (!rpmsg_ns) {
>>>> + ret = request_module(KBUILD_MODNAME);
>>>
>>> Is this code requesting the module in which it is located?.. I must be missing
>>> something...
>>
>> Right this is stupid...Thanks to highlight this!
>>
>> That being said, your remark is very interesting: we need to load the module to
>> access to this function. This means that calling this function ensures that the
>> module is loaded. In this case no need to add the piece of code to find
>> module... here is the call stack associated (associated patch is available below):
>
> Yes, as I wrote 10 hours ago:
>
>> Now, as for how to actually load the
>> module, I'd really propose to move rpmsg_ns_register_device() into the .c
>> file and then the problem will be resolved automatically: as a symbol
>> dependence the module will be loaded whenever another module, calling
>> rpmsg_ns_register_device() is loaded.

It's not a good day for me today... it seems I read your explanation too quickly
this morning, which is, however, very clear.
My apologies

Arnaud

>
> Thanks
> Guennadi
>
>> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
>> [ 11.858748] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
>> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
>> [ 11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
>> (really_probe+0x208/0x4f0)
>> [ 11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
>> (driver_probe_device+0x78/0x16c)
>> [ 11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
>> (bus_for_each_drv+0x84/0xd0)
>> [ 11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
>> (__device_attach+0xf0/0x188)
>> [ 11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
>> (bus_probe_device+0x84/0x8c)
>> [ 11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
>> (device_add+0x3b0/0x7b0)
>> [ 11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
>> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
>> [ 11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
>> [<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
>> [ 11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
>> (virtio_dev_probe+0x1f4/0x2c4)
>> [ 11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
>> (really_probe+0x208/0x4f0)
>> [ 11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
>> (driver_probe_device+0x78/0x16c)
>> [ 11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
>> (device_driver_attach+0x58/0x60)
>> [ 11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
>> (__driver_attach+0xb4/0x154)
>> [ 11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
>> (bus_for_each_dev+0x78/0xc0)
>> [ 11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
>> (bus_add_driver+0x170/0x20c)
>> [ 11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
>> (driver_register+0x74/0x108)
>> [ 12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
>> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
>> [ 12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
>> (do_one_initcall+0x58/0x2bc)
>> [
>>
>> This would make the patch very simple. I tested following patch on my platform,
>> applying it, i do not reproduce the initial issue.
>>
>>
>> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..5867281188de 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
>> *data, int len,
>> return 0;
>> }
>>
>> +/**
>> + * 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.
>> + */
>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> + strcpy(rpdev->id.name, KBUILD_MODNAME);
>> + rpdev->driver_override = KBUILD_MODNAME;
>> + rpdev->src = RPMSG_NS_ADDR;
>> + rpdev->dst = RPMSG_NS_ADDR;
>> +
>> + return rpmsg_register_device(rpdev);
>> +}
>> +EXPORT_SYMBOL(rpmsg_ns_register_device);
>> +
>> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> {
>> struct rpmsg_endpoint *ns_ept;
>> @@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> }
>>
>> static struct rpmsg_driver rpmsg_ns_driver = {
>> - .drv.name = "rpmsg_ns",
>> + .drv.name = KBUILD_MODNAME,
>> .probe = rpmsg_ns_probe,
>> };
>>
>> @@ -104,5 +122,5 @@ module_exit(rpmsg_ns_exit);
>>
>> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
>> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
>> -MODULE_ALIAS("rpmsg_ns");
>> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
>> MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
>> index bdc1ea278814..68eac2b42075 100644
>> --- a/include/linux/rpmsg/ns.h
>> +++ b/include/linux/rpmsg/ns.h
>> @@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
>> /* Address 53 is reserved for advertising remote services */
>> #define RPMSG_NS_ADDR (53)
>>
>> -/**
>> - * rpmsg_ns_register_device() - register name service device based on rpdev
>> - * @rpdev: prepared rpdev to be used for creating endpoints
>> - *
>> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> - * basis for the rpmsg name service device.
>> - */
>> -static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> -{
>> - strcpy(rpdev->id.name, "rpmsg_ns");
>> - rpdev->driver_override = "rpmsg_ns";
>> - rpdev->src = RPMSG_NS_ADDR;
>> - rpdev->dst = RPMSG_NS_ADDR;
>> -
>> - return rpmsg_register_device(rpdev);
>> -}
>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
>>
>> #endif
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks
>>> Guennadi
>>>

2020-11-17 20:42:43

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, Nov 17, 2020 at 06:30:54PM +0100, Arnaud POULIQUEN wrote:

[snip]

> It's not a good day for me today... it seems I read your explanation too quickly
> this morning, which is, however, very clear.
> My apologies

Oh, I did the same to one of your earlier emails one of these days -
I missed a paragraph at the end and then "re-discovered" it in a
later email, so, I can do that too! :-D

Cheers
Guennadi

2020-11-18 00:09:41

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> > On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
> >
> > [snip]
> >
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..80c2cc23bada 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> >> *data, int len,
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * 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.
> >> + */
> >> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> +#ifdef MODULES
> >> + int ret;
> >> + struct module *rpmsg_ns;
> >> +
> >> + mutex_lock(&module_mutex);
> >> + rpmsg_ns = find_module(KBUILD_MODNAME);
> >> + mutex_unlock(&module_mutex);
> >> +
> >> + if (!rpmsg_ns) {
> >> + ret = request_module(KBUILD_MODNAME);
> >
> > Is this code requesting the module in which it is located?.. I must be missing
> > something...
>
> Right this is stupid...Thanks to highlight this!
>
> That being said, your remark is very interesting: we need to load the module to
> access to this function. This means that calling this function ensures that the
> module is loaded. In this case no need to add the piece of code to find
> module... here is the call stack associated (associated patch is available below):
>
>
> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.858748] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
> (bus_for_each_drv+0x84/0xd0)
> [ 11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
> (__device_attach+0xf0/0x188)
> [ 11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
> (bus_probe_device+0x84/0x8c)
> [ 11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
> (device_add+0x3b0/0x7b0)
> [ 11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
> [<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
> [ 11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
> (device_driver_attach+0x58/0x60)
> [ 11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
> (__driver_attach+0xb4/0x154)
> [ 11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
> (bus_for_each_dev+0x78/0xc0)
> [ 11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
> (bus_add_driver+0x170/0x20c)
> [ 11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
> (driver_register+0x74/0x108)
> [ 12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
> [ 12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
> (do_one_initcall+0x58/0x2bc)
> [
>
> This would make the patch very simple. I tested following patch on my platform,
> applying it, i do not reproduce the initial issue.
>
>
> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..5867281188de 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * 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.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, KBUILD_MODNAME);
> + rpdev->driver_override = KBUILD_MODNAME;
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +EXPORT_SYMBOL(rpmsg_ns_register_device);
> +
> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> {
> struct rpmsg_endpoint *ns_ept;
> @@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> }
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> - .drv.name = "rpmsg_ns",
> + .drv.name = KBUILD_MODNAME,
> .probe = rpmsg_ns_probe,
> };
>
> @@ -104,5 +122,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <[email protected]>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index bdc1ea278814..68eac2b42075 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
> /* Address 53 is reserved for advertising remote services */
> #define RPMSG_NS_ADDR (53)
>
> -/**
> - * rpmsg_ns_register_device() - register name service device based on rpdev
> - * @rpdev: prepared rpdev to be used for creating endpoints
> - *
> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
> - * basis for the rpmsg name service device.
> - */
> -static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> -{
> - strcpy(rpdev->id.name, "rpmsg_ns");
> - rpdev->driver_override = "rpmsg_ns";
> - rpdev->src = RPMSG_NS_ADDR;
> - rpdev->dst = RPMSG_NS_ADDR;
> -
> - return rpmsg_register_device(rpdev);
> -}
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);

I confirm that all this is working as expected - I will send a new revision of
this set tomorrow afternoon.

Guennadi, can I add a Co-developed-by and Signed-off-by with your name on the
patch?

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

2020-11-18 07:10:41

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver

On Tue, Nov 17, 2020 at 05:06:47PM -0700, Mathieu Poirier wrote:

[snip]

> I confirm that all this is working as expected - I will send a new revision of
> this set tomorrow afternoon.
>
> Guennadi, can I add a Co-developed-by and Signed-off-by with your name on the
> patch?

You can add the "Co-developed-by" tag, sure, if you like. As for the SOB: I'm
not sure if this is a proper use of it? AFAIK SOB is used when that person
"transmitted" the patch, e.g. if they developed and submitted it to a list,
or if they received it from someone and forwarded it upstream (maintainer
case). I'm not sure about this case, but well, feel free, don't think we'd
be violating anything since I did send versions of code, similar to parts of
that, some with my SOF, so, should be fine.

Thanks
Guennadi

2020-11-18 16:19:08