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

[Arnaud POULIQUEN]

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