| Message ID | 20221202224744.1447448-1-allenwebb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/5] module: Add empty modalias sysfs attribute | expand |
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@google.com> > --- > 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); > } > > /*
On Fri, Dec 02, 2022 at 04:47:40PM -0600, Allen Webb wrote: > This adds the modalias sysfs attribute in preparation for its > implementation. What implementation? This changelog doesn't make too much sense on it's own, and that's not good. Please explain what this attribute is for and what it is going to do, AND include the documentation for the attribute please. > > Signed-off-by: Allen Webb <allenwebb@google.com> > --- > 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); This can not fail? > +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. > + */ > +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. > + */ Odd commenting style. And yes, if this fails, that is NOT good, please fix that up. But why is this a static attribute like this as part of the module_kobject structure and not just a "normal" attribute that is shared by all kobjects? > + return; > + } > +} > + > +static void remove_modalias_attr(struct module_kobject *mk) > +{ > + sysfs_remove_bin_file(&mk->kobj, &mk->modalias_attr); Why? Isn't this automatically cleaned up by the kobject core? > +} > + > 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); Again, error checking :( > > 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 */ #ifdef does not belong in .c files, and mucking around in kernel/params to go into module/internal.h feels like a layering problem. > + > #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); You drop the reference on a kobject and then add a sysfs file to it? Why is this being called here at all? thanks, greg k-h
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. + */ +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; + } +} + +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 */ + #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); } /*
This adds the modalias sysfs attribute in preparation for its implementation. Signed-off-by: Allen Webb <allenwebb@google.com> --- include/linux/module.h | 1 + kernel/module/internal.h | 2 ++ kernel/module/sysfs.c | 33 +++++++++++++++++++++++++++++++++ kernel/params.c | 7 +++++++ 4 files changed, 43 insertions(+)