Re: [PATCH v9 08/10] ASoC: qcom: Add QAIF IRQ handling and platform register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: qcom: qaif: uninitialized stack memory read of iommu_spec.args[0]
- [Medium] ASoC: qcom: qaif: missing error rollback in trigger() leaves DMA running
- [Low] ASoC: qcom: qaif: error code masking in sync_stop() drops timeout error
--

commit 76d65e08a800bae1b501d23afbe5c442b2a5c8cc
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF IRQ handling and platform register
    
This commit finishes the QAIF PCM platform support by allowing streams to be
registered, started, stopped, and serviced by DMA interrupts. It registers
the ASoC component, requests the QAIF interrupt, and adds trigger handling
to manage DMA and its interrupt sources.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> index e24c4252825e2..facba3be2d8ea 100644
> --- a/sound/soc/qcom/qaif-platform.c
> +++ b/sound/soc/qcom/qaif-platform.c
[ ... ]
> @@ -462,3 +470,416 @@ static int qaif_platform_pcm_new(struct snd_soc_component *component,
[ ... ]
> +static int qaif_platform_pcmops_trigger(struct snd_soc_component *component,
> +					struct snd_pcm_substream *substream,
> +					int cmd)
> +{
[ ... ]
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
[ ... ]
> +		ret = regmap_update_bits(map,
> +					 qaif_dmactl_reg(v, idx, substream->stream, dai_id),
> +					 QAIF_DMACTL_ENABLE_BIT, QAIF_DMACTL_ENABLE_BIT);
> +		if (ret) {
> +			dev_err(soc_runtime->dev, "error enabling DMA: %d\n", ret);
> +			break;
> +		}
> +		irq_op_err = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> +						  idx, QAIF_IRQ_ENABLE);
> +		if (irq_op_err)
> +			dev_err(soc_runtime->dev, "error enabling irq regs: %d\n", irq_op_err);
> +		break;

[Severity: Medium]
Does this error path leave the DMA hardware and dynamic clock running
indefinitely?

If qaif_platform_irq_op() fails to enable the IRQ registers, the switch
breaks and an error is returned, but there is no rollback to disable the
dynamic clock and DMA control bits. Since ALSA core does not automatically
issue a STOP trigger if START fails, the hardware might remain active.

[ ... ]
> +static int qaif_platform_pcmops_sync_stop(struct snd_soc_component *component,
> +					  struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	/*
> +	 * Wait for the DMA channel to report shutdown complete before
> +	 * clearing its interrupts, so a fast STOP->START or suspend/resume
> +	 * does not race a channel still shutting down.
> +	 */
> +	ret = regmap_read_poll_timeout(map,
> +				       qaif_dma_shutdown_stat_reg(v, idx,
> +								  substream->stream,
> +								  dai_id),
> +				       val,
> +				       !(val & QAIF_DMA_SHUTDOWN_IN_PROGRESS_BIT),
> +				       QAIF_DMA_SHUTDOWN_POLL_US,
> +				       QAIF_DMA_SHUTDOWN_TIMEOUT_US);
> +	if (ret)
> +		dev_err(soc_runtime->dev, "timeout waiting for DMA shutdown: %d\n", ret);
> +
> +	synchronize_irq(drvdata->audio_qaif_irq);
> +	ret = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> +				   idx, QAIF_IRQ_CLEAR);
> +	if (ret)
> +		dev_err(soc_runtime->dev, "error clearing irq regs: %d\n", ret);
> +	return ret;

[Severity: Low]
Is the error code from regmap_read_poll_timeout() intentionally overwritten
here?

If the DMA shutdown times out, ret is set to -ETIMEDOUT. However, the
subsequent call to qaif_platform_irq_op() overwrites ret with its own
success status (0). This causes the timeout error to be silently discarded
and reported as a success to the ALSA core.

[ ... ]
> +int asoc_qcom_qaif_platform_register(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +	struct of_phandle_args iommu_spec;
> +	u32 sid;
> +	int ret;
[ ... ]
> +	ret = of_parse_phandle_with_args(pdev->dev.of_node,
> +					 "iommus", "#iommu-cells", 0,
> +					 &iommu_spec);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to parse iommus property\n");
> +	sid = iommu_spec.args[0];

[Severity: High]
Is it possible for iommu_spec.args[0] to contain uninitialized stack data
here?

If the target iommus node specifies #iommu-cells = <0>,
of_parse_phandle_with_args() successfully sets args_count = 0 but leaves
the args array uninitialized. Unconditionally reading args[0] could lead to
configuring the SMMU stream ID with random kernel stack memory.

> +	of_node_put(iommu_spec.np);
> +	drvdata->smmu_csid_bits = sid & QAIF_CSID_MASK;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-qaif-shikra-audio-v9-0-f3022e28f2fd@oss.qualcomm.com?part=8