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

[Arnaud POULIQUEN]

Hi Mathieu, Guennadi,

On 11/9/20 6:55 PM, Mathieu Poirier wrote:
> On Mon, Nov 09, 2020 at 11:20:24AM +0100, Guennadi Liakhovetski wrote:
>> 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@xxxxxx>
>>>>>>>
>>>>>>> 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");
> 
> Yes, I'm good with that part.
> 
>>
>> 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.
>>
> 
> Working with a completion is the kind of thing I had in mind.  But I would still
> like to keep the drivers separate and that's the part I need to think about.

I reproduce the problem: the rpmsg_ns might not be probed on first message reception.
What makes the fix not simple is that the virtio forces the virtio status to ready
after the probe of the virtio unit [1].
Set this status tiggs the remote processor first messages.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio.c#L253

Guennadi: I'm not sure that your patch will solve the problem , look like it just reduces the
delay between the rpmsg_virtio and the rpmsg_ns probe (the module loading time is saved)

Based on my observations, I can see two alternatives.
- rpmsg_ns.c is no longer an rpmsg driver but a kind of function library to manage a generic name service.
- we implement a completion as proposed by Mathieu. 

I tried this second solution based on the component bind mechanism. 
I added the patch at the end of the mail (the patch is a POC, so not ready for upstream). 
Maybe something simpler is possible. I'm just keeping in mind that we may have to add similar
services in the future.

Regards,
Arnaud