Re: [PATCH v3 3/3] ASoC: qcom: q6apm-dai: add SCM buffer assignment for mDSP platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: qcom: missing SCM memory assignment for compressed streams
- [Medium] ASoC: qcom: PCM buffer allocation exceeds reserved memory limits
--

commit 649b8a689f5d756ca2e792c97278213430b739cd
Author: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>

ASoC: qcom: q6apm-dai: add SCM buffer assignment for mDSP platforms

This commit introduces SCM buffer assignment to grant the modem DSP (mDSP)
access to audio memory regions. It adds logic to dynamically assign the
control-path carveout and per-stream data-path buffers on platforms where
the DSP runs in a stage-2 protected context.

> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index 033197bdd8705..8fa8ef774c4bc 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
[ ... ]
> @@ -603,6 +708,17 @@ static int q6apm_dai_pcm_new(struct snd_soc_component *component, struct snd_soc
>  		if (ret)
>  			return ret;
>  
> +		if (pdata->use_scm_assign) {
> +			ret = q6apm_dai_scm_assign(pdata,
> +						   substream->dma_buffer.addr,
> +						   ALIGN(size, PAGE_SIZE));

[Severity: High]
While SCM assignment is properly handled for PCM streams here, is a similar
call to q6apm_dai_scm_assign missing for compressed audio streams?

In q6apm_dai_compr_open, the driver allocates a DMA buffer for the compress
stream without calling the SCM assignment function:

	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size,
				  &prtd->dma_buffer);

If this buffer isn't assigned before exposing it to the hardware, wouldn't
this cause a stage-2 protection fault when the modem DSP attempts to read
the compressed audio stream on mDSP platforms?

> +			if (ret) {
> +				dev_err(component->dev,
> +					"SCM assign buffer failed: %d\n", ret);
> +				return ret;
> +			}
> +		}
> +
>  		ret = q6apm_dai_memory_map(component, substream, graph_id, is_push_pull);
[ ... ]
> @@ -1029,12 +1157,104 @@ static int q6apm_dai_probe(struct platform_device *pdev)
[ ... ]
> +			rc = devm_add_action_or_reset(dev,
> +						      q6apm_dai_reserved_mem_release,
> +						      dev);
> +			if (rc)
> +				return rc;
> +
> +			pdata->reserved_buf_size =
> +				min_t(size_t, per_stream - POS_BUFFER_BYTES,
> +				      BUFFER_BYTES_MAX);

[Severity: Medium]
Does this calculation fail to account for the additional PAGE_SIZE padding
that is unconditionally added during stream allocation?

In q6apm_dai_pcm_new, the final allocation size includes PAGE_SIZE when the
reserved memory pool is present:

	if (pdata->has_reserved_mem)
		size = pdata->reserved_buf_size + PAGE_SIZE;

If the reserved pool is tightly sized based on the per_stream limits,
wouldn't adding PAGE_SIZE to each stream cause the total allocation to
exceed the memory pool budget? Could this lead to an allocation failure
when opening the maximum allowed number of concurrent streams?

> +			pdata->has_reserved_mem = true;
> +		}
> +	}
> +
>  	dev_set_drvdata(dev, pdata);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-vmid-v3-v3-0-f1cbf47bf173@oss.qualcomm.com?part=3