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

[Arnaud POULIQUEN]

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
>>>

[...]