Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ALSA: hda/hdmi: fix failure at PCM open #2223

Closed
wants to merge 3 commits into from
Closed

ALSA: hda/hdmi: fix failure at PCM open #2223

wants to merge 3 commits into from

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Jun 22, 2020 •
edited
Loading 

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.

Fixes: #2216

@kv2019i
ASoC: hdac_hda: fix deadlock after PCM open error
3c4189c 
Commit 202bac2 ("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: 202bac2 ("ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence")
BugLink: thesofproject#2159
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i
ALSA: hda/hdmi: fix failure at PCM open
8a59b42 
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 <kai.vehmanen@linux.intel.com>
@kv2019i
ALSA: hda/hdmi: improve debug traces
50d6266 
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 <kai.vehmanen@linux.intel.com>
@kv2019i kv2019i mentioned this pull request Jun 22, 2020
Closed
plbossart
plbossart requested changes Jun 22, 2020
View reviewed changes
sound/pci/hda/patch_hdmi.c
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit message nit-pick:
Fix the problem by prioritizing the converter with the same index as PCM

This isn't really prioritizing, you first try with the nopin version then fallback to the existing solution.

This was referenced Jun 23, 2020
Closed
Closed
@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 26, 2020

This did not fix the full problem. I found some further issues and am working on a fix. Closing this one.

@kv2019i kv2019i closed this Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plbossart plbossart plbossart requested changes

@bardliao bardliao Awaiting requested review from bardliao bardliao is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@lgirdwood lgirdwood Awaiting requested review from lgirdwood lgirdwood is a code owner

@ranj063 ranj063 Awaiting requested review from ranj063 ranj063 is a code owner

Assignees
No one assigned
Labels
Draft-for-testing-not-ready-for-review
Projects
None yet
Milestone
No milestone
2 participants
@kv2019i @plbossart