Re: [PATCH v3 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Clock references acquired via of_clk_get_by_name() are leaked on early probe error returns, driver component registration failure, and driver unbind.
- [High] q6i2s_set_sysclk() silently ignores frequency updates if the clock is already enabled, preventing ALSA stream reconfiguration.
- [High] MI2S clocks are disabled before the DSP graph is safely stopped during DAI shutdown, which can cause hardware lockups.
--

commit 194ae39f86b1dc7d38cb5e3de67fcbfe435cfeea
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control

This commit adds support for MI2S clock control within the q6apm-lpass
DAIs, handling MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
It allows per-port clock configuration and proper enable/disable
during startup and shutdown.

> diff --git a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> index 006b283484d9e..461e655265060 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
[ ... ]
> @@ -251,6 +262,73 @@ static int q6apm_lpass_dai_startup(struct snd_pcm_substream *substream, struct s
>  	return 0;
>  }
>  
> +static int q6i2s_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	return q6apm_lpass_dai_startup(substream, dai);
> +}
> +
> +static void q6i2s_lpass_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
> +
> +	if (dai_data->priv[dai->id].mclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].mclk);
> +		dai_data->priv[dai->id].mclk_enabled = false;
> +	}
> +
> +	if (dai_data->priv[dai->id].bclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].bclk);
> +		dai_data->priv[dai->id].bclk_enabled = false;
> +	}
> +
> +	if (dai_data->priv[dai->id].eclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].eclk);
> +		dai_data->priv[dai->id].eclk_enabled = false;
> +	}
> +	q6apm_lpass_dai_shutdown(substream, dai);

[Severity: High]
Does this code disable the physical MI2S clocks before stopping the DSP graph?

Since q6apm_lpass_dai_shutdown() sends the stop command to the DSP, disabling
the clocks first could violate hardware teardown requirements and cause
hardware lockups while the DSP is still actively streaming.

Should the clocks be disabled only after the DSP graph has been completely
stopped?

> +}
> +
> +static int q6i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
> +	struct clk *sysclk = NULL;
> +	bool *enabled = NULL;
> +	int ret = 0;
> +
> +	switch (clk_id) {
> +	case LPAIF_MI2S_MCLK:
> +		sysclk = dai_data->priv[dai->id].mclk;
> +		enabled = &dai_data->priv[dai->id].mclk_enabled;
> +		break;
> +	case LPAIF_MI2S_BCLK:
> +		sysclk = dai_data->priv[dai->id].bclk;
> +		enabled = &dai_data->priv[dai->id].bclk_enabled;
> +		break;
> +	case LPAIF_MI2S_ECLK:
> +		sysclk = dai_data->priv[dai->id].eclk;
> +		enabled = &dai_data->priv[dai->id].eclk_enabled;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (sysclk) {
> +		if (*enabled)
> +			return 0;

[Severity: High]
Can this prevent ALSA from reconfiguring the sample rate?

The ALSA framework can call set_sysclk() multiple times for a stream, such as
to dynamically reconfigure the rate. Because this early return ignores
frequency updates if the clock is already enabled, the physical clock may
remain at its initial rate and cause audio distortion.

> +
> +		clk_set_rate(sysclk, freq);
> +		ret = clk_prepare_enable(sysclk);
> +		if (ret) {
> +			dev_err(dai->dev, "Error, Unable to prepare (%d) sysclk\n", clk_id);
> +			return ret;
> +		}
[ ... ]
> @@ -297,6 +376,73 @@ static const struct snd_soc_component_driver q6apm_lpass_dai_component = {
>  	.remove_order   = SND_SOC_COMP_ORDER_FIRST,
>  };
>  
> +static int of_q6apm_parse_dai_data(struct device *dev,
> +				   struct q6apm_lpass_dai_data *data)
> +{
> +	int ret;
> +
> +	for_each_child_of_node_scoped(dev->of_node, node) {
> +		struct q6apm_dai_priv_data *priv;
> +		int id;
> +
> +		ret = of_property_read_u32(node, "reg", &id);
> +		if (ret || id < 0 || id >= APM_PORT_MAX) {
> +			dev_err(dev, "valid dai id not found:%d\n", ret);
> +			continue;
> +		}
> +
> +		switch (id) {
> +		/* MI2S specific properties */
> +		case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
> +		case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
> +		case SENARY_MI2S_RX ... SENARY_MI2S_TX:
> +			priv = &data->priv[id];
> +			priv->mclk = of_clk_get_by_name(node, "mclk");
> +			if (IS_ERR(priv->mclk)) {
> +				if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
> +					return dev_err_probe(dev, PTR_ERR(priv->mclk),
> +							     "unable to get mi2s mclk\n");
> +				priv->mclk = NULL;
> +			}
> +
> +			priv->bclk = of_clk_get_by_name(node, "bclk");
> +			if (IS_ERR(priv->bclk)) {
> +				if (PTR_ERR(priv->bclk) == -EPROBE_DEFER) {
> +					if (priv->mclk) {
> +						clk_put(priv->mclk);
> +						priv->mclk = NULL;
> +					}
> +					return dev_err_probe(dev, PTR_ERR(priv->bclk),
> +							     "unable to get mi2s bclk\n");
> +				}
> +				priv->bclk = NULL;
> +			}
> +
> +			priv->eclk = of_clk_get_by_name(node, "eclk");
> +			if (IS_ERR(priv->eclk)) {
> +				if (PTR_ERR(priv->eclk) == -EPROBE_DEFER) {
> +					if (priv->mclk) {
> +						clk_put(priv->mclk);
> +						priv->mclk = NULL;
> +					}
> +					if (priv->bclk) {
> +						clk_put(priv->bclk);
> +						priv->bclk = NULL;
> +					}
> +					return dev_err_probe(dev, PTR_ERR(priv->eclk),
> +							     "unable to get mi2s eclk\n");

[Severity: High]
Does this leak clock references acquired in previous iterations of the loop?

When a clock lookup returns -EPROBE_DEFER, the current code properly cleans up
clocks for the current node, but it returns without cleaning up clocks acquired
for any previous child nodes. Since probe deferrals can occur multiple times,
this could accumulate leaks over time.

> +				}
> +				priv->eclk = NULL;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int q6apm_lpass_dai_dev_probe(struct platform_device *pdev)
>  {
>  	struct q6dsp_audio_port_dai_driver_config cfg;
> @@ -304,12 +450,16 @@ static int q6apm_lpass_dai_dev_probe(struct platform_device *pdev)
>  	struct snd_soc_dai_driver *dais;
>  	struct device *dev = &pdev->dev;
>  	int num_dais;
> +	int ret;
>  
>  	dai_data = devm_kzalloc(dev, sizeof(*dai_data), GFP_KERNEL);
>  	if (!dai_data)
>  		return -ENOMEM;
>  
>  	dev_set_drvdata(dev, dai_data);
> +	ret = of_q6apm_parse_dai_data(dev, dai_data);
> +	if (ret)
> +		return ret;
>  
>  	memset(&cfg, 0, sizeof(cfg));
>  	cfg.q6i2s_ops = &q6i2s_ops;

[Severity: High]
If devm_snd_soc_register_component() fails later in this function, are the
manually acquired clocks leaked?

There seems to be no error handling path or devres action to call clk_put()
for the clocks acquired in of_q6apm_parse_dai_data(), leaving them permanently
leaked on registration failure.

Additionally, since q6apm_lpass_dai_platform_driver lacks a .remove callback,
does this also guarantee that all acquired clocks leak during device unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706132009.1496321-1-mohammad.rafi.shaik@oss.qualcomm.com?part=2