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

From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	"ohad@wizery.com" <ohad@wizery.com>,
	"bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
	"linux-remoteproc@vger.kernel.org"
	<linux-remoteproc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Date: Tue, 17 Nov 2020 17:58:16 +0100	[thread overview]
Message-ID: <20201117165816.GB15538@ubuntu> (raw)
In-Reply-To: <a653c503-7fd1-7b87-88a5-88c9002ba410@st.com>

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@st.com>");
> -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
> > 