[alsa-devel] ALSA: hda - Fix possible races in HDMI driver - lockup on shutdown when radeon.audio=1 after using audacity

[Takashi Iwai]

At Tue, 21 Jan 2014 11:50:43 +1030,
Arthur Marsh wrote:
> 
> 
> 
> Takashi Iwai wrote, on 20/01/14 19:22:
> > At Sun, 19 Jan 2014 17:32:16 +1030,
> > Arthur Marsh wrote:
> >>
> >> I have had reproducible lock-ups on shut-down (at the shutting down ALSA
> >> stage) of my AMD64 machine (Asus M3A78Pro motherboard, BIOS 1701
> >> 01/27/2011, CPU AMD Athlon(tm) II X4 640 Processor) running the 64 bit
> >> Linux kernel more recent than 3.12 when *both* radeon.audio=1 was set
> >> and I had been running audacity 2.0.5. (iommu=noaperture is also set).
> >>
> >> The problem was reproducible with the stock Debian kernel
> >> linux-image-3.13-rc6-amd64 version 3.13~rc6-1~exp1.
> >>
> >> The machine is using an ATI/AMD 3850HD video card with a DVI cable to a
> >> DVI input on my monitor, and the default audio device is the
> >> motherboard's on-board audio device:
> >>
> >> 00:14.2 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] SBx00
> >> Azalia (Intel HDA)
> >>
> >> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.
> >> [AMD/ATI] RV670 [Radeon HD 3690/3850]
> >> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] RV670/680
> >> HDMI Audio [Radeon HD 3690/3800 Series]
> >>
> >> $ git bisect bad
> >> cbbaa603a03cc46681e24d6b2804b62fde95a2af is the first bad commit
> >> commit cbbaa603a03cc46681e24d6b2804b62fde95a2af
> >> Author: Takashi Iwai <tiwai at suse.de>
> >> Date:   Thu Oct 17 18:03:24 2013 +0200
> >>
> >>       ALSA: hda - Fix possible races in HDMI driver
> >>
> >>       Some per_pin fields and ELD contents might be changed dynamically in
> >>       multiple ways where the concurrent accesses are still opened in the
> >>       current code.  This patch fixes such possible races by using eld->lock
> >>       in appropriate places.
> >>
> >>       Reported-by: Anssi Hannula <anssi.hannula at iki.fi>
> >>       Signed-off-by: Takashi Iwai <tiwai at suse.de>
> >>
> >> :040000 040000 0c29281f82a3ebd9a704d481114f9cfcefea07c8
> >> d71fd101125cd29a628cb5e66c7ee4f56e28329b M      sound
> >>
> >> When running audacity from the command line there was the following output:
> >>
> >> ALSA lib pcm_dsnoop.c:618:(snd_pcm_dsnoop_open) unable to open slave
> >> ALSA lib pcm_dmix.c:1022:(snd_pcm_dmix_open) unable to open slave
> >> ALSA lib pcm.c:2239:(snd_pcm_open_noupdate) Unknown PCM cards.pcm.rear
> >> ALSA lib pcm.c:2239:(snd_pcm_open_noupdate) Unknown PCM cards.pcm.center_lfe
> >> ALSA lib pcm.c:2239:(snd_pcm_open_noupdate) Unknown PCM cards.pcm.side
> >> ALSA lib pcm_dmix.c:1022:(snd_pcm_dmix_open) unable to open slave
> >> Expression 'stream->playback.pcm' failed in
> >> 'src/hostapi/alsa/pa_linux_alsa.c', line: 4611
> >> Expression 'stream->playback.pcm' failed in
> >> 'src/hostapi/alsa/pa_linux_alsa.c', line: 4611
> >> Expression 'stream->playback.pcm' failed in
> >> 'src/hostapi/alsa/pa_linux_alsa.c', line: 4611
> >>
> >> I am happy to supply further information or run further tests to help in
> >> isolating the problem and verifying a solution.
> >
> > Could you build the kernel with lockdep kconfig and see whether it
> > reports errors?
> >
> > Reverting the commit doesn't work cleanly.  Instead, you can try to
> > simply comment out all mutex_lock(&per_pin->lock) and
> > mutex_unlock(&per_pin->lock) calls in patch_hdmi.c to see whether it's
> > a mutex deadlock.
> >
> >
> > thanks,
> >
> > Takashi
> >
> 
> I rebuilt the kernel after commenting out all mutex_lock(&per_pin->lock) 
> and mutex_unlock(&per_pin->lock) calls in patch_hdmi.c, and the 
> resulting kernel shutdown without hanging.

Thanks.  Could you try the patch below?  It's already queued for
3.14.  If this patch really fixes the issue, I'm going to send it to
stable 3.13.x.


Takashi

-- 8< --
From: David Henningsson <david.henningsson at canonical.com>
Subject: [PATCH] ALSA: hda - Explicitly keep codec powered up in
 hdmi_present_sense

This should help us avoid the following mutex deadlock:

[] mutex_lock+0x2a/0x50
[] hdmi_present_sense+0x53/0x3a0 [snd_hda_codec_hdmi]
[] generic_hdmi_resume+0x5a/0x70 [snd_hda_codec_hdmi]
[] hda_call_codec_resume+0xec/0x1d0 [snd_hda_codec]
[] snd_hda_power_save+0x1e4/0x280 [snd_hda_codec]
[] codec_exec_verb+0x5f/0x290 [snd_hda_codec]
[] snd_hda_codec_read+0x5b/0x90 [snd_hda_codec]
[] snd_hdmi_get_eld_size+0x1e/0x20 [snd_hda_codec_hdmi]
[] snd_hdmi_get_eld+0x2c/0xd0 [snd_hda_codec_hdmi]
[] hdmi_present_sense+0x9a/0x3a0 [snd_hda_codec_hdmi]
[] hdmi_repoll_eld+0x34/0x50 [snd_hda_codec_hdmi]

Signed-off-by: David Henningsson <david.henningsson at canonical.com>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 sound/pci/hda/patch_hdmi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index f5060fc7c303..977db17db26c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1496,11 +1496,14 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	 * specification worked this way. Hence, we just ignore the data in
 	 * the unsolicited response to avoid custom WARs.
 	 */
-	int present = snd_hda_pin_sense(codec, pin_nid);
+	int present;
 	bool update_eld = false;
 	bool eld_changed = false;
 	bool ret;
 
+	snd_hda_power_up(codec);
+	present = snd_hda_pin_sense(codec, pin_nid);
+
 	mutex_lock(&per_pin->lock);
 	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
 	if (pin_eld->monitor_present)
@@ -1573,6 +1576,7 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 		jack->block_report = !ret;
 
 	mutex_unlock(&per_pin->lock);
+	snd_hda_power_down(codec);
 	return ret;
 }
 
-- 
1.8.5.2