Re: [PATCH v7 07/10] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: double-free of ALSA DMA buffer on open() error path
- [Low] style: misleading DMA control macro used for config register
--

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

ASoC: qcom: Add QAIF PCM operations

Add the PCM platform callbacks needed to use QAIF DAIs for playback
and capture. Allocates DMA buffers in open(), configures in hw_params()
and prepares, and reports current DMA position.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> index 0000000000000..84ce3ad9a1f2d
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-platform.c

[ ... ]

> @@ -279,25 +279,25 @@ static int qaif_platform_pcmops_open(struct snd_soc_component *component,
> +	/*
> +	 * The DMA buffer/period length registers are programmed in 64-bit
> +	 * (8-byte) words, so constrain buffer and period sizes to that step
> +	 * to keep the ALSA and hardware sizes identical.
> +	 */
> +	ret = snd_pcm_hw_constraint_step(runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> +					 BIT(QAIF_DMA_BYTES_TO_WORDS_SHIFT));
> +	if (ret >= 0)
> +		ret = snd_pcm_hw_constraint_step(runtime, 0,
> +						 SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> +						 BIT(QAIF_DMA_BYTES_TO_WORDS_SHIFT));
> +	if (ret >= 0)
> +		ret = snd_pcm_hw_constraint_integer(runtime,
> +						    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0) {
> +		dev_err(soc_runtime->dev, "setting constraints failed: %d\n", ret);
> +		if (qaif_is_cif_port(dai_id)) {
> +			WRITE_ONCE(drvdata->cif_substream[stream_dma_idx], NULL);
> +			drvdata->cif_dma_heap[stream_dma_idx] = NULL;
> +		} else {
> +			WRITE_ONCE(drvdata->aif_substream[stream_dma_idx], NULL);
> +			drvdata->aif_dma_heap[stream_dma_idx] = NULL;
> +		}
> +		mutex_lock(&drvdata->stream_lock);
> +		v->free_stream_dma_idx(drvdata, stream_dma_idx, dai_id);
> +		mutex_unlock(&drvdata->stream_lock);
> +		goto err_free_dma;
> +	}
> +	return 0;
> +
> +err_free_dma:
> +	dma_free_coherent(component->dev, dma_mem_info->alloc_size,
> +			  dma_mem_info->vaddr, dma_mem_info->dma_addr);

[Severity: High]
Does this error path leave a dangling pointer in substream->dma_buffer.area?

In qaif_platform_pcmops_open(), if setting the hardware constraints fails
(for example, if snd_pcm_hw_constraint_step() encounters an error), the code
jumps to the err_free_dma label to free the DMA memory.

However, earlier in the function, the substream's DMA buffer fields were
already populated and its type was set to SNDRV_DMA_TYPE_DEV. Because this
error path does not reset substream->dma_buffer.area to NULL, ALSA core's
teardown path (snd_pcm_lib_preallocate_free_for_all()) will find the area
pointer non-NULL and attempt to free it again using snd_dma_free_pages(),
leading to a double-free.

Could we reset the substream->dma_buffer area, bytes, and addr fields to
zero or NULL before returning the error?

> +err_free_mem_info:
> +	kfree(dma_mem_info);
> +err_free_data:
> +	kfree(data);
> +	return ret;
> +}

[ ... ]

> @@ -377,9 +377,9 @@ static int qaif_platform_pcmops_hw_params(struct snd_soc_component *component,
> +	ret = regmap_update_bits(drvdata->audio_qaif_map,
> +				 qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACFG_SHRAM_WM_MASK,
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACTL_WM_5);

[Severity: Low]
Is the use of QAIF_DMACTL_WM_5 intentional here?

We're writing to the Configuration (CFG) register and applying the
QAIF_DMACFG_SHRAM_WM_MASK mask, but the value macro QAIF_DMACTL_WM_5 has a
Control (CTL) prefix. While this may evaluate to the correct value for the
watermark, the mismatched prefix could be misleading.

> +	if (ret)
> +		dev_err(soc_runtime->dev, "error updating DMA CFG: %d\n", ret);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-b4-qaif-shikra-audio-v7-0-d5b6f056568e@oss.qualcomm.com?part=7