[PATCH] BUG-REPORT: snd-hda: hacked-together EPROBE_DEFER support

* [PATCH] BUG-REPORT: snd-hda: hacked-together EPROBE_DEFER support
@ 2017-06-21 15:08 ` Daniel Vetter
  0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2017-06-21 15:08 UTC (permalink / raw)
  To: Intel Graphics Development
  Cc: Chris Wilson, Daniel Vetter, Jaroslav Kysela, Takashi Iwai,
	Guneshwor Singh, Hardik T Shah, Julia Lawall, Vinod Koul,
	Subhransu S. Prusty, Libin Yang, linux-kernel

So back when the i915 power well support landed in

commit 99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6
Author: Wang Xingchao <xingchao.wang@linux.intel.com>
Date:   Thu May 30 22:07:10 2013 +0800

    ALSA: hda - Add power-welll support for haswell HDA

the logic to handle the cross-module depencies was hand-rolled using a
async work item, and that just doesn't work.

The correct way to handle cross-module deps is either:
- request_module + failing when the other module isn't there

OR

- failing the module load with EPROBE_DEFER.

You can't mix them, if you do then the entire load path just
busy-spins blowing through cpu cycles forever with no way to stop
this.

snd-hda-intel does mix it, because the hda codec drivers are loaded
using request_module, but the i915 depency is handled using
PROBE_DEFER (or well, should be, but I haven't found any code at all).
This is a major pain when trying to debug i915 load failures.

This patch here is a horrible hackish attempt at somewhat correctly
wriing EPROBE_DEFER through. Stuff that's missing:
- Check all the other places where load errors are conveniently
  dropped on the floor.
- Also fix up the firmware_cb path.
- Drop the debug noise I've left in to make it clear this isn't
  anything for merging.

Cheers, Daniel

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: "GitAuthor: Daniel Vetter" <daniel.vetter@ffwll.ch>
Cc: Guneshwor Singh <guneshwor.o.singh@intel.com>
Cc: Hardik T Shah <hardik.t.shah@intel.com>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Cc: Libin Yang <libin.yang@intel.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/base/dd.c              |  2 ++
 sound/pci/hda/hda_bind.c       |  6 +++---
 sound/pci/hda/hda_controller.c |  8 +++++++-
 sound/pci/hda/hda_intel.c      | 13 +++++++++----
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4882f06d12df..842bc8782124 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -17,6 +17,8 @@
  * This file is released under the GPLv2
  */
 
+#define DEBUG
+
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index 6efadbfb3fe3..0bc164a17493 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -253,7 +253,7 @@ static int codec_bind_generic(struct hda_codec *codec)
 	request_codec_module(codec);
 	if (codec_probed(codec))
 		return 0;
-	return -ENODEV;
+	return -EPROBE_DEFER;
 }
 
 #if IS_ENABLED(CONFIG_SND_HDA_GENERIC)
@@ -289,8 +289,8 @@ int snd_hda_codec_configure(struct hda_codec *codec)
 		codec_bind_module(codec);
 	if (!codec->preset) {
 		err = codec_bind_generic(codec);
-		if (err < 0) {
-			codec_err(codec, "Unable to bind the codec\n");
+		if (WARN_ON(err < 0)) {
+			codec_err(codec, "Unable to bind the codec, err=%i\n", err);
 			goto error;
 		}
 	}
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 3715a5725613..4b4262c72327 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1337,9 +1337,15 @@ EXPORT_SYMBOL_GPL(azx_probe_codecs);
 /* configure each codec instance */
 int azx_codec_configure(struct azx *chip)
 {
+	int ret;
+
 	struct hda_codec *codec;
 	list_for_each_codec(codec, &chip->bus) {
-		snd_hda_codec_configure(codec);
+		ret = snd_hda_codec_configure(codec);
+		if (ret) {
+			printk("bailing real hard %i\n", ret);
+			return ret;
+		}
 	}
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 07ea7f48aa01..8241387cc8ca 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1649,7 +1649,8 @@ static void azx_check_snoop_available(struct azx *chip)
 static void azx_probe_work(struct work_struct *work)
 {
 	struct hda_intel *hda = container_of(work, struct hda_intel, probe_work);
-	azx_probe_continue(&hda->chip);
+
+	WARN_ON(1);
 }
 
 static int default_bdl_pos_adj(struct azx *chip)
@@ -2158,7 +2159,6 @@ static int azx_probe(struct pci_dev *pci,
 					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
-		schedule_probe = false; /* continued in azx_firmware_cb() */
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
@@ -2167,8 +2167,13 @@ static int azx_probe(struct pci_dev *pci,
 		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
 #endif
 
-	if (schedule_probe)
-		schedule_work(&hda->probe_work);
+	if (schedule_probe) {
+		err = azx_probe_continue(chip);
+		if (err) {
+			printk("hit the right error return finally! err=%i\n", err);
+			goto out_free;
+		}
+	}
 
 	dev++;
 	if (chip->disabled)
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread