Re: [PATCH RFC 16/23] ASoC: qcom: lpass: enable the right codec_mem clock when usin LPM memory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: lpass: LPM clocks not parsed when only
  mi2s_lpm_enable is set
- [Medium] ASoC: qcom: lpass: unchecked mem_clk enable and leak in
  startup()
--

commit 6968d3f5a8288f5418d4e48725abed1263de22d5
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: lpass: enable the right codec_mem clock when usin LPM memory

This commit adds a helper to select the codec_mem clock matching a given LPM
buffer offset and enables the corresponding LPM memory clock during DAI
startup and shutdown for Codec DMA and LPM-backed MI2S ports.

> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 4bbcd59ebd602..302c31b23090f 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -89,6 +89,17 @@ static int lpass_cpu_daiops_startup(struct snd_pcm_substream *substream,
>  	unsigned int port_id = mi2s_port_id(dai->driver->id);
>  	int ret;
>  
> +	if (is_lpm_mi2s_port(drvdata, dai->driver->id)) {
> +		struct clk *mem_clk = drvdata->va_mem0;
> +
> +		if (drvdata->mi2s_lpm[port_id] == LPASS_RXTX_CDC_LPM)
> +			mem_clk = lpass_cdc_dma_mem_clk(drvdata,
> +							drvdata->mi2s_lpm_offset[port_id]);

[Severity: Medium]
Can mem_clk be NULL here when a board enables LPM-backed MI2S ports without
any Codec DMA DAIs?

In asoc_qcom_lpass_cpu_platform_probe(), of_lpass_cdc_dma_clks_parse() is only
called when drvdata->codec_dma_enable is set, whereas the LPM memory resources
are fetched when either codec_dma_enable or mi2s_lpm_enable is set:

sound/soc/qcom/lpass-cpu.c:asoc_qcom_lpass_cpu_platform_probe() {
    ...
		ret = of_lpass_cdc_dma_clks_parse(dev, drvdata);
		if (ret) {
			dev_err(dev, "failed to get cdc dma clocks %d\n", ret);
			return ret;
		}
	}
    ...
	if (drvdata->codec_dma_enable || drvdata->mi2s_lpm_enable) {
		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-rxtx-cdc-dma-lpm");
    ...
}

When only drvdata->mi2s_lpm_enable is set, drvdata->va_mem0 and
drvdata->codec_mem0..3 are never populated and remain NULL.  Passing NULL to
clk_set_rate() and clk_prepare_enable() is a no-op, leaving the LPM SRAM
unclocked when the stream accesses the LPM buffer.  Calling
of_lpass_cdc_dma_clks_parse() under the codec_dma_enable || mi2s_lpm_enable
check would ensure the clocks are acquired.

> +
> +		clk_set_rate(mem_clk, LPASS_CODEC_MEM_HZ_NORMAL);
> +		clk_prepare_enable(mem_clk);
> +	}
> +
>  	ret = clk_prepare_enable(drvdata->mi2s_osr_clk[port_id]);
>  	if (ret) {
>  		dev_err(dai->dev, "error in enabling mi2s osr clk: %d\n", ret);

[Severity: Medium]
Does this leak the prepare and enable references on mem_clk if enabling
mi2s_osr_clk or preparing mi2s_bit_clk fails?

Whendai->driver->ops->startup() returns an error, snd_soc_dai_startup() does
not mark the DAI as active, so snd_soc_dai_shutdown() will not call
lpass_cpu_daiops_shutdown() to release mem_clk.  In addition, neither
lpass_cpu_daiops_startup() nor lpass_cdc_dma_daiops_startup() checks the
return values of clk_set_rate() and clk_prepare_enable() on mem_clk, which can
leave LPM memory unclocked and cause an unbalanced clk_disable_unprepare() on
shutdown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=16