Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Christophe Leroy]

Le 04/02/2025 à 06:22, Shyam Saini a écrit :
> [Vous ne recevez pas souvent de courriers de shyamsaini@xxxxxxxxxxxxxxxxxxx. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Move the following to module.h to allow common usages:
>   - lookup_or_create_module_kobject()
>   - to_module_attr
>   - to_module_kobject
>
> This makes lookup_or_create_module_kobject() as inline.

Why an inline ? It looks quite big for an inline. Why not just make it 
global ?

> Signed-off-by: Shyam Saini <shyamsaini@xxxxxxxxxxxxxxxxxxx>
> ---
>   include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
>   kernel/params.c        | 39 ---------------------------------------
>   2 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
>   #endif /* CONFIG_MODULES */
>
>   #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
>   extern struct kset *module_kset;
>   extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init 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);

Would look cleaner without the else. Something like:

	if (kobj)
		return to_module_kobject(kobj);

	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
...


> +       } else {
> +               mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +               BUG_ON(!mk);

Why a BUG_ON() ? Why not just return NULL and let the caller handle the 
failure ?

> +
> +               mk->mod = THIS_MODULE;
> +               mk->kobj.kset = module_kset;
> +               err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> +                                          "%s", name);
> +#ifdef CONFIG_MODULES

This #ifdef is not needed, module_uevent is declared at all time in 
module.h, IS_ENABLED(CONFIG_MODULES) should be enough.

> +               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);
> +       }
> +
> +       return mk;
> +}
> +
>   #endif /* CONFIG_SYSFS */
>
>   #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..3c0798b79f76 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -537,10 +537,6 @@ const struct kernel_param_ops param_ops_string = {
>   };
>   EXPORT_SYMBOL(param_ops_string);
>
> -/* sysfs output in /sys/modules/XYZ/parameters/ */
> -#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> -#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> -
>   struct param_attribute
>   {
>          struct module_attribute mattr;
> @@ -763,41 +759,6 @@ 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 *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,
> -                                          "%s", name);
> -#ifdef CONFIG_MODULES
> -               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);
> -       }
> -
> -       return mk;
> -}
> -
>   static void __init kernel_add_sysfs_param(const char *name,
>                                            const struct kernel_param *kparam,
>                                            unsigned int name_skip)
> --
> 2.34.1