On 11/11/20 1:37 AM, Mathieu Poirier wrote:
> On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx> wrote:
>>
>> 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.
>
> That option joins Guennadi's vision - I think he just expressed it in
> a different way. The more I think about it, the more I find that
> option appealing. With the code separation already achieved in this
> patchset it wouldn't be hard to implement.
Right, similar to Guennadi's version, if we want to keep it simpler this is
probably the preferred option.
>From my point of view the main requierement is that the ns announcement service
is generic.
>
>> - 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.
>>
>
> Wasn't familiar with the "component" infrastructure - I suppose you
> stumbled on it while working on sound drivers. I have to spend more
> time looking at it.
Used in DRM framework mainly, i implemented this in my RFC[1] concerning the
refactoring of the rproc_virtio in a platform driver. The idea was to ensure
that all rproc sub-devices are registered before starting the remote processor.
[1]https://lkml.org/lkml/2020/4/16/1817
The principle it to attach child components to a master component, this
relationship allows to synchronize all using component_master_add_with_match
and component_bind_all after the drivers probing step.
The drawback of this solution is that it make code more complex.
> But if you have time and want to spin off a new
> revision that implements the library concept, I'll invest time on that
> instead.
Time is always a major issue :)
No time this week, but i will try to send patches next week.
Regards,
Arnaud
>
>> Regards,
>> Arnaud
>>
>> From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
>> From: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
>> Date: Tue, 10 Nov 2020 18:39:29 +0100
>> Subject: [PATCH] rpmsg_ns: add synchronization based on component mechanism
>>
>> Implement the component bind mechanism to ensure that the rpmsg virtio bus
>> driver are probed before treating the first RPMsg.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
>> ---
>> drivers/rpmsg/rpmsg_ns.c | 26 ++++++++++++-
>> drivers/rpmsg/virtio_rpmsg_bus.c | 65 ++++++++++++++++++++++++++++++++
>> 2 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..057e5d1d29a0 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -2,6 +2,7 @@
>> /*
>> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> */
>> +#include <linux/component.h>
>> #include <linux/device.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> @@ -55,6 +56,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>> return 0;
>> }
>>
>> +static int rpmsg_ns_bind(struct device *dev, struct device *master, void *data)
>> +{
>> + dev_info(dev, "rpmsg ns bound\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void rpmsg_ns_unbind(struct device *dev, struct device *master,
>> + void *data)
>> +{
>> + dev_info(dev, "rpmsg ns unbound\n");
>> +}
>> +
>> +static const struct component_ops rpmsg_ns_ops = {
>> + .bind = rpmsg_ns_bind,
>> + .unbind = rpmsg_ns_unbind,
>> +};
>> +
>> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> {
>> struct rpmsg_endpoint *ns_ept;
>> @@ -63,6 +82,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> .dst = RPMSG_NS_ADDR,
>> .name = "name_service",
>> };
>> + int ret;
>>
>> /*
>> * Create the NS announcement service endpoint associated to the RPMsg
>> @@ -76,7 +96,9 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>> }
>> rpdev->ept = ns_ept;
>>
>> - return 0;
>> + ret = component_add(&rpdev->dev, &rpmsg_ns_ops);
>> +
>> + return ret;
>> }
>>
>> static struct rpmsg_driver rpmsg_ns_driver = {
>> @@ -104,5 +126,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 30ef4a5de4ed..c28aac1295fa 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -11,6 +11,7 @@
>>
>> #define pr_fmt(fmt) "%s: " fmt, __func__
>>
>> +#include <linux/component.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/idr.h>
>> #include <linux/jiffies.h>
>> @@ -67,11 +68,16 @@ struct virtproc_info {
>> struct mutex endpoints_lock;
>> wait_queue_head_t sendq;
>> atomic_t sleepers;
>> + struct component_match *match;
>> + struct completion completed;
>> + int bind_status;
>> };
>>
>> /* The feature bitmap for virtio rpmsg */
>> #define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
>>
>> +#define BIND_TIMEOUT_MS 1000
>> +
>> /**
>> * struct rpmsg_hdr - common header for all rpmsg messages
>> * @src: source address
>> @@ -768,6 +774,17 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
>> unsigned int len, msgs_received = 0;
>> int err;
>>
>> + /* Wait for all children to be bound */
>> + if (vrp->bind_status) {
>> + dev_dbg(dev, "cwait bind\n");
>> + if (!wait_for_completion_timeout(&vrp->completed,
>> + msecs_to_jiffies(BIND_TIMEOUT_MS)))
>> + dev_err(dev, "child device(s) binding timeout\n");
>> +
>> + if (vrp->bind_status)
>> + dev_err(dev, "failed to bind RPMsg sub device(s)\n");
>> + }
>> +
>> msg = virtqueue_get_buf(rvq, &len);
>> if (!msg) {
>> dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
>> @@ -808,6 +825,39 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>> wake_up_interruptible(&vrp->sendq);
>> }
>>
>> +static int virtio_rpmsg_compare(struct device *dev, void *data)
>> +{
>> + return dev == data;
>> +}
>> +
>> +static void virtio_rpmsg_unbind(struct device *dev)
>> +{
>> + /* undbind all child components */
>> + component_unbind_all(dev, NULL);
>> +}
>> +
>> +static int virtio_rpmsg_bind(struct device *dev)
>> +{
>> + struct virtio_device *vdev = dev_to_virtio(dev);
>> + struct virtproc_info *vrp = vdev->priv;
>> +
>> + dev_dbg(dev, "Bind virtio rpmsg sub devices\n");
>> +
>> + vdev = container_of(dev, struct virtio_device, dev);
>> + vrp->bind_status = component_bind_all(dev, NULL);
>> + if (vrp->bind_status)
>> + dev_err(dev, "bind virtio rpmsg failed\n");
>> +
>> + complete(&vrp->completed);
>> +
>> + return vrp->bind_status;
>> +}
>> +
>> +static const struct component_master_ops virtio_rpmsg_cmp_ops = {
>> + .bind = virtio_rpmsg_bind,
>> + .unbind = virtio_rpmsg_unbind,
>> +};
>> +
>> static int rpmsg_probe(struct virtio_device *vdev)
>> {
>> vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>> @@ -892,6 +942,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> /* if supported by the remote processor, enable the name service */
>> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>> vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> +
>> if (!vch) {
>> err = -ENOMEM;
>> goto free_coherent;
>> @@ -911,6 +962,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> err = rpmsg_ns_register_device(rpdev_ns);
>> if (err)
>> goto free_coherent;
>> + /* register a component associated to the virtio platform */
>> + component_match_add_release(&vdev->dev, &vrp->match,
>> + NULL, virtio_rpmsg_compare,
>> + &rpdev_ns->dev);
>> +
>> + vrp->bind_status = -ENXIO;
>> + init_completion(&vrp->completed);
>> + err = component_master_add_with_match(&vdev->dev,
>> + &virtio_rpmsg_cmp_ops,
>> + vrp->match);
>> + if (err) {
>> + dev_err(&vdev->dev, "failed to bind virtio rpmsg\n");
>> + goto free_coherent;
>> + }
>> }
>>
>> /*
>> --
>> 2.17.1
>>
>>
>>
>>
