Re: [PATCH v8 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] ASoC: qaif-cpu: concurrent MI2S hw_params clobbers shared frame config
- [Medium] ASoC: qaif-cpu: stopping stream in loopback mode breaks concurrent stream
- [Low] ASoC: qaif-cpu: integer underflow in max_register calculation bypasses bounds
--

commit 96b925c5161486efa5953d5523b45fed9502f610
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 for CDC DMA and serial AIF audio links. It implements
CIF and AIF DAI operations, including channel mask programming, dynamic
clock control, and format setup.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> new file mode 100644
> --- /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)
> +{

[ ... ]

> +	if (qaif_is_aif_mi2s_port(dai->driver->id) && !cfg->slot_width) {
> +		slot_width = bitwidth;
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			drvdata->aif_intf_cfg[idx].slot_en_tx_mask =
> +						GENMASK(channels - 1, 0);
> +		else
> +			drvdata->aif_intf_cfg[idx].slot_en_rx_mask =
> +						GENMASK(channels - 1, 0);
> +		drvdata->aif_intf_cfg[idx].slot_num = QAIF_MI2S_SLOTS;
> +	} else {

[ ... ]

> +	ret = regmap_update_bits(map, QAIF_AUD_INTF_FRAME_CFG_REG(idx),
> +				 QAIF_AIF_BITS_PER_LANE_MASK,
> +				 (slot_width * cfg->slot_num) - 1);

[Severity: High]
Does this dynamically derived slot_width risk clobbering the shared frame
configuration register when concurrent playback and capture streams on the
same MI2S port use different formats?

Since ALSA exposes these as separate DAIs without enforcing
symmetric_sample_bits, an active playback stream could be disrupted by a
capture hw_params call that overwrites the physical frame size here.

[ ... ]

> +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_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]
When stopping a stream in loopback mode, does unconditionally clearing this
global enable bit prematurely disable the shared interface?

Loopback requires both directions to run concurrently, so stopping one
stream might cause hardware DMA stalls for the other active stream.

[ ... ]

> +int asoc_qcom_qaif_cpu_platform_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get MMIO resource\n");
> +
> +	regmap_cfg.max_register = resource_size(res) - regmap_cfg.reg_stride;

[Severity: Low]
Can this calculation underflow if a malformed device tree provides a memory
region smaller than regmap_cfg.reg_stride (4 bytes)?

An underflow would wrap max_register to a massive 32-bit unsigned value,
bypassing regmap's bounds checking protections.

> +	drvdata->audio_qaif_map = devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> +							&regmap_cfg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-b4-qaif-shikra-audio-v8-0-ac618057fe85@oss.qualcomm.com?part=4