Re: [PATCH v3 6/9] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `.pointer` callback can underflow and return garbage if called before the hardware DMA is started.
- [High] High-order contiguous DMA memory allocation in `.open()` is susceptible to runtime fragmentation failures.
- [High] Double free of DMA buffer memory due to orphaned `substream->dma_buffer` pointer.
- [Medium] Using `GENMASK` with an upper bound of `-1` for variants with zero channels causes undefined behavior.
- [Low] A DMA Control (DMACTL) macro is written to the DMA Configuration (DMACFG) register.
--

--- Patch [6]: [PATCH v3 6/9] ASoC: qcom: Add QAIF PCM operations ---
commit 7b414074bdf8efca30967e8f81371a7469f1980b
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>
Subject: ASoC: qcom: Add QAIF PCM operations

This commit adds the ALSA SoC PCM platform driver for the Qualcomm Audio
Interface (QAIF). It allocates and maps DMA buffers, initializes the QAIF
hardware, and handles the stream operations like open, prepare, pointer,
and close. It also supports mmap and custom copy operations for transferring
data.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> index 0000000000000..4b4647641d113
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-platform.c
[ ... ]
> +static int qaif_map_ee_resource(struct qaif_drv_data *drvdata)
> +{
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct regmap *map = drvdata->audio_qaif_map;
> +	int ret;
> +
> +	ret = regmap_write(map, QAIF_EE_RDDMA_MAP_REG(v), GENMASK(v->num_rddma - 1, 0));

[Severity: Medium]
What happens if a variant has num_rddma or num_wrdma equal to 0?

Using GENMASK(v->num_rddma - 1, 0) without guarding against zero counts causes
undefined behavior, potentially leading to a negative bitwise shift and
garbage mask values.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int qaif_platform_pcmops_open(struct snd_soc_component *component,
> +				     struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	dma_mem_info->alloc_size = qaif_platform_hardware.buffer_bytes_max;
> +	dma_mem_info->vaddr = dma_alloc_coherent(component->dev,
> +						 dma_mem_info->alloc_size,
> +						 &dma_mem_info->dma_addr,
> +						 GFP_KERNEL);

[Severity: High]
Is it safe to dynamically allocate an order-5 contiguous block (~122 KB) via
dma_alloc_coherent() each time the PCM stream is opened?

Over time, memory fragmentation could make this allocation highly unreliable,
causing random stream failures on systems with high uptime. Should this use
preallocated managed buffers in probe instead?

> +	if (!dma_mem_info->vaddr) {
> +		ret = -ENOMEM;
> +		goto err_free_mem_info;
> +	}
[ ... ]
> +	buf->bytes = qaif_platform_hardware.buffer_bytes_max;
> +	buf->addr = dma_mem_info->dma_addr;
> +	buf->area = (unsigned char *)dma_mem_info->vaddr;

[Severity: High]
Does this create a double-free risk?

The open callback stores dynamically allocated memory directly into the
core's substream->dma_buffer.area. The close callback frees this memory
using dma_free_coherent() but fails to set substream->dma_buffer.area
to NULL.

If the driver is unloaded or unbound, the ALSA core's teardown path
snd_pcm_lib_preallocate_free_for_all() could blindly call
snd_dma_free_pages() on this non-NULL area pointer.

> +
> +	if (qaif_is_cif_dma_port(dai_id)) {
> +		WRITE_ONCE(drvdata->cif_substream[stream_dma_idx], substream);
[ ... ]
> +static int qaif_platform_pcmops_close(struct snd_soc_component *component,
> +				      struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	if (dma_mem_info) {
> +		dma_free_coherent(component->dev, dma_mem_info->alloc_size,
> +				  dma_mem_info->vaddr, dma_mem_info->dma_addr);
> +		kfree(dma_mem_info);
> +	}
> +
> +	snd_pcm_set_runtime_buffer(substream, NULL);
> +	substream->runtime->private_data = NULL;
> +
> +	kfree(data);
> +	return 0;
> +}
[ ... ]
> +static int qaif_platform_pcmops_hw_params(struct snd_soc_component *component,
> +					  struct snd_pcm_substream *substream,
> +					  struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	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 this using the wrong macro prefix?

QAIF_DMACTL_WM_5 is used to populate the QAIF_DMACFG_SHRAM_WM_MASK bitfield.
The prefix is DMACTL instead of DMACFG, which creates a naming mismatch here.

> +	if (ret)
> +		dev_err(soc_runtime->dev, "error updating DMA CFG: %d\n", ret);
> +	return ret;
> +}
[ ... ]
> +static snd_pcm_uframes_t qaif_platform_pcmops_pointer(struct snd_soc_component *component,
> +						      struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	return bytes_to_frames(substream->runtime,
> +			       (curr_addr - base_addr) %
> +			       snd_pcm_lib_buffer_bytes(substream));

[Severity: High]
Could this underflow if ALSA queries the pointer before the DMA is running
and curr_addr reads as 0?

Since base_addr is the physical IOVA, 0 - base_addr underflows, and modulo
buffer_bytes yields a massive random offset which causes immediate XRUN errors.

> +}

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