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

[Arthur Marsh]

Takashi Iwai wrote, on 21/01/14 21:43:
> 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;
>   }
>
>

The patch worked thanks.

I had to revert this patch to update the kernel to the current Linus' 
git head.

Regards,

Arthur.