Re: [PATCH v2 2/4] ASoC: qcom: qdsp6: Zero-initialize AudioReach module config

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing dma_rmb() between reads of frame_counter and index in q6apm_dai_pointer().
- [Medium] Memory leak of dynamically allocated fragment buffers on error paths.
--

--- Patch [2]: [PATCH v2 2/4] ASoC: qcom: qdsp6: Zero-initialize AudioReach module config ---
commit 8c62450bf2680c8f23f31e1b33efb46a0ed00f8a
Author: Prasad Kumpatla <[email protected]>

ASoC: qcom: qdsp6: Zero-initialize AudioReach module config

q6apm_dai_prepare() and q6apm_dai_compr_set_params() build an AudioReach
module config on the stack. Initialize the whole config structure so omitted
fields reliably retain their zero/default meaning.

Link: https://sashiko.dev/#/patchset/[email protected]

> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index bf1f872a09f45..033197bdd8705 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
> @@ -216,7 +216,7 @@ static int q6apm_dai_prepare(struct snd_soc_component *component,
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct q6apm_dai_rtd *prtd = runtime->private_data;
> -	struct audioreach_module_config cfg;
> +	struct audioreach_module_config cfg = {};

[Severity: Medium]
This is a pre-existing issue, but while looking at q6apm_dai_prepare() in
sound/soc/qcom/qdsp6/q6apm-dai.c, could there be a memory leak of the
dynamically allocated fragment buffers on the error paths?

If a failure occurs during setup, such as q6apm_graph_media_format_pcm()
returning an error:

	ret = q6apm_alloc_fragments(prtd->graph, substream->stream, prtd->phys,
				(prtd->pcm_size / prtd->periods), prtd->periods);
...
	ret = q6apm_graph_media_format_pcm(prtd->graph, &cfg);
	if (ret < 0) {
		dev_err(dev, "%s: CMD Format block failed\n", __func__);
		return ret;
	}

The function returns without setting prtd->state to Q6APM_STREAM_RUNNING.

Later, when ALSA cleans up and calls q6apm_dai_close(), the teardown
logic skips the cleanup of the buffers because prtd->state is 0:

	if (prtd->state) {
		/* only stop graph that is started */
		q6apm_graph_stop(prtd->graph);
		q6apm_free_fragments(prtd->graph, substream->stream);
	}

	q6apm_graph_close(prtd->graph);

This frees the graph structure itself, losing the pointers to the buffers
and permanently leaking the memory.

Should the error paths in q6apm_dai_prepare() clean up the allocated
fragments, or should q6apm_dai_close() be adjusted to free them even if
the stream isn't fully running?

>  	struct device *dev = component->dev;
>  	struct q6apm_dai_data *pdata;
>  	int ret;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the changes
in sound/soc/qcom/qdsp6/q6apm-dai.c, I noticed a potential issue in
q6apm_dai_pointer(). Are read memory barriers needed in the retry loop
when reading from shared DMA memory?

q6apm_dai_pointer() contains this loop:

		/* index is valid if frame_counter does not change while reading. */
		do {
			fc1 = READ_ONCE(prtd->pos_buffer->frame_counter);
			index = READ_ONCE(prtd->pos_buffer->index);
			fc2 = READ_ONCE(prtd->pos_buffer->frame_counter);
		} while (fc1 != fc2 && --retries);

On weakly-ordered architectures like ARM64, is the CPU permitted to reorder
these reads from the coherent DMA memory? If index is read before fc1, or
fc2 is read before index, might the validation fail to detect a concurrent
update by the DSP, leading to an incorrect hardware pointer snapshot being
returned?

Would adding dma_rmb() between these reads ensure the correct ordering?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2