| Message ID | cover.1651348913.git.mchehab@kernel.org |
|---|---|
| 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) > {