Re: [PATCH v4 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] Premature destruction of clocks due to inverted devm teardown order causes hardware lockups.
- [Medium] Passing an error pointer to clk_put() during probe deferral triggers a kernel warning.
--

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

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

This commit adds MI2S clock control within q6apm-lpass DAIs, including
handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
Each MI2S port now retrieves its clock handles from the device tree,
allowing 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..5743586ffda1e 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
[ ... ]
> @@ -297,6 +409,65 @@ 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) {
> +					q6apm_lpass_dai_put_clocks(data);

[Severity: Medium]
Will this pass an error pointer to clk_put() during probe deferral?

If of_clk_get_by_name() returns -EPROBE_DEFER, the error pointer is stored
in priv->mclk. When q6apm_lpass_dai_put_clocks() is immediately called for
cleanup, it iterates over all ports and calls clk_put() on any non-NULL
clock pointer:

q6apm_lpass_dai_put_clocks() {
	...
	if (dai_data->priv[i].mclk) {
		clk_put(dai_data->priv[i].mclk);
	...
}

Since an ERR_PTR evaluates as true, clk_put() is called with the error
pointer, which will trigger a WARN_ON_ONCE(IS_ERR(clk)) in the core clock
code.

> +					return dev_err_probe(dev, PTR_ERR(priv->mclk),
> +							     "unable to get mi2s mclk\n");
> +				}
> +				priv->mclk = NULL;
> +			}
[ ... ]
> @@ -317,7 +492,18 @@ static int q6apm_lpass_dai_dev_probe(struct platform_device *pdev)
>  	cfg.q6hdmi_ops = &q6hdmi_ops;
>  	dais = q6dsp_audio_ports_set_config(dev, &cfg, &num_dais);
>  
> -	return devm_snd_soc_register_component(dev, &q6apm_lpass_dai_component, dais, num_dais);
> +	ret = devm_snd_soc_register_component(dev, &q6apm_lpass_dai_component, dais, num_dais);
> +	if (ret)
> +		q6apm_lpass_dai_put_clocks(dai_data);
> +
> +	return ret;
> +}
> +
> +static void q6apm_lpass_dai_dev_remove(struct platform_device *pdev)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(&pdev->dev);
> +
> +	q6apm_lpass_dai_put_clocks(dai_data);

[Severity: High]
Does this premature destruction of clocks due to an inverted devm teardown
order cause hardware lockups?

Because the ALSA component is registered using
devm_snd_soc_register_component(), its unregistration is deferred to the
devres cleanup phase, which runs after this .remove() callback returns.

If the driver is unbound while audio is actively streaming, .remove()
manually invokes q6apm_lpass_dai_put_clocks(), which turns off and frees
the MI2S clocks while the DSP is actively executing a graph on the bus.

The ALSA component attempts to cleanly stop the DSP graph only during
devres cleanup, but by then the physical clocks are already cut, which
can lead to DSP crashes or NOC (Network-on-Chip) errors.

>  }

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