Re: [PATCH] dmaengine: mmp_pdma: fix wrong extended DRCMR base for SpacemiT K3

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The interrupt handler `mmp_pdma_chan_handler` dereferences `phy->vchan` without checking if it is NULL, leading to a kernel panic.
- [High] Disabling a DMA channel by clearing all `run_bits` simultaneously drops 64-bit addressing capabilities, potentially corrupting DMA transfers while the hardware halts.
- [High] The driver destroys the DMA descriptor pool without synchronously killing the channel's tasklet, allowing a running tasklet to access freed resources.
--

commit 8ee69ab96e073f81f565ee82dce7689d911272ac
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>

dmaengine: mmp_pdma: fix wrong extended DRCMR base for SpacemiT K3

K3 PDMA shares the same DRCMR layout as K1, where the extended DRCMR
base address is 0x1100. Commit 6587b8661a0b ("dmaengine: mmp_pdma: add
SpacemiT K3 support") incorrectly defined DRCMR_EXT_BASE_K3 as 0x1000,
causing all DRCMR accesses for channels >= 64 to be off by 0x100. Drop the
bogus DRCMR_EXT_BASE_K3 macro and reuse DRCMR_EXT_BASE_DEFAULT for the K3 ops.

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..78e3e07e681df 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c

[ ... ]

> @@ -1219,7 +1218,7 @@ static const struct mmp_pdma_ops spacemit_k3_pdma_ops = {
>  	.get_desc_dst_addr = get_desc_dst_addr_64,
>  	.run_bits = (DCSR_RUN | DCSR_LPAEEN | DCSR_EORIRQEN | DCSR_EORSTOPEN),

[Severity: High]
This is a pre-existing issue, but can clearing all run_bits simultaneously
corrupt DMA transfers?

In disable_chan(), the driver stops the DMA by clearing pdev->ops->run_bits
from the DCSR register:

drivers/dma/mmp_pdma.c:disable_chan() {
    ...
	if (phy->vchan) {
		struct mmp_pdma_device *pdev;

		pdev = to_mmp_pdma_dev(phy->vchan->chan.device);
		writel(dcsr & ~pdev->ops->run_bits, phy->base + reg);
    ...
}

Since a DMA channel does not halt instantaneously when the RUN bit is
cleared, simultaneously clearing DCSR_LPAEEN strips the upper 32 bits from
the DMA addresses of any remaining in-flight bursts.

This causes the final bursts to overwrite unintended memory in the lower 4GB.

Should only DCSR_RUN be toggled when stopping a channel?

>  	.dma_width = 64,
> -	.drcmr_ext_base = DRCMR_EXT_BASE_K3,
> +	.drcmr_ext_base = DRCMR_EXT_BASE_DEFAULT,
>  };

[Severity: High]
This is a pre-existing issue, but can this code cause a kernel panic due
to a NULL pointer dereference?

In mmp_pdma_probe(), IRQs are requested for all channels while phy->vchan
is initialized to NULL. It is only set later when a client allocates a
channel.

If the hardware has pending interrupts from boot, or a spurious interrupt
fires, mmp_pdma_chan_handler() executes this:

drivers/dma/mmp_pdma.c:mmp_pdma_chan_handler() {
    ...
	if (clear_chan_irq(phy) != 0)
		return IRQ_NONE;

	tasklet_schedule(&phy->vchan->tasklet);
    ...
}

Since clear_chan_irq() returns 0 even if vchan is NULL, does the
unconditional dereference of phy->vchan->tasklet trigger a crash?


[Severity: High]
This is a pre-existing issue, but does this code allow a tasklet to access
freed resources?

In mmp_pdma_free_chan_resources(), the descriptor pool is destroyed:

drivers/dma/mmp_pdma.c:mmp_pdma_free_chan_resources() {
    ...
	dma_pool_destroy(chan->desc_pool);
	chan->desc_pool = NULL;
	chan->idle = true;
    ...
}

However, the driver never calls tasklet_kill(&chan->tasklet) to synchronize
with the tasklet.

If dma_do_tasklet() is running concurrently on another CPU, it may block
until mmp_pdma_free_chan_resources() completes. Once it resumes, it attempts
to free descriptors using dma_pool_free() on the destroyed pool.

Can this lead to a use-after-free or a kernel oops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-k3-pdma-fix-drcmr-base-v1-1-ee1af124199f@linux.spacemit.com?part=1