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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing freq == 0 check in set_sysclk causes permanent clock enable leak
- [Low] Memory leak of clock references on duplicate DAI nodes in DT
--

--- Patch [2]: [PATCH v7 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control ---
commit bf9b1226eaeb15dbad44a1a60c78519c600713d4
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

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

This patch adds support for MI2S clock control within q6apm-lpass DAIs.
It retrieves MCLK and BCLK handles from the device tree for each MI2S port
and manages them via the DAI set_sysclk callback.

> diff --git a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> index 006b283484d9e..e68e8b000e079 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c

[ ... ]

> @@ -251,6 +292,62 @@ static int q6apm_lpass_dai_startup(struct snd_pcm_substream *substream, struct s
> +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;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (sysclk) {
> +		ret = clk_set_rate(sysclk, freq);
> +		if (ret) {
> +			dev_err(dai->dev, "Error, Unable to set rate (%d) for sysclk %d\n",
> +				freq, clk_id);
> +			return ret;
> +		}
> +
> +		if (*enabled)
> +			return 0;
> +
> +		ret = clk_prepare_enable(sysclk);

[Severity: High]
Will this leak the clock enable count if a machine driver calls
snd_soc_dai_set_sysclk() with freq == 0 during teardown?

Many ASoC machine drivers call snd_soc_dai_set_sysclk() with a frequency
of 0 to explicitly disable clocks. If this happens after the DAI's shutdown
hook has already cleared the enabled boolean to false, this function will
see enabled == false and incorrectly call clk_prepare_enable() again.

Without a subsequent hook to turn it off, the clock would remain permanently
enabled, preventing the system from entering low-power states.

Should there be a check for freq == 0 to handle clock disable requests?

> +		if (ret) {
> +			dev_err(dai->dev, "Error, Unable to prepare (%d) sysclk\n", clk_id);
> +			return ret;
> +		}
> +
> +		*enabled = true;
> +	}
> +
> +	return ret;
> +}

[ ... ]

> @@ -297,6 +395,64 @@ static const struct snd_soc_component_driver q6apm_lpass_dai_component = {
> +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");

[Severity: Low]
Does this leak the CCF clock references if a malformed device tree provides
multiple DAI child nodes with the same reg value?

If multiple nodes map to the same id index, the assignments to priv->mclk
and priv->bclk will overwrite the previously acquired clock pointers from
earlier iterations without calling clk_put(), leaking the previous clock
references.

> +			if (IS_ERR(priv->mclk)) {
> +				int err = PTR_ERR(priv->mclk);
> +
> +				priv->mclk = NULL;
> +				if (err == -EPROBE_DEFER) {
> +					q6apm_lpass_dai_put_clocks(data);
> +					return dev_err_probe(dev, err,
> +							     "unable to get mi2s mclk\n");
> +				}
> +			}
> +
> +			priv->bclk = of_clk_get_by_name(node, "bclk");

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