Re: [PATCH v6 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] Access of uninitialized stack memory `iommu_spec.args[0]` if the parsed device tree node specifies `#iommu-cells = <0>`.
- [High] Improper error handling and missing resource teardown on failure paths in `qaif_platform_pcmops_trigger` and `qaif_platform_irq_op`.
- [Medium] GENMASK underflow due to unchecked zero values for DMA stream counts.
--

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

ASoC: qcom: Add QAIF IRQ handling and platform register

Finish the QAIF PCM platform support so streams can be registered, started,
stopped and serviced by DMA interrupts.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> index 84ce3ad9a1f2d..762ba3eb65753 100644
> --- a/sound/soc/qcom/qaif-platform.c
> +++ b/sound/soc/qcom/qaif-platform.c
> @@ -537,3 +537,395 @@ static int qaif_platform_copy(struct snd_soc_component *component,

[ ... ]

> +static int qaif_platform_irq_op(struct qaif_drv_data *drvdata, int dir,
> +				enum qaif_irq_type irq_type, int idx,
> +				enum qaif_irq_op op)
> +{

[ ... ]

> +	ret = regmap_write_bits(map, per_reg, bit, val);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write_bits(map, xrun_reg, bit, val);
> +	if (ret)
> +		return ret;

[Severity: High]
Does returning early here leave the error response interrupts enabled?

If regmap_write_bits() fails for xrun_reg, it returns immediately and skips
writing to err_reg, which could leave the hardware in an inconsistent state.

> +	return regmap_write_bits(map, err_reg, bit, val);
> +}
> +
> +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);
> +			return ret;
> +		}
> +		ret = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> +					   idx, QAIF_IRQ_ENABLE);
> +		if (ret)
> +			dev_err(soc_runtime->dev, "error enabling irq regs: %d\n", ret);
> +		break;

[Severity: High]
If qaif_platform_irq_op() fails, this breaks out without rolling back the
DMA enable bit. Can this leave the DMA engine running without interrupts?

> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		ret = regmap_update_bits(map,
> +					 qaif_dmactl_reg(v, idx, substream->stream, dai_id),
> +					 QAIF_DMACTL_ENABLE_BIT, 0);
> +		if (ret) {
> +			dev_err(soc_runtime->dev, "error disabling DMA: %d\n", ret);
> +			return ret;
> +		}

[Severity: High]
Does this early return leave the dynamic clock and interrupts enabled if
the DMA disable step fails?

> +		ret = regmap_update_bits(map,
> +					 qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> +					 QAIF_DMACFG_DYNCLK_BIT, 0);
> +		if (ret)
> +			dev_err(soc_runtime->dev, "error disabling dma_dynclk: %d\n", ret);
> +		ret = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> +					   idx, QAIF_IRQ_DISABLE);

[Severity: High]
If disabling the dynamic clock fails, is the error code in 'ret' masked by
the subsequent assignment from qaif_platform_irq_op()?

> +		if (ret)
> +			dev_err(soc_runtime->dev, "error disabling irq regs: %d\n", ret);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
> +}

[ ... ]

> +int asoc_qcom_qaif_platform_register(struct platform_device *pdev)
> +{

[ ... ]

> +	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]
Does this code access uninitialized stack memory?

If the parsed device tree node specifies #iommu-cells = <0>,
of_parse_phandle_with_args() can succeed with args_count = 0 but does not
populate the args array. This would leave iommu_spec.args[0] uninitialized
when used as the SMMU Stream ID.

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

[ ... ]

Outside of the provided diff, in qaif_map_ee_resource():

[Severity: Medium]
Is there a risk of a GENMASK() underflow when mapping resources?

If a variant specifies a DMA count like num_rddma as 0,
GENMASK(v->num_rddma - 1, 0) is evaluated. This creates a negative high
bit index parameter, leading to undefined behavior.

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