On Thu, Feb 06 2025, Shyam Saini <shyamsa...@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 <li...@rasmusvillemoes.dk> > Signed-off-by: Shyam Saini <shyamsa...@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
