Skip to content

ASoC: hdac_hda: fix deadlock after PCM open error #2208

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

Merged
kv2019i merged 1 commit into from
Jun 24, 2020
Merged

ASoC: hdac_hda: fix deadlock after PCM open error #2208

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions sound/soc/codecs/hdac_hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Copy link
Member

@plbossart plbossart Jun 17, 2020

Choose a reason for hiding this comment

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

the reference in the commit message seems incorrect, I reverted some changes in 5bd7044 ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

Copy link
Collaborator Author

@kv2019i kv2019i Jun 18, 2020

Choose a reason for hiding this comment

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

@plbossart OK, that's indeed a bit complicated history. But it seems in your commit
"ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence"
.. you didn't revert all of the original changes. This part remains and triggers the HDMI problem:

-machine_err:
-       i = rtd->num_codecs;
-
 codec_dai_err:
-       for_each_rtd_codec_dai_rollback(rtd, i, codec_dai)
+       for_each_rtd_codec_dai(rtd, i, codec_dai)
                snd_soc_dai_shutdown(codec_dai, substream);

I.e. it removed the rollback and just called shutdown on all DAIs. Hmm, so what is the policy now, should shutdown() expect startup() to have succeeded or not? We've been going back and forth on this now. Looking at DAI impementations, most simply don't care about this and I couldn't find a single instance that would hit bugs like we have in hdac-hda. But for any resource allocation done in startup(), this can be a big issue.

Copy link
Member

@plbossart plbossart Jun 18, 2020

Choose a reason for hiding this comment

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

I was only worried about the Fixes tag, you probably want to use the last know fix to avoid any misunderstanding/collisions. Or use two Fixes tag :-)

Copy link
Collaborator Author

@kv2019i kv2019i Jun 18, 2020

Choose a reason for hiding this comment

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

@plbossart Ack. Probably better to clarify. I'll try to cook up something... not exactly straightforward to explain.

Copy link
Collaborator Author

@kv2019i kv2019i Jun 18, 2020

Choose a reason for hiding this comment

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

@plbossart I ended up only mentioning your commit. The original patch actually didn't change the semantics yet as it tracked whether startup() had succeeded or not. When you reverted all the startup() tracking, the rollback change stayed in the code. Considering nothing else has stopped working, I think just fixing hdac_hda is a good way forward. Need to keep a close eye on soc-pcm.c changes though. It is pretty easy to break this again.

}

static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
Expand Down