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

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

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 <arnaud.pouliquen@st.com>
> >>>>
> >>>> Make the RPMSG name service announcement a stand alone driver so that it
> >>>> can be reused by other subsystems.  It is also the first step in making the
> >>>> functionatlity transport independent, i.e that is not tied to virtIO.
> >>>
> >>> 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 <guennadi.liakhovetski@linux.intel.com>
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@linux.intel.com>
---
 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 <arnaud.pouliquen@st.com>");
-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