[v5,0/2] Let userspace know when snd-hda-intel needs i915

Currently, kernel/module annotates module dependencies when
request_symbol is used, but it doesn't cover more complex inter-driver
dependencies that are subsystem and/or driver-specific.

In the case of hdmi sound, depending on the CPU/GPU, sometimes the
snd_hda_driver can talk directly with the hardware, but sometimes, it
uses the i915 driver. When the snd_hda_driver uses i915, it should
first be unbind/rmmod, as otherwise trying to unbind/rmmod the i915
driver cause driver issues, as as reported by CI tools with different
GPU models:
	https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6415/fi-tgl-1115g4/igt@core_hotunplug@unbind-rebind.html
	https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11495/bat-adlm-1/igt@i915_module_load@reload.html

In the past, just a few CPUs were doing such bindings, but this issue now
applies to all "modern" Intel CPUs  that have onboard graphics, as well as
to the  newer discrete GPUs.

With the discrete GPU case, the HDA controller is physically separate and
requires i915 to power on the hardware for all hardware  access. In this
case, the issue is hit basicly 100% of the time.

With on-board graphics, i915 driver is needed only when the display
codec is accessed. If i915 is unbind during runtime suspend, while
snd-hda-intel is still bound, nothing bad happens, but unbinding i915
on other situations may also cause issues.

So, add support at kernel/modules to allow snd-hda drivers to properly
annotate when a dependency on a DRM driver dependencies exists,
and add a call to such new function at the snd-hda driver when it
successfully binds into the DRM driver.

This would allow userspace tools to check and properly remove the
audio driver before trying to remove or unbind the GPU driver.

It should be noticed that this series conveys the hidden module
dependencies. Other changes are needed in order to allow
removing or unbinding the i915 driver while keeping the snd-hda-intel
driver loaded/bound. With that regards, there are some discussions on
how to improve this at alsa-devel a while  back:

https://mailman.alsa-project.org/pipermail/alsa-devel/2021-September/190099.html

So, future improvements on both in i915 and the audio drivers could be made.
E.g. with  discrete GPUs, it's the only codec of the card, so it seems feasible
to detach the ALSA card if i915 is bound (using infra made for VGA
switcheroo), but,  until these improvements are done and land in
upstream, audio drivers needs to be unbound if i915 driver goes unbind.

Yet, even if such fixes got merged, this series is still needed, as it makes
such dependencies more explicit and easier to debug.

PS.: This series was generated against next-20220428.

---

v5:
- while v4 works fine, it ends calling try_module_format() recursively, which
  is not what it it was supposed to do. So, change the logic to avoid such
  recursion, by adding a static __try_module_format() and renaming the
  new version that takes two arguments as try_module_format_owner().

v4:
 - fix a compilation warning reported by Intel's Kernel robot when
   !CONFIG_MODULE_UNLOAD or !CONFIG_MODULE.

v3: minor fixes:
 - fixed a checkpatch warning;
 - use a single line for the new function prototype.

v2:
 - the dependencies are now handled directly at try_module_get().

Mauro Carvalho Chehab (2):
  module: update dependencies at try_module_get()
  ALSA: hda - identify when audio is provided by a video driver

 include/linux/module.h     |  8 +++--
 kernel/module/main.c       | 65 ++++++++++++++++++++++++++++++--------
 sound/hda/hdac_component.c |  2 +-
 3 files changed, 57 insertions(+), 18 deletions(-)

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)
>   {