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

[Shyam Saini]

Hi Rasmus,

> 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?

Unfortunately, we couldn't detect it earlier, sorry about that.
I am fairly suprised no one else reported this issue.

> 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.

I have reworked this patch as per your suggestions, will send it shortly.

Thanks,
Shyam