Re: [PATCH 7/13] 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] Trigger error handling masks hardware enable failures and lacks rollback for clock enable failures.
- [High] Shared AIF enable bit in loopback mode breaks concurrent streams.
- [Medium] The CPU DAI improperly configures the CODEC DAI directly with hardcoded TDM masks, ignoring dynamic configurations.
- [Medium] The MI2S bit clock is left enabled during system suspend and idle periods due to unbalanced clock management.
- [Low] The pointer `aif_intf_cfg` is assigned the address of an inline array element and will never be NULL, rendering the subsequent NULL check useless.
--

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

Add the 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.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> --- a/sound/soc/qcom/qaif-cpu.c
> +++ b/sound/soc/qcom/qaif-cpu.c

[ ... ]

> @@ -495,12 +495,12 @@ static int qaif_aif_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
>  
>  	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) {
>  		dev_err(dai->dev, "AIF interface config not found\n");
>  		return -EINVAL;
>  	}

[Severity: Low]
Does this code assign the address of an inline array element? Since it takes
the address directly from the drvdata structure, won't this pointer never be
NULL, making the following check dead code?

[ ... ]

> @@ -603,17 +603,17 @@ static int qaif_aif_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
>  	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]);

[Severity: Medium]
Since this clock is enabled here and also in the trigger callback for
SNDRV_PCM_TRIGGER_START, does this double enable sequence mean the reference
count stays elevated during system suspend? ALSA does not call hw_free on
suspend, so it seems the clock might physically remain active.

>  	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: Medium]
Is it intentional for a CPU DAI's hw_params to configure the codec DAI
directly with hardcoded values? This seems to force a mismatch by overriding
dynamic configurations like slot_en_tx_mask and tightly couples the CPU
driver to a specific codec.

[ ... ]

> @@ -638,28 +638,28 @@ static int qaif_aif_cpu_daiops_trigger(struct snd_pcm_substream *substream,
>  	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, won't the error code in ret be
overwritten by clk_enable here? Also, if clk_enable fails, it looks like
the function returns an error but leaves the AIF interface active without
rolling back the hardware state.

>  		if (ret) {
>  			dev_err(dai->dev, "error in enabling mi2s bit clk: %d\n", ret);
>  			return ret;
>  		}
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  
>  		if (aif_intf_cfg->loopback_en)
>  			ret = regmap_fields_write(drvdata->aif_intfctl->enable, idx, QAIF_AIF_CTL_ENABLE_OFF);

[Severity: High]
If loopback is enabled and a user stops one substream during full-duplex
operation, won't writing QAIF_AIF_CTL_ENABLE_OFF to the global enable
register disrupt the other running stream? Should this global register state
be reference-counted to support concurrent streams?

>  		else if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  			ret = regmap_fields_write(drvdata->aif_intfctl->enable_tx, idx, QAIF_AIF_CTL_ENABLE_OFF);

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