[alsa-devel] New snd-hda warning spew

[Takashi Iwai]

On Thu, 17 Mar 2016 14:38:42 +0100,
Takashi Iwai wrote:
> 
> On Wed, 16 Mar 2016 15:04:20 +0100,
> Ville Syrjälä wrote:
> > 
> > But now I got a lockdep spew when I enabled the HDMI video output [1]
> > 
> > And sure enough mplayer got stuck in the kernel when I tried to use
> > the HDMI audio device [2]
> > 
> > [1]
> > [ 1939.476458] =============================================
> > [ 1939.476460] [ INFO: possible recursive locking detected ]
> > [ 1939.476463] 4.5.0-vga+ #13 Not tainted
> > [ 1939.476464] ---------------------------------------------
> > [ 1939.476466] kworker/2:2/1016 is trying to acquire lock:
> > [ 1939.476469]  (&spec->pcm_lock){+.+...}, at: [<ffffffffa020b868>] hdmi_present_sense+0x38/0x300 [snd_hda_codec_hdmi]
> > [ 1939.476480] 
> >                but task is already holding lock:
> > [ 1939.476482]  (&spec->pcm_lock){+.+...}, at: [<ffffffffa020b868>] hdmi_present_sense+0x38/0x300 [snd_hda_codec_hdmi]
> > [ 1939.476489] 
> >                other info that might help us debug this:
> > [ 1939.476491]  Possible unsafe locking scenario:
> > 
> > [ 1939.476493]        CPU0
> > [ 1939.476495]        ----
> > [ 1939.476496]   lock(&spec->pcm_lock);
> > [ 1939.476499]   lock(&spec->pcm_lock);
> > [ 1939.476502] 
> >                 *** DEADLOCK ***
> > 
> > [ 1939.476504]  May be due to missing lock nesting notation
> 
> Unfortunately, no this is a real deadlock.
> Let's see below: hdmi_present_sense() gets called twice because the
> function issues a verb that does self-resume and it invokes
> hdmi_present_sense() again in runtime resume.
> 
> > [ 1939.476622]  [<ffffffffa020b868>] hdmi_present_sense+0x38/0x300 [snd_hda_codec_hdmi]
> ....
> > [ 1939.476642]  [<ffffffffa020bd6d>] generic_hdmi_resume+0x4d/0x60 [snd_hda_codec_hdmi]
> ....
> > [ 1939.476690]  [<ffffffffa017de62>] snd_hdac_power_up_pm+0x52/0x60 [snd_hda_core]
> > [ 1939.476694]  [<ffffffffa020b9c3>] hdmi_present_sense+0x193/0x300 [snd_hda_codec_hdmi]
> > [ 1939.476699]  [<ffffffffa020bba0>] check_presence_and_report+0x70/0x90 [snd_hda_codec_hdmi]
> > [ 1939.476703]  [<ffffffffa020bcba>] hdmi_unsol_event+0x9a/0xb0 [snd_hda_codec_hdmi]
> 
> This wasn't seen until now because the code path using i915 audio
> notifier doesn't need to power up the codec.  Now we switched to the
> old method for old chips, and the bug is revealed.  It's good to have
> caught it now, because basically this hits all non-Intel chips.

... and the fix patch is attached below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: hda - Fix mutex deadlock at HDMI/DP hotplug

The recent change in HD-audio HDMI/DP codec driver for allowing the
dynamic PCM binding introduced a new spec->pcm_mutex.  One of the
protected area by this mutex is hdmi_present_sense().  As reported by
Intel CI tests, unfortunately, the new mutex causes a deadlock when
the hotplug/unplug is triggered during the codec is in runtime
suspend.  The buggy code path is like the following:

  hdmi_unsol_event() -> ...
    -> hdmi_present_sense()
==>     ** here taking pcm_mutex
      -> hdmi_present_sense_via_verbs()
        -> snd_hda_power_up_pm() -> ... (runtime resume calls)
          -> generic_hdmi_resume()
            -> hdmi_present_sense()
==>           ** here taking pcm_mutex again!

As we can see here, the problem is that the mutex is taken before
snd_hda_power_up_pm() call that triggers the runtime resume.  That is,
the obvious solution is to move the power up/down call outside the
mutex; it is exactly what this patch provides.

The patch also clarifies why this bug wasn't caught beforehand.  We
used to have the i915 audio component for hotplug for all Intel chips,
and in that code path, there is no power up required but the
information is taken directly from the graphics side.  However, we
recently switched back to the old method for some old Intel chips due
to regressions, and now the deadlock issue is surfaced.

Fixes: a76056f2e57e ('ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug')
Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 sound/pci/hda/patch_hdmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index cde9746cda8e..49ee4e55dd16 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1405,7 +1405,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	bool ret;
 	bool do_repoll = false;
 
-	snd_hda_power_up_pm(codec);
 	present = snd_hda_pin_sense(codec, pin_nid);
 
 	mutex_lock(&per_pin->lock);
@@ -1444,7 +1443,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 		jack->block_report = !ret;
 
 	mutex_unlock(&per_pin->lock);
-	snd_hda_power_down_pm(codec);
 	return ret;
 }
 
@@ -1522,6 +1520,10 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	struct hdmi_spec *spec = codec->spec;
 	int ret;
 
+	/* no temporary power up/down needed for component notifier */
+	if (!codec_has_acomp(codec))
+		snd_hda_power_up_pm(codec);
+
 	mutex_lock(&spec->pcm_lock);
 	if (codec_has_acomp(codec)) {
 		sync_eld_via_acomp(codec, per_pin);
@@ -1531,6 +1533,9 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	}
 	mutex_unlock(&spec->pcm_lock);
 
+	if (!codec_has_acomp(codec))
+		snd_hda_power_down_pm(codec);
+
 	return ret;
 }
 
-- 
2.7.3