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

From: Guennadi Liakhovetski
Date: Tue Nov 17 2020 - 11:59:23 EST

Next message: Rafael J. Wysocki: "Re: [PATCH V2] cpufreq: stats: Use local_clock() instead of jiffies"
Previous message: Konrad Rzeszutek Wilk: "Re: [PATCH v3] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests."
In reply to: Arnaud POULIQUEN: "Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver"
Next in thread: Arnaud POULIQUEN: "Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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(&module_mutex);
> >> + rpmsg_ns = find_module(KBUILD_MODNAME);
> >> + mutex_unlock(&module_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] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
> (rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
> [ 11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
> (bus_for_each_drv+0x84/0xd0)
> [ 11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
> (__device_attach+0xf0/0x188)
> [ 11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
> (bus_probe_device+0x84/0x8c)
> [ 11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
> (device_add+0x3b0/0x7b0)
> [ 11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
> (rpmsg_register_device+0x54/0x88 [rpmsg_core])
> [ 11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
> [<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
> [ 11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
> (virtio_dev_probe+0x1f4/0x2c4)
> [ 11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
> (really_probe+0x208/0x4f0)
> [ 11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
> (driver_probe_device+0x78/0x16c)
> [ 11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
> (device_driver_attach+0x58/0x60)
> [ 11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
> (__driver_attach+0xb4/0x154)
> [ 11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
> (bus_for_each_dev+0x78/0xc0)
> [ 11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
> (bus_add_driver+0x170/0x20c)
> [ 11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
> (driver_register+0x74/0x108)
> [ 12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
> (rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
> [ 12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
> (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 <arnaud.pouliquen@xxxxxx>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index bdc1ea278814..68eac2b42075 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
> /* Address 53 is reserved for advertising remote services */
> #define RPMSG_NS_ADDR (53)
>
> -/**
> - * 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.
> - */
> -static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
> -{
> - strcpy(rpdev->id.name, "rpmsg_ns");
> - rpdev->driver_override = "rpmsg_ns";
> - rpdev->src = RPMSG_NS_ADDR;
> - rpdev->dst = RPMSG_NS_ADDR;
> -
> - return rpmsg_register_device(rpdev);
> -}
> +int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
>
> #endif
>
> Thanks,
> Arnaud
>
> >
> > Thanks
> > Guennadi
> >

Next message: Rafael J. Wysocki: "Re: [PATCH V2] cpufreq: stats: Use local_clock() instead of jiffies"
Previous message: Konrad Rzeszutek Wilk: "Re: [PATCH v3] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests."
In reply to: Arnaud POULIQUEN: "Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver"
Next in thread: Arnaud POULIQUEN: "Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]