| Message ID | 20250207054538.1110340-3-shyamsaini@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Properly handle module_kobject creation | expand |
On 2025-02-06 21:45:37-0800, Shyam Saini wrote: > lookup_or_create_module_kobject() is static and marked as __init, > this is not ideal for global usage. FYI missing "PATCH" in patch subject. > Fix this limitation by refactoring and declaring this as global: > - Refactor it by removing BUG_ON() and 'if else' construct by returning > early This does look like an unrelated change, could be in its own patch. > - Remove static and __init markers from the function and add its > declaration in module.h > - Mark this function as "__modinit". To facilitate this, move the > __modinit macro construct to module.h > > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> > --- > include/linux/module.h | 8 +++++++ > kernel/params.c | 48 ++++++++++++++++++------------------------ > 2 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 12f8a7d4fc1c..57d09b4e4385 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -162,6 +162,14 @@ extern void cleanup_module(void); > #define __INITRODATA_OR_MODULE __INITRODATA > #endif /*CONFIG_MODULES*/ > > +#ifdef CONFIG_MODULES > +#define __modinit > +#else > +#define __modinit __init > +#endif > + > +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name); __init / __modinit is not necessary on the declaration. You can remove it here and keep the #define private. > + > /* Generic info of form tag = "info" */ > #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info) > > diff --git a/kernel/params.c b/kernel/params.c > index 4b43baaf7c83..5d16696b1daa 100644 > --- a/kernel/params.c > +++ b/kernel/params.c > @@ -595,12 +595,6 @@ static ssize_t param_attr_store(const struct module_attribute *mattr, > } > #endif > > -#ifdef CONFIG_MODULES > -#define __modinit > -#else > -#define __modinit __init > -#endif > - > #ifdef CONFIG_SYSFS > void kernel_param_lock(struct module *mod) > { > @@ -763,38 +757,38 @@ void destroy_params(const struct kernel_param *params, unsigned num) > params[i].ops->free(params[i].arg); > } > > -static struct module_kobject * __init lookup_or_create_module_kobject(const char *name) > +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name) > { > struct module_kobject *mk; > struct kobject *kobj; > int err; > > kobj = kset_find_obj(module_kset, name); > - if (kobj) { > - mk = to_module_kobject(kobj); > - } else { > - mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); > - BUG_ON(!mk); > - > - mk->mod = THIS_MODULE; > - mk->kobj.kset = module_kset; > - err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, > + if (kobj) > + return to_module_kobject(kobj); > + > + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); > + if (!mk) > + return NULL; > + > + mk->mod = THIS_MODULE; > + mk->kobj.kset = module_kset; > + err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, > "%s", name); > #ifdef CONFIG_MODULES As you are cleaning this up anyways: The #ifdef above should become if (IS_ENABLED(CONFIG_MODULES)) > - if (!err) > - err = sysfs_create_file(&mk->kobj, &module_uevent.attr); > + if (!err) > + err = sysfs_create_file(&mk->kobj, &module_uevent.attr); > #endif > - if (err) { > - kobject_put(&mk->kobj); > - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", > - name, err); > - return NULL; > - } > - > - /* So that we hold reference in both cases. */ > - kobject_get(&mk->kobj); > + if (err) { > + kobject_put(&mk->kobj); > + pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", > + name, err); > + return NULL; > } > > + /* So that we hold reference in both cases. */ > + kobject_get(&mk->kobj); > + > return mk; > } > > -- > 2.34.1 >
On Thu, Feb 06 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote: > lookup_or_create_module_kobject() is static and marked as __init, > this is not ideal for global usage. > > Fix this limitation by refactoring and declaring this as global: > - Refactor it by removing BUG_ON() and 'if else' construct by returning > early > - Remove static and __init markers from the function and add its > declaration in module.h > - Mark this function as "__modinit". To facilitate this, move the > __modinit macro construct to module.h > I think this does too much in one patch. More or less any time you find yourself writing such a bullet list, ask yourself it that doesn't mean you're really writing three distinct commit messages (and thus should make it three patches). This also forces you to explicitly justify the removal of BUG_ON(), instead of merely stating that the patch does that (the "what" is clear from the patch itself, the commit message is for the "why"). Something like In the unlikely event of the allocation failing, it is better to let the machine boot with a not fully populated sysfs than to kill it with this BUG_ON(). All callers are already prepared for lookup_or_create_module_kobject() returning NULL. This is also preparation for calling this function from non-__init code, where using BUG_ON for allocation failure handling is not acceptable. That will also make it much easier to verify that the refactoring commit does only that and doesn't accidentally introduce some subtle change. Actually, since you're not moving the function anymore (and shouldn't), you don't really _need_ to include that refactoring patch, but it would be a nice thing to have since you're touching this code anyway. > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> > --- > include/linux/module.h | 8 +++++++ > kernel/params.c | 48 ++++++++++++++++++------------------------ > 2 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 12f8a7d4fc1c..57d09b4e4385 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -162,6 +162,14 @@ extern void cleanup_module(void); > #define __INITRODATA_OR_MODULE __INITRODATA > #endif /*CONFIG_MODULES*/ > > +#ifdef CONFIG_MODULES > +#define __modinit > +#else > +#define __modinit __init > +#endif > + > +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name); > + No, the section placement doesn't need to, and should not, be on the declaration, that just belongs with the definition. So there's no reason to move that __modinit definition and make it globally visible. Rasmus
diff --git a/include/linux/module.h b/include/linux/module.h index 12f8a7d4fc1c..57d09b4e4385 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -162,6 +162,14 @@ extern void cleanup_module(void); #define __INITRODATA_OR_MODULE __INITRODATA #endif /*CONFIG_MODULES*/ +#ifdef CONFIG_MODULES +#define __modinit +#else +#define __modinit __init +#endif + +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name); + /* Generic info of form tag = "info" */ #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info) diff --git a/kernel/params.c b/kernel/params.c index 4b43baaf7c83..5d16696b1daa 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -595,12 +595,6 @@ static ssize_t param_attr_store(const struct module_attribute *mattr, } #endif -#ifdef CONFIG_MODULES -#define __modinit -#else -#define __modinit __init -#endif - #ifdef CONFIG_SYSFS void kernel_param_lock(struct module *mod) { @@ -763,38 +757,38 @@ void destroy_params(const struct kernel_param *params, unsigned num) params[i].ops->free(params[i].arg); } -static struct module_kobject * __init lookup_or_create_module_kobject(const char *name) +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name) { struct module_kobject *mk; struct kobject *kobj; int err; kobj = kset_find_obj(module_kset, name); - if (kobj) { - mk = to_module_kobject(kobj); - } else { - mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); - BUG_ON(!mk); - - mk->mod = THIS_MODULE; - mk->kobj.kset = module_kset; - err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, + if (kobj) + return to_module_kobject(kobj); + + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL); + if (!mk) + return NULL; + + mk->mod = THIS_MODULE; + mk->kobj.kset = module_kset; + err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, "%s", name); #ifdef CONFIG_MODULES - if (!err) - err = sysfs_create_file(&mk->kobj, &module_uevent.attr); + if (!err) + err = sysfs_create_file(&mk->kobj, &module_uevent.attr); #endif - if (err) { - kobject_put(&mk->kobj); - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", - name, err); - return NULL; - } - - /* So that we hold reference in both cases. */ - kobject_get(&mk->kobj); + if (err) { + kobject_put(&mk->kobj); + pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", + name, err); + return NULL; } + /* So that we hold reference in both cases. */ + kobject_get(&mk->kobj); + return mk; }
lookup_or_create_module_kobject() is static and marked as __init, this is not ideal for global usage. Fix this limitation by refactoring and declaring this as global: - Refactor it by removing BUG_ON() and 'if else' construct by returning early - Remove static and __init markers from the function and add its declaration in module.h - Mark this function as "__modinit". To facilitate this, move the __modinit macro construct to module.h Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> --- include/linux/module.h | 8 +++++++ kernel/params.c | 48 ++++++++++++++++++------------------------ 2 files changed, 29 insertions(+), 27 deletions(-)