[alsa-devel] [PATCH 0/2] ASoC: dpcm: fix BE dai not hw_free and shutdown
These two patch is to solve problem found in the case of one BE is used by multiple FE.
The error log "ASoC: Unable to apply rate constraint" will happen when be_substream->runtime is use after free. Which is due to BE not being hw_free/shutdown correctly.
Also to resolve fe_substream->runtime is not constrained by BE dai symmetry property.
KaiChieh Chuang (2): ASoC: dpcm: fix BE dai not hw_free and shutdown ASoC: dpcm: symmetry constraint on FE substream
include/sound/soc-dpcm.h | 2 ++ sound/soc/soc-pcm.c | 22 +++++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-)
In case, one BE is used by two FE1/FE2 FE1--->BE--> FE2--->
When FE1/FE2 call dpcm_be_dai_hw_free() together the BE users will be 2 (> 1), hence cannot be hw_free. The be state will leave at, ex. SND_SOC_DPCM_STATE_STOP
Later FE1/FE2 call dpcm_be_dai_shutdown(), will be skip due to wrong state. Leaving the BE not being hw_free and shutdown.
This patch add a flag in snd_soc_dpcm to denote the hw_free cannot be excute for this fe->be dpcm. The BE dai will be hw_free later when calling dpcm_be_dai_shutdown() if still in invalid state.
Signed-off-by: KaiChieh Chuang kaichieh.chuang@mediatek.com --- include/sound/soc-dpcm.h | 2 ++ sound/soc/soc-pcm.c | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h index 8060590..52d9b5d 100644 --- a/include/sound/soc-dpcm.h +++ b/include/sound/soc-dpcm.h @@ -86,6 +86,8 @@ struct snd_soc_dpcm { #ifdef CONFIG_DEBUG_FS struct dentry *debugfs_state; #endif + + bool be_hw_free_deferred; };
/* diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 87c9af2..481604a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1866,8 +1866,16 @@ int dpcm_be_dai_shutdown(struct snd_soc_pcm_runtime *fe, int stream) continue;
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE) && - (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN)) - continue; + (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN)) { + if (dpcm->be_hw_free_deferred) { + soc_pcm_hw_free(be_substream); + be->dpcm[stream].state = + SND_SOC_DPCM_STATE_HW_FREE; + dpcm->be_hw_free_deferred = false; + } else { + continue; + } + }
dev_dbg(be->dev, "ASoC: close BE %s\n", be->dai_link->name); @@ -1924,8 +1932,10 @@ int dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream) continue;
/* do not free hw if this BE is used by other FE */ - if (be->dpcm[stream].users > 1) + if (be->dpcm[stream].users > 1) { + dpcm->be_hw_free_deferred = true; continue; + }
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_PARAMS) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) && @@ -1941,6 +1951,7 @@ int dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream) soc_pcm_hw_free(be_substream);
be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE; + dpcm->be_hw_free_deferred = false; }
return 0;
On Tue, May 22, 2018 at 05:13:27PM +0800, KaiChieh Chuang wrote:
In case, one BE is used by two FE1/FE2 FE1--->BE--> FE2--->
When FE1/FE2 call dpcm_be_dai_hw_free() together the BE users will be 2 (> 1), hence cannot be hw_free. The be state will leave at, ex. SND_SOC_DPCM_STATE_STOP
Later FE1/FE2 call dpcm_be_dai_shutdown(), will be skip due to wrong state. Leaving the BE not being hw_free and shutdown.
This patch add a flag in snd_soc_dpcm to denote the hw_free cannot be excute for this fe->be dpcm. The BE dai will be hw_free later when calling dpcm_be_dai_shutdown() if still in invalid state.
This works but feels messy and fragile - the problem here is that we use the users count to decide if we can do a hw_free() but we don't decrement that users count until shutdown which leaves the race condition you're fixing here. We probably need to add a second refcount here for hw_free() which also feels a bit messy but is probably robust.
Another option is to just unconditionally do the hw_free() and clean up if we're in the wrong state rather than checking the flag (so basically your patch but ignoring the flag), that is simpler and should be robust - I can't think of any reason that'd be a problem?
We should set BE symmetric constraint on FE substream.
in case one BE is used by two FE1/FE2, the first BE runtime will use FE1's substream->runtime. hence the FE1's will be constrained by BE symmetry property.
Though, second FE2 call dpcm_apply_symmetry, the be_substream->runtime == FE1's substream->runtime. The FE2's substream->runtime will not be constrained by BE's symmetry property.
Signed-off-by: KaiChieh Chuang kaichieh.chuang@mediatek.com --- sound/soc/soc-pcm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 481604a..1ef7514 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1779,14 +1779,15 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
/* Symmetry only applies if we've got an active stream. */ if (rtd->cpu_dai->active) { - err = soc_pcm_apply_symmetry(be_substream, rtd->cpu_dai); + err = soc_pcm_apply_symmetry(fe_substream, + rtd->cpu_dai); if (err < 0) return err; }
for (i = 0; i < rtd->num_codecs; i++) { if (rtd->codec_dais[i]->active) { - err = soc_pcm_apply_symmetry(be_substream, + err = soc_pcm_apply_symmetry(fe_substream, rtd->codec_dais[i]); if (err < 0) return err;
The patch
ASoC: dpcm: symmetry constraint on FE substream
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 99bcedbdebc57fe5d02fb470b7265f2208c2cf96 Mon Sep 17 00:00:00 2001
From: Kai Chieh Chuang kaichieh.chuang@mediatek.com Date: Mon, 28 May 2018 10:18:19 +0800 Subject: [PATCH] ASoC: dpcm: symmetry constraint on FE substream
We should set BE symmetric constraint on FE substream.
in case one BE is used by two FE1/FE2, the first BE runtime will use FE1's substream->runtime. hence the FE1's will be constrained by BE symmetry property.
Though, second FE2 call dpcm_apply_symmetry, the be_substream->runtime == FE1's substream->runtime. The FE2's substream->runtime will not be constrained by BE's symmetry property.
Signed-off-by: KaiChieh Chuang kaichieh.chuang@mediatek.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 2d846b3dd70c..0e2b2c6c60bd 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1779,14 +1779,15 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
/* Symmetry only applies if we've got an active stream. */ if (rtd->cpu_dai->active) { - err = soc_pcm_apply_symmetry(be_substream, rtd->cpu_dai); + err = soc_pcm_apply_symmetry(fe_substream, + rtd->cpu_dai); if (err < 0) return err; }
for (i = 0; i < rtd->num_codecs; i++) { if (rtd->codec_dais[i]->active) { - err = soc_pcm_apply_symmetry(be_substream, + err = soc_pcm_apply_symmetry(fe_substream, rtd->codec_dais[i]); if (err < 0) return err;
participants (2)
-
KaiChieh Chuang -
Mark Brown