On Thu, Mar 17, 2016 at 03:12:22PM +0100, Takashi Iwai wrote:
> 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@xxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
Yep, my ILK seems happy with this. No deadlocks, and HDMI audio works.
Tested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
