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 <guennadi.liakhovetski@xxxxxxxxxxxxxxx>
> > 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 <guennadi.liakhovetski@xxxxxxxxxxxxxxx>
> > ---
> > 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 <arnaud.pouliquen@xxxxxx>");
> > -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
> >
