Re: [v2 2/3] kernel: refactor and globalize lookup_or_create_module_kobject()

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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