Re: [PATCH RFC v2 4/6] ASoC: qcom: sdm845: Set codec dai and component sysclk during startup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Resource leak (clock and refcount) on error path in sdm845_snd_startup.

Pre-existing issues:
- [High] Kernel panic due to NULL pointer dereference in `strcmp()`.
--

commit 620251c05b4c5ac4fb80bf96411a9fcc1f4c402d
Author: David Heidelberg <david@ixit.cz>

ASoC: qcom: sdm845: Set codec dai and component sysclk during startup

The cs35l36 codec needs the codec dai and component sysclk to be set
during TDM startup. Set these for all codec DAIs on the QUATERNARY_TDM
path, gracefully handling codecs that don't support sysclk by ignoring
-ENOTSUPP returns.

> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> index 59cfa3b26cd4d..f0fbec9c8df25 100644
> --- a/sound/soc/qcom/sdm845.c
> +++ b/sound/soc/qcom/sdm845.c
> @@ -412,6 +412,28 @@ static int sdm845_snd_startup(struct snd_pcm_substream *substream)
>  					return ret;
>  				}
>  			}

[Severity: High]
This is a pre-existing issue, but can codec_dai->component->name_prefix be
NULL when evaluated in the strcmp() calls just above this block?

If the device tree lacks a sound-name-prefix property for a codec on the
QUATERNARY_TDM link, passing a NULL name_prefix to strcmp() will cause a
kernel panic during startup.

> +
> +			/* Set codec sysclk needed by codecs like cs35l36. */
> +			ret = snd_soc_dai_set_sysclk(codec_dai, 0,
> +						     TDM_BCLK_RATE,
> +						     SND_SOC_CLOCK_IN);
> +			if (ret < 0 && ret != -ENOTSUPP) {
> +				dev_err(codec_dai->dev,
> +					"Failed to set codec dai sysclk: %d\n",
> +					ret);
> +				return ret;

[Severity: Medium]
Does this early return leak the CPU DAI clock and the clock reference count?

Earlier in sdm845_snd_startup(), the quat_tdm_clk_count is incremented and
the clock is enabled:

    if (++(data->quat_tdm_clk_count) == 1) {
        snd_soc_dai_set_sysclk(cpu_dai,
            Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
            TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
    }

If setting the codec DAI sysclk fails and returns here, it bypasses
decrementing this reference count and leaves the CPU DAI sysclk active. The
ALSA/ASoC core does not invoke the machine driver's shutdown callback when
startup fails, meaning this state is permanently leaked.

Is there a need to unwind these resources before returning?

> +			}
> +
> +			ret = snd_soc_component_set_sysclk(codec_dai->component,
> +							   0, 0,
> +							   TDM_BCLK_RATE,
> +							   SND_SOC_CLOCK_IN);
> +			if (ret < 0 && ret != -ENOTSUPP) {
> +				dev_err(codec_dai->dev,
> +					"Failed to set codec component sysclk: %d\n",
> +					ret);
> +				return ret;

[Severity: Medium]
Does this early return also leak the CPU DAI clock and the reference count?

Similar to the previous early return, failing to set the codec component
sysclk will leave the quat_tdm_clk_count incremented and the CPU DAI sysclk
active.

> +			}
>  		}
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-pixel3-audio-v2-0-7c22ed915291@ixit.cz?part=4