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@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.
> 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@xxxxxx>
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@xxxxxx>
---
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@xxxxxx>");
-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
