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

[Arnaud POULIQUEN <arnaud.pouliquen@st.com>]

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 <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}            | 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 <arnaud.pouliquen@st.com>
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 <arnaud.pouliquen@st.com>
---
 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 <arnaud.pouliquen@st.com>");
-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