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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Uninitialized pointers `sysclk` and `enabled` in `q6i2s_set_sysclk` can cause arbitrary memory writes and kernel crashes.
- [High] Clock references acquired via `of_clk_get_by_name()` are never released with `clk_put()`, causing permanent resource leaks.
- [High] Unbalanced clock enable/disable logic between `.set_sysclk` and `.shutdown` causes clock enable count leaks and hardware state desync.
- [Medium] Missing `of_node_put()` on early return inside OF node iterator loop causes device node reference leak.
--

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

ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
    
Add support for 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.

> --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c

[ ... ]

> +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);
> +}
> +
> +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;
> +	bool *enabled;
> +	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:
> +		break;
> +	}
> +
> +	if (sysclk) {

[Severity: Critical]
Are the pointers sysclk and enabled initialized when clk_id falls into the
default case?

If an unsupported clk_id is provided, the switch statement completes without
assigning values to sysclk and enabled. The local stack memory could
evaluate as non-NULL, causing clk_set_rate() to be called with a garbage
pointer.

Additionally, does this lead to memory corruption when it executes
*enabled = true; writing a boolean to an arbitrary, uninitialized address?

> +		clk_set_rate(sysclk, freq);
> +		ret = clk_prepare_enable(sysclk);

[Severity: High]
Can this lead to unbalanced clock enable/disable counts and broken concurrent
streams?

In q6i2s_set_sysclk(), clk_prepare_enable() is called unconditionally,
meaning multiple invocations will increment the enable count multiple times.

However, q6i2s_lpass_dai_shutdown() runs per-substream. If both playback
and capture are active on the same DAI, shutting down the first stream
will disable the shared clock and set the enabled boolean to false. Does
this prematurely kill the clock for the remaining active stream?

Similarly, if the enable count was incremented multiple times, but shutdown
only disables it once before clearing the flag, does this leak the clock
enable refcount?

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

[ ... ]

> +static int of_q6apm_parse_dai_data(struct device *dev,
> +				   struct q6apm_lpass_dai_data *data)
> +{
> +	struct device_node *node;
> +	int ret;
> +
> +	for_each_child_of_node(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:
> +			priv = &data->priv[id];
> +			priv->mclk = of_clk_get_by_name(node, "mclk");

[Severity: High]
Are these clock references ever released?

The clocks are acquired here via of_clk_get_by_name(), which increments the
underlying refcount and requires a matching clk_put().

If a later clock lookup within this loop fails with -EPROBE_DEFER, the
function returns early without freeing previously acquired clocks. Furthermore,
because these are not managed via devm_ and there is no teardown logic or
.remove callback provided, does this cause a permanent resource leak when
the device unbinds?

> +			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");

[Severity: Medium]
Does returning here leak the device node reference?

The for_each_child_of_node() macro automatically manages reference counts
during iteration, but an early return requires a manual of_node_put(node)
before exiting. If this loop exits early via return, will the device node
reference be leaked?

> +				priv->mclk = NULL;
> +			}
> +
> +			priv->bclk = of_clk_get_by_name(node, "bclk");
> +			if (IS_ERR(priv->bclk)) {
> +				if (PTR_ERR(priv->bclk) == -EPROBE_DEFER)
> +					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)
> +					return dev_err_probe(dev, PTR_ERR(priv->eclk),
> +							     "unable to get mi2s eclk\n");
> +				priv->eclk = NULL;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

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