[PATCH v2 0/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[PATCH v2 0/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[Chancel Liu]

It's reasonable that DAI parameters should be cleared if current DAI becomes
inactive.

Only check DAI active status in soc_pcm_hw_free() is not enough since there's
the risk that DAI parameters never be cleared if there're more than one stream
[see A]. Only check DAI active status in soc_pcm_close() is also not enough
since it will cause the cleanup just happening in soc_pcm_close() [see B].

[A] For example, we have stream1 and stream2 for DAI about to stop. stream2
executes soc_pcm_hw_free() before stream1 executes soc_pcm_close(). At the
moment, stream2 should clear current DAI parameters because stream1 passed the
cleanup stage in soc_pcm_hw_free() and stream2 in fact is the only active
stream which has a chance to do clean up. Since DAI active status is not yet
updated by stream1 in soc_pcm_close(), stream2 doesn't know itself should clear
DAI parameters. In result both stream1 and stream2 don't clear the parameters.

[B] Suppose a case: aplay -Dhw:0 44100.wav 48000.wav
In this case, we call soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free()
->soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The DAI
parameters would be remained in the system even if the playback of 44100.wav is
finished.

In conclusion, it's better to check DAI active status in both soc_pcm_hw_free()
and soc_pcm_close() which makes sure DAI parameters cleared if the DAI becomes
inactive.

changes in v2:
- Patch v1 link: https://lore.kernel.org/all/20230112065834.580192-1-chancel.liu@nxp.com/
  Patch v1 tries to introduce a usage count called hw_params_count to fix issue
  on DAI parameters cleanup. However it's not a good fix because not
  considering hw_params() and hw_free() are not symmetrical and hw_params()
  might be called multilpe times by user.
- Both check DAI active status in soc_pcm_hw_free() and soc_pcm_close() which
  makes sure DAI parameters cleared if the DAI becomes inactive.

Chancel Liu (1):
  ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes
    inactive

 sound/soc/soc-pcm.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

--
2.25.1

[PATCH v2 1/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[Chancel Liu]

The commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after
stream_active is updated") tries to make sure DAI parameters can be
cleared properly through moving the cleanup to the place where stream
active status is updated. However, it will cause the cleanup only
happening in soc_pcm_close().

Suppose a case: aplay -Dhw:0 44100.wav 48000.wav. The case calls
soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free()->
soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The
parameters would be remained in the system even if the playback of
44100.wav is finished.

The case requires us clearing parameters in phase of soc_pcm_hw_free().
However, moving the DAI parameters cleanup back to soc_pcm_hw_free()
has the risk that DAIs parameters never be cleared if there're more
than one stream, see commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs
parameters after stream_active is updated") for more details.

To meet all these requirements, in addition to do DAI parameters
cleanup in soc_pcm_hw_free(), also check it in soc_pcm_close() to make
sure DAI parameters cleared if the DAI becomes inactive.

Fixes: 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated")
Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
---
 sound/soc/soc-pcm.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 3aa6b988cb4b..6cf4cd667d03 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -698,14 +698,12 @@ static int soc_pcm_clean(struct snd_soc_pcm_runtime *rtd,
 
 	if (!rollback) {
 		snd_soc_runtime_deactivate(rtd, substream->stream);
-		/* clear the corresponding DAIs parameters when going to be inactive */
-		for_each_rtd_dais(rtd, i, dai) {
-			if (snd_soc_dai_active(dai) == 0)
-				soc_pcm_set_dai_params(dai, NULL);
 
-			if (snd_soc_dai_stream_active(dai, substream->stream) == 0)
-				snd_soc_dai_digital_mute(dai, 1, substream->stream);
-		}
+		/* Make sure DAI parameters cleared if the DAI becomes inactive */
+		for_each_rtd_dais(rtd, i, dai)
+			if (snd_soc_dai_active(dai) == 0 &&
+			    (dai->rate || dai->channels || dai->sample_bits))
+				soc_pcm_set_dai_params(dai, NULL);
 	}
 
 	for_each_rtd_dais(rtd, i, dai)
@@ -936,6 +934,15 @@ static int soc_pcm_hw_clean(struct snd_soc_pcm_runtime *rtd,
 
 	snd_soc_dpcm_mutex_assert_held(rtd);
 
+	/* clear the corresponding DAIs parameters when going to be inactive */
+	for_each_rtd_dais(rtd, i, dai) {
+		if (snd_soc_dai_active(dai) == 1)
+			soc_pcm_set_dai_params(dai, NULL);
+
+		if (snd_soc_dai_stream_active(dai, substream->stream) == 1)
+			snd_soc_dai_digital_mute(dai, 1, substream->stream);
+	}
+
 	/* run the stream event */
 	snd_soc_dapm_stream_stop(rtd, substream->stream);
 
-- 
2.25.1

Re: [PATCH v2 0/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Wed, Sep 20, 2023 at 11:36:20PM +0800, Chancel Liu wrote:
> It's reasonable that DAI parameters should be cleared if current DAI becomes
> inactive.
> 
> Only check DAI active status in soc_pcm_hw_free() is not enough since there's
> the risk that DAI parameters never be cleared if there're more than one stream
> [see A]. Only check DAI active status in soc_pcm_close() is also not enough
> since it will cause the cleanup just happening in soc_pcm_close() [see B].

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

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

Re: [PATCH v2 0/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[Mark Brown]

On Wed, 20 Sep 2023 23:36:20 +0800, Chancel Liu wrote:
> It's reasonable that DAI parameters should be cleared if current DAI becomes
> inactive.
> 
> Only check DAI active status in soc_pcm_hw_free() is not enough since there's
> the risk that DAI parameters never be cleared if there're more than one stream
> [see A]. Only check DAI active status in soc_pcm_close() is also not enough
> since it will cause the cleanup just happening in soc_pcm_close() [see B].
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive
      commit: 3efcb471f871cc095841d411f98c593228ecbac6

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

Re: [PATCH v2 1/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive

[Johan Hovold]

On Wed, Sep 20, 2023 at 11:36:21PM +0800, Chancel Liu wrote:
> The commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after
> stream_active is updated") tries to make sure DAI parameters can be
> cleared properly through moving the cleanup to the place where stream
> active status is updated. However, it will cause the cleanup only
> happening in soc_pcm_close().
> 
> Suppose a case: aplay -Dhw:0 44100.wav 48000.wav. The case calls
> soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free()->
> soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The
> parameters would be remained in the system even if the playback of
> 44100.wav is finished.
> 
> The case requires us clearing parameters in phase of soc_pcm_hw_free().
> However, moving the DAI parameters cleanup back to soc_pcm_hw_free()
> has the risk that DAIs parameters never be cleared if there're more
> than one stream, see commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs
> parameters after stream_active is updated") for more details.
> 
> To meet all these requirements, in addition to do DAI parameters
> cleanup in soc_pcm_hw_free(), also check it in soc_pcm_close() to make
> sure DAI parameters cleared if the DAI becomes inactive.
> 
> Fixes: 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated")
> Signed-off-by: Chancel Liu <chancel.liu@nxp.com>

For the record, this change incidentally also fixed the remaining click
sounds I heard when stopping pulseaudio (e.g. on reboot) with the Lenovo
ThinkPad X13s, which have also been discussed here:

	https://lore.kernel.org/lkml/ZTukaxUhgY4WLgEs@hovoldconsulting.com/

Johan