| Message ID | ad2a9fe66cf502e2e2e2325f1f04d0fae36aa82b.1651348913.git.mchehab@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Let userspace know when snd-hda-intel needs i915 | expand |
Le 30/04/2022 à 22:04, Mauro Carvalho Chehab a écrit : > Sometimes, device drivers are bound into each other via try_module_get(), > making such references invisible when looking at /proc/modules or lsmod. > > Add a function to allow setting up module references for such > cases, and call it when try_module_get() is used. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > > See [PATCH v5 0/2] at: https://lore.kernel.org/all/cover.1651348913.git.mchehab@kernel.org/ > > include/linux/module.h | 8 ++++-- > kernel/module/main.c | 65 +++++++++++++++++++++++++++++++++--------- > 2 files changed, 56 insertions(+), 17 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 46d4d5f2516e..3d9d38c426b4 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -620,12 +620,12 @@ extern void __module_get(struct module *module); > > /* This is the Right Way to get a module: if it fails, it's being removed, > * so pretend it's not there. */ > -extern bool try_module_get(struct module *module); > +extern bool try_module_get_owner(struct module *module, struct module *this); You may want to remove that useless 'extern'. 'checkpatch --strict' will likely tell you to do so. > > extern void module_put(struct module *module); > > #else /*!CONFIG_MODULE_UNLOAD*/ > -static inline bool try_module_get(struct module *module) > +static inline bool try_module_get_owner(struct module *module, struct module *this) > { > return !module || module_is_live(module); > } > @@ -740,7 +740,7 @@ static inline void __module_get(struct module *module) > { > } > > -static inline bool try_module_get(struct module *module) > +static inline bool try_module_get_owner(struct module *module, struct module *this) > { > return true; > } > @@ -875,6 +875,8 @@ static inline bool module_sig_ok(struct module *module) > } > #endif /* CONFIG_MODULE_SIG */ > > +#define try_module_get(mod) try_module_get_owner(mod, THIS_MODULE) > + > int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > struct module *, unsigned long), > void *data); > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 05a42d8fcd7a..218c4308bb7a 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -150,6 +150,24 @@ int unregister_module_notifier(struct notifier_block *nb) > } > EXPORT_SYMBOL(unregister_module_notifier); > > +static bool __try_module_get(struct module *module) > +{ > + bool ret = true; > + > + if (module) { > + preempt_disable(); > + /* Note: here, we can fail to get a reference */ > + if (likely(module_is_live(module) && > + atomic_inc_not_zero(&module->refcnt) != 0)) > + trace_module_get(module, _RET_IP_); > + else > + ret = false; > + > + preempt_enable(); > + } > + return ret; > +} > + > /* > * We require a truly strong try_module_get(): 0 means success. > * Otherwise an error is returned due to ongoing or failed > @@ -160,7 +178,7 @@ static inline int strong_try_module_get(struct module *mod) > BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); > if (mod && mod->state == MODULE_STATE_COMING) > return -EBUSY; > - if (try_module_get(mod)) > + if (__try_module_get(mod)) > return 0; > else > return -ENOENT; > @@ -631,6 +649,33 @@ static int ref_module(struct module *a, struct module *b) > return 0; > } > > +static int ref_module_dependency(struct module *mod, struct module *this) What is 'this' ? Can we give it a more precise name ? Is it a child, a parent, a owner, something else ? > +{ > + int ret; > + > + if (!this || !this->name) > + return -EINVAL; > + > + if (mod == this) > + return 0; > + > + mutex_lock(&module_mutex); > + > + ret = ref_module(this, mod); > + > +#ifdef CONFIG_MODULE_UNLOAD Looks like that #ifdef could be avoided and replaced by IS_ENABLED(CONFIG_MODULE_UNLOAD) Something like: if (!IS_ENABLED(CONFIG_MODULE_UNLOAD) || ret) goto ret; > + if (ret) > + goto ret; > + > + ret = sysfs_create_link(mod->holders_dir, > + &this->mkobj.kobj, this->name); > +#endif > + > +ret: > + mutex_unlock(&module_mutex); > + return ret; > +} > + > /* Clear the unload stuff of the module. */ > static void module_unload_free(struct module *mod) > { > @@ -841,24 +886,16 @@ void __module_get(struct module *module) > } > EXPORT_SYMBOL(__module_get); > > -bool try_module_get(struct module *module) > +bool try_module_get_owner(struct module *module, struct module *this) Same here, what is 'this' exactly ? > { > - bool ret = true; > + int ret = __try_module_get(module); > > - if (module) { > - preempt_disable(); > - /* Note: here, we can fail to get a reference */ > - if (likely(module_is_live(module) && > - atomic_inc_not_zero(&module->refcnt) != 0)) > - trace_module_get(module, _RET_IP_); > - else > - ret = false; > + if (ret) > + ref_module_dependency(module, this); > > - preempt_enable(); > - } > return ret; > } > -EXPORT_SYMBOL(try_module_get); > +EXPORT_SYMBOL(try_module_get_owner); > > void module_put(struct module *module) > {
Hi Mauro, [...] > +static int ref_module_dependency(struct module *mod, struct module *this) > +{ > + int ret; > + > + if (!this || !this->name) > + return -EINVAL; > + > + if (mod == this) > + return 0; > + > + mutex_lock(&module_mutex); > + > + ret = ref_module(this, mod); > + > +#ifdef CONFIG_MODULE_UNLOAD > + if (ret) > + goto ret; > + > + ret = sysfs_create_link(mod->holders_dir, > + &this->mkobj.kobj, this->name); > +#endif > + > +ret: > + mutex_unlock(&module_mutex); > + return ret; > +} > + > /* Clear the unload stuff of the module. */ > static void module_unload_free(struct module *mod) > { > @@ -841,24 +886,16 @@ void __module_get(struct module *module) > } > EXPORT_SYMBOL(__module_get); > > -bool try_module_get(struct module *module) > +bool try_module_get_owner(struct module *module, struct module *this) > { > - bool ret = true; > + int ret = __try_module_get(module); > > - if (module) { > - preempt_disable(); > - /* Note: here, we can fail to get a reference */ > - if (likely(module_is_live(module) && > - atomic_inc_not_zero(&module->refcnt) != 0)) > - trace_module_get(module, _RET_IP_); > - else > - ret = false; > + if (ret) > + ref_module_dependency(module, this); do we care about the return value here? Andi > > - preempt_enable(); > - } > return ret; > } > -EXPORT_SYMBOL(try_module_get); > +EXPORT_SYMBOL(try_module_get_owner); > > void module_put(struct module *module) > { > -- > 2.35.1
Em Thu, 5 May 2022 23:35:29 +0200 Andi Shyti <andi.shyti@linux.intel.com> escreveu: > Hi Mauro, > > [...] > > > +static int ref_module_dependency(struct module *mod, struct module *this) > > +{ > > + int ret; > > + > > + if (!this || !this->name) > > + return -EINVAL; > > + > > + if (mod == this) > > + return 0; > > + > > + mutex_lock(&module_mutex); > > + > > + ret = ref_module(this, mod); > > + > > +#ifdef CONFIG_MODULE_UNLOAD > > + if (ret) > > + goto ret; > > + > > + ret = sysfs_create_link(mod->holders_dir, > > + &this->mkobj.kobj, this->name); > > +#endif > > + > > +ret: > > + mutex_unlock(&module_mutex); > > + return ret; > > +} > > + > > /* Clear the unload stuff of the module. */ > > static void module_unload_free(struct module *mod) > > { > > @@ -841,24 +886,16 @@ void __module_get(struct module *module) > > } > > EXPORT_SYMBOL(__module_get); > > > > -bool try_module_get(struct module *module) > > +bool try_module_get_owner(struct module *module, struct module *this) > > { > > - bool ret = true; > > + int ret = __try_module_get(module); > > > > - if (module) { > > - preempt_disable(); > > - /* Note: here, we can fail to get a reference */ > > - if (likely(module_is_live(module) && > > - atomic_inc_not_zero(&module->refcnt) != 0)) > > - trace_module_get(module, _RET_IP_); > > - else > > - ret = false; > > + if (ret) > > + ref_module_dependency(module, this); > > do we care about the return value here? I don't think it should care about the return value, as a failure to create a sysfs node for the holder or to add it to the holders list is not fatal: modules can still continue working without that. Also, I opted to be conservative here: currently, not creating these doesn't cause try_module_get() to fail. I'm not sure what would be the impact if this starts to fail. So, right now, I'm opting to just ignore the return value. Perhaps in the future this could a warning (similarly to what sysfs create link does). Regards, Mauro > > Andi > > > > > - preempt_enable(); > > - } > > return ret; > > } > > -EXPORT_SYMBOL(try_module_get); > > +EXPORT_SYMBOL(try_module_get_owner); > > > > void module_put(struct module *module) > > { > > -- > > 2.35.1 Thanks, Mauro
diff --git a/include/linux/module.h b/include/linux/module.h index 46d4d5f2516e..3d9d38c426b4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -620,12 +620,12 @@ extern void __module_get(struct module *module); /* This is the Right Way to get a module: if it fails, it's being removed, * so pretend it's not there. */ -extern bool try_module_get(struct module *module); +extern bool try_module_get_owner(struct module *module, struct module *this); extern void module_put(struct module *module); #else /*!CONFIG_MODULE_UNLOAD*/ -static inline bool try_module_get(struct module *module) +static inline bool try_module_get_owner(struct module *module, struct module *this) { return !module || module_is_live(module); } @@ -740,7 +740,7 @@ static inline void __module_get(struct module *module) { } -static inline bool try_module_get(struct module *module) +static inline bool try_module_get_owner(struct module *module, struct module *this) { return true; } @@ -875,6 +875,8 @@ static inline bool module_sig_ok(struct module *module) } #endif /* CONFIG_MODULE_SIG */ +#define try_module_get(mod) try_module_get_owner(mod, THIS_MODULE) + int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *, unsigned long), void *data); diff --git a/kernel/module/main.c b/kernel/module/main.c index 05a42d8fcd7a..218c4308bb7a 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -150,6 +150,24 @@ int unregister_module_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(unregister_module_notifier); +static bool __try_module_get(struct module *module) +{ + bool ret = true; + + if (module) { + preempt_disable(); + /* Note: here, we can fail to get a reference */ + if (likely(module_is_live(module) && + atomic_inc_not_zero(&module->refcnt) != 0)) + trace_module_get(module, _RET_IP_); + else + ret = false; + + preempt_enable(); + } + return ret; +} + /* * We require a truly strong try_module_get(): 0 means success. * Otherwise an error is returned due to ongoing or failed @@ -160,7 +178,7 @@ static inline int strong_try_module_get(struct module *mod) BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); if (mod && mod->state == MODULE_STATE_COMING) return -EBUSY; - if (try_module_get(mod)) + if (__try_module_get(mod)) return 0; else return -ENOENT; @@ -631,6 +649,33 @@ static int ref_module(struct module *a, struct module *b) return 0; } +static int ref_module_dependency(struct module *mod, struct module *this) +{ + int ret; + + if (!this || !this->name) + return -EINVAL; + + if (mod == this) + return 0; + + mutex_lock(&module_mutex); + + ret = ref_module(this, mod); + +#ifdef CONFIG_MODULE_UNLOAD + if (ret) + goto ret; + + ret = sysfs_create_link(mod->holders_dir, + &this->mkobj.kobj, this->name); +#endif + +ret: + mutex_unlock(&module_mutex); + return ret; +} + /* Clear the unload stuff of the module. */ static void module_unload_free(struct module *mod) { @@ -841,24 +886,16 @@ void __module_get(struct module *module) } EXPORT_SYMBOL(__module_get); -bool try_module_get(struct module *module) +bool try_module_get_owner(struct module *module, struct module *this) { - bool ret = true; + int ret = __try_module_get(module); - if (module) { - preempt_disable(); - /* Note: here, we can fail to get a reference */ - if (likely(module_is_live(module) && - atomic_inc_not_zero(&module->refcnt) != 0)) - trace_module_get(module, _RET_IP_); - else - ret = false; + if (ret) + ref_module_dependency(module, this); - preempt_enable(); - } return ret; } -EXPORT_SYMBOL(try_module_get); +EXPORT_SYMBOL(try_module_get_owner); void module_put(struct module *module) {