Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Wed, 18 Nov 2020 at 00:08, Guennadi Liakhovetski wrote: > > On Tue, Nov 17, 2020 at 05:06:47PM -0700, Mathieu Poirier wrote: > > [snip] > > > I confirm that all this is working as expected - I will send a new revision > > of > > this set tomorrow afternoon. > > > > Guennadi, can I add a Co-developed-by and Signed-off-by with your name on > > the > > patch? > > You can add the "Co-developed-by" tag, sure, if you like. As for the SOB: I'm > not sure if this is a proper use of it? AFAIK SOB is used when that person > "transmitted" the patch, e.g. if they developed and submitted it to a list, > or if they received it from someone and forwarded it upstream (maintainer > case). I'm not sure about this case, but well, feel free, don't think we'd > be violating anything since I did send versions of code, similar to parts of > that, some with my SOF, so, should be fine. > If a Co-developed-by is present, a SOB _has_ to be present as well. > Thanks > Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, Nov 17, 2020 at 05:06:47PM -0700, Mathieu Poirier wrote: [snip] > I confirm that all this is working as expected - I will send a new revision of > this set tomorrow afternoon. > > Guennadi, can I add a Co-developed-by and Signed-off-by with your name on the > patch? You can add the "Co-developed-by" tag, sure, if you like. As for the SOB: I'm not sure if this is a proper use of it? AFAIK SOB is used when that person "transmitted" the patch, e.g. if they developed and submitted it to a list, or if they received it from someone and forwarded it upstream (maintainer case). I'm not sure about this case, but well, feel free, don't think we'd be violating anything since I did send versions of code, similar to parts of that, some with my SOF, so, should be fine. Thanks Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> > On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
> >
> > [snip]
> >
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..80c2cc23bada 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> >> *data, int len,
> >>return 0;
> >> }
> >>
> >> +/**
> >> + * rpmsg_ns_register_device() - register name service device based on
> >> rpdev
> >> + * @rpdev: prepared rpdev to be used for creating endpoints
> >> + *
> >> + * This function wraps rpmsg_register_device() preparing the rpdev for
> >> use as
> >> + * basis for the rpmsg name service device.
> >> + */
> >> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> +#ifdef MODULES
> >> + int ret;
> >> + struct module *rpmsg_ns;
> >> +
> >> + mutex_lock(_mutex);
> >> + rpmsg_ns = find_module(KBUILD_MODNAME);
> >> + mutex_unlock(_mutex);
> >> +
> >> + if (!rpmsg_ns) {
> >> + ret = request_module(KBUILD_MODNAME);
> >
> > Is this code requesting the module in which it is located?.. I must be
> > missing
> > something...
>
> Right this is stupid...Thanks to highlight this!
>
> That being said, your remark is very interesting: we need to load the module
> to
> access to this function. This means that calling this function ensures that
> the
> module is loaded. In this case no need to add the piece of code to find
> module... here is the call stack associated (associated patch is available
> below):
>
>
> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.858748] [] (rpmsg_ns_probe [rpmsg_ns]) from []
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.869047] [] (rpmsg_dev_probe [rpmsg_core]) from []
> (really_probe+0x208/0x4f0)
> [ 11.878117] [] (really_probe) from []
> (driver_probe_device+0x78/0x16c)
> [ 11.886404] [] (driver_probe_device) from []
> (bus_for_each_drv+0x84/0xd0)
> [ 11.894887] [] (bus_for_each_drv) from []
> (__device_attach+0xf0/0x188)
> [ 11.903142] [] (__device_attach) from []
> (bus_probe_device+0x84/0x8c)
> [ 11.911314] [] (bus_probe_device) from []
> (device_add+0x3b0/0x7b0)
> [ 11.919227] [] (device_add) from []
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.928541] [] (rpmsg_register_device [rpmsg_core]) from
> [] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
> [ 11.939748] [] (rpmsg_probe [virtio_rpmsg_bus]) from []
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.949377] [] (virtio_dev_probe) from []
> (really_probe+0x208/0x4f0)
> [ 11.957454] [] (really_probe) from []
> (driver_probe_device+0x78/0x16c)
> [ 11.965710] [] (driver_probe_device) from []
> (device_driver_attach+0x58/0x60)
> [ 11.974574] [] (device_driver_attach) from []
> (__driver_attach+0xb4/0x154)
> [ 11.983177] [] (__driver_attach) from []
> (bus_for_each_dev+0x78/0xc0)
> [ 11.991344] [] (bus_for_each_dev) from []
> (bus_add_driver+0x170/0x20c)
> [ 11.999600] [] (bus_add_driver) from []
> (driver_register+0x74/0x108)
> [ 12.007693] [] (driver_register) from []
> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
> [ 12.017168] [] (rpmsg_init [virtio_rpmsg_bus]) from []
> (do_one_initcall+0x58/0x2bc)
> [
>
> This would make the patch very simple. I tested following patch on my
> platform,
> applying it, i do not reproduce the initial issue.
>
>
> 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..5867281188de 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, KBUILD_MODNAME);
> + rpdev->driver_override = KBUILD_MODNAME;
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return rpmsg_register_device(rpdev);
> +}
> +EXPORT_SYMBOL(rpmsg_ns_register_device);
> +
> static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> {
> struct rpmsg_endpoint *ns_ept;
> @@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> }
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> - .drv.name =
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, Nov 17, 2020 at 06:30:54PM +0100, Arnaud POULIQUEN wrote: [snip] > It's not a good day for me today... it seems I read your explanation too > quickly > this morning, which is, however, very clear. > My apologies Oh, I did the same to one of your earlier emails one of these days - I missed a paragraph at the end and then "re-discovered" it in a later email, so, I can do that too! :-D Cheers Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On 11/17/20 5:58 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
>>> On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
>>>
>>> [snip]
>>>
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..80c2cc23bada 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}
+/**
+ * rpmsg_ns_register_device() - register name service device based on
rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for
use as
+ * basis for the rpmsg name service device.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+#ifdef MODULES
+ int ret;
+ struct module *rpmsg_ns;
+
+ mutex_lock(_mutex);
+ rpmsg_ns = find_module(KBUILD_MODNAME);
+ mutex_unlock(_mutex);
+
+ if (!rpmsg_ns) {
+ ret = request_module(KBUILD_MODNAME);
>>>
>>> Is this code requesting the module in which it is located?.. I must be
>>> missing
>>> something...
>>
>> Right this is stupid...Thanks to highlight this!
>>
>> That being said, your remark is very interesting: we need to load the module
>> to
>> access to this function. This means that calling this function ensures that
>> the
>> module is loaded. In this case no need to add the piece of code to find
>> module... here is the call stack associated (associated patch is available
>> below):
>
> Yes, as I wrote 10 hours ago:
>
>> Now, as for how to actually load the
>> module, I'd really propose to move rpmsg_ns_register_device() into the .c
>> file and then the problem will be resolved automatically: as a symbol
>> dependence the module will be loaded whenever another module, calling
>> rpmsg_ns_register_device() is loaded.
It's not a good day for me today... it seems I read your explanation too quickly
this morning, which is, however, very clear.
My apologies
Arnaud
>
> Thanks
> Guennadi
>
>> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
>> [ 11.858748] [] (rpmsg_ns_probe [rpmsg_ns]) from []
>> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
>> [ 11.869047] [] (rpmsg_dev_probe [rpmsg_core]) from []
>> (really_probe+0x208/0x4f0)
>> [ 11.878117] [] (really_probe) from []
>> (driver_probe_device+0x78/0x16c)
>> [ 11.886404] [] (driver_probe_device) from []
>> (bus_for_each_drv+0x84/0xd0)
>> [ 11.894887] [] (bus_for_each_drv) from []
>> (__device_attach+0xf0/0x188)
>> [ 11.903142] [] (__device_attach) from []
>> (bus_probe_device+0x84/0x8c)
>> [ 11.911314] [] (bus_probe_device) from []
>> (device_add+0x3b0/0x7b0)
>> [ 11.919227] [] (device_add) from []
>> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
>> [ 11.928541] [] (rpmsg_register_device [rpmsg_core]) from
>> [] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
>> [ 11.939748] [] (rpmsg_probe [virtio_rpmsg_bus]) from
>> []
>> (virtio_dev_probe+0x1f4/0x2c4)
>> [ 11.949377] [] (virtio_dev_probe) from []
>> (really_probe+0x208/0x4f0)
>> [ 11.957454] [] (really_probe) from []
>> (driver_probe_device+0x78/0x16c)
>> [ 11.965710] [] (driver_probe_device) from []
>> (device_driver_attach+0x58/0x60)
>> [ 11.974574] [] (device_driver_attach) from []
>> (__driver_attach+0xb4/0x154)
>> [ 11.983177] [] (__driver_attach) from []
>> (bus_for_each_dev+0x78/0xc0)
>> [ 11.991344] [] (bus_for_each_dev) from []
>> (bus_add_driver+0x170/0x20c)
>> [ 11.999600] [] (bus_add_driver) from []
>> (driver_register+0x74/0x108)
>> [ 12.007693] [] (driver_register) from []
>> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
>> [ 12.017168] [] (rpmsg_init [virtio_rpmsg_bus]) from []
>> (do_one_initcall+0x58/0x2bc)
>> [
>>
>> This would make the patch very simple. I tested following patch on my
>> platform,
>> applying it, i do not reproduce the initial issue.
>>
>>
>> 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..5867281188de 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
>> *data, int len,
>> return 0;
>> }
>>
>> +/**
>> + * rpmsg_ns_register_device() - register name service device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use
>> as
>> + *
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> > On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
> >
> > [snip]
> >
> >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> >> index 5bda7cb44618..80c2cc23bada 100644
> >> --- a/drivers/rpmsg/rpmsg_ns.c
> >> +++ b/drivers/rpmsg/rpmsg_ns.c
> >> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> >> *data, int len,
> >>return 0;
> >> }
> >>
> >> +/**
> >> + * rpmsg_ns_register_device() - register name service device based on
> >> rpdev
> >> + * @rpdev: prepared rpdev to be used for creating endpoints
> >> + *
> >> + * This function wraps rpmsg_register_device() preparing the rpdev for
> >> use as
> >> + * basis for the rpmsg name service device.
> >> + */
> >> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> +#ifdef MODULES
> >> + int ret;
> >> + struct module *rpmsg_ns;
> >> +
> >> + mutex_lock(_mutex);
> >> + rpmsg_ns = find_module(KBUILD_MODNAME);
> >> + mutex_unlock(_mutex);
> >> +
> >> + if (!rpmsg_ns) {
> >> + ret = request_module(KBUILD_MODNAME);
> >
> > Is this code requesting the module in which it is located?.. I must be
> > missing
> > something...
>
> Right this is stupid...Thanks to highlight this!
>
> That being said, your remark is very interesting: we need to load the module
> to
> access to this function. This means that calling this function ensures that
> the
> module is loaded. In this case no need to add the piece of code to find
> module... here is the call stack associated (associated patch is available
> below):
Yes, as I wrote 10 hours ago:
> Now, as for how to actually load the
> module, I'd really propose to move rpmsg_ns_register_device() into the .c
> file and then the problem will be resolved automatically: as a symbol
> dependence the module will be loaded whenever another module, calling
> rpmsg_ns_register_device() is loaded.
Thanks
Guennadi
> (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
> [ 11.858748] [] (rpmsg_ns_probe [rpmsg_ns]) from []
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.869047] [] (rpmsg_dev_probe [rpmsg_core]) from []
> (really_probe+0x208/0x4f0)
> [ 11.878117] [] (really_probe) from []
> (driver_probe_device+0x78/0x16c)
> [ 11.886404] [] (driver_probe_device) from []
> (bus_for_each_drv+0x84/0xd0)
> [ 11.894887] [] (bus_for_each_drv) from []
> (__device_attach+0xf0/0x188)
> [ 11.903142] [] (__device_attach) from []
> (bus_probe_device+0x84/0x8c)
> [ 11.911314] [] (bus_probe_device) from []
> (device_add+0x3b0/0x7b0)
> [ 11.919227] [] (device_add) from []
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.928541] [] (rpmsg_register_device [rpmsg_core]) from
> [] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
> [ 11.939748] [] (rpmsg_probe [virtio_rpmsg_bus]) from []
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.949377] [] (virtio_dev_probe) from []
> (really_probe+0x208/0x4f0)
> [ 11.957454] [] (really_probe) from []
> (driver_probe_device+0x78/0x16c)
> [ 11.965710] [] (driver_probe_device) from []
> (device_driver_attach+0x58/0x60)
> [ 11.974574] [] (device_driver_attach) from []
> (__driver_attach+0xb4/0x154)
> [ 11.983177] [] (__driver_attach) from []
> (bus_for_each_dev+0x78/0xc0)
> [ 11.991344] [] (bus_for_each_dev) from []
> (bus_add_driver+0x170/0x20c)
> [ 11.999600] [] (bus_add_driver) from []
> (driver_register+0x74/0x108)
> [ 12.007693] [] (driver_register) from []
> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
> [ 12.017168] [] (rpmsg_init [virtio_rpmsg_bus]) from []
> (do_one_initcall+0x58/0x2bc)
> [
>
> This would make the patch very simple. I tested following patch on my
> platform,
> applying it, i do not reproduce the initial issue.
>
>
> 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..5867281188de 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, KBUILD_MODNAME);
> + rpdev->driver_override = KBUILD_MODNAME;
> + rpdev->src = RPMSG_NS_ADDR;
> + rpdev->dst = RPMSG_NS_ADDR;
> +
> + return
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
>
> [snip]
>
>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
>> index 5bda7cb44618..80c2cc23bada 100644
>> --- a/drivers/rpmsg/rpmsg_ns.c
>> +++ b/drivers/rpmsg/rpmsg_ns.c
>> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
>> *data, int len,
>> return 0;
>> }
>>
>> +/**
>> + * rpmsg_ns_register_device() - register name service device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use
>> as
>> + * basis for the rpmsg name service device.
>> + */
>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>> +{
>> +#ifdef MODULES
>> +int ret;
>> +struct module *rpmsg_ns;
>> +
>> +mutex_lock(_mutex);
>> +rpmsg_ns = find_module(KBUILD_MODNAME);
>> +mutex_unlock(_mutex);
>> +
>> +if (!rpmsg_ns) {
>> +ret = request_module(KBUILD_MODNAME);
>
> Is this code requesting the module in which it is located?.. I must be
> missing
> something...
Right this is stupid...Thanks to highlight this!
That being said, your remark is very interesting: we need to load the module to
access to this function. This means that calling this function ensures that the
module is loaded. In this case no need to add the piece of code to find
module... here is the call stack associated (associated patch is available
below):
(rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
[ 11.858748] [] (rpmsg_ns_probe [rpmsg_ns]) from []
(rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
[ 11.869047] [] (rpmsg_dev_probe [rpmsg_core]) from []
(really_probe+0x208/0x4f0)
[ 11.878117] [] (really_probe) from []
(driver_probe_device+0x78/0x16c)
[ 11.886404] [] (driver_probe_device) from []
(bus_for_each_drv+0x84/0xd0)
[ 11.894887] [] (bus_for_each_drv) from []
(__device_attach+0xf0/0x188)
[ 11.903142] [] (__device_attach) from []
(bus_probe_device+0x84/0x8c)
[ 11.911314] [] (bus_probe_device) from []
(device_add+0x3b0/0x7b0)
[ 11.919227] [] (device_add) from []
(rpmsg_register_device+0x54/0x88 [rpmsg_core])
[ 11.928541] [] (rpmsg_register_device [rpmsg_core]) from
[] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
[ 11.939748] [] (rpmsg_probe [virtio_rpmsg_bus]) from []
(virtio_dev_probe+0x1f4/0x2c4)
[ 11.949377] [] (virtio_dev_probe) from []
(really_probe+0x208/0x4f0)
[ 11.957454] [] (really_probe) from []
(driver_probe_device+0x78/0x16c)
[ 11.965710] [] (driver_probe_device) from []
(device_driver_attach+0x58/0x60)
[ 11.974574] [] (device_driver_attach) from []
(__driver_attach+0xb4/0x154)
[ 11.983177] [] (__driver_attach) from []
(bus_for_each_dev+0x78/0xc0)
[ 11.991344] [] (bus_for_each_dev) from []
(bus_add_driver+0x170/0x20c)
[ 11.999600] [] (bus_add_driver) from []
(driver_register+0x74/0x108)
[ 12.007693] [] (driver_register) from []
(rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
[ 12.017168] [] (rpmsg_init [virtio_rpmsg_bus]) from []
(do_one_initcall+0x58/0x2bc)
[
This would make the patch very simple. I tested following patch on my platform,
applying it, i do not reproduce the initial issue.
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..5867281188de 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,24 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}
+/**
+ * rpmsg_ns_register_device() - register name service device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg name service device.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+ strcpy(rpdev->id.name, KBUILD_MODNAME);
+ rpdev->driver_override = KBUILD_MODNAME;
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_ns_register_device);
+
static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
{
struct rpmsg_endpoint *ns_ept;
@@ -80,7 +98,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}
static struct rpmsg_driver rpmsg_ns_driver = {
- .drv.name = "rpmsg_ns",
+ .drv.name = KBUILD_MODNAME,
.probe = rpmsg_ns_probe,
};
@@ -104,5 +122,5 @@ module_exit(rpmsg_ns_exit);
MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen ");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
MODULE_LICENSE("GPL v2");
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
[snip]
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..80c2cc23bada 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
> *data, int len,
> return 0;
> }
>
> +/**
> + * rpmsg_ns_register_device() - register name service device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg name service device.
> + */
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> +{
> +#ifdef MODULES
> + int ret;
> + struct module *rpmsg_ns;
> +
> + mutex_lock(_mutex);
> + rpmsg_ns = find_module(KBUILD_MODNAME);
> + mutex_unlock(_mutex);
> +
> + if (!rpmsg_ns) {
> + ret = request_module(KBUILD_MODNAME);
Is this code requesting the module in which it is located?.. I must be missing
something...
Thanks
Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Mathieu,
On 11/16/20 11:40 PM, Mathieu Poirier wrote:
> On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>> On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:
[snip]
> I did a lot of probing, went deep in the bowels of the user mode helper
> subsystem and looked at sys_load_module(). Especially at do_init_module()
> where
> function do_one_initcall()[1] is called on mod->init, which happens to be
> rpmsg_ns_init() where the name space driver is registered. I am confident we
> can rely on this mechanism.
>
> More comments below...
>
> [1]. https://elixir.bootlin.com/linux/v5.10-rc3/source/kernel/module.c#L3604
>
>> So if we can't conclude, adding completion would be OK for me.
>>
>> Thanks,
>> Arnaud
>>
[snip]
>
> We came up with almost exactly the same thing:>
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..ab86b603c54e 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -81,6 +81,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> .drv.name = "rpmsg_ns",
> + .drv.mod_name = "rpmsg_ns",
This does not work for me, in built-in the kernel freezes on start
i just have the " Starting kernel ..." print
Are you sure that is useful? it working for me without this.
> .probe = rpmsg_ns_probe,
> };
>
> @@ -104,5 +105,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen ");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index e267dd5f909b..28251fd1b2e0 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -3,6 +3,7 @@
> #ifndef _LINUX_RPMSG_NS_H
> #define _LINUX_RPMSG_NS_H
>
> +#include
> #include
> #include
> #include
> @@ -49,11 +50,27 @@ enum rpmsg_ns_flags {
> */
> static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
Agree with the Guennadi's pertinent remarks, better to put this in rpmsg_ns.c
> {
> + int ret;
> + struct module *rpmsg_ns;
> + const char name[] = "rpmsg_ns";
> +
> strcpy(rpdev->id.name, "rpmsg_ns");
> rpdev->driver_override = "rpmsg_ns";
> rpdev->src = RPMSG_NS_ADDR;
> rpdev->dst = RPMSG_NS_ADDR;
>
> +#ifdef CONFIG_MODULES
This piece of code is executed also if rppmsg_ns is built-in
> + mutex_lock(_mutex);
> + rpmsg_ns = find_module(name);
> + mutex_unlock(_mutex);
> +
> + if (!rpmsg_ns) {
> + ret = request_module(name);
> + if (ret)
> + pr_err("Can not find module %s (err %d)\n", name,
> ret);
As consequence for built-in this error is printed.
To avoid this
- #ifdef CONFIG_MODULES
+ #ifdef MODULES
Then for me here we should return the error.
> + }
> +#endif
> +
>
> I think it is better to be in rpmsg_ns_register_devices() because it makes the
> solution stand by itself, i.e no need to call the registration code from
> another
> driver. Everything is self contained.
That's a very good idea!
In addition I would keep dependency between virtio and rpmsg_ns in kconfig. This
would ensure that rpmsg ns is built for the virtio bus. Then the device will be
probed on demand.
>
> Also note the drv.mod_name = "rpmsg_ns" part. I took a look at find_module()
> and that is what is supposed to be used.
>
> That works on my side - please test on your setup.
Please find update of your patch integrating my remarks:
- suppress drv.mod_name,
- migrate rpmsg_ns_register_device in rpmsg_ns.c,
- use KBUILD_MODNAME for the module name,
- add select RPMSG_NS for RPMSG_VIRTIO config.
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..80c2cc23bada 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}
+/**
+ * rpmsg_ns_register_device() - register name service device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg name service device.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+#ifdef MODULES
+ int ret;
+ struct module *rpmsg_ns;
+
+ mutex_lock(_mutex);
+ rpmsg_ns = find_module(KBUILD_MODNAME);
+ mutex_unlock(_mutex);
+
+ if (!rpmsg_ns) {
+ ret =
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Mathieu, On Mon, Nov 16, 2020 at 03:40:03PM -0700, Mathieu Poirier wrote: > On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote: [snip] > > Having said that, does this guarantee the probe, a good question! > > Maybe you or Mathieu have the answer, not me... > > I did a lot of probing, went deep in the bowels of the user mode helper > subsystem and looked at sys_load_module(). Especially at do_init_module() > where > function do_one_initcall()[1] is called on mod->init, which happens to be > rpmsg_ns_init() where the name space driver is registered. I am confident we > can rely on this mechanism. Thanks for investigating and confirming that! So, we can be confident, that if the module is already loaded at the time when the NS device is registered, the probing happens synchronously. Now, as for how to actually load the module, I'd really propose to move rpmsg_ns_register_device() into the .c file and then the problem will be resolved automatically: as a symbol dependence the module will be loaded whenever another module, calling rpmsg_ns_register_device() is loaded. Thanks Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote: > Hi Guennadi, > > On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote: > > Hi Arnaud, > > > > On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote: > >> Hi Guennadi, Mathieu, > > > > [snip] > > > >> 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. > > > > Thanks for the patch. Yes, this would guarantee, that the NS module is > > loaded. But > > does it also guarantee, that the NS probing completes faster than an NS > > announcement > > arrives? We have a race here: > > > > rpmsg_ns_register_device() -\ > > | | > > virtio_device_ready() | > > | | > > remote sends NS announcement rpmsg_register_device() > > | | > > |rpmsg_ns_probe() > > | | > > |rpmsg_create_ept() > > rpmsg_ns_cb() > > > > In practice we *should* be fine - maybe the whole probing (the right > > branch) happens > > synchronously on the same core as where rpmsg_ns_register_device() was > > called, or > > even if not, the probing runs locally and the NS announcement either talks > > to a > > remote core or a VM. So, it *should* be safe, but unless we can make > > guarantee, that > > the probing is synchronous, I wouldn't rely on this. So, without a > > completion this > > still seems incomplete to me. > > Thanks for this description! > I tested on a dual core, and I expected what you are describing above but in > fact I observed following: > > rpmsg_ns_register_device() > | > rpmsg_register_device() > | > rpmsg_ns_probe() > | > rpmsg_create_ept() > | > virtio_device_ready() > | > remote sends NS announcement > rpmsg_ns_cb() > > Here is the associated call stack generated in rpmsg_ns_probe using "warn_on" > > [ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54 > [ 11.504106] Hardware name: STM32 (Device Tree Support) > [ 11.509271] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [ 11.516992] [] (show_stack) from [] > (dump_stack+0xb8/0xcc) > [ 11.524204] [] (dump_stack) from [] (__warn+0xd8/0xf0) > [ 11.531066] [] (__warn) from [] > (warn_slowpath_fmt+0x64/0xc4) > [ 11.538547] [] (warn_slowpath_fmt) from [] > (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns]) > [ 11.547681] [] (rpmsg_ns_probe [rpmsg_ns]) from [] > (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core]) > [ 11.557933] [] (rpmsg_dev_probe [rpmsg_core]) from [] > (really_probe+0x208/0x4f0) > [ 11.567050] [] (really_probe) from [] > (driver_probe_device+0x78/0x16c) > [ 11.575305] [] (driver_probe_device) from [] > (bus_for_each_drv+0x84/0xd0) > [ 11.583822] [] (bus_for_each_drv) from [] > (__device_attach+0xf0/0x188) > [ 11.592075] [] (__device_attach) from [] > (bus_probe_device+0x84/0x8c) > [ 11.600248] [] (bus_probe_device) from [] > (device_add+0x3b0/0x7b0) > [ 11.608165] [] (device_add) from [] > (rpmsg_register_device+0x54/0x88 [rpmsg_core]) > [ 11.617486] [] (rpmsg_register_device [rpmsg_core]) from > [] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus]) > [ 11.628696] [] (rpmsg_probe [virtio_rpmsg_bus]) from [] > (virtio_dev_probe+0x1f4/0x2c4) > [ 11.638352] [] (virtio_dev_probe) from [] > (really_probe+0x208/0x4f0) > [ 11.646406] [] (really_probe) from [] > (driver_probe_device+0x78/0x16c) > [ 11.654658] [] (driver_probe_device) from [] > (device_driver_attach+0x58/0x60) > [ 11.663522] [] (device_driver_attach) from [] > (__driver_attach+0xb4/0x154) > [ 11.672134] [] (__driver_attach) from [] > (bus_for_each_dev+0x78/0xc0) > [ 11.680300] [] (bus_for_each_dev) from [] > (bus_add_driver+0x170/0x20c) > [ 11.688599] [] (bus_add_driver) from [] > (driver_register+0x74/0x108) > [ 11.696662] [] (driver_register) from [] > (rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus]) > [ 11.706136] [] (rpmsg_init [virtio_rpmsg_bus]) from [] > (do_one_initcall+0x58/0x2bc) > [ 11.713616] usb33: supplied by vdd_usb > [ 11.715500] [] (do_one_initcall) from [] > (do_init_module+0x60/0x248) > [ 11.715525] [] (do_init_module) from [] > (load_module+0x12e8/0x1638) > [ 11.715546] [] (load_module) from [] > (sys_finit_module+0xd4/0x130) > [ 11.715575] [] (sys_finit_module) from [] > (ret_fast_syscall+0x0/0x54) > > Having said that, does this guarantee the probe, a good question! > Maybe you or Mathieu have the answer, not me... I did a lot of probing,
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote: > Hi Guennadi, > > On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote: > > Hi Arnaud, > > > > On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote: > >> Hi Guennadi, Mathieu, > > > > [snip] > > > >> 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. > > > > Thanks for the patch. Yes, this would guarantee, that the NS module is > > loaded. But > > does it also guarantee, that the NS probing completes faster than an NS > > announcement > > arrives? We have a race here: > > > > rpmsg_ns_register_device() -\ > > | | > > virtio_device_ready() | > > | | > > remote sends NS announcement rpmsg_register_device() > > | | > > |rpmsg_ns_probe() > > | | > > |rpmsg_create_ept() > > rpmsg_ns_cb() > > > > In practice we *should* be fine - maybe the whole probing (the right > > branch) happens > > synchronously on the same core as where rpmsg_ns_register_device() was > > called, or > > even if not, the probing runs locally and the NS announcement either talks > > to a > > remote core or a VM. So, it *should* be safe, but unless we can make > > guarantee, that > > the probing is synchronous, I wouldn't rely on this. So, without a > > completion this > > still seems incomplete to me. > > Thanks for this description! > I tested on a dual core, and I expected what you are describing above but in > fact I observed following: > > rpmsg_ns_register_device() > | > rpmsg_register_device() > | > rpmsg_ns_probe() > | > rpmsg_create_ept() > | > virtio_device_ready() > | > remote sends NS announcement > rpmsg_ns_cb() > > Here is the associated call stack generated in rpmsg_ns_probe using "warn_on" > > [ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54 > [ 11.504106] Hardware name: STM32 (Device Tree Support) > [ 11.509271] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [ 11.516992] [] (show_stack) from [] > (dump_stack+0xb8/0xcc) > [ 11.524204] [] (dump_stack) from [] (__warn+0xd8/0xf0) > [ 11.531066] [] (__warn) from [] > (warn_slowpath_fmt+0x64/0xc4) > [ 11.538547] [] (warn_slowpath_fmt) from [] > (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns]) > [ 11.547681] [] (rpmsg_ns_probe [rpmsg_ns]) from [] > (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core]) > [ 11.557933] [] (rpmsg_dev_probe [rpmsg_core]) from [] > (really_probe+0x208/0x4f0) > [ 11.567050] [] (really_probe) from [] > (driver_probe_device+0x78/0x16c) > [ 11.575305] [] (driver_probe_device) from [] > (bus_for_each_drv+0x84/0xd0) > [ 11.583822] [] (bus_for_each_drv) from [] > (__device_attach+0xf0/0x188) > [ 11.592075] [] (__device_attach) from [] > (bus_probe_device+0x84/0x8c) > [ 11.600248] [] (bus_probe_device) from [] > (device_add+0x3b0/0x7b0) > [ 11.608165] [] (device_add) from [] > (rpmsg_register_device+0x54/0x88 [rpmsg_core]) > [ 11.617486] [] (rpmsg_register_device [rpmsg_core]) from > [] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus]) > [ 11.628696] [] (rpmsg_probe [virtio_rpmsg_bus]) from [] > (virtio_dev_probe+0x1f4/0x2c4) Right, yes, that's also what I expected to happen, but I wasn't sure whether we can *rely* on this. It does indeed look like we can (by only looking at your trace, without scrutinising the code). So, if everybody is happy with this "empiric proof" we can use either this or the one-module approach without a completion, I'd be fine either way. Thanks Guennadi > [ 11.638352] [] (virtio_dev_probe) from [] > (really_probe+0x208/0x4f0) > [ 11.646406] [] (really_probe) from [] > (driver_probe_device+0x78/0x16c) > [ 11.654658] [] (driver_probe_device) from [] > (device_driver_attach+0x58/0x60) > [ 11.663522] [] (device_driver_attach) from [] > (__driver_attach+0xb4/0x154) > [ 11.672134] [] (__driver_attach) from [] > (bus_for_each_dev+0x78/0xc0) > [ 11.680300] [] (bus_for_each_dev) from [] > (bus_add_driver+0x170/0x20c) > [ 11.688599] [] (bus_add_driver) from [] > (driver_register+0x74/0x108) > [ 11.696662] [] (driver_register) from [] > (rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus]) > [ 11.706136] [] (rpmsg_init [virtio_rpmsg_bus]) from [] > (do_one_initcall+0x58/0x2bc) > [ 11.713616] usb33: supplied by vdd_usb > [ 11.715500] [] (do_one_initcall) from [] > (do_init_module+0x60/0x248) > [
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Guennadi, On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote: > Hi Arnaud, > > On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote: >> Hi Guennadi, Mathieu, > > [snip] > >> 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. > > Thanks for the patch. Yes, this would guarantee, that the NS module is > loaded. But > does it also guarantee, that the NS probing completes faster than an NS > announcement > arrives? We have a race here: > > rpmsg_ns_register_device() -\ > | | > virtio_device_ready() | > | | > remote sends NS announcement rpmsg_register_device() > | | > |rpmsg_ns_probe() > | | > |rpmsg_create_ept() > rpmsg_ns_cb() > > In practice we *should* be fine - maybe the whole probing (the right branch) > happens > synchronously on the same core as where rpmsg_ns_register_device() was > called, or > even if not, the probing runs locally and the NS announcement either talks to > a > remote core or a VM. So, it *should* be safe, but unless we can make > guarantee, that > the probing is synchronous, I wouldn't rely on this. So, without a completion > this > still seems incomplete to me. Thanks for this description! I tested on a dual core, and I expected what you are describing above but in fact I observed following: rpmsg_ns_register_device() | rpmsg_register_device() | rpmsg_ns_probe() | rpmsg_create_ept() | virtio_device_ready() | remote sends NS announcement rpmsg_ns_cb() Here is the associated call stack generated in rpmsg_ns_probe using "warn_on" [ 11.498678] CPU: 1 PID: 348 Comm: systemd-udevd Not tainted 5.10.0-rc2 #54 [ 11.504106] Hardware name: STM32 (Device Tree Support) [ 11.509271] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 11.516992] [] (show_stack) from [] (dump_stack+0xb8/0xcc) [ 11.524204] [] (dump_stack) from [] (__warn+0xd8/0xf0) [ 11.531066] [] (__warn) from [] (warn_slowpath_fmt+0x64/0xc4) [ 11.538547] [] (warn_slowpath_fmt) from [] (rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns]) [ 11.547681] [] (rpmsg_ns_probe [rpmsg_ns]) from [] (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core]) [ 11.557933] [] (rpmsg_dev_probe [rpmsg_core]) from [] (really_probe+0x208/0x4f0) [ 11.567050] [] (really_probe) from [] (driver_probe_device+0x78/0x16c) [ 11.575305] [] (driver_probe_device) from [] (bus_for_each_drv+0x84/0xd0) [ 11.583822] [] (bus_for_each_drv) from [] (__device_attach+0xf0/0x188) [ 11.592075] [] (__device_attach) from [] (bus_probe_device+0x84/0x8c) [ 11.600248] [] (bus_probe_device) from [] (device_add+0x3b0/0x7b0) [ 11.608165] [] (device_add) from [] (rpmsg_register_device+0x54/0x88 [rpmsg_core]) [ 11.617486] [] (rpmsg_register_device [rpmsg_core]) from [] (rpmsg_probe+0x2c4/0x3f4 [virtio_rpmsg_bus]) [ 11.628696] [] (rpmsg_probe [virtio_rpmsg_bus]) from [] (virtio_dev_probe+0x1f4/0x2c4) [ 11.638352] [] (virtio_dev_probe) from [] (really_probe+0x208/0x4f0) [ 11.646406] [] (really_probe) from [] (driver_probe_device+0x78/0x16c) [ 11.654658] [] (driver_probe_device) from [] (device_driver_attach+0x58/0x60) [ 11.663522] [] (device_driver_attach) from [] (__driver_attach+0xb4/0x154) [ 11.672134] [] (__driver_attach) from [] (bus_for_each_dev+0x78/0xc0) [ 11.680300] [] (bus_for_each_dev) from [] (bus_add_driver+0x170/0x20c) [ 11.688599] [] (bus_add_driver) from [] (driver_register+0x74/0x108) [ 11.696662] [] (driver_register) from [] (rpmsg_init+0x6c/0x1000 [virtio_rpmsg_bus]) [ 11.706136] [] (rpmsg_init [virtio_rpmsg_bus]) from [] (do_one_initcall+0x58/0x2bc) [ 11.713616] usb33: supplied by vdd_usb [ 11.715500] [] (do_one_initcall) from [] (do_init_module+0x60/0x248) [ 11.715525] [] (do_init_module) from [] (load_module+0x12e8/0x1638) [ 11.715546] [] (load_module) from [] (sys_finit_module+0xd4/0x130) [ 11.715575] [] (sys_finit_module) from [] (ret_fast_syscall+0x0/0x54) Having said that, does this guarantee the probe, a good question! Maybe you or Mathieu have the answer, not me... So if we can't conclude, adding completion would be OK for me. Thanks, Arnaud > > Thanks > Guennadi > 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
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Arnaud,
On Mon, Nov 16, 2020 at 03:43:35PM +0100, Arnaud POULIQUEN wrote:
> Hi Guennadi, Mathieu,
[snip]
> 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.
Thanks for the patch. Yes, this would guarantee, that the NS module is loaded.
But
does it also guarantee, that the NS probing completes faster than an NS
announcement
arrives? We have a race here:
rpmsg_ns_register_device() -\
| |
virtio_device_ready() |
| |
remote sends NS announcement rpmsg_register_device()
| |
|rpmsg_ns_probe()
| |
|rpmsg_create_ept()
rpmsg_ns_cb()
In practice we *should* be fine - maybe the whole probing (the right branch)
happens
synchronously on the same core as where rpmsg_ns_register_device() was called,
or
even if not, the probing runs locally and the NS announcement either talks to a
remote core or a VM. So, it *should* be safe, but unless we can make guarantee,
that
the probing is synchronous, I wouldn't rely on this. So, without a completion
this
still seems incomplete to me.
Thanks
Guennadi
> >> 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
> 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
> ---
> 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 ");
> -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(_mutex);
> + ns =
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
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
---
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
#include
#include
+#include
#include
#include
#include
@@ -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(_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(_bus);
return ret;
}
postcore_initcall(rpmsg_init);
-static void __exit rpmsg_fini(void)
+static void rpmsg_fini(void)
{
+ rpmsg_ns_exit();
bus_unregister(_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
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Thu, Nov 12, 2020 at 10:04:17AM +0100, Arnaud POULIQUEN wrote:
>
>
> On 11/11/20 1:37 AM, Mathieu Poirier wrote:
> > On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN
> > 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
> >
> > 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
> >
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
>>> 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
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -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(_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(_bus);
>>>
>>> return ret;
>>> }
>>> postcore_initcall(rpmsg_init);
>>>
>>> -static void __exit rpmsg_fini(void)
>>> +static void rpmsg_fini(void)
>>> {
>>> + rpmsg_ns_exit();
>>> bus_unregister(_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
>
>>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
>>> similarity index 76%
>>> rename from drivers/rpmsg/rpmsg_ns.c
>>> rename to drivers/rpmsg/ns.c
>>> index 8e26824ca328..86c011bfb62f 100644
>>> --- a/drivers/rpmsg/rpmsg_ns.c
>>> +++ b/drivers/rpmsg/ns.c
>>> @@ -2,15 +2,47 @@
>>> /*
>>> * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>>> */
>>> +#include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> -#include
>>> #include
>>> -#include
>>> +#include
>>> +#include
>>>
>>> #include "rpmsg_internal.h"
>>>
>>> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>>> +{
>>> + int ret;
>>> +
>>> + strcpy(rpdev->id.name, "rpmsg_ns");
>>> + rpdev->driver_override = "rpmsg_ns";
>>> + rpdev->src = RPMSG_NS_ADDR;
>>> + rpdev->dst = RPMSG_NS_ADDR;
>>> +
>>> + ret = rpmsg_register_device(rpdev);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (!wait_for_completion_timeout(>ns_ready,
>>> +msecs_to_jiffies(1))) {
>>
>> Does this work if called in
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
> > 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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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(_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(_bus);
> >
> > return ret;
> > }
> > postcore_initcall(rpmsg_init);
> >
> > -static void __exit rpmsg_fini(void)
> > +static void rpmsg_fini(void)
> > {
> > + rpmsg_ns_exit();
> > bus_unregister(_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.
> > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/ns.c
> > similarity index 76%
> > rename from drivers/rpmsg/rpmsg_ns.c
> > rename to drivers/rpmsg/ns.c
> > index 8e26824ca328..86c011bfb62f 100644
> > --- a/drivers/rpmsg/rpmsg_ns.c
> > +++ b/drivers/rpmsg/ns.c
> > @@ -2,15 +2,47 @@
> > /*
> > * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> > */
> > +#include
> > #include
> > +#include
> > #include
> > #include
> > -#include
> > #include
> > -#include
> > +#include
> > +#include
> >
> > #include "rpmsg_internal.h"
> >
> > +int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> > +{
> > + int ret;
> > +
> > + strcpy(rpdev->id.name, "rpmsg_ns");
> > + rpdev->driver_override = "rpmsg_ns";
> > + rpdev->src = RPMSG_NS_ADDR;
> > + rpdev->dst = RPMSG_NS_ADDR;
> > +
> > + ret = rpmsg_register_device(rpdev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!wait_for_completion_timeout(>ns_ready,
> > +msecs_to_jiffies(1))) {
>
> Does this work if called in rproc_virtio_probe? i tried a similar
> implementation
> but it always falls in timeout because rpmsg_ns_probe never called, probably
> due
> to the serial probing.The rpmsg_ns probe always occurs after the end of the
> virtio probe.
It works, yes. As you see, rpmsg_register_device() is called first, that can
already result in the .probe() being called and the completion being signalled
before we actually start a wait on it. That works well. BTW, the version here
is
missing a call to
+
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Guennadi,
On 11/11/20 3:49 PM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
>
> On Tue, Nov 10, 2020 at 07:18:45PM +0100, Arnaud POULIQUEN 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
>
> 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)
>
> Right, as I mentioned in the email, that specific patch version only makes the
> race window smaller, but doesn't close it completely. However, I think, we
> could
> use a completion to close it fully, we discussed it with Mathieu and I think
> he
> is working on a solution ATM.
>
>> 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.
>
> Right, this is basically the current state of the virtio_rpmsg_bus.c driver.
> You'd
> just need to make __rpmsg_create_ept() and rpmsg_ns_cb() global and generic.
Yes this is what
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On 11/11/20 1:37 AM, Mathieu Poirier wrote:
> On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN
> 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
>
> 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
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Arnaud,
On Tue, Nov 10, 2020 at 07:18:45PM +0100, Arnaud POULIQUEN 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
> >>>
> >>> 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)
Right, as I mentioned in the email, that specific patch version only makes the
race window smaller, but doesn't close it completely. However, I think, we could
use a completion to close it fully, we discussed it with Mathieu and I think he
is working on a solution ATM.
> 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.
Right, this is basically the current state of the virtio_rpmsg_bus.c driver.
You'd
just need to make __rpmsg_create_ept() and rpmsg_ns_cb() global and generic.
> - we implement a completion as proposed by Mathieu.
Exactly, this is the second option. And I think if we link NS with the
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
On Tue, 10 Nov 2020 at 11:18, Arnaud POULIQUEN 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
> >>>
> >>> 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.
> - 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
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
>>>
>>> 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
>From f2de77027f4a3836f8bf46aa257e5592af6529b7 Mon Sep 17 00:00:00 2001
From: Arnaud Pouliquen
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
---
drivers/rpmsg/rpmsg_ns.c | 26 -
drivers/rpmsg/virtio_rpmsg_bus.c | 65
2 files changed, 89 insertions(+), 2
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
> >
> > 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.
> Thanks
> Guennadi
>
> > if rpmsg_virtio_bus is built-in
> > -> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that
> > rpmsg_ns is also built-in
> > if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
> > -> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c
> >
> > Thanks,
> > Arnaud
>
> From: Guennadi Liakhovetski
> 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
> ---
> drivers/rpmsg/Makefile | 2 +-
> drivers/rpmsg/{rpmsg_core.c => core.c} | 13 ++---
> drivers/rpmsg/{rpmsg_ns.c => ns.c} | 13 +++--
> include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 6 +-
> 4 files changed, 19 insertions(+), 15 deletions(-)
> rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
> rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (87%)
> rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (95%)
>
> 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
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
>
> 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");
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.
Thanks
Guennadi
> if rpmsg_virtio_bus is built-in
> -> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that
> rpmsg_ns is also built-in
> if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
> -> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c
>
> Thanks,
> Arnaud
From: Guennadi Liakhovetski
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
---
drivers/rpmsg/Makefile | 2 +-
drivers/rpmsg/{rpmsg_core.c => core.c} | 13 ++---
drivers/rpmsg/{rpmsg_ns.c => ns.c} | 13 +++--
include/linux/{rpmsg_ns.h => rpmsg/ns.h} | 6 +-
4 files changed, 19 insertions(+), 15 deletions(-)
rename drivers/rpmsg/{rpmsg_core.c => core.c} (99%)
rename drivers/rpmsg/{rpmsg_ns.c => ns.c} (87%)
rename include/linux/{rpmsg_ns.h => rpmsg/ns.h} (95%)
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
#include
#include
+#include
#include
#include
#include
@@ -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;
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
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.
if rpmsg_virtio_bus is built-in
-> using "select RPMSG_NS" in RPMSG_VIRTIO kconfig should ensure that rpmsg_ns
is also built-in
if rpmsg_virtio_bus is build as module rpmsg_ns.ko should be loaded first.
-> MODULE_SOFTDEP could be used in virtio_rpmsg_bus.c
Thanks,
Arnaud
>
> Thanks for reporting this,
> Mathieu
>
>>
>> Thanks
>> Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
> > >
> > > 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.
Thanks for reporting this,
Mathieu
>
> Thanks
> Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
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
> >
> > 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.
Thanks
Guennadi
Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Mathieu, Arnaud,
On Thu, Nov 05, 2020 at 03:50:28PM -0700, Mathieu Poirier wrote:
> From: Arnaud Pouliquen
>
> 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.
Thanks
Guennadi
> Co-developed-by: Mathieu Poirier
> Signed-off-by: Mathieu Poirier
> Signed-off-by: Arnaud Pouliquen
> ---
> drivers/rpmsg/Kconfig| 8 +++
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/rpmsg_ns.c | 108 +++
> drivers/rpmsg/virtio_rpmsg_bus.c | 86 ++--
> include/linux/rpmsg/ns.h | 18 ++
> 5 files changed, 154 insertions(+), 67 deletions(-)
> create mode 100644 drivers/rpmsg/rpmsg_ns.c
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index f96716893c2a..c3fc75e6514b 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -15,6 +15,14 @@ config RPMSG_CHAR
> in /dev. They make it possible for user-space programs to send and
> receive rpmsg packets.
>
> +config RPMSG_NS
> + tristate "RPMSG name service announcement"
> + depends on RPMSG
> + help
> + Say Y here to enable the support of the name service announcement
> + channel that probes the associated RPMsg device on remote endpoint
> + service announcement.
> +
> config RPMSG_MTK_SCP
> tristate "MediaTek SCP"
> depends on MTK_SCP
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index ffe932ef6050..8d452656f0ee 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> 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_ns.c b/drivers/rpmsg/rpmsg_ns.c
> new file mode 100644
> index ..5bda7cb44618
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "rpmsg_internal.h"
> +
> +/* invoked when a name service announcement arrives */
> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> +void *priv, u32 src)
> +{
> + struct rpmsg_ns_msg *msg = data;
> + struct rpmsg_device *newch;
> + struct rpmsg_channel_info chinfo;
> + struct device *dev = rpdev->dev.parent;
> + int ret;
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> + data, len, true);
> +#endif
> +
> + if (len != sizeof(*msg)) {
> + dev_err(dev, "malformed ns msg (%d)\n", len);
> + return -EINVAL;
> + }
> +
> + /* don't trust the remote processor for null terminating the name */
> + msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> +
> + strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> + chinfo.src = RPMSG_ADDR_ANY;
> + chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> +
> + dev_info(dev, "%sing channel %s addr 0x%x\n",
> + rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> + "destroy" : "creat", msg->name, chinfo.dst);
> +
> + if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) {
> + ret = rpmsg_release_channel(rpdev, );
> + if (ret)
> + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> + } else {
> + newch = rpmsg_create_channel(rpdev, );
> + if (!newch)
> + dev_err(dev, "rpmsg_create_channel failed\n");
> + }
> +
> + return 0;
> +}
> +
> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
> +{
> + struct rpmsg_endpoint *ns_ept;
> + struct rpmsg_channel_info ns_chinfo = {
> + .src = RPMSG_NS_ADDR,
> + .dst = RPMSG_NS_ADDR,
> + .name = "name_service",
> + };
> +
> + /*
> + * Create the NS announcement service endpoint associated to the RPMsg
> + * device. The endpoint will be automatically destroyed when the RPMsg
> + * device will be deleted.
> + */
>
