[PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[Ravulapati Vishnu vardhan rao]

If we play audio back to back, which kills one playback
and immediately start another, we can hear clicks.
This patch fixes the issue.

Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
---
 sound/soc/amd/raven/acp3x-pcm-dma.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/sound/soc/amd/raven/acp3x-pcm-dma.c b/sound/soc/amd/raven/acp3x-pcm-dma.c
index 5c3ec3c..916649a 100644
--- a/sound/soc/amd/raven/acp3x-pcm-dma.c
+++ b/sound/soc/amd/raven/acp3x-pcm-dma.c
@@ -344,25 +344,28 @@ static int acp3x_dma_close(struct snd_soc_component *component,
 {
 	struct snd_soc_pcm_runtime *prtd;
 	struct i2s_dev_data *adata;
+	struct i2s_stream_instance *rtd;
 
 	prtd = substream->private_data;
 	component = snd_soc_rtdcom_lookup(prtd, DRV_NAME);
 	adata = dev_get_drvdata(component->dev);
+	rtd = substream->runtime->private_data;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		adata->play_stream = NULL;
-		adata->i2ssp_play_stream = NULL;
-	} else {
-		adata->capture_stream = NULL;
-		adata->i2ssp_capture_stream = NULL;
-	}
 
 	/* Disable ACP irq, when the current stream is being closed and
 	 * another stream is also not active.
 	 */
+	kfree(rtd);
 	if (!adata->play_stream && !adata->capture_stream &&
 		!adata->i2ssp_play_stream && !adata->i2ssp_capture_stream)
 		rv_writel(0, adata->acp3x_base + mmACP_EXTERNAL_INTR_ENB);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		adata->play_stream = NULL;
+		adata->i2ssp_play_stream = NULL;
+	} else {
+		adata->capture_stream = NULL;
+		adata->i2ssp_capture_stream = NULL;
+	}
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[Mark Brown]

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

On Tue, Jan 21, 2020 at 04:13:35PM +0530, Ravulapati Vishnu vardhan rao wrote:

> If we play audio back to back, which kills one playback
> and immediately start another, we can hear clicks.
> This patch fixes the issue.

>  	/* Disable ACP irq, when the current stream is being closed and
>  	 * another stream is also not active.
>  	 */
> +	kfree(rtd);

This free looks like a separate change which seems good and useful but
should be in a separate patch?

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

Re: [PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[Mark Brown]

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

On Tue, Jan 21, 2020 at 05:03:43PM +0000, RAVULAPATI, VISHNU VARDHAN RAO wrote:

> > +     kfree(rtd);

> This free looks like a separate change which seems good and useful but
> should be in a separate patch?

> No Mark,

> That is part of the fix. please let this be included in this patch.

In what way is it part of the fix?  This at least needs some sort of
explanation somewhere, the changelog at least if not the code.

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

Re: [PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[Mark Brown]

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

On Tue, Jan 21, 2020 at 05:12:42PM +0000, RAVULAPATI, VISHNU VARDHAN RAO wrote:

Please fix your mailer to quote replies normally, it's hard to read your
mails.

> In what way is it part of the fix?  This at least needs some sort of
> explanation somewhere, the changelog at least if not the code.

> When we play subsequently we hear last played sound for moment.kfree clears the structure.

If the rtd is still being referenced after the kfree() you have a use
after free bug so there's a serious problem there and this change is
introducing a bug, you can only free things if they are not in use.  At
a bare minimum I need the changelog to clearly explain what the cause of
the clicks is and how the change fixes that.

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

Re: [PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[vishnu]

On 22/01/20 5:44 AM, Kuninori Morimoto wrote:
> 
> Hi Ravulapati
> 
>> If we play audio back to back, which kills one playback
>> and immediately start another, we can hear clicks.
>> This patch fixes the issue.
>>
>> Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
>> ---
> (snip)
>> +	kfree(rtd);
> 
> Please double check soc_new_pcm_runtime() and
> soc_free_pcm_runtime() at soc-core.c.
> Because rtd is created via devm_kzalloc(),
> and has many related resources which need to be cared when rtd was freed.
> Just kfree() is not good/enough, I think.
> 
> I think you want to use is snd_soc_remove_pcm_runtime()
> instead of kfree()
> 
> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto
> 
I will create a separate patch for kfree and separate one for subsequent 
play back issue.

[PATCH] ASoC: amd: Fix for Subsequent Playback issue.

[Ravulapati Vishnu vardhan rao]

If we play audio back to back, which kills one playback
and immediately start another, we can hear clicks.
This patch fixes the issue.

Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
---
 sound/soc/amd/raven/acp3x-pcm-dma.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/amd/raven/acp3x-pcm-dma.c b/sound/soc/amd/raven/acp3x-pcm-dma.c
index 5c3ec3c..aecc3c0 100644
--- a/sound/soc/amd/raven/acp3x-pcm-dma.c
+++ b/sound/soc/amd/raven/acp3x-pcm-dma.c
@@ -349,13 +349,6 @@ static int acp3x_dma_close(struct snd_soc_component *component,
 	component = snd_soc_rtdcom_lookup(prtd, DRV_NAME);
 	adata = dev_get_drvdata(component->dev);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		adata->play_stream = NULL;
-		adata->i2ssp_play_stream = NULL;
-	} else {
-		adata->capture_stream = NULL;
-		adata->i2ssp_capture_stream = NULL;
-	}
 
 	/* Disable ACP irq, when the current stream is being closed and
 	 * another stream is also not active.
@@ -363,6 +356,13 @@ static int acp3x_dma_close(struct snd_soc_component *component,
 	if (!adata->play_stream && !adata->capture_stream &&
 		!adata->i2ssp_play_stream && !adata->i2ssp_capture_stream)
 		rv_writel(0, adata->acp3x_base + mmACP_EXTERNAL_INTR_ENB);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		adata->play_stream = NULL;
+		adata->i2ssp_play_stream = NULL;
+	} else {
+		adata->capture_stream = NULL;
+		adata->i2ssp_capture_stream = NULL;
+	}
 	return 0;
 }
 
-- 
2.7.4