Re: [PATCH RFC 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:

Pre-existing issues:
- [High] Error paths in `sdm845_snd_startup()` fail to decrement clock reference counters, causing permanent resource leaks.
- [High] Un-guarded `strcmp()` on potentially NULL `name_prefix` causes a kernel panic.
--

commit 6324d11aea931e9e3bac52b9aaf6e703b5d354f0
Author: David Heidelberg <david@ixit.cz>

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

This commit sets the codec dai and component sysclk during TDM startup
for all codec DAIs on the QUATERNARY_TDM path. It gracefully ignores
-ENOTSUPP errors for codecs that do not support sysclk configuration.

> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> index 8b65aa3982d47..b2d51e472bc26 100644
> --- a/sound/soc/qcom/sdm845.c
> +++ b/sound/soc/qcom/sdm845.c
> @@ -410,6 +410,28 @@ static int sdm845_snd_startup(struct snd_pcm_substream *substream)

[Severity: High]
This isn't a bug introduced by this patch, but does the pre-existing strcmp()
slightly earlier in this loop safely handle missing device tree properties?

Looking at the code just before this diff in sdm845_snd_startup():

	if (!strcmp(codec_dai->component->name_prefix, "Left")) {

If a device tree is missing the sound-name-prefix property, the component's
name_prefix pointer is left as NULL. Since strcmp() doesn't gracefully handle
NULL pointers, does this cause a direct kernel panic during stream startup?

>  					return ret;
>  				}
>  			}
> +
> +			/* 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: High]
This is a pre-existing issue, but do these early returns (both the new ones
and the pre-existing format setup checks) leak the clock reference?

At the start of the QUATERNARY_TDM_RX_0 case in sdm845_snd_startup(),
data->quat_tdm_clk_count is incremented and the clock is enabled.

If snd_soc_dai_set_sysclk() fails and returns early here, the ASoC core
handles the .startup failure by calling soc_pcm_clean(), which in turn calls
snd_soc_link_shutdown().

However, snd_soc_link_shutdown() skips the machine driver's .shutdown
callback if the .startup callback itself failed.

Since .shutdown is skipped, the clock reference counts are never decremented,
leaving the CPU DAI clocks permanently enabled. Could we add a cleanup path
to decrement the counters 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;
> +			}
>  		}
>  		break;
>  	case SLIMBUS_0_RX...SLIMBUS_6_TX:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-pixel3-audio-v1-0-3b66f33859f1@ixit.cz?part=4