Let userspace know when snd-hda-intel needs i915

PATCH Superseded Mauro Carvalho Chehab 2 participants 6 messages Apr 30, 2022

Subsystems (2)

Sound Module support

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2022-04-30 17:10 UTC #0 lore raw

v4 cover 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@[email protected]
	https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11495/bat-adlm-1/igt@[email protected]

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.

---

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       | 33 +++++++++++++++++++++++++++++++--
 sound/hda/hdac_component.c |  2 +-
 3 files changed, 37 insertions(+), 6 deletions(-)

Signature · 1 line

2.35.1

Mauro Carvalho Chehab

author

2022-04-30 17:10 UTC #1 lore raw

PATCH v4 1/2 module: update dependencies at try_module_get()

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 <[email protected]>

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v4 0/2] at: https://lore.kernel.org/all/[email protected]/

 include/linux/module.h |  8 +++++---
 kernel/module/main.c   | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 5 deletions(-)

include/linux/module.h +5 -3

index 46d4d5f2516e..3b67720faf3b 100644
@@ -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(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(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(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(mod, THIS_MODULE)
+
 int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 					     struct module *, unsigned long),
 				   void *data);

kernel/module/main.c +31 -2

index 05a42d8fcd7a..d63ebf52392b 100644
@@ -631,6 +631,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,7 +868,7 @@ void __module_get(struct module *module)
 }
 EXPORT_SYMBOL(__module_get);
 
-bool try_module_get(struct module *module)
+bool __try_module_get(struct module *module, struct module *this)
 {
 	bool ret = true;

@@ -856,9 +883,11 @@ bool try_module_get(struct module *module)
 
 		preempt_enable();
 	}
+	if (ret)
+		ref_module_dependency(module, this);
 	return ret;
 }
-EXPORT_SYMBOL(try_module_get);
+EXPORT_SYMBOL(__try_module_get);
 
 void module_put(struct module *module)
 {

Signature · 1 line

2.35.1

Mauro Carvalho Chehab

author

2022-04-30 17:10 UTC #2 lore raw

PATCH v4 2/2 ALSA: hda - identify when audio is provided by a video driver

In reply to Mauro Carvalho Chehab

On some devices, the hda driver needs to hook into a video driver, in order to be able to properly access the audio hardware and/or the power management function.

That's the case of several snd_hda_intel devices that depends on i915 driver.

Ensure that a proper reference between the snd-hda driver needing such binding is shown at /proc/modules, in order to allow userspace to know about such binding.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v4 0/2] at: https://lore.kernel.org/all/[email protected]/

 sound/hda/hdac_component.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/hda/hdac_component.c +1 -1

index bb37e7e0bd79..30e130457272 100644
@@ -199,7 +199,7 @@ static int hdac_component_master_bind(struct device *dev)
 	}
 
 	/* pin the module to avoid dynamic unbinding, but only if given */
-	if (!try_module_get(acomp->ops->owner)) {
+	if (!__try_module_get(acomp->ops->owner, dev->driver->owner)) {
 		ret = -ENODEV;
 		goto out_unbind;
 	}

Signature · 1 line

2.35.1

Patchwork

2022-04-30 17:24 UTC #3 lore raw

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Let userspace know when snd-hda-intel needs i915

In reply to Mauro Carvalho Chehab

== Series Details ==

Series: Let userspace know when snd-hda-intel needs i915
URL : https://patchwork.freedesktop.org/series/103400/
State : warning

== Summary ==

Error: dim checkpatch failed
61a1e180c395 module: update dependencies at try_module_get() -:25: CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files #25: FILE: include/linux/module.h:615:
+extern bool __try_module_get(struct module *module, struct module *this);

total: 0 errors, 0 warnings, 1 checks, 83 lines checked 401c0ab5d354 ALSA: hda - identify when audio is provided by a video driver

Patchwork

2022-04-30 17:24 UTC #4 lore raw

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Let userspace know when snd-hda-intel needs i915

In reply to Mauro Carvalho Chehab

== Series Details ==

Series: Let userspace know when snd-hda-intel needs i915
URL : https://patchwork.freedesktop.org/series/103400/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Patchwork

2022-04-30 17:51 UTC #5 lore raw

[Intel-gfx] ✗ Fi.CI.BAT: failure for Let userspace know when snd-hda-intel needs i915

In reply to Mauro Carvalho Chehab

== Series Details ==

Series: Let userspace know when snd-hda-intel needs i915
URL : https://patchwork.freedesktop.org/series/103400/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11584 -> Patchwork_103400v1
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_103400v1 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_103400v1, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/index.html

Participating hosts (44 -> 41)
------------------------------

Additional (2): bat-rpls-1 bat-dg1-5
Missing (5): fi-bdw-5557u bat-adlm-1 fi-bsw-cyan fi-kbl-8809g fi-bsw-nick

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_103400v1:

### IGT changes ###

#### Possible regressions ####

* igt@i915_suspend@system-suspend-without-i915:
- bat-dg1-5: NOTRUN -> [INCOMPLETE][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

#### Suppressed ####

The following results come from untrusted machines, tests, or statuses. They do not affect the overall result.

* igt@gem_exec_suspend@basic-s3@smem:
- {bat-rpls-1}: NOTRUN -> [INCOMPLETE][2]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-rpls-1/igt@gem_exec_suspend@[email protected]

Known issues
------------

Here are the changes found in Patchwork_103400v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@fbdev@write:
- bat-dg1-5: NOTRUN -> [SKIP][3] ([i915#2582]) +4 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@gem_mmap@basic:
- bat-dg1-5: NOTRUN -> [SKIP][4] ([i915#4083])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@gem_tiled_fence_blits@basic:
- bat-dg1-5: NOTRUN -> [SKIP][5] ([i915#4077]) +2 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@gem_tiled_pread_basic:
- bat-dg1-5: NOTRUN -> [SKIP][6] ([i915#4079]) +1 similar issue
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@gem_tiled_pread_basic.html

* igt@i915_pm_backlight@basic-brightness:
- bat-dg1-5: NOTRUN -> [SKIP][7] ([i915#1155])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@i915_selftest@live@execlists:
- fi-icl-u2: [PASS][8] -> [INCOMPLETE][9] ([i915#5060])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11584/fi-icl-u2/igt@i915_selftest@[email protected]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-icl-u2/igt@i915_selftest@[email protected]

* igt@i915_selftest@live@hangcheck:
- fi-hsw-g3258: [PASS][10] -> [INCOMPLETE][11] ([i915#3303] / [i915#4785])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11584/fi-hsw-g3258/igt@i915_selftest@[email protected]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-hsw-g3258/igt@i915_selftest@[email protected]

* igt@kms_addfb_basic@basic-y-tiled-legacy:
- bat-dg1-5: NOTRUN -> [SKIP][12] ([i915#4215])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_addfb_basic@tile-pitch-mismatch:
- bat-dg1-5: NOTRUN -> [SKIP][13] ([i915#4212]) +7 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_busy@basic:
- bat-dg1-5: NOTRUN -> [SKIP][14] ([i915#4303])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_chamelium@hdmi-hpd-fast:
- bat-dg1-5: NOTRUN -> [SKIP][15] ([fdo#111827]) +7 similar issues
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- bat-dg1-5: NOTRUN -> [SKIP][16] ([i915#4103] / [i915#4213]) +1 similar issue
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_flip@basic-flip-vs-dpms:
- bat-dg1-5: NOTRUN -> [SKIP][17] ([i915#4078]) +23 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_force_connector_basic@force-load-detect:
- bat-dg1-5: NOTRUN -> [SKIP][18] ([fdo#109285])
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@kms_psr@primary_page_flip:
- bat-dg1-5: NOTRUN -> [SKIP][19] ([i915#1072] / [i915#4078]) +3 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@kms_psr@primary_page_flip.html

* igt@kms_setmode@basic-clone-single-crtc:
- bat-dg1-5: NOTRUN -> [SKIP][20] ([i915#3555])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@prime_vgem@basic-fence-mmap:
- bat-dg1-5: NOTRUN -> [SKIP][21] ([i915#3708] / [i915#4077]) +1 similar issue
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@prime_vgem@basic-userptr:
- bat-dg1-5: NOTRUN -> [SKIP][22] ([i915#3708] / [i915#4873])
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@prime_vgem@basic-write:
- bat-dg1-5: NOTRUN -> [SKIP][23] ([i915#3708]) +3 similar issues
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-dg1-5/igt@[email protected]

* igt@runner@aborted:
- fi-icl-u2: NOTRUN -> [FAIL][24] ([i915#4312])
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-icl-u2/igt@[email protected]
- fi-hsw-g3258: NOTRUN -> [FAIL][25] ([fdo#109271] / [i915#4312])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-hsw-g3258/igt@[email protected]

#### Possible fixes ####

* igt@core_auth@basic-auth:
- fi-kbl-soraka: [DMESG-WARN][26] ([i915#1982]) -> [PASS][27]
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11584/fi-kbl-soraka/igt@[email protected]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-kbl-soraka/igt@[email protected]

* igt@i915_pm_rpm@module-reload:
- fi-cfl-8109u: [DMESG-WARN][28] ([i915#62]) -> [PASS][29] +16 similar issues
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11584/fi-cfl-8109u/igt@[email protected]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/fi-cfl-8109u/igt@[email protected]

* igt@kms_flip@basic-flip-vs-modeset@b-edp1:
- {bat-adlp-6}: [DMESG-WARN][30] ([i915#3576]) -> [PASS][31] +2 similar issues
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11584/bat-adlp-6/igt@kms_flip@[email protected]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/bat-adlp-6/igt@kms_flip@[email protected]

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

Build changes
-------------

* Linux: CI_DRM_11584 -> Patchwork_103400v1

CI-20190529: 20190529
CI_DRM_11584: 3ae2e00290c290713e21118220a817a24b44d39f @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_6464: eddc67c5c85b8ee6eb4d13752ca43da5073dc985 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_103400v1: 3ae2e00290c290713e21118220a817a24b44d39f @ git://anongit.freedesktop.org/gfx-ci/linux

### Linux commits

9b005832e0e1 ALSA: hda - identify when audio is provided by a video driver 5d544f60b660 module: update dependencies at try_module_get()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103400v1/index.html