[PATCH 0/3] ALSA: PM-related fixes/cleanups

[PATCH 0/3] ALSA: PM-related fixes/cleanups

[Takashi Iwai]

Hi,

this is a patch set addressing some minor issues found in PM-related
stuff, as well as the Intel HDMI fallback issue.  The first patch
covers the unusual error paths from runtime PM, the second one is a
code refactoring and the last one fixes the superfluous (rather
harmful) binding with the generic HDMI driver as fallback of i915
component binding.


Takashi

===

Takashi Iwai (3):
  ALSA: hda - Handle error from snd_hda_power_up*()
  ALSA: hda - Move in_pm accessors to HDA core
  ALSA: hda/hdmi - Don't fall back to generic when i915 binding fails

 include/sound/hdaudio.h        | 27 +++++++++++++++++
 sound/pci/hda/hda_beep.c       |  3 +-
 sound/pci/hda/hda_codec.c      | 25 ++++++----------
 sound/pci/hda/hda_controller.c |  4 ++-
 sound/pci/hda/hda_proc.c       |  3 +-
 sound/pci/hda/hda_sysfs.c      |  4 ++-
 sound/pci/hda/patch_ca0132.c   | 53 +++++++++++++++++++++++++++-------
 sound/pci/hda/patch_hdmi.c     |  4 ++-
 sound/pci/hda/patch_realtek.c  |  3 +-
 9 files changed, 93 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai]

Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
haven't dealt with the error properly in many places.  It's an unusual
situation but still possible.

This patch spots these places and adds the proper error paths.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_beep.c       |  3 +-
 sound/pci/hda/hda_codec.c      |  4 ++-
 sound/pci/hda/hda_controller.c |  4 ++-
 sound/pci/hda/hda_proc.c       |  3 +-
 sound/pci/hda/hda_sysfs.c      |  4 ++-
 sound/pci/hda/patch_ca0132.c   | 53 +++++++++++++++++++++++++++-------
 sound/pci/hda/patch_realtek.c  |  3 +-
 7 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c
index 066b5b59c4d7..e9d5fbd6c13a 100644
--- a/sound/pci/hda/hda_beep.c
+++ b/sound/pci/hda/hda_beep.c
@@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone)
 	struct hda_codec *codec = beep->codec;
 
 	if (tone && !beep->playing) {
-		snd_hda_power_up(codec);
+		if (snd_hda_power_up(codec) < 0)
+			return;
 		if (beep->power_hook)
 			beep->power_hook(beep, true);
 		beep->playing = 1;
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 20a171ac4bb2..44165f3e344e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd,
 		return -1;
 
  again:
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 	mutex_lock(&bus->core.cmd_mutex);
 	if (flags & HDA_RW_NO_RESPONSE_FALLBACK)
 		bus->no_response_fallback = 1;
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index a12e594d4e3b..4273be1f3eaa 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 				   buff_step);
 	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
 				   buff_step);
-	snd_hda_power_up(apcm->codec);
+	err = snd_hda_power_up(apcm->codec);
+	if (err < 0)
+		return err;
 	if (hinfo->ops.open)
 		err = hinfo->ops.open(hinfo, apcm->codec, substream);
 	else
diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
index c6b778b2580c..4206749d5130 100644
--- a/sound/pci/hda/hda_proc.c
+++ b/sound/pci/hda/hda_proc.c
@@ -760,7 +760,8 @@ static void print_codec_info(struct snd_info_entry *entry,
 	fg = codec->core.afg;
 	if (!fg)
 		return;
-	snd_hda_power_up(codec);
+	if (snd_hda_power_up(codec) < 0)
+		return;
 	snd_iprintf(buffer, "Default PCM:\n");
 	print_pcm_caps(buffer, codec, fg);
 	snd_iprintf(buffer, "Default Amp-In caps: ");
diff --git a/sound/pci/hda/hda_sysfs.c b/sound/pci/hda/hda_sysfs.c
index 6ec79c58d48d..d17a808e72d2 100644
--- a/sound/pci/hda/hda_sysfs.c
+++ b/sound/pci/hda/hda_sysfs.c
@@ -130,7 +130,9 @@ static int reconfig_codec(struct hda_codec *codec)
 {
 	int err;
 
-	snd_hda_power_up(codec);
+	err = snd_hda_power_up(codec);
+	if (err < 0)
+		return err;
 	codec_info(codec, "hda-codec: reconfiguring\n");
 	err = snd_hda_codec_reset(codec);
 	if (err < 0) {
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 4ff5320378e2..3b58f6032250 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -3575,12 +3575,15 @@ static int tuning_ctl_set(struct hda_codec *codec, hda_nid_t nid,
 			  unsigned int *lookup, int idx)
 {
 	int i = 0;
+	int err;
 
 	for (i = 0; i < TUNING_CTLS_COUNT; i++)
 		if (nid == ca0132_tuning_ctls[i].nid)
 			break;
 
-	snd_hda_power_up(codec);
+	err = snd_hda_power_up(codec);
+	if (err < 0)
+		return err;
 	dspio_set_param(codec, ca0132_tuning_ctls[i].mid, 0x20,
 			ca0132_tuning_ctls[i].req,
 			&(lookup[idx]), sizeof(unsigned int));
@@ -3801,7 +3804,9 @@ static int ca0132_select_out(struct hda_codec *codec)
 
 	codec_dbg(codec, "ca0132_select_out\n");
 
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 
 	auto_jack = spec->vnode_lswitch[VNID_HP_ASEL - VNODE_START_NID];
 
@@ -3914,7 +3919,9 @@ static int ca0132_alt_select_out(struct hda_codec *codec)
 
 	codec_dbg(codec, "%s\n", __func__);
 
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 
 	auto_jack = spec->vnode_lswitch[VNID_HP_ASEL - VNODE_START_NID];
 
@@ -4225,10 +4232,13 @@ static int ca0132_select_mic(struct hda_codec *codec)
 	struct ca0132_spec *spec = codec->spec;
 	int jack_present;
 	int auto_jack;
+	int err;
 
 	codec_dbg(codec, "ca0132_select_mic\n");
 
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 
 	auto_jack = spec->vnode_lswitch[VNID_AMIC1_ASEL - VNODE_START_NID];
 
@@ -4276,10 +4286,13 @@ static int ca0132_alt_select_in(struct hda_codec *codec)
 {
 	struct ca0132_spec *spec = codec->spec;
 	unsigned int tmp;
+	int err;
 
 	codec_dbg(codec, "%s\n", __func__);
 
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 
 	chipio_set_stream_control(codec, 0x03, 0);
 	chipio_set_stream_control(codec, 0x04, 0);
@@ -4701,6 +4714,8 @@ static int ca0132_alt_slider_ctl_set(struct hda_codec *codec, hda_nid_t nid,
 {
 	int i = 0;
 	unsigned int y;
+	int err;
+
 	/*
 	 * For X_BASS, req 2 is actually crossover freq instead of
 	 * effect level
@@ -4710,7 +4725,9 @@ static int ca0132_alt_slider_ctl_set(struct hda_codec *codec, hda_nid_t nid,
 	else
 		y = 1;
 
-	snd_hda_power_up(codec);
+	err = snd_hda_power_up(codec);
+	if (err < 0)
+		return err;
 	if (nid == XBASS_XOVER) {
 		for (i = 0; i < OUT_EFFECTS_COUNT; i++)
 			if (ca0132_effects[i].nid == X_BASS)
@@ -5221,11 +5238,14 @@ static int ca0132_switch_put(struct snd_kcontrol *kcontrol,
 	int ch = get_amp_channels(kcontrol);
 	long *valp = ucontrol->value.integer.value;
 	int changed = 1;
+	int err;
 
 	codec_dbg(codec, "ca0132_switch_put: nid=0x%x, val=%ld\n",
 		    nid, *valp);
 
-	snd_hda_power_up(codec);
+	err = snd_hda_power_up(codec);
+	if (err < 0)
+		return err;
 	/* vnode */
 	if ((nid >= VNODE_START_NID) && (nid < VNODE_END_NID)) {
 		if (ch & 1) {
@@ -5390,6 +5410,7 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
 	hda_nid_t shared_nid = 0;
 	bool effective;
 	int changed = 1;
+	int err;
 
 	/* store the left and right volume */
 	if (ch & 1) {
@@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
 		int dir = get_amp_direction(kcontrol);
 		unsigned long pval;
 
-		snd_hda_power_up(codec);
+		err = snd_hda_power_up(codec);
+		if (err < 0)
+			return err;
 		mutex_lock(&codec->control_mutex);
 		pval = kcontrol->private_value;
 		kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,
@@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
 	long *valp = ucontrol->value.integer.value;
 	hda_nid_t vnid = 0;
 	int changed = 1;
+	int err;
 
 	switch (nid) {
 	case 0x02:
@@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
 		valp++;
 	}
 
-	snd_hda_power_up(codec);
+	err = snd_hda_power_up(codec);
+	if (err < 0)
+		return err;
 	ca0132_alt_dsp_volume_put(codec, vnid);
 	mutex_lock(&codec->control_mutex);
 	changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);
@@ -7190,6 +7216,7 @@ static int ca0132_init(struct hda_codec *codec)
 	struct auto_pin_cfg *cfg = &spec->autocfg;
 	int i;
 	bool dsp_loaded;
+	int err;
 
 	/*
 	 * If the DSP is already downloaded, and init has been entered again,
@@ -7220,7 +7247,9 @@ static int ca0132_init(struct hda_codec *codec)
 	if (spec->quirk == QUIRK_SBZ)
 		sbz_region2_startup(codec);
 
-	snd_hda_power_up_pm(codec);
+	err = snd_hda_power_up_pm(codec);
+	if (err < 0)
+		return err;
 
 	ca0132_init_unsol(codec);
 	ca0132_init_params(codec);
@@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec)
 	struct ca0132_spec *spec = codec->spec;
 
 	cancel_delayed_work_sync(&spec->unsol_hp_work);
-	snd_hda_power_up(codec);
+	if (snd_hda_power_up(codec) < 0)
+		goto skip_shutdown;
 	switch (spec->quirk) {
 	case QUIRK_SBZ:
 		sbz_exit_chip(codec);
@@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec)
 		break;
 	}
 	snd_hda_power_down(codec);
+ skip_shutdown:
 	if (spec->mem_base)
 		iounmap(spec->mem_base);
 	kfree(spec->spec_init_verbs);
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 5ad6c7e5f92e..b0d757cf7aab 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled)
 	pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80;
 	if (spec->mute_led_nid) {
 		/* temporarily power up/down for setting VREF */
-		snd_hda_power_up_pm(codec);
+		if (snd_hda_power_up_pm(codec) < 0)
+			return;
 		snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval);
 		snd_hda_power_down_pm(codec);
 	}
-- 
2.17.1

[PATCH 2/3] ALSA: hda - Move in_pm accessors to HDA core

[Takashi Iwai]

The in_pm atomic in hdac_device is an important field used as a flag
as well as a refcount for PM.  The existing snd_hdac_power_up/down
helpers already refer to it in the HD-audio core code, while the code
to actually setting the value (atomic_inc() / _dec()) is open-coded in
HDA legacy side, which is hard to find.

This patch adds the helper functions to set/reset the in_pm counter to
HDA core and use them in HDA legacy side, for making it clearer who /
where the PM is managed.

There is no functional changes, just code refactoring.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h    | 27 +++++++++++++++++++++++++++
 sound/pci/hda/hda_codec.c  | 21 ++++++---------------
 sound/pci/hda/patch_hdmi.c |  2 +-
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index c052afc27547..294a5a21937b 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -8,6 +8,7 @@
 
 #include <linux/device.h>
 #include <linux/interrupt.h>
+#include <linux/pm_runtime.h>
 #include <linux/timecounter.h>
 #include <sound/core.h>
 #include <sound/memalloc.h>
@@ -171,12 +172,38 @@ int snd_hdac_power_down(struct hdac_device *codec);
 int snd_hdac_power_up_pm(struct hdac_device *codec);
 int snd_hdac_power_down_pm(struct hdac_device *codec);
 int snd_hdac_keep_power_up(struct hdac_device *codec);
+
+/* call this at entering into suspend/resume callbacks in codec driver */
+static inline void snd_hdac_enter_pm(struct hdac_device *codec)
+{
+	atomic_inc(&codec->in_pm);
+}
+
+/* call this at leaving from suspend/resume callbacks in codec driver */
+static inline void snd_hdac_leave_pm(struct hdac_device *codec)
+{
+	atomic_dec(&codec->in_pm);
+}
+
+static inline bool snd_hdac_is_in_pm(struct hdac_device *codec)
+{
+	return atomic_read(&codec->in_pm);
+}
+
+static inline bool snd_hdac_is_power_on(struct hdac_device *codec)
+{
+	return !pm_runtime_suspended(&codec->dev);
+}
 #else
 static inline int snd_hdac_power_up(struct hdac_device *codec) { return 0; }
 static inline int snd_hdac_power_down(struct hdac_device *codec) { return 0; }
 static inline int snd_hdac_power_up_pm(struct hdac_device *codec) { return 0; }
 static inline int snd_hdac_power_down_pm(struct hdac_device *codec) { return 0; }
 static inline int snd_hdac_keep_power_up(struct hdac_device *codec) { return 0; }
+static inline void snd_hdac_enter_pm(struct hdac_device *codec) {}
+static inline void snd_hdac_leave_pm(struct hdac_device *codec) {}
+static inline bool snd_hdac_is_in_pm(struct hdac_device *codec) { return 0; }
+static inline bool snd_hdac_is_power_on(struct hdac_device *codec) { return 1; }
 #endif
 
 /*
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 44165f3e344e..27ce6ff6354e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -37,15 +37,8 @@
 #include "hda_jack.h"
 #include <sound/hda_hwdep.h>
 
-#ifdef CONFIG_PM
-#define codec_in_pm(codec)	atomic_read(&(codec)->core.in_pm)
-#define hda_codec_is_power_on(codec) \
-	(!pm_runtime_suspended(hda_codec_dev(codec)))
-#else
-#define codec_in_pm(codec)	0
-#define hda_codec_is_power_on(codec)	1
-#endif
-
+#define codec_in_pm(codec)		snd_hdac_is_in_pm(&codec->core)
+#define hda_codec_is_power_on(codec)	snd_hdac_is_power_on(&codec->core)
 #define codec_has_epss(codec) \
 	((codec)->core.power_caps & AC_PWRST_EPSS)
 #define codec_has_clkstop(codec) \
@@ -2848,14 +2841,13 @@ static unsigned int hda_call_codec_suspend(struct hda_codec *codec)
 {
 	unsigned int state;
 
-	atomic_inc(&codec->core.in_pm);
-
+	snd_hdac_enter_pm(&codec->core);
 	if (codec->patch_ops.suspend)
 		codec->patch_ops.suspend(codec);
 	hda_cleanup_all_streams(codec);
 	state = hda_set_power_state(codec, AC_PWRST_D3);
 	update_power_acct(codec, true);
-	atomic_dec(&codec->core.in_pm);
+	snd_hdac_leave_pm(&codec->core);
 	return state;
 }
 
@@ -2864,8 +2856,7 @@ static unsigned int hda_call_codec_suspend(struct hda_codec *codec)
  */
 static void hda_call_codec_resume(struct hda_codec *codec)
 {
-	atomic_inc(&codec->core.in_pm);
-
+	snd_hdac_enter_pm(&codec->core);
 	if (codec->core.regmap)
 		regcache_mark_dirty(codec->core.regmap);
 
@@ -2888,7 +2879,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 		hda_jackpoll_work(&codec->jackpoll_work.work);
 	else
 		snd_hda_jack_report_sync(codec);
-	atomic_dec(&codec->core.in_pm);
+	snd_hdac_leave_pm(&codec->core);
 }
 
 static int hda_codec_runtime_suspend(struct device *dev)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index e7d99a802bcc..c182e619ad58 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2489,7 +2489,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
 	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
 		return;
 	/* ditto during suspend/resume process itself */
-	if (atomic_read(&(codec)->core.in_pm))
+	if (snd_hdac_is_in_pm(&codec->core))
 		return;
 
 	snd_hdac_i915_set_bclk(&codec->bus->core);
-- 
2.17.1

[PATCH 3/3] ALSA: hda/hdmi - Don't fall back to generic when i915 binding fails

[Takashi Iwai]

When i915 component binding fails, it means that HDMI isn't applicable
anyway.  Although the probe with the generic HDMI parser would still
work, it's essentially useless, hence better to be left unbound.

This patch mimics the probe_id field at failing the i915 component
binding so that the generic HDMI won't be bound after that.

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

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index c182e619ad58..1fed59f8ed32 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2542,6 +2542,8 @@ static int alloc_intel_hdmi(struct hda_codec *codec)
 	/* requires i915 binding */
 	if (!codec->bus->core.audio_component) {
 		codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n");
+		/* set probe_id here to prevent generic fallback binding */
+		codec->probe_id = HDA_CODEC_ID_GENERIC_HDMI;
 		return -ENODEV;
 	}
 
-- 
2.17.1

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Chris Wilson]

Quoting Takashi Iwai (2018-06-27 10:10:32)
> Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> haven't dealt with the error properly in many places.  It's an unusual
> situation but still possible.
> 
> This patch spots these places and adds the proper error paths.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> @@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
>                 int dir = get_amp_direction(kcontrol);
>                 unsigned long pval;
>  
> -               snd_hda_power_up(codec);
> +               err = snd_hda_power_up(codec);
> +               if (err < 0)
> +                       return err;
>                 mutex_lock(&codec->control_mutex);
>                 pval = kcontrol->private_value;
>                 kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,

Should this be deferred until pm is next acquired?
Or are we regarding pm can only fail nonrecoverably?

> @@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
>         long *valp = ucontrol->value.integer.value;
>         hda_nid_t vnid = 0;
>         int changed = 1;
> +       int err;
>  
>         switch (nid) {
>         case 0x02:
> @@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
>                 valp++;
>         }
>  
> -       snd_hda_power_up(codec);
> +       err = snd_hda_power_up(codec);
> +       if (err < 0)
> +               return err;
>         ca0132_alt_dsp_volume_put(codec, vnid);
>         mutex_lock(&codec->control_mutex);
>         changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);

> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 5ad6c7e5f92e..b0d757cf7aab 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled)
>         pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80;
>         if (spec->mute_led_nid) {
>                 /* temporarily power up/down for setting VREF */
> -               snd_hda_power_up_pm(codec);
> +               if (snd_hda_power_up_pm(codec) < 0)
> +                       return;
>                 snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval);
>                 snd_hda_power_down_pm(codec);
>         }

Similar questions as for deferred application.

I did not find any imbalance introduced and the error is propagated or
handled,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH 2/3] ALSA: hda - Move in_pm accessors to HDA core

[Chris Wilson]

Quoting Takashi Iwai (2018-06-27 10:10:33)
> The in_pm atomic in hdac_device is an important field used as a flag
> as well as a refcount for PM.  The existing snd_hdac_power_up/down
> helpers already refer to it in the HD-audio core code, while the code
> to actually setting the value (atomic_inc() / _dec()) is open-coded in
> HDA legacy side, which is hard to find.
> 
> This patch adds the helper functions to set/reset the in_pm counter to
> HDA core and use them in HDA legacy side, for making it clearer who /
> where the PM is managed.
> 
> There is no functional changes, just code refactoring.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Mechanical code change that helps explain what it is doing,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai]

On Wed, 27 Jun 2018 11:36:12 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2018-06-27 10:10:32)
> > Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> > haven't dealt with the error properly in many places.  It's an unusual
> > situation but still possible.
> > 
> > This patch spots these places and adds the proper error paths.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > @@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
> >                 int dir = get_amp_direction(kcontrol);
> >                 unsigned long pval;
> >  
> > -               snd_hda_power_up(codec);
> > +               err = snd_hda_power_up(codec);
> > +               if (err < 0)
> > +                       return err;
> >                 mutex_lock(&codec->control_mutex);
> >                 pval = kcontrol->private_value;
> >                 kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,
> 
> Should this be deferred until pm is next acquired?
> Or are we regarding pm can only fail nonrecoverably?

The power up path is already pm_runtime_get_sync(), so it's a fatal
error.  So I suppose that the failure must be very exceptional, and no
need to complicate things too much.

> 
> > @@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
> >         long *valp = ucontrol->value.integer.value;
> >         hda_nid_t vnid = 0;
> >         int changed = 1;
> > +       int err;
> >  
> >         switch (nid) {
> >         case 0x02:
> > @@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
> >                 valp++;
> >         }
> >  
> > -       snd_hda_power_up(codec);
> > +       err = snd_hda_power_up(codec);
> > +       if (err < 0)
> > +               return err;
> >         ca0132_alt_dsp_volume_put(codec, vnid);
> >         mutex_lock(&codec->control_mutex);
> >         changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);
> 
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 5ad6c7e5f92e..b0d757cf7aab 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled)
> >         pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80;
> >         if (spec->mute_led_nid) {
> >                 /* temporarily power up/down for setting VREF */
> > -               snd_hda_power_up_pm(codec);
> > +               if (snd_hda_power_up_pm(codec) < 0)
> > +                       return;
> >                 snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval);
> >                 snd_hda_power_down_pm(codec);
> >         }
> 
> Similar questions as for deferred application.
> 
> I did not find any imbalance introduced and the error is propagated or
> handled,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

Takashi

Re: [PATCH 0/3] ALSA: PM-related fixes/cleanups

[Chris Wilson]

Quoting Takashi Iwai (2018-06-27 10:10:31)
> Hi,
> 
> this is a patch set addressing some minor issues found in PM-related
> stuff, as well as the Intel HDMI fallback issue.  The first patch
> covers the unusual error paths from runtime PM, the second one is a
> code refactoring and the last one fixes the superfluous (rather
> harmful) binding with the generic HDMI driver as fallback of i915
> component binding.

Hmm, our CI casts a shade of doubt over these.

https://intel-gfx-ci.01.org/tree/drm-tip/combined-issues-by-run.html

>From CI_DRM_4385 onwards, a small explosion

79527147382a drm-tip: 2018y-06m-27d-13h-05m-01s UTC integration manifest
67ca07e7ac10 drm/i915/icl: Add power well support
828e10e61e0a drm-tip: 2018y-06m-27d-12h-10m-06s UTC integration manifest
43c489277bdd ALSA: hda/hdmi - Don't fall back to generic when i915
binding fails
bd0afa8b3573 ALSA: hda - Move in_pm accessors to HDA core
644a790aed11 ALSA: hda - Handle error from snd_hda_power_up*()
401caff70cd3 ALSA: hda - Kill snd_hda_codec_update_cache()

I haven't ruled out the icl patch, but I presume that was run past CI
for pre-merge checking.

Just a heads up,
-Chris

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Chris Wilson]

Quoting Takashi Iwai (2018-06-27 10:10:32)
> Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> haven't dealt with the error properly in many places.  It's an unusual
> situation but still possible.
> 
> This patch spots these places and adds the proper error paths.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Verdict from CI,

https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html

is that this one causes a bunch of pm fallout.

Do you mind doing a quick revert? Or working with our CI to find the bad
chunk?

On a second look,

> @@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec)
>         struct ca0132_spec *spec = codec->spec;
>  
>         cancel_delayed_work_sync(&spec->unsol_hp_work);
> -       snd_hda_power_up(codec);
> +       if (snd_hda_power_up(codec) < 0)
> +               goto skip_shutdown;
>         switch (spec->quirk) {
>         case QUIRK_SBZ:
>                 sbz_exit_chip(codec);
> @@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec)
>                 break;
>         }
>         snd_hda_power_down(codec);
> + skip_shutdown:
>         if (spec->mem_base)
>                 iounmap(spec->mem_base);
>         kfree(spec->spec_init_verbs);

would seem to be the only chunk more complicated than the rest.
-Chris

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai]

On Wed, 27 Jun 2018 17:54:31 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2018-06-27 10:10:32)
> > Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> > haven't dealt with the error properly in many places.  It's an unusual
> > situation but still possible.
> > 
> > This patch spots these places and adds the proper error paths.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Verdict from CI,
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html
> 
> is that this one causes a bunch of pm fallout.
> 
> Do you mind doing a quick revert? Or working with our CI to find the bad
> chunk?

Hrm, it doesn't look good -- I revert the branch merge now.
Thanks for the quick heads up.


Takashi


> 
> On a second look,
> 
> > @@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec)
> >         struct ca0132_spec *spec = codec->spec;
> >  
> >         cancel_delayed_work_sync(&spec->unsol_hp_work);
> > -       snd_hda_power_up(codec);
> > +       if (snd_hda_power_up(codec) < 0)
> > +               goto skip_shutdown;
> >         switch (spec->quirk) {
> >         case QUIRK_SBZ:
> >                 sbz_exit_chip(codec);
> > @@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec)
> >                 break;
> >         }
> >         snd_hda_power_down(codec);
> > + skip_shutdown:
> >         if (spec->mem_base)
> >                 iounmap(spec->mem_base);
> >         kfree(spec->spec_init_verbs);
> 
> would seem to be the only chunk more complicated than the rest.
> -Chris
> 

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Ville Syrjälä]

On Wed, Jun 27, 2018 at 11:10:32AM +0200, Takashi Iwai wrote:
> Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> haven't dealt with the error properly in many places.  It's an unusual
> situation but still possible.
> 
> This patch spots these places and adds the proper error paths.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_beep.c       |  3 +-
>  sound/pci/hda/hda_codec.c      |  4 ++-
>  sound/pci/hda/hda_controller.c |  4 ++-
>  sound/pci/hda/hda_proc.c       |  3 +-
>  sound/pci/hda/hda_sysfs.c      |  4 ++-
>  sound/pci/hda/patch_ca0132.c   | 53 +++++++++++++++++++++++++++-------
>  sound/pci/hda/patch_realtek.c  |  3 +-
>  7 files changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c
> index 066b5b59c4d7..e9d5fbd6c13a 100644
> --- a/sound/pci/hda/hda_beep.c
> +++ b/sound/pci/hda/hda_beep.c
> @@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone)
>  	struct hda_codec *codec = beep->codec;
>  
>  	if (tone && !beep->playing) {
> -		snd_hda_power_up(codec);
> +		if (snd_hda_power_up(codec) < 0)
> +			return;
>  		if (beep->power_hook)
>  			beep->power_hook(beep, true);
>  		beep->playing = 1;
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 20a171ac4bb2..44165f3e344e 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd,
>  		return -1;
>  
>   again:
> -	snd_hda_power_up_pm(codec);
> +	err = snd_hda_power_up_pm(codec);
> +	if (err < 0)
> +		return err;
>  	mutex_lock(&bus->core.cmd_mutex);
>  	if (flags & HDA_RW_NO_RESPONSE_FALLBACK)
>  		bus->no_response_fallback = 1;
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index a12e594d4e3b..4273be1f3eaa 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
>  				   buff_step);
>  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>  				   buff_step);
> -	snd_hda_power_up(apcm->codec);
> +	err = snd_hda_power_up(apcm->codec);
> +	if (err < 0)
> +		return err;

Missing azx_release_device() here?

>  	if (hinfo->ops.open)
>  		err = hinfo->ops.open(hinfo, apcm->codec, substream);
>  	else
-- 
Ville Syrjälä
Intel

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai]

On Wed, 27 Jun 2018 18:09:35 +0200,
Takashi Iwai wrote:
> 
> On Wed, 27 Jun 2018 17:54:31 +0200,
> Chris Wilson wrote:
> > 
> > Quoting Takashi Iwai (2018-06-27 10:10:32)
> > > Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> > > haven't dealt with the error properly in many places.  It's an unusual
> > > situation but still possible.
> > > 
> > > This patch spots these places and adds the proper error paths.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > Verdict from CI,
> > 
> > https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html
> > 
> > is that this one causes a bunch of pm fallout.
> > 
> > Do you mind doing a quick revert? Or working with our CI to find the bad
> > chunk?
> 
> Hrm, it doesn't look good -- I revert the branch merge now.
> Thanks for the quick heads up.

After a deeper look, I found that it's an error -EACCES from
pm_runtime_get_sync().  Actually it's no real error but indicates that
the runtime PM is disabled.  That's the reason it broke things
easily...

That is, dealing a negative code always as a fatal error is simply
wrong.  We may filter out -EACCES, but I think we need more careful
checks.  So the patch isn't worth, so far.


thanks,

Takashi

Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai]

On Wed, 27 Jun 2018 18:12:06 +0200,
Ville Syrjälä wrote:
> 
> On Wed, Jun 27, 2018 at 11:10:32AM +0200, Takashi Iwai wrote:
> > Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> > haven't dealt with the error properly in many places.  It's an unusual
> > situation but still possible.
> > 
> > This patch spots these places and adds the proper error paths.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/pci/hda/hda_beep.c       |  3 +-
> >  sound/pci/hda/hda_codec.c      |  4 ++-
> >  sound/pci/hda/hda_controller.c |  4 ++-
> >  sound/pci/hda/hda_proc.c       |  3 +-
> >  sound/pci/hda/hda_sysfs.c      |  4 ++-
> >  sound/pci/hda/patch_ca0132.c   | 53 +++++++++++++++++++++++++++-------
> >  sound/pci/hda/patch_realtek.c  |  3 +-
> >  7 files changed, 57 insertions(+), 17 deletions(-)
> > 
> > diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c
> > index 066b5b59c4d7..e9d5fbd6c13a 100644
> > --- a/sound/pci/hda/hda_beep.c
> > +++ b/sound/pci/hda/hda_beep.c
> > @@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone)
> >  	struct hda_codec *codec = beep->codec;
> >  
> >  	if (tone && !beep->playing) {
> > -		snd_hda_power_up(codec);
> > +		if (snd_hda_power_up(codec) < 0)
> > +			return;
> >  		if (beep->power_hook)
> >  			beep->power_hook(beep, true);
> >  		beep->playing = 1;
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 20a171ac4bb2..44165f3e344e 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd,
> >  		return -1;
> >  
> >   again:
> > -	snd_hda_power_up_pm(codec);
> > +	err = snd_hda_power_up_pm(codec);
> > +	if (err < 0)
> > +		return err;
> >  	mutex_lock(&bus->core.cmd_mutex);
> >  	if (flags & HDA_RW_NO_RESPONSE_FALLBACK)
> >  		bus->no_response_fallback = 1;
> > diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> > index a12e594d4e3b..4273be1f3eaa 100644
> > --- a/sound/pci/hda/hda_controller.c
> > +++ b/sound/pci/hda/hda_controller.c
> > @@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
> >  				   buff_step);
> >  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> >  				   buff_step);
> > -	snd_hda_power_up(apcm->codec);
> > +	err = snd_hda_power_up(apcm->codec);
> > +	if (err < 0)
> > +		return err;
> 
> Missing azx_release_device() here?

Yes, and even worse, it skips the mutex unlock :-<
Let's scratch this patch.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/3] ALSA: hda - Move in_pm accessors to HDA core

[Takashi Iwai]

On Wed, 27 Jun 2018 11:38:40 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2018-06-27 10:10:33)
> > The in_pm atomic in hdac_device is an important field used as a flag
> > as well as a refcount for PM.  The existing snd_hdac_power_up/down
> > helpers already refer to it in the HD-audio core code, while the code
> > to actually setting the value (atomic_inc() / _dec()) is open-coded in
> > HDA legacy side, which is hard to find.
> > 
> > This patch adds the helper functions to set/reset the in_pm counter to
> > HDA core and use them in HDA legacy side, for making it clearer who /
> > where the PM is managed.
> > 
> > There is no functional changes, just code refactoring.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Mechanical code change that helps explain what it is doing,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This patch still seems OK, so merged to for-next branch alone.


Takashi

Re: [alsa-devel] [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Dan Carpenter]

Hi Takashi,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-hda-Handle-error-from-snd_hda_power_up/20180627-171524
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next

smatch warnings:
sound/pci/hda/hda_controller.c:688 azx_pcm_open() warn: inconsistent returns 'mutex:&chip->open_mutex'.
  Locked on:   line 650
  Unlocked on: line 681

# https://github.com/0day-ci/linux/commit/2e036d491f4b7a4a100b2e1cd7056fac63868245
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2e036d491f4b7a4a100b2e1cd7056fac63868245
vim +688 sound/pci/hda/hda_controller.c

05e84878 Dylan Reid           2014-02-28  591  
05e84878 Dylan Reid           2014-02-28  592  static int azx_pcm_open(struct snd_pcm_substream *substream)
05e84878 Dylan Reid           2014-02-28  593  {
05e84878 Dylan Reid           2014-02-28  594  	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
820cc6cf Takashi Iwai         2015-02-20  595  	struct hda_pcm_stream *hinfo = to_hda_pcm_stream(substream);
05e84878 Dylan Reid           2014-02-28  596  	struct azx *chip = apcm->chip;
05e84878 Dylan Reid           2014-02-28  597  	struct azx_dev *azx_dev;
05e84878 Dylan Reid           2014-02-28  598  	struct snd_pcm_runtime *runtime = substream->runtime;
05e84878 Dylan Reid           2014-02-28  599  	int err;
05e84878 Dylan Reid           2014-02-28  600  	int buff_step;
05e84878 Dylan Reid           2014-02-28  601  
9a6246ff Takashi Iwai         2015-02-27  602  	snd_hda_codec_pcm_get(apcm->info);
05e84878 Dylan Reid           2014-02-28  603  	mutex_lock(&chip->open_mutex);
05e84878 Dylan Reid           2014-02-28  604  	azx_dev = azx_assign_device(chip, substream);
18486508 Libin Yang           2015-05-12  605  	trace_azx_pcm_open(chip, azx_dev);
05e84878 Dylan Reid           2014-02-28  606  	if (azx_dev == NULL) {
61ca4107 Takashi Iwai         2015-02-27  607  		err = -EBUSY;
61ca4107 Takashi Iwai         2015-02-27  608  		goto unlock;
05e84878 Dylan Reid           2014-02-28  609  	}
ccc98865 Takashi Iwai         2015-04-14  610  	runtime->private_data = azx_dev;
50279d9b Guneshwor Singh      2016-08-04  611  
50279d9b Guneshwor Singh      2016-08-04  612  	if (chip->gts_present)
50279d9b Guneshwor Singh      2016-08-04  613  		azx_pcm_hw.info = azx_pcm_hw.info |
50279d9b Guneshwor Singh      2016-08-04  614  			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
50279d9b Guneshwor Singh      2016-08-04  615  
05e84878 Dylan Reid           2014-02-28  616  	runtime->hw = azx_pcm_hw;
05e84878 Dylan Reid           2014-02-28  617  	runtime->hw.channels_min = hinfo->channels_min;
05e84878 Dylan Reid           2014-02-28  618  	runtime->hw.channels_max = hinfo->channels_max;
05e84878 Dylan Reid           2014-02-28  619  	runtime->hw.formats = hinfo->formats;
05e84878 Dylan Reid           2014-02-28  620  	runtime->hw.rates = hinfo->rates;
05e84878 Dylan Reid           2014-02-28  621  	snd_pcm_limit_hw_rates(runtime);
05e84878 Dylan Reid           2014-02-28  622  	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
05e84878 Dylan Reid           2014-02-28  623  
05e84878 Dylan Reid           2014-02-28  624  	/* avoid wrap-around with wall-clock */
05e84878 Dylan Reid           2014-02-28  625  	snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_TIME,
05e84878 Dylan Reid           2014-02-28  626  				     20,
05e84878 Dylan Reid           2014-02-28  627  				     178000000);
05e84878 Dylan Reid           2014-02-28  628  
05e84878 Dylan Reid           2014-02-28  629  	if (chip->align_buffer_size)
05e84878 Dylan Reid           2014-02-28  630  		/* constrain buffer sizes to be multiple of 128
05e84878 Dylan Reid           2014-02-28  631  		   bytes. This is more efficient in terms of memory
05e84878 Dylan Reid           2014-02-28  632  		   access but isn't required by the HDA spec and
05e84878 Dylan Reid           2014-02-28  633  		   prevents users from specifying exact period/buffer
05e84878 Dylan Reid           2014-02-28  634  		   sizes. For example for 44.1kHz, a period size set
05e84878 Dylan Reid           2014-02-28  635  		   to 20ms will be rounded to 19.59ms. */
05e84878 Dylan Reid           2014-02-28  636  		buff_step = 128;
05e84878 Dylan Reid           2014-02-28  637  	else
05e84878 Dylan Reid           2014-02-28  638  		/* Don't enforce steps on buffer sizes, still need to
05e84878 Dylan Reid           2014-02-28  639  		   be multiple of 4 bytes (HDA spec). Tested on Intel
05e84878 Dylan Reid           2014-02-28  640  		   HDA controllers, may not work on all devices where
05e84878 Dylan Reid           2014-02-28  641  		   option needs to be disabled */
05e84878 Dylan Reid           2014-02-28  642  		buff_step = 4;
05e84878 Dylan Reid           2014-02-28  643  
05e84878 Dylan Reid           2014-02-28  644  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
05e84878 Dylan Reid           2014-02-28  645  				   buff_step);
05e84878 Dylan Reid           2014-02-28  646  	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
05e84878 Dylan Reid           2014-02-28  647  				   buff_step);
2e036d49 Takashi Iwai         2018-06-27  648  	err = snd_hda_power_up(apcm->codec);
2e036d49 Takashi Iwai         2018-06-27  649  	if (err < 0)
2e036d49 Takashi Iwai         2018-06-27  650  		return err;
                                                        ^^^^^^^^^^
Needs to be a "goto unlock;"

61ca4107 Takashi Iwai         2015-02-27  651  	if (hinfo->ops.open)
05e84878 Dylan Reid           2014-02-28  652  		err = hinfo->ops.open(hinfo, apcm->codec, substream);
61ca4107 Takashi Iwai         2015-02-27  653  	else
61ca4107 Takashi Iwai         2015-02-27  654  		err = -ENODEV;
05e84878 Dylan Reid           2014-02-28  655  	if (err < 0) {
05e84878 Dylan Reid           2014-02-28  656  		azx_release_device(azx_dev);
61ca4107 Takashi Iwai         2015-02-27  657  		goto powerdown;
05e84878 Dylan Reid           2014-02-28  658  	}
05e84878 Dylan Reid           2014-02-28  659  	snd_pcm_limit_hw_rates(runtime);
05e84878 Dylan Reid           2014-02-28  660  	/* sanity check */
05e84878 Dylan Reid           2014-02-28  661  	if (snd_BUG_ON(!runtime->hw.channels_min) ||
05e84878 Dylan Reid           2014-02-28  662  	    snd_BUG_ON(!runtime->hw.channels_max) ||
05e84878 Dylan Reid           2014-02-28  663  	    snd_BUG_ON(!runtime->hw.formats) ||
05e84878 Dylan Reid           2014-02-28  664  	    snd_BUG_ON(!runtime->hw.rates)) {
05e84878 Dylan Reid           2014-02-28  665  		azx_release_device(azx_dev);
61ca4107 Takashi Iwai         2015-02-27  666  		if (hinfo->ops.close)
05e84878 Dylan Reid           2014-02-28  667  			hinfo->ops.close(hinfo, apcm->codec, substream);
61ca4107 Takashi Iwai         2015-02-27  668  		err = -EINVAL;
61ca4107 Takashi Iwai         2015-02-27  669  		goto powerdown;
05e84878 Dylan Reid           2014-02-28  670  	}
05e84878 Dylan Reid           2014-02-28  671  
9e94df3a Pierre-Louis Bossart 2015-02-13  672  	/* disable LINK_ATIME timestamps for capture streams
05e84878 Dylan Reid           2014-02-28  673  	   until we figure out how to handle digital inputs */
9e94df3a Pierre-Louis Bossart 2015-02-13  674  	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
9e94df3a Pierre-Louis Bossart 2015-02-13  675  		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */
9e94df3a Pierre-Louis Bossart 2015-02-13  676  		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
9e94df3a Pierre-Louis Bossart 2015-02-13  677  	}
05e84878 Dylan Reid           2014-02-28  678  
05e84878 Dylan Reid           2014-02-28  679  	snd_pcm_set_sync(substream);
05e84878 Dylan Reid           2014-02-28  680  	mutex_unlock(&chip->open_mutex);
05e84878 Dylan Reid           2014-02-28  681  	return 0;
61ca4107 Takashi Iwai         2015-02-27  682  
61ca4107 Takashi Iwai         2015-02-27  683   powerdown:
61ca4107 Takashi Iwai         2015-02-27  684  	snd_hda_power_down(apcm->codec);
61ca4107 Takashi Iwai         2015-02-27  685   unlock:
61ca4107 Takashi Iwai         2015-02-27  686  	mutex_unlock(&chip->open_mutex);
9a6246ff Takashi Iwai         2015-02-27  687  	snd_hda_codec_pcm_put(apcm->info);
61ca4107 Takashi Iwai         2015-02-27 @688  	return err;
05e84878 Dylan Reid           2014-02-28  689  }
05e84878 Dylan Reid           2014-02-28  690