[PATCH 0/4] ALSA: HD-audio HDMI binding fixes

[PATCH 0/4] ALSA: HD-audio HDMI binding fixes

[Takashi Iwai]

Hi,

this is a patch serires to address the issue recently reported by
Intel gfx people.  The first patch is the critical one, and it'll go
to for-linus branch, possibly for 4.12, while others are some
following works and cleanups.


Takashi

===

Takashi Iwai (4):
  ALSA: hda - Fix endless loop of codec configure
  ALSA: hda - Skip card registration when no codec is found
  ALSA: hda - Bind with i915 component before codec binding
  ALSA: hda - Minor code refactoring for Intel HDMI codec parsers

 sound/pci/hda/hda_codec.h      |  2 +
 sound/pci/hda/hda_controller.c | 11 ++++-
 sound/pci/hda/hda_intel.c      |  4 +-
 sound/pci/hda/patch_hdmi.c     | 94 +++++++++++++++---------------------------
 4 files changed, 47 insertions(+), 64 deletions(-)

-- 
2.13.2

[PATCH 1/4] ALSA: hda - Fix endless loop of codec configure

[Takashi Iwai]

azx_codec_configure() loops over the codecs found on the given
controller via a linked list.  The code used to work in the past, but
in the current version, this may lead to an endless loop when a codec
binding returns an error.

The culprit is that the snd_hda_codec_configure() unregisters the
device upon error, and this eventually deletes the given codec object
from the bus.  Since the list is initialized via list_del_init(), the
next object points to the same device itself.  This behavior change
was introduced at splitting the HD-audio code code, and forgotten to
adapt it here.

For fixing this bug, just use a *_safe() version of list iteration.

Fixes: d068ebc25e6e ("ALSA: hda - Move some codes up to hdac_bus struct")
Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.h      | 2 ++
 sound/pci/hda/hda_controller.c | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index d6fb2d5d01a7..60ce1cfc300f 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -295,6 +295,8 @@ struct hda_codec {
 
 #define list_for_each_codec(c, bus) \
 	list_for_each_entry(c, &(bus)->core.codec_list, core.list)
+#define list_for_each_codec_safe(c, n, bus)				\
+	list_for_each_entry_safe(c, n, &(bus)->core.codec_list, core.list)
 
 /* snd_hda_codec_read/write optional flags */
 #define HDA_RW_NO_RESPONSE_FALLBACK	(1 << 0)
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 3715a5725613..1c60beb5b70a 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1337,8 +1337,12 @@ EXPORT_SYMBOL_GPL(azx_probe_codecs);
 /* configure each codec instance */
 int azx_codec_configure(struct azx *chip)
 {
-	struct hda_codec *codec;
-	list_for_each_codec(codec, &chip->bus) {
+	struct hda_codec *codec, *next;
+
+	/* use _safe version here since snd_hda_codec_configure() deregisters
+	 * the device upon error and deletes itself from the bus list.
+	 */
+	list_for_each_codec_safe(codec, next, &chip->bus) {
 		snd_hda_codec_configure(codec);
 	}
 	return 0;
-- 
2.13.2

[PATCH 2/4] ALSA: hda - Skip card registration when no codec is found

[Takashi Iwai]

It's nonsense to register a card object when no codec is bound on it,
as we don't support the deferred codec binding.  Instead of
registering an empty card object, just skip the registration by
returning an error from azx_codec_configure().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_controller.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 1c60beb5b70a..d1eb14842340 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1345,6 +1345,9 @@ int azx_codec_configure(struct azx *chip)
 	list_for_each_codec_safe(codec, next, &chip->bus) {
 		snd_hda_codec_configure(codec);
 	}
+
+	if (!azx_bus(chip)->num_codecs)
+		return -ENODEV;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(azx_codec_configure);
-- 
2.13.2

[PATCH 3/4] ALSA: hda - Bind with i915 component before codec binding

[Takashi Iwai]

We used a on-demand i915 component binding for IvyBridge and
SandyBridge HDMI codecs, but it has a potential problem of the nested
module loading.  For avoiding that situation, assure the i915 binding
happening at the controller driver level for PCH controller devices,
where the initialization is performed in a detached work, instead of
calling from the codec driver probe.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c  |  4 ++--
 sound/pci/hda/patch_hdmi.c | 17 +++++------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 01eb1dc7b5b3..2db3203090ed 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -294,11 +294,11 @@ enum {
 
 /* PCH up to IVB; no runtime PM */
 #define AZX_DCAPS_INTEL_PCH_NOPM \
-	(AZX_DCAPS_INTEL_PCH_BASE)
+	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_I915_POWERWELL)
 
 /* PCH for HSW/BDW; with runtime PM */
 #define AZX_DCAPS_INTEL_PCH \
-	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
+	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_PM_RUNTIME)
 
 /* HSW HDMI */
 #define AZX_DCAPS_INTEL_HASWELL \
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 90e4ff87445e..feed8e8de2af 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -174,7 +174,6 @@ struct hdmi_spec {
 	/* i915/powerwell (Haswell+/Valleyview+) specific */
 	bool use_acomp_notifier; /* use i915 eld_notify callback for hotplug */
 	struct i915_audio_component_audio_ops i915_audio_ops;
-	bool i915_bound; /* was i915 bound in this driver? */
 
 	struct hdac_chmap chmap;
 	hda_nid_t vendor_nid;
@@ -2234,8 +2233,6 @@ static void generic_spec_free(struct hda_codec *codec)
 	struct hdmi_spec *spec = codec->spec;
 
 	if (spec) {
-		if (spec->i915_bound)
-			snd_hdac_i915_exit(&codec->bus->core);
 		hdmi_array_free(spec);
 		kfree(spec);
 		codec->spec = NULL;
@@ -2607,21 +2604,17 @@ static int patch_i915_cpt_hdmi(struct hda_codec *codec)
 	struct hdmi_spec *spec;
 	int err;
 
-	/* no i915 component should have been bound before this */
-	if (WARN_ON(codec->bus->core.audio_component))
-		return -EBUSY;
+	/* requires i915 binding */
+	if (!codec->bus->core.audio_component) {
+		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
+		return -ENODEV;
+	}
 
 	err = alloc_generic_hdmi(codec);
 	if (err < 0)
 		return err;
 	spec = codec->spec;
 
-	/* Try to bind with i915 now */
-	err = snd_hdac_i915_init(&codec->bus->core);
-	if (err < 0)
-		goto error;
-	spec->i915_bound = true;
-
 	err = hdmi_parse_codec(codec);
 	if (err < 0)
 		goto error;
-- 
2.13.2

[PATCH 4/4] ALSA: hda - Minor code refactoring for Intel HDMI codec parsers

[Takashi Iwai]

No functional change, just a slight reduction of lines.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c | 87 ++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 53 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index feed8e8de2af..8704ea43381e 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2503,19 +2503,41 @@ static void i915_pin_cvt_fixup(struct hda_codec *codec,
 	}
 }
 
-/* Intel Haswell and onwards; audio component with eld notifier */
-static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
+/* precondition and allocation for Intel codecs */
+static int alloc_intel_hdmi(struct hda_codec *codec)
 {
-	struct hdmi_spec *spec;
-	int err;
-
-	/* HSW+ requires i915 binding */
+	/* requires i915 binding */
 	if (!codec->bus->core.audio_component) {
 		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
 		return -ENODEV;
 	}
 
-	err = alloc_generic_hdmi(codec);
+	return alloc_generic_hdmi(codec);
+}
+
+/* parse and post-process for Intel codecs */
+static int parse_intel_hdmi(struct hda_codec *codec)
+{
+	int err;
+
+	err = hdmi_parse_codec(codec);
+	if (err < 0) {
+		generic_spec_free(codec);
+		return err;
+	}
+
+	generic_hdmi_init_per_pins(codec);
+	register_i915_notifier(codec);
+	return 0;
+}
+
+/* Intel Haswell and onwards; audio component with eld notifier */
+static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
+{
+	struct hdmi_spec *spec;
+	int err;
+
+	err = alloc_intel_hdmi(codec);
 	if (err < 0)
 		return err;
 	spec = codec->spec;
@@ -2539,15 +2561,7 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
 	spec->ops.setup_stream = i915_hsw_setup_stream;
 	spec->ops.pin_cvt_fixup = i915_pin_cvt_fixup;
 
-	err = hdmi_parse_codec(codec);
-	if (err < 0) {
-		generic_spec_free(codec);
-		return err;
-	}
-
-	generic_hdmi_init_per_pins(codec);
-	register_i915_notifier(codec);
-	return 0;
+	return parse_intel_hdmi(codec);
 }
 
 static int patch_i915_hsw_hdmi(struct hda_codec *codec)
@@ -2566,13 +2580,7 @@ static int patch_i915_byt_hdmi(struct hda_codec *codec)
 	struct hdmi_spec *spec;
 	int err;
 
-	/* requires i915 binding */
-	if (!codec->bus->core.audio_component) {
-		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
-		return -ENODEV;
-	}
-
-	err = alloc_generic_hdmi(codec);
+	err = alloc_intel_hdmi(codec);
 	if (err < 0)
 		return err;
 	spec = codec->spec;
@@ -2587,45 +2595,18 @@ static int patch_i915_byt_hdmi(struct hda_codec *codec)
 
 	spec->ops.pin_cvt_fixup = i915_pin_cvt_fixup;
 
-	err = hdmi_parse_codec(codec);
-	if (err < 0) {
-		generic_spec_free(codec);
-		return err;
-	}
-
-	generic_hdmi_init_per_pins(codec);
-	register_i915_notifier(codec);
-	return 0;
+	return parse_intel_hdmi(codec);
 }
 
 /* Intel IronLake, SandyBridge and IvyBridge; with eld notifier */
 static int patch_i915_cpt_hdmi(struct hda_codec *codec)
 {
-	struct hdmi_spec *spec;
 	int err;
 
-	/* requires i915 binding */
-	if (!codec->bus->core.audio_component) {
-		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
-		return -ENODEV;
-	}
-
-	err = alloc_generic_hdmi(codec);
+	err = alloc_intel_hdmi(codec);
 	if (err < 0)
 		return err;
-	spec = codec->spec;
-
-	err = hdmi_parse_codec(codec);
-	if (err < 0)
-		goto error;
-
-	generic_hdmi_init_per_pins(codec);
-	register_i915_notifier(codec);
-	return 0;
-
- error:
-	generic_spec_free(codec);
-	return err;
+	return parse_intel_hdmi(codec);
 }
 
 /*
-- 
2.13.2

Re: [PATCH 3/4] ALSA: hda - Bind with i915 component before codec binding

[Takashi Iwai]

On Wed, 28 Jun 2017 14:58:53 +0200,
Takashi Iwai wrote:
> 
> We used a on-demand i915 component binding for IvyBridge and
> SandyBridge HDMI codecs, but it has a potential problem of the nested
> module loading.  For avoiding that situation, assure the i915 binding
> happening at the controller driver level for PCH controller devices,
> where the initialization is performed in a detached work, instead of
> calling from the codec driver probe.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_intel.c  |  4 ++--
>  sound/pci/hda/patch_hdmi.c | 17 +++++------------
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 01eb1dc7b5b3..2db3203090ed 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -294,11 +294,11 @@ enum {
>  
>  /* PCH up to IVB; no runtime PM */
>  #define AZX_DCAPS_INTEL_PCH_NOPM \
> -	(AZX_DCAPS_INTEL_PCH_BASE)
> +	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_I915_POWERWELL)
>  
>  /* PCH for HSW/BDW; with runtime PM */
>  #define AZX_DCAPS_INTEL_PCH \
> -	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME)
> +	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_PM_RUNTIME)

When I read back the code again, this doesn't look like a good
solution.  The bit flag is referred as the capability for the display
power toggle, and old PCH doesn't need / have it.

The revised patch is below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2 3/4] ALSA: hda - Bind with i915 component before codec binding

We used a on-demand i915 component binding for IvyBridge and
SandyBridge HDMI codecs, but it has a potential problem of the nested
module loading.  For avoiding that situation, assure the i915 binding
happening at the controller driver level for PCH controller devices,
where the initialization is performed in a detached work, instead of
calling from the codec driver probe.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Check AZX_DRIVER_PCH instead of hacking
        AZX_DCAPS_I915_POWERWELL bit flag

 sound/pci/hda/hda_intel.c  | 35 +++++++++++++++++++++--------------
 sound/pci/hda/patch_hdmi.c | 17 +++++------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 01eb1dc7b5b3..433a2df9edad 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1384,8 +1384,10 @@ static int azx_free(struct azx *chip)
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 		if (hda->need_i915_power)
 			snd_hdac_display_power(bus, false);
-		snd_hdac_i915_exit(bus);
 	}
+	if (chip->driver_type == AZX_DRIVER_PCH ||
+	    (chip->driver_caps & AZX_DCAPS_I915_POWERWELL))
+		snd_hdac_i915_exit(bus);
 	kfree(hda);
 
 	return 0;
@@ -2201,16 +2203,9 @@ static int azx_probe_continue(struct azx *chip)
 
 	hda->probe_continued = 1;
 
-	/* Request display power well for the HDA controller or codec. For
-	 * Haswell/Broadwell, both the display HDA controller and codec need
-	 * this power. For other platforms, like Baytrail/Braswell, only the
-	 * display codec needs the power and it can be released after probe.
-	 */
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		/* HSW/BDW controllers need this power */
-		if (CONTROLLER_IN_GPU(pci))
-			hda->need_i915_power = 1;
-
+	/* bind with i915 if needed */
+	if (chip->driver_type == AZX_DRIVER_PCH ||
+	    (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)) {
 		err = snd_hdac_i915_init(bus);
 		if (err < 0) {
 			/* if the controller is bound only with HDMI/DP
@@ -2222,9 +2217,22 @@ static int azx_probe_continue(struct azx *chip)
 				dev_err(chip->card->dev,
 					"HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n");
 				goto out_free;
-			} else
-				goto skip_i915;
+			} else {
+				/* don't bother any longer */
+				chip->driver_caps &= ~AZX_DCAPS_I915_POWERWELL;
+			}
 		}
+	}
+
+	/* Request display power well for the HDA controller or codec. For
+	 * Haswell/Broadwell, both the display HDA controller and codec need
+	 * this power. For other platforms, like Baytrail/Braswell, only the
+	 * display codec needs the power and it can be released after probe.
+	 */
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		/* HSW/BDW controllers need this power */
+		if (CONTROLLER_IN_GPU(pci))
+			hda->need_i915_power = 1;
 
 		err = snd_hdac_display_power(bus, true);
 		if (err < 0) {
@@ -2234,7 +2242,6 @@ static int azx_probe_continue(struct azx *chip)
 		}
 	}
 
- skip_i915:
 	err = azx_first_init(chip);
 	if (err < 0)
 		goto out_free;
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 90e4ff87445e..feed8e8de2af 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -174,7 +174,6 @@ struct hdmi_spec {
 	/* i915/powerwell (Haswell+/Valleyview+) specific */
 	bool use_acomp_notifier; /* use i915 eld_notify callback for hotplug */
 	struct i915_audio_component_audio_ops i915_audio_ops;
-	bool i915_bound; /* was i915 bound in this driver? */
 
 	struct hdac_chmap chmap;
 	hda_nid_t vendor_nid;
@@ -2234,8 +2233,6 @@ static void generic_spec_free(struct hda_codec *codec)
 	struct hdmi_spec *spec = codec->spec;
 
 	if (spec) {
-		if (spec->i915_bound)
-			snd_hdac_i915_exit(&codec->bus->core);
 		hdmi_array_free(spec);
 		kfree(spec);
 		codec->spec = NULL;
@@ -2607,21 +2604,17 @@ static int patch_i915_cpt_hdmi(struct hda_codec *codec)
 	struct hdmi_spec *spec;
 	int err;
 
-	/* no i915 component should have been bound before this */
-	if (WARN_ON(codec->bus->core.audio_component))
-		return -EBUSY;
+	/* requires i915 binding */
+	if (!codec->bus->core.audio_component) {
+		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
+		return -ENODEV;
+	}
 
 	err = alloc_generic_hdmi(codec);
 	if (err < 0)
 		return err;
 	spec = codec->spec;
 
-	/* Try to bind with i915 now */
-	err = snd_hdac_i915_init(&codec->bus->core);
-	if (err < 0)
-		goto error;
-	spec->i915_bound = true;
-
 	err = hdmi_parse_codec(codec);
 	if (err < 0)
 		goto error;
-- 
2.13.2