Re: [PATCH v5 04/10] ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware configuration registers and shared MI2S bit clock rates are reprogrammed on an active interface without concurrency protection across separate DAIs.
- [Medium] Stopping one substream clears the shared loopback enable bit, halting the entire QAIF AIF interface and breaking concurrent active streams.
- [Medium] Device tree parsing of audio format drops crucial inversion properties and enforces hardcoded settings.
--

commit f1f12015dacd26e0ef5a35ee04272fd9e0c2d320
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init

This commit adds the QAIF CPU DAI driver to support Qualcomm audio interface 
paths used for CDC DMA and serial AIF audio links. It implements DAI operations,
configures the MMIO regmap, and parses per-interface configuration from the 
devicetree.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> new file mode 100644
> index 0000000000000..c75b139ecb2ae
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-cpu.c
[ ... ]
> +static int qaif_aif_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> +					 struct snd_pcm_hw_params *params,
> +					 struct snd_soc_dai *dai)
> +{
[ ... ]
> +	/*
> +	 * The sync, lane and frame configuration and the MI2S bit clock are
> +	 * shared by both directions of an AIF. Concurrent playback and
> +	 * capture on the same interface are therefore expected to use a
> +	 * compatible format, rate and slot configuration.
> +	 */
> +	ret = regmap_update_bits(map, QAIF_AUD_INTF_SYNC_CFG_REG(idx),
> +				 QAIF_AIF_SYNC_SRC_BIT |
> +				 QAIF_AIF_SYNC_MODE_MASK |
> +				 QAIF_AIF_SYNC_DELAY_MASK |
> +				 QAIF_AIF_SYNC_INV_BIT,
> +				 (cfg->sync_src ? QAIF_AIF_SYNC_SRC_BIT : 0) |
> +				 (cfg->sync_mode << QAIF_AIF_SYNC_MODE_SHIFT) |
> +				 (cfg->sync_delay << QAIF_AIF_SYNC_DELAY_SHIFT) |
> +				 (cfg->invert_sync ? QAIF_AIF_SYNC_INV_BIT : 0));

[Severity: High]
Since playback and capture paths are separate DAIs that share this physical 
interface, what prevents a concurrent stream startup from unconditionally 
overwriting these shared configuration bits while the other stream is already 
active? Could this cause hardware lockups or audio corruption for the active
stream?

[ ... ]
> +static int qaif_aif_cpu_daiops_trigger(struct snd_pcm_substream *substream,
> +				       int cmd, struct snd_soc_dai *dai)
> +{
[ ... ]
> +	cfg = &drvdata->aif_intf_cfg[idx];
> +	if (cfg->loopback_en)
> +		enable_bit = QAIF_AIF_CTL_ENABLE_BIT;
> +	else if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		enable_bit = QAIF_AIF_CTL_ENABLE_TX_BIT;
> +	else
> +		enable_bit = QAIF_AIF_CTL_ENABLE_RX_BIT;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = regmap_update_bits(drvdata->audio_qaif_map,
> +					 QAIF_AUD_INTF_CTL_REG(idx), enable_bit, enable_bit);
> +		if (ret)
> +			dev_err(dai->dev, "error setting AIF enable bit: %d\n", ret);
> +		return ret;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		ret = regmap_update_bits(drvdata->audio_qaif_map,
> +					 QAIF_AUD_INTF_CTL_REG(idx), enable_bit, 0);

[Severity: Medium]
If both playback and capture streams are active on a loopback-enabled 
interface, wouldn't stopping just one of the streams blindly clear the shared 
QAIF_AIF_CTL_ENABLE_BIT here? It seems this might abruptly halt the entire 
interface and break the remaining concurrent active stream.

[ ... ]
> +static int qaif_aif_cpu_daiops_set_sysclk(struct snd_soc_dai *dai,
> +					  int clk_id, unsigned int freq,
> +					  int dir)
> +{
[ ... ]
> +	/*
> +	 * The bit clock is prepared and enabled for the lifetime of the
> +	 * stream by startup()/shutdown(); only the rate is set here.
> +	 */
> +	return clk_set_rate(drvdata->mi2s_bit_clk[idx], freq);

[Severity: High]
Does this unconditionally set the rate on a shared bit clock that might already 
be actively running for the other stream direction? This appears to be another 
place where concurrent streams across separate DAIs could race and overwrite 
shared hardware state mid-stream.

[ ... ]
> +static int of_qaif_parse_aif_intf_cfg(struct device *dev,
> +				      struct qaif_drv_data *data)
> +{
[ ... ]
> +		switch (snd_soc_daifmt_parse_format(intf_np, NULL) &
> +			SND_SOC_DAIFMT_FORMAT_MASK) {

[Severity: Medium]
Does masking the return value with SND_SOC_DAIFMT_FORMAT_MASK here discard 
crucial inversion properties (like bitclock-inversion or frame-inversion) that 
snd_soc_daifmt_parse_format() normally returns?

> +		case SND_SOC_DAIFMT_I2S:
> +			cfg->sync_mode = QAIF_AIF_SYNC_MODE_LONG;
> +			cfg->sync_delay = QAIF_AIF_SYNC_DELAY_ONE;
> +			cfg->invert_sync = true;
> +			break;

Because of the mask above, it looks like users cannot override this hardcoded 
invert_sync = true behavior for I2S via standard devicetree properties.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-b4-qaif-shikra-audio-v5-0-6d8ad9ba5c9f@oss.qualcomm.com?part=4