Re: [PATCH v6 1/5] module: Add empty modalias sysfs attribute

[Christophe Leroy]

Le 02/12/2022 à 23:47, Allen Webb a écrit :
> This adds the modalias sysfs attribute in preparation for its
> implementation.
> 
> Signed-off-by: Allen Webb <allenwebb@xxxxxxxxxx>
> ---
>   include/linux/module.h   |  1 +
>   kernel/module/internal.h |  2 ++
>   kernel/module/sysfs.c    | 33 +++++++++++++++++++++++++++++++++
>   kernel/params.c          |  7 +++++++
>   4 files changed, 43 insertions(+)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index ec61fb53979a9..0bfa859a21566 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -47,6 +47,7 @@ struct module_kobject {
>   	struct kobject *drivers_dir;
>   	struct module_param_attrs *mp;
>   	struct completion *kobj_completion;
> +	struct bin_attribute modalias_attr;
>   } __randomize_layout;
>   
>   struct module_attribute {
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 2e2bf236f5582..8d7ae37584868 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -259,11 +259,13 @@ static inline void add_kallsyms(struct module *mod, const struct load_info *info
>   #endif /* CONFIG_KALLSYMS */
>   
>   #ifdef CONFIG_SYSFS
> +void add_modalias_attr(struct module_kobject *mk);
>   int mod_sysfs_setup(struct module *mod, const struct load_info *info,
>   		    struct kernel_param *kparam, unsigned int num_params);
>   void mod_sysfs_teardown(struct module *mod);
>   void init_param_lock(struct module *mod);
>   #else /* !CONFIG_SYSFS */
> +static inline void add_modalias_attr(struct module_kobject *mk) {}
>   static inline int mod_sysfs_setup(struct module *mod,
>   			   	  const struct load_info *info,
>   			   	  struct kernel_param *kparam,
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index ce68f821dcd12..8dafec7455fbe 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -240,6 +240,37 @@ static inline void add_notes_attrs(struct module *mod, const struct load_info *i
>   static inline void remove_notes_attrs(struct module *mod) { }
>   #endif /* CONFIG_KALLSYMS */
>   
> +static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *bin_attr,
> +				    char *buf, loff_t pos, size_t count)
> +{
> +	return 0;
> +}
> +
> +/* Used in kernel/params.c for builtin modules.
> + *
> + * `struct module_kobject` is used instead of `struct module` because for
> + * builtin modules, the `struct module` is not available when this is called.
> + */

You are using network comment style. Other parts of the kernel have 
different style. See 
https://docs.kernel.org/process/coding-style.html#commenting

> +void add_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_bin_attr_init(&mk->modalias_attr);
> +	mk->modalias_attr.attr.name = "modalias";
> +	mk->modalias_attr.attr.mode = 0444;
> +	mk->modalias_attr.read = module_modalias_read;
> +	if (sysfs_create_bin_file(&mk->kobj, &mk->modalias_attr)) {
> +		/* We shouldn't ignore the return type, but there is nothing to
> +		 * do.
> +		 */
> +		return;
> +	}

I would have put the comment before the if, it would have been a single 
line comment and you would have avoided the { }

> +}
> +
> +static void remove_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_remove_bin_file(&mk->kobj, &mk->modalias_attr);
> +}
> +
>   static void del_usage_links(struct module *mod)
>   {
>   #ifdef CONFIG_MODULE_UNLOAD
> @@ -398,6 +429,7 @@ int mod_sysfs_setup(struct module *mod,
>   
>   	add_sect_attrs(mod, info);
>   	add_notes_attrs(mod, info);
> +	add_modalias_attr(&mod->mkobj);
>   
>   	return 0;
>   
> @@ -415,6 +447,7 @@ int mod_sysfs_setup(struct module *mod,
>   
>   static void mod_sysfs_fini(struct module *mod)
>   {
> +	remove_modalias_attr(&mod->mkobj);
>   	remove_notes_attrs(mod);
>   	remove_sect_attrs(mod);
>   	mod_kobject_put(mod);
> diff --git a/kernel/params.c b/kernel/params.c
> index 5b92310425c50..b7fd5313a3118 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -14,6 +14,12 @@
>   #include <linux/ctype.h>
>   #include <linux/security.h>
>   
> +#ifdef CONFIG_MODULES
> +#include "module/internal.h"
> +#else
> +static inline void add_modalias_attr(struct module_kobject *mk) {}
> +#endif /* !CONFIG_MODULES */

It is odd to include module internal.h outside of module.

If you really need a function from module, I think you should move its 
prototype to include/linux/module.h

> +
>   #ifdef CONFIG_SYSFS
>   /* Protects all built-in parameters, modules use their own param_lock */
>   static DEFINE_MUTEX(param_lock);
> @@ -815,6 +821,7 @@ static void __init kernel_add_sysfs_param(const char *name,
>   	BUG_ON(err);
>   	kobject_uevent(&mk->kobj, KOBJ_ADD);
>   	kobject_put(&mk->kobj);
> +	add_modalias_attr(mk);
>   }
>   
>   /*