Re: [PATCH 4/7] dmaengine: mmp_tdma: synchronize tasklet when freeing a channel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: mmp_tdma: shared IRQ race in mmp_tdma_free_chan_resources()
- [High] dmaengine: mmp_tdma: unclocked MMIO access in mmp_tdma_free_chan_resources()
--

commit 6a728febb9516f46ef428e03caafe8fc8e1dbec8
Author: Jiale Yao <yaojiale02@163.com>

dmaengine: mmp_tdma: synchronize tasklet when freeing a channel

This commit disables the DMA channel and kills the completion tasklet in
mmp_tdma_free_chan_resources() before freeing the descriptor storage.

> diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
> index ba03321eeff79..e0de0967d7e70 100644
> --- a/drivers/dma/mmp_tdma.c
> +++ b/drivers/dma/mmp_tdma.c
> @@ -399,8 +399,10 @@ static void mmp_tdma_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct mmp_tdma_chan *tdmac = to_mmp_tdma_chan(chan);
>  
> +	mmp_tdma_disable_chan(chan);

[Severity: High]
Can calling mmp_tdma_disable_chan() unconditionally here access TDCR while
the MMP2 Audio island clock and power domain are disabled?

In drivers/dma/mmp_tdma.c, mmp_tdma_disable_chan() performs MMIO accesses on
TDCR:

drivers/dma/mmp_tdma.c:mmp_tdma_disable_chan() {
	tdcr = readl(tdmac->reg_base + TDCR);
	tdcr |= TDCR_ABR;
	tdcr &= ~TDCR_CHANEN;
	writel(tdcr, tdmac->reg_base + TDCR);
}

On MMP2 (arch/arm/boot/dts/marvell/mmp2.dtsi), the adma0 and adma1
controllers reside in the Audio subsystem power island alongside sspa0/sspa1.
The mmp_tdma driver does not manage clocks or power domains itself; its
consumer driver in sound/soc/pxa/mmp-sspa.c controls sspa->audio_clk and the
runtime PM domain.

In sound/soc/pxa/mmp-sspa.c, asoc_mmp_sspa_probe() registers dmaengine PCM
(which requests the DMA channels) before enabling runtime PM and
sspa->audio_clk:

sound/soc/pxa/mmp-sspa.c:asoc_mmp_sspa_probe() {
	if (pdev->dev.of_node) {
		ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
						      &mmp_pcm_config, 0);
		if (ret)
			return ret;
	}

	ret = devm_snd_soc_register_component(&pdev->dev, &mmp_sspa_component,
					      &mmp_sspa_dai, 1);
	if (ret)
		return ret;

	pm_runtime_enable(&pdev->dev);
	clk_prepare_enable(sspa->audio_clk);
}

And on driver unbind, asoc_mmp_sspa_remove() disables sspa->audio_clk and
runtime PM before devres releases the DMA channels:

sound/soc/pxa/mmp-sspa.c:asoc_mmp_sspa_remove() {
	clk_disable_unprepare(sspa->audio_clk);
	pm_runtime_disable(&pdev->dev);
}

When devres subsequently runs snd_dmaengine_pcm_unregister() ->
dmaengine_pcm_release_chan() -> dma_release_channel() ->
mmp_tdma_free_chan_resources() (either after asoc_mmp_sspa_remove() or if
devm_snd_soc_register_component() fails during probe), won't
mmp_tdma_disable_chan() read and write TDCR while the Audio island is powered
off and unclocked, causing an external abort or hang?

>  	if (tdmac->irq)
>  		devm_free_irq(tdmac->dev, tdmac->irq, tdmac);
> +	tasklet_kill(&tdmac->tasklet);

[Severity: High]
Does this prevent tdmac->tasklet from being scheduled again when the
controller uses a shared interrupt (tdmac->irq == 0)?

In mmp_tdma_probe(), when irq_num != chan_num, a single shared handler
mmp_tdma_int_handler() is registered on tdev and each channel has
tdmac->irq = 0:

drivers/dma/mmp_tdma.c:mmp_tdma_probe() {
	if (irq_num != chan_num) {
		irq = platform_get_irq(pdev, 0);
		ret = devm_request_irq(&pdev->dev, irq,
			mmp_tdma_int_handler, IRQF_SHARED, "tdma", tdev);
		if (ret)
			return ret;
	}

	/* initialize channel parameters */
	for (i = 0; i < chan_num; i++) {
		irq = (irq_num != chan_num) ? 0 : platform_get_irq(pdev, i);
		ret = mmp_tdma_chan_init(tdev, i, irq, type, pool);
}

When mmp_tdma_free_chan_resources() runs on such a controller:

1. Unlike mmp_tdma_terminate_all(), which calls both mmp_tdma_disable_chan()
   and mmp_tdma_enable_irq(tdmac, false), mmp_tdma_free_chan_resources() only
   calls mmp_tdma_disable_chan(), which updates TDCR without masking TDIMR or
   clearing a latched TDISR_COMP bit in TDISR.

2. Because tdmac->irq is 0, devm_free_irq() is skipped and nothing
   synchronizes with a concurrently running mmp_tdma_int_handler() ->
   mmp_tdma_chan_handler() before tasklet_kill(&tdmac->tasklet) is called.

If mmp_tdma_int_handler() -> mmp_tdma_chan_handler() is already executing on
another CPU, or if TDISR_COMP was latched before TDCR_CHANEN was cleared and
the shared interrupt fires (for instance from the sibling channel in
tdev->tdmac[]), mmp_tdma_chan_handler() will still schedule tdmac->tasklet
after tasklet_kill() and mmp_tdma_free_descriptor() have completed:

drivers/dma/mmp_tdma.c:mmp_tdma_chan_handler() {
	if (mmp_tdma_clear_chan_irq(tdmac) == 0) {
		tasklet_schedule(&tdmac->tasklet);
		return IRQ_HANDLED;
	} else
		return IRQ_NONE;
}

Wouldn't that allow dma_do_tasklet() to invoke the client callback after
mmp_tdma_free_chan_resources() returns and dereference a NULL tdmac->desc_arr
if the callback queries mmp_tdma_tx_status() -> mmp_tdma_get_pos()?

>  	mmp_tdma_free_descriptor(tdmac);
>  	return;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018040724.534641-1-yaojiale02@163.com?part=4