Re: [RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[Rasmus Villemoes]

On Thu, Jan 30 2025, Shyam Saini <shyamsaini@xxxxxxxxxxxxxxxxxxx> wrote:

> This reverts commit 96a1a2412acba8c057c041833641d9b7dbf52170,
> as it breaks the creation of /sys/module/<built-in-module>/drivers.
>
> The reverted commit changed the initcall order for
> param_sysfs_builtin() from subsys_initcall() to late_initcall(),
> which impacts the module_kset list and its population.
>
> Drivers which are initialized from subsys_initcall() or any other
> higher precedence initcall couldn't find the related kobject entry
> in the module_kset list because module_kset is not fully populated
> at this point due to the reverted change. As a consequence,
> module_add_driver() returns early without calling make_driver_name().
> Therefore, /sys/module/<built-in-module>/drivers is never created.
>
> This breaks user-space applications for eg: DPDK, which expect
> /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id to be present.
>

:(

Unfortunately, the init time saved by the mentioned commit is important
for some boards, and reverting that commit now will mean that some of
those boards may spuriously fail to boot due to random timing and the
external watchdog firing.

I wonder why this has taken 2+ years to be noticed?

Since the problem is that /sys/module/vfio_pci is not (yet) created as a side
effect of version_sysfs_builtin() and/or param_sysfs_builtin(), both of
which use locate_module_kobject() to lookup-or-create that, maybe the
code in module_add_driver() could be reworked to use that.

It's of course not entirely trivial, as locate_module_kobject() is
currently __init and static, but that should be fixable if we want to go
that route. Maybe it should be refactored a little to be callable from
the builtin-module branch of module_add_driver(), but ignoring that,
something like

diff --git a/drivers/base/module.c b/drivers/base/module.c
index 5bc71bea883a..6b32c5fec283 100644
--- a/drivers/base/module.c
+++ b/drivers/base/module.c
@@ -42,16 +42,13 @@ int module_add_driver(struct module *mod, const struct device_driver *drv)
 	if (mod)
 		mk = &mod->mkobj;
 	else if (drv->mod_name) {
-		struct kobject *mkobj;
-
 		/* Lookup built-in module entry in /sys/modules */
-		mkobj = kset_find_obj(module_kset, drv->mod_name);
-		if (mkobj) {
-			mk = container_of(mkobj, struct module_kobject, kobj);
+		mk = locate_module_kobject(drv->mod_name);
+		if (mk) {
 			/* remember our module structure */
 			drv->p->mkobj = mk;
-			/* kset_find_obj took a reference */
-			kobject_put(mkobj);
+			/* locate_module_kobject took a reference */
+			kobject_put(&mk->mkobj);
 		}
 	}
 
It also seems to me that if a module has neither a MODULE_VERSION nor
any module parameters, /sys/module/<modname> wouldn't be created as part
of that param_sysfs_builtin(), regardless of commit 96a1a2412acb. So
relying on that sysfs directory existing seems to be a little fragile;
IOW I do think that module_add_driver() should itself ensure that the
module_kobject exists.

Rasmus