[PATCH 0/2] ASoC: dpcm: fix BE dai not hw_free and shutdown

[PATCH 0/2] ASoC: dpcm: fix BE dai not hw_free and shutdown

[KaiChieh Chuang]

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(-)

-- 
1.9.1

[PATCH 1/2] ASoC: dpcm: fix BE dai not hw_free and shutdown

[KaiChieh Chuang]

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;
-- 
1.9.1

Re: [PATCH 1/2] ASoC: dpcm: fix BE dai not hw_free and shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1281 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/2] ASoC: dpcm: symmetry constraint on FE substream

[KaiChieh Chuang]

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;
-- 
1.9.1

Applied "ASoC: dpcm: symmetry constraint on FE substream" to the asoc tree

[Mark Brown]

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;
-- 
2.17.0