Re: [PATCH v2 05/11] ASoC: qcom: Add QAIF AIF DAI ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Clock reference tracking bugs leading to unbalanced clk_disable and power management failure.
- [High] Silent failure on stream trigger (START) due to overwritten error code and missing hardware rollback.
- [High] The CPU DAI directly configures the Codec DAI with hardcoded slots, causing out-of-bounds array reads and violating ASoC component architecture.
- [High] Concurrent playback and capture streams on the same MI2S interface will overwrite and corrupt shared hardware registers and clock rates.
- [Low] Useless NULL check on an inline array element address.
--

commit d1f7f6e70f39b363c15c95838477463b38010acc
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>
Subject: ASoC: qcom: Add QAIF AIF DAI ops

This commit adds CPU DAI operations and regmap field setup needed to use the
QAIF AIF (MI2S) path. This allows QAIF to route playback through RDDMA
channels and capture through WRDMA channels to external serial audio devices
over MI2S.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> index 072c1bec078aa..08c7aa4779382 100644
> --- a/sound/soc/qcom/qaif-cpu.c
> +++ b/sound/soc/qcom/qaif-cpu.c
[ ... ]
> +static int qaif_aif_cpu_daiops_hw_free(struct snd_pcm_substream *substream,
> +				       struct snd_soc_dai *dai)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +	const struct qaif_variant *v = drvdata->variant;
> +	int idx = v->get_dma_idx(dai->driver->id);
> +
> +	if (idx < 0)
> +		return 0;
> +
> +	clk_disable(drvdata->mi2s_bit_clk[idx]);

[Severity: High]
If hw_params fails early, it might not enable the clock. Will the ALSA core
still call hw_free during stream cleanup? If so, does calling clk_disable()
on a clock that was never enabled cause a reference underflow and kernel
warning? 

Also, since the clock is enabled in both hw_params and trigger(START), but
only disabled in trigger(STOP) and hw_free, could this lead to an unbalanced
enable count that prevents the clock from ever powering down?

> +	return 0;
> +}
> +
> +static int qaif_aif_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> +					 struct snd_pcm_hw_params *params,
> +					 struct snd_soc_dai *dai)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);

[Severity: High]
Is it possible for the CPU DAI to be part of a link with no codecs, such as
certain dummy or hostless links? If rtd->dai_link->num_codecs is 0, will
snd_soc_rtd_to_codec(rtd, 0) read past the end of the rtd->dais array?

> +	const struct qaif_variant *v = drvdata->variant;
> +	struct qaif_aud_intfctl *aif_intfctl = drvdata->aif_intfctl;
> +	const struct qaif_aif_config *aif_intf_cfg = NULL;
[ ... ]
> +	idx = v->get_dma_idx(dai->driver->id);
> +
> +	if (idx < 0) {
> +		dev_err(dai->dev, "%s: Invalid DMA index: %d\n", __func__, idx);
> +		return -EINVAL;
> +	}
> +
> +	aif_intf_cfg = &drvdata->aif_intf_cfg[idx];
> +
> +	if (!aif_intf_cfg) {

[Severity: Low]
Does evaluating the address of the array element &drvdata->aif_intf_cfg[idx]
always result in a non-NULL pointer offset from the parent structure, making
this NULL check structurally impossible to fail?

> +		dev_err(dai->dev, "AIF interface config not found\n");
> +		return -EINVAL;
> +	}
> +	bitwidth = snd_pcm_format_width(format);
> +	if (bitwidth < 0) {
> +		dev_err(dai->dev, "invalid bit width given: %d\n", bitwidth);
> +		return bitwidth;
> +	}
> +
> +	/* SYNC_CFG: write all four sync fields */
> +	ret = regmap_fields_write(aif_intfctl->inv_sync, idx, aif_intf_cfg->invert_sync);

[Severity: High]
Since the hardware supports concurrent, full-duplex playback and capture on the
same DAI physical index, what happens if one stream is actively playing while
the opposite stream is started? Does unconditionally overwriting shared
physical interface registers and resetting the clock rate corrupt the ongoing
audio stream?

> +	if (ret) {
> +		dev_err(dai->dev, "Failed to write inv_sync: %d\n", ret);
> +		return ret;
> +	}
[ ... ]
> +	ret = clk_set_rate(drvdata->mi2s_bit_clk[idx],
> +			   rate * slot_width * aif_intf_cfg->bits_per_lane);
> +	if (ret) {
> +		dev_err(dai->dev, "error setting mi2s bitclk to %u: %d\n",
> +			rate * slot_width * aif_intf_cfg->bits_per_lane, ret);
> +		return ret;
> +	}
> +	dev_dbg(dai->dev, "setting IBIT clock to %u\n",
> +		rate * slot_width * aif_intf_cfg->bits_per_lane);
> +
> +	ret = clk_enable(drvdata->mi2s_bit_clk[idx]);
> +	if (ret) {
> +		dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
> +		return ret;
> +	}
> +	snd_soc_dai_set_tdm_slot(codec_dai, 0x0f, 0b11, aif_intf_cfg->bits_per_lane, slot_width);
> +	snd_soc_dai_set_sysclk(codec_dai, 0, rate * aif_intf_cfg->bits_per_lane * slot_width, 0);

[Severity: High]
Is it standard for a CPU DAI driver to directly configure the Codec DAI using
snd_soc_dai_set_tdm_slot and snd_soc_dai_set_sysclk? This appears to bypass
the machine driver, which is typically responsible for linking and configuring
both endpoints.

Additionally, since the return values of these configuration calls are ignored,
will the CPU DAI proceed incorrectly if the codec rejects these parameters?

> +
> +	return 0;
> +}
> +
> +static int qaif_aif_cpu_daiops_trigger(struct snd_pcm_substream *substream,
> +				       int cmd, struct snd_soc_dai *dai)
> +{
[ ... ]
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		if (aif_intf_cfg->loopback_en)
> +			ret = regmap_fields_write(drvdata->aif_intfctl->enable,
> +						  idx, QAIF_AIF_CTL_ENABLE_ON);
> +		else if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			ret = regmap_fields_write(drvdata->aif_intfctl->enable_tx,
> +						  idx, QAIF_AIF_CTL_ENABLE_ON);
> +		else
> +			ret = regmap_fields_write(drvdata->aif_intfctl->enable_rx,
> +						  idx, QAIF_AIF_CTL_ENABLE_ON);
> +		if (ret)
> +			dev_err(dai->dev, "error writing to AIF CTL reg: %d\n", ret);
> +
> +		ret = clk_enable(drvdata->mi2s_bit_clk[idx]);

[Severity: High]
If the regmap_fields_write() call fails and logs an error to 'ret', does
execution continue to clk_enable()? If clk_enable() succeeds and returns 0,
will it overwrite the previous error code in 'ret', causing the function to
return success while the hardware interface is left disabled?

Also, if the regmap write succeeds but clk_enable() fails, should the hardware
state be rolled back to QAIF_AIF_CTL_ENABLE_OFF before returning the error?

> +		if (ret) {
> +			dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
> +			return ret;
> +		}
> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701105757.2779738-1-harendra.gautam@oss.qualcomm.com?part=5