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 <arnaud.pouliquen@xxxxxx>"); > -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 <linux/module.h> > #include <linux/mod_devicetable.h> > #include <linux/rpmsg.h> > #include <linux/rpmsg/byteorder.h> > @@ -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(&module_mutex); > + rpmsg_ns = find_module(name); > + mutex_unlock(&module_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(&module_mutex); + rpmsg_ns = find_module(KBUILD_MODNAME); + mutex_unlock(&module_mutex); + + if (!rpmsg_ns) { + ret = request_module(KBUILD_MODNAME); + if (ret) + pr_err("Can not find module %s (err %d)\n", KBUILD_MODNAME, ret); + } +#endif + + 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 +113,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 +137,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 Regards, Arnaud
