Re: [PATCH v2 08/11] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Setting ALSA DMA buffer type to SNDRV_DMA_TYPE_CONTINUOUS for dma_alloc_coherent() memory breaks mmap cache coherency.
- [High] qaif_platform_cdc_dma_mmap() uses the wrong device pointer, bypassing IOMMU translation.
- [High] The pointer callback returns negative error codes directly, corrupting the ALSA ring buffer state.
- [High] The close callback frees the DMA buffer and clears substream pointers without synchronizing against in-flight IRQ handlers.
- [Medium] Missing ALSA hardware step constraint for period and buffer bytes causes truncated DMA lengths.
- [Medium] The DMA engine is started before pending interrupts are cleared and enabled, and lacks error rollback.
- [Medium] Unhandled default case for unsupported dai_id in qaif_platform_pcmops_open() leads to leaked memory and uninitialized stream usage.
- [Low] Bitwise accumulation of negative error codes in IRQ configuration helpers corrupts the returned errno.
--

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

ASoC: qcom: Add QAIF PCM operations
    
Adds PCM operation callbacks for the QAIF platform driver so AIF and CIF
DAIs can manage the DMA stream lifecycle.

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

[ ... ]

> @@ -291,3 +291,630 @@ static int qaif_platform_pcmops_open(struct snd_soc_component *component,
>  				     struct snd_pcm_substream *substream)
>  {
[ ... ]
>  	buf = &substream->dma_buffer;
>  	buf->dev.dev = component->dev;
>  	buf->private_data = NULL;
>  	buf->dev.type = SNDRV_DMA_TYPE_CONTINUOUS;
>  
>  	dma_mem_info = qaif_mem_alloc_attach(component,

[Severity: High]
Does setting the buffer type to SNDRV_DMA_TYPE_CONTINUOUS for memory
allocated via dma_alloc_coherent() break mmap cache coherency in the
fallback snd_pcm_lib_default_mmap() handler?

[ ... ]

>  	switch (dai_id) {
>  	case QAIF_MI2S_TDM_AIF0 ... QAIF_MI2S_TDM_AIF12:
[ ... ]
>  	case QAIF_CDC_DMA_RX0 ... QAIF_CDC_DMA_RX9:
[ ... ]
>  		snd_soc_set_runtime_hwparams(substream, &qaif_platform_cif_hardware);
>  		runtime->dma_bytes = qaif_platform_cif_hardware.buffer_bytes_max;
>  		break;
>  	default:
>  		break;
>  	}
>  
>  	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);

[Severity: Medium]
Does this unhandled default case leak the allocated dma_mem_info and leave
the stream data uninitialized instead of returning an error for an
unsupported dai_id?

[ ... ]

>  static int qaif_platform_pcmops_close(struct snd_soc_component *component,
>  				      struct snd_pcm_substream *substream)
>  {
[ ... ]
>  	switch (dai_id) {
>  	case QAIF_MI2S_TDM_AIF0 ... QAIF_MI2S_TDM_AIF12:
>  		drvdata->aif_substream[data->stream_dma_idx] = NULL;
>  		qaif_mem_dealloc_detach(component->dev,
>  					drvdata->aif_dma_heap[data->stream_dma_idx]);
>  		drvdata->aif_dma_heap[data->stream_dma_idx] = NULL;
>  		break;

[Severity: High]
Can this free the DMA buffer and clear the substream pointers while an
in-flight IRQ handler is still executing on another CPU, since there is no
synchronize_irq() or sync_stop callback used here?

[ ... ]

>  static int qaif_platform_pcmops_prepare(struct snd_soc_component *component,
>  					struct snd_pcm_substream *substream)
>  {
[ ... ]
>  	ret = regmap_write(map, QAIF_DMABUFF_REG(v, idx, dir, dai_id),
>  			   (snd_pcm_lib_buffer_bytes(substream) >>
>  			    QAIF_DMA_BYTES_TO_WORDS_SHIFT) - 1);

[Severity: Medium]
Can this bit-shift truncate the DMA length if userspace requests a buffer
size that isn't a multiple of 8 bytes, since there is no
snd_pcm_hw_constraint_step() enforced in qaif_platform_pcmops_open()?

[ ... ]

>  static int qaif_platform_irq_enable(struct qaif_drv_data *drvdata,
>  				    int dir,
>  				    enum qaif_irq_type irq_type,
>  				    int idx)
>  {
>  	int ret = 0;
[ ... ]
>  	if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>  		ret |= regmap_write_bits(map,
>  					 QAIF_EE_RDDMA_PERIOD_IRQ_EN_REG(v, irq_type),
>  					 val_irqen, val_irqen);
>  		ret |= regmap_write_bits(map,
>  					 QAIF_EE_RDDMA_UNDERFLOW_IRQ_EN_REG(v, irq_type),
>  					 val_irqen, val_irqen);

[Severity: Low]
Does accumulating negative error codes with a bitwise OR corrupt the
returned errno?

[ ... ]

>  static int qaif_platform_pcmops_trigger(struct snd_soc_component *component,
>  					struct snd_pcm_substream *substream,
>  					int cmd)
>  {
[ ... ]
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		ret = regmap_fields_write(dmactl->dma_dyncclk, idx, QAIF_DMACTL_DYNCLK_ON);
>  		if (ret) {
[ ... ]
>  		}
>  		ret = regmap_fields_write(dmactl->enable, idx, QAIF_DMACTL_ENABLE_ON);
>  		if (ret) {
[ ... ]
>  		}
>  		switch (dai_id) {
>  		case QAIF_MI2S_TDM_AIF0 ... QAIF_MI2S_TDM_AIF12:
>  			ret = qaif_platform_irq_clear(drvdata,
>  						      substream->stream,
>  						      QAIF_AIF_IRQ, idx);

[Severity: Medium]
Does enabling the DMA engine before clearing and enabling interrupts open a
race window where the first period interrupt can be lost? Also, if the IRQ
setup fails, should the error path roll back the DMA engine state?

[ ... ]

>  static snd_pcm_uframes_t qaif_platform_pcmops_pointer(struct snd_soc_component *component,
>  						      struct snd_pcm_substream *substream)
>  {
[ ... ]
>  	ret = regmap_read(map,
>  			  QAIF_DMABASE_REG(v, idx, dir, dai_id),
>  			  &base_addr);
>  	if (ret) {
>  		dev_err(soc_runtime->dev,
>  			"error reading from rdmabase reg: %d\n", ret);
>  		return ret;
>  	}

[Severity: High]
Can returning a negative error code from regmap_read() as an unsigned
snd_pcm_uframes_t corrupt the ALSA ring buffer state?

[ ... ]

>  static int qaif_platform_cdc_dma_mmap(struct snd_pcm_substream *substream,
>  				      struct vm_area_struct *vma)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
>  	return dma_mmap_coherent(substream->pcm->card->dev, vma,
>  				 runtime->dma_area, runtime->dma_addr,
>  				 runtime->dma_bytes);
>  }

[Severity: High]
Should this use the component device that was used during allocation instead
of the card device to avoid bypassing IOMMU translation?

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