Re: [PATCH v3 7/9] ASoC: qcom: Add QAIF IRQ handling, suspend/resume and platform register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardcoded cell count for 'iommus' property parsing ignores `#iommu-cells`.
- [High] Incomplete hardware context restore during system resume.
- [Medium] Early return in SNDRV_PCM_TRIGGER_STOP leaves interrupts enabled on failure.
--

--- Patch [7]: [PATCH v3 7/9] ASoC: qcom: Add QAIF IRQ handling, suspend/resume and platform register ---
commit e0106aaa99b0a121b196914494fc9a99b9c5df66
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

    ASoC: qcom: Add QAIF IRQ handling, suspend/resume and platform register
    
    Register the ASoC component, request the QAIF interrupt and add trigger
    handling to enable or disable DMA and its interrupt sources. Dispatch the
    top-level IRQ status to the AIF and CIF DMA handlers, report period elapsed
    events to ALSA, warn on xrun conditions and stop the stream on bus errors.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> index 4b4647641d113..2e5dc5b081cd1 100644
> --- a/sound/soc/qcom/qaif-platform.c
> +++ b/sound/soc/qcom/qaif-platform.c
> @@ -542,3 +542,394 @@ static int qaif_platform_copy(struct snd_soc_component *component,

[ ... ]

> +static int qaif_platform_pcmops_trigger(struct snd_soc_component *component,
> +					struct snd_pcm_substream *substream,
> +					int cmd)
> +{
> +	struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct qaif_pcm_data *data = substream->runtime->private_data;
> +	unsigned int dai_id = cpu_dai->driver->id;
> +	enum qaif_irq_type irq_type = qaif_is_cif_dma_port(dai_id) ? QAIF_CIF_IRQ : QAIF_AIF_IRQ;
> +	struct regmap *map = drvdata->audio_qaif_map;
> +	int idx = data->dma_reg_idx;
> +	int ret;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> +					   idx, QAIF_IRQ_CLEAR);

[Severity: High]
Does this code fully restore the hardware context during a system resume?

If power is lost during suspend, the hardware registers might be zeroed. It
looks like the trigger path handles SNDRV_PCM_TRIGGER_RESUME by enabling
the DMA control bit and interrupts, but without reprogramming the lost DMA
configuration such as base address, buffer limits, and period lengths.

Could this cause the hardware to fetch from unmapped memory and trigger
IOMMU faults or memory corruption if we resume a paused or active stream?

> +		if (ret) {
> +			dev_err(soc_runtime->dev, "error clearing irq regs: %d\n", ret);
> +			return ret;
> +		}
> +		ret = regmap_update_bits(map,
> +					 qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> +					 QAIF_DMACFG_DYNCLK_BIT, QAIF_DMACFG_DYNCLK_BIT);
> +		if (ret) {
> +			dev_err(soc_runtime->dev, "error enabling dma_dynclk: %d\n", ret);
> +			return ret;
> +		}
> +		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;
> +	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: Medium]
Can this early return lead to an interrupt storm?

If regmap_update_bits() fails during SNDRV_PCM_TRIGGER_STOP, this skips
the subsequent qaif_platform_irq_op() call that disables the interrupts.

Even if disabling DMA fails, wouldn't we still want to ensure interrupts
are disabled during the teardown sequence?

> +		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);
> +		if (ret)
> +			dev_err(soc_runtime->dev, "error disabling irq regs: %d\n", ret);
> +		break;

[ ... ]

> +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;
> +
> +	if (!drvdata || !drvdata->variant)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "Invalid drvdata or variant\n");
> +
> +	ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +					       "iommus", 2, 0,
> +					       &iommu_spec);

[Severity: High]
Does this code correctly handle generic IOMMU bindings?

The driver uses a fixed cell count of 2 to parse the "iommus" property.
If the target IOMMU configuration on a platform uses a different
#iommu-cells value (e.g., 1), won't this cause parsing to fail during
driver probe?

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to parse iommus property\n");
> +	sid = iommu_spec.args[0];
> +	of_node_put(iommu_spec.np);
> +	drvdata->smmu_csid_bits = sid & QAIF_CSID_MASK;

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