From 3c4189cb25e934e749cab2cece5f22443c309dce Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 17 Jun 2020 16:58:27 +0300 Subject: [PATCH 1/3] ASoC: hdac_hda: fix deadlock after PCM open error Commit 202bac220c8b ("ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence"), introduced a slight change of semantics to DAI startup/shutdown. If startup() returns an error, shutdown() is now called for the DAI. This causes a deadlock in hdac_hda which issues a call to snd_hda_codec_pcm_put() in case open fails. Upon error, soc_pcm_open() will call shutdown(), and pcm_put() ends up getting called twice. Result is a deadlock on pcm->open_mutex, as snd_device_free() gets called from within snd_pcm_open(). Typical task backtrace looks like this: [ 334.244627] snd_pcm_dev_disconnect+0x49/0x340 [snd_pcm] [ 334.244634] __snd_device_disconnect.part.0+0x2c/0x50 [snd] [ 334.244640] __snd_device_free+0x7f/0xc0 [snd] [ 334.244650] snd_hda_codec_pcm_put+0x87/0x120 [snd_hda_codec] [ 334.244660] soc_pcm_open+0x6a0/0xbe0 [snd_soc_core] [ 334.244676] ? dpcm_add_paths.isra.0+0x491/0x590 [snd_soc_core] [ 334.244679] ? kfree+0x9a/0x230 [ 334.244686] dpcm_be_dai_startup+0x255/0x300 [snd_soc_core] [ 334.244695] dpcm_fe_dai_open+0x20e/0xf30 [snd_soc_core] [ 334.244701] ? snd_pcm_hw_rule_muldivk+0x110/0x110 [snd_pcm] [ 334.244709] ? dpcm_be_dai_startup+0x300/0x300 [snd_soc_core] [ 334.244714] ? snd_pcm_attach_substream+0x3c4/0x540 [snd_pcm] [ 334.244719] snd_pcm_open_substream+0x69a/0xb60 [snd_pcm] [ 334.244729] ? snd_pcm_release_substream+0x30/0x30 [snd_pcm] [ 334.244732] ? __mutex_lock_slowpath+0x10/0x10 [ 334.244736] snd_pcm_open+0x1b3/0x3c0 [snd_pcm] Fixes: 202bac220c8b ("ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence") BugLink: https://github.com/thesofproject/linux/issues/2159 Signed-off-by: Kai Vehmanen --- sound/soc/codecs/hdac_hda.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 473efe9ef998a7..b0370bb10c1422 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -289,7 +289,6 @@ static int hdac_hda_dai_open(struct snd_pcm_substream *substream, struct hdac_hda_priv *hda_pvt; struct hda_pcm_stream *hda_stream; struct hda_pcm *pcm; - int ret; hda_pvt = snd_soc_component_get_drvdata(component); pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai); @@ -300,11 +299,7 @@ static int hdac_hda_dai_open(struct snd_pcm_substream *substream, hda_stream = &pcm->stream[substream->stream]; - ret = hda_stream->ops.open(hda_stream, &hda_pvt->codec, substream); - if (ret < 0) - snd_hda_codec_pcm_put(pcm); - - return ret; + return hda_stream->ops.open(hda_stream, &hda_pvt->codec, substream); } static void hdac_hda_dai_close(struct snd_pcm_substream *substream, From 8a59b42343fdd870c26fbda823aad3273eeaa3de Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Mon, 22 Jun 2020 17:55:51 +0300 Subject: [PATCH 2/3] ALSA: hda/hdmi: fix failure at PCM open As legacy of the HDMI codec driver, PCM open does not report errors if device is opened when no receiver is connected. This was implemented to keep Pulseaudio happy when it does the initial scan of PCM devices. On systems using UCM, this is no longer needed as Pulseaudio can get list of PCM devices from UCM. As this does not cover all systems, the legacy mode needs to be supported as well. To implement the behaviour, codec driver uses two code paths for PCM open -- one when receiver is connected and other for "no pin" case. Problem occurs in following sequence: 1) pcm0 connected, pcm1 not connected - pcm0 is attached to cvt0 2) application opens pcm1 - no monitor connected to pcm1, so use the first free CVT cvt0 for pcm1 3) application opens pcm0 - cvt0 not available - cvt1...N not attached to pcm0, hdmi_choose_cvt() will fail - open fails to -EBUSY Fix the problem by prioritizing the converter with the same index as PCM (i.e. mimic logic in hdmi_find_pcm_slot()). E.g. in above example, in step 2, driver would assign cvt1 for pcm1. Signed-off-by: Kai Vehmanen --- sound/pci/hda/patch_hdmi.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e2b21ef5d7d129..8b7b8d6f6b440b 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -952,6 +952,26 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid, return 0; } +static int hdmi_choose_cvt_nopin(struct hda_codec *codec, + int pcm_idx, int *cvt_id) +{ + struct hdmi_spec *spec = codec->spec; + struct hdmi_spec_per_cvt *per_cvt; + + if (pcm_idx >= spec->num_cvts) + return -EINVAL; + + per_cvt = get_cvt(spec, pcm_idx); + + if (per_cvt->assigned) + return -EBUSY; + + if (cvt_id) + *cvt_id = pcm_idx; + + return 0; +} + /* Try to find an available converter * If pin_idx is less then zero, just try to find an available converter. * Otherwise, try to find an available converter and get the cvt mux index @@ -1162,7 +1182,9 @@ static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo, if (pcm_idx < 0) return -EINVAL; - err = hdmi_choose_cvt(codec, -1, &cvt_idx); + err = hdmi_choose_cvt_nopin(codec, pcm_idx, &cvt_idx); + if (err) + err = hdmi_choose_cvt(codec, -1, &cvt_idx); if (err) return err; From 50d626629bb78e883979b2fd12018e74fd24dba3 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Mon, 22 Jun 2020 17:55:27 +0300 Subject: [PATCH 3/3] ALSA: hda/hdmi: improve debug traces The HDMI codec driver has two debug traces printed from different functions but with identical message content: "HDMI: hinfo 000000006a6b84d9 not registered" Fix this this duplication and also add a bit more context in addition to raw object pointer, to help analysis of kernel logs. Signed-off-by: Kai Vehmanen --- sound/pci/hda/patch_hdmi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8b7b8d6f6b440b..c8fc8034ac8bc8 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -259,7 +259,7 @@ static int hinfo_to_pcm_index(struct hda_codec *codec, if (get_pcm_rec(spec, pcm_idx)->stream == hinfo) return pcm_idx; - codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo); + codec_warn(codec, "HDMI: hinfo %p not tied to a PCM\n", hinfo); return -EINVAL; } @@ -277,7 +277,8 @@ static int hinfo_to_pin_index(struct hda_codec *codec, return pin_idx; } - codec_dbg(codec, "HDMI: hinfo %p not registered\n", hinfo); + codec_dbg(codec, "HDMI: hinfo %p (pcm %u) not registered\n", hinfo, + hinfo_to_pcm_index(codec, hinfo)); return -EINVAL; }