Re: [PATCH] ASoC: qdsp6: q6asm-dai: fix q6asm_dai_compr_set_params error path

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 27/03/2025 15:46, Alexey Klimov wrote:
> In case of attempts to compress playback something, for instance,
> when audio routing is not set up correctly, the audio DSP is left in
> inconsistent state because we are not doing the correct things in
> the error path of q6asm_dai_compr_set_params().
> 
> So, when routing is not set up and compress playback is attempted
> the following errors are present (simplified log):
> 
> q6routing routing: Routing not setup for MultiMedia-1 Session
> q6asm-dai dais: Stream reg failed ret:-22
> q6asm-dai dais: ASoC error (-22): at snd_soc_component_compr_set_params()
> on 17300000.remoteproc:glink-edge:apr:service@7:dais
> 
> After setting the correct routing the compress playback will always fail:
> 
> q6asm-dai dais: cmd = 0x10db3 returned error = 0x9
> q6asm-dai dais: DSP returned error[9]
> q6asm-dai dais: q6asm_open_write failed
> q6asm-dai dais: ASoC error (-22): at snd_soc_component_compr_set_params()
> on 17300000.remoteproc:glink-edge:apr:service@7:dais
> 
> 0x9 here means "Operation is already processed". The CMD_OPEN here was
> sent the second time hence DSP responds that it was already done.
> 
> Turns out the CMD_CLOSE should be sent after the q6asm_open_write()
> succeeded but something failed after that, for instance, routing
> setup.
> 
> Fix this by slightly reworking the error path in
> q6asm_dai_compr_set_params().
> 
> Tested on QRB5165 RB5 and SDM845 RB3 boards.
> 
> Cc: stable@vger.kernel.org
> Fixes: 5b39363e54cc ("ASoC: q6asm-dai: prepare set params to accept profile change")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
> ---

thanks for the patch,

This is now pretty much inline with what we have in non-compressed patch.

LGTM.

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

--srini
>   sound/soc/qcom/qdsp6/q6asm-dai.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index 045100c94352..a400c9a31fea 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -892,9 +892,7 @@ static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
>   
>   		if (ret < 0) {
>   			dev_err(dev, "q6asm_open_write failed\n");
> -			q6asm_audio_client_free(prtd->audio_client);
> -			prtd->audio_client = NULL;
> -			return ret;
> +			goto open_err;
>   		}
>   	}
>   
> @@ -903,7 +901,7 @@ static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
>   			      prtd->session_id, dir);
>   	if (ret) {
>   		dev_err(dev, "Stream reg failed ret:%d\n", ret);
> -		return ret;
> +		goto q6_err;
>   	}
>   
>   	ret = __q6asm_dai_compr_set_codec_params(component, stream,
> @@ -911,7 +909,7 @@ static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
>   						 prtd->stream_id);
>   	if (ret) {
>   		dev_err(dev, "codec param setup failed ret:%d\n", ret);
> -		return ret;
> +		goto q6_err;
>   	}
>   
>   	ret = q6asm_map_memory_regions(dir, prtd->audio_client, prtd->phys,
> @@ -920,12 +918,21 @@ static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
>   
>   	if (ret < 0) {
>   		dev_err(dev, "Buffer Mapping failed ret:%d\n", ret);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto q6_err;
>   	}
>   
>   	prtd->state = Q6ASM_STREAM_RUNNING;
>   
>   	return 0;
> +
> +q6_err:
> +	q6asm_cmd(prtd->audio_client, prtd->stream_id, CMD_CLOSE);
> +
> +open_err:
> +	q6asm_audio_client_free(prtd->audio_client);
> +	prtd->audio_client = NULL;
> +	return ret;
>   }
>   
>   static int q6asm_dai_compr_set_metadata(struct snd_soc_component *component,