Re: [PATCH v2 2/3] dmaengine: dw-edma: Configure remote interrupt routing

[sashiko-bot@kernel.org]

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

New issues:
- [High] dw_edma_device_config silently ignores standard generic DMA configuration fields when peripheral_config is provided for non-HDMA devices.
- [Medium] Lockless reads of `chan->irq_mode` in shared interrupt handlers will race with writes in `dw_edma_free_chan_resources` and `dw_edma_device_config_irq_mode`.

Pre-existing issues:
- [High] If a DMA channel fails to terminate within 5 seconds, dw_edma_wait_termination times out but leaves the hardware engine running, leading to potential corruption of future transfers.
--

commit ea103c42c7be1020b46df769c1c7bb35f1349b52
Author: Koichiro Den <den@valinux.co.jp>
Subject: dmaengine: dw-edma: Configure remote interrupt routing

The patch uses dma_slave_config to set dw_edma_ch_irq_mode on idle channels
of a local eDMA-compatible instance, allowing endpoint functions to route
interrupts remotely. It also adds hardware quiesce during synchronization
when restoring channel routing.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index a678c70a78fe6..a8c6bd508fcd4 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -177,6 +177,30 @@ dw_edma_get_default_irq_mode(struct dw_edma_chan *chan)
[ ... ]
> +static int dw_edma_device_config_irq_mode(struct dw_edma_chan *chan,
> +					  struct dma_slave_config *config)
> +{
[ ... ]
> +	chan->irq_mode = mode;

[Severity: Medium]
Could this plain C write to chan->irq_mode race with lockless reads in shared
interrupt handlers?

While dw_edma_device_config_irq_mode() (and dw_edma_free_chan_resources())
writes to chan->irq_mode, an interrupt for another channel on the same shared
IRQ line could fire. If the handler iterates over all channels and locklessly
reads chan->irq_mode via dw_edma_core_ch_ignore_irq(), this could trigger
KCSAN warnings and theoretically risk torn reads.

> +
> +	return 0;
> +}
> +
>  static int dw_edma_device_config(struct dma_chan *dchan,
>  				 struct dma_slave_config *config)
>  {
> @@ -184,6 +208,10 @@ static int dw_edma_device_config(struct dma_chan *dchan,
>  	bool cfg_non_ll;
>  	int non_ll = 0;
>  
> +	if (chan->dw->chip->mf != EDMA_MF_HDMA_NATIVE &&
> +	    config->peripheral_config)
> +		return dw_edma_device_config_irq_mode(chan, config);

[Severity: High]
Does this early return silently discard standard generic DMA configuration
fields?

If a client driver calls dmaengine_slave_config() on a non-HDMA channel and
provides both standard configuration fields (like src_addr) and a
peripheral_config (to set IRQ mode), the early return bypasses copying the
standard config fields.

Since it also leaves chan->configured = false, subsequent dmaengine_prep_*
calls could unexpectedly fail, even though this function returns success.

[ ... ]
> @@ -893,6 +917,17 @@ static void dw_edma_wait_termination(struct dma_chan *dchan)

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing issue
where dw_edma_wait_termination() can leave the hardware running on timeout:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_wait_termination() {
    ...
	while (time_before(jiffies, timeout)) {
		scoped_guard(spinlock_irqsave, &chan->vc.lock)
			stopping = chan->request == EDMA_REQ_STOP;

		if (!stopping)
			return;

		fsleep(1000);
	}

	dev_warn(chan->dw->chip->dev,
		 "timeout waiting for channel termination\n");
}

If a local DMA channel is requested to terminate via dmaengine_terminate_all()
and chan->request = EDMA_REQ_STOP is set, but the hardware is not forcefully
stopped via registers, could this lead to potential memory corruption or a
use-after-free?

If dw_edma_wait_termination() times out after 5 seconds and returns, software
will free the DMA client buffers. If the hardware engine subsequently completes
the transfer or unsticks, it may write to the freed memory.

>  static void dw_edma_device_synchronize(struct dma_chan *dchan)
>  {
[ ... ]
> @@ -903,12 +938,18 @@ static void dw_edma_device_synchronize(struct dma_chan *dchan)
>  static void dw_edma_free_chan_resources(struct dma_chan *dchan)
>  {
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +	enum dw_edma_ch_irq_mode default_mode =
> +			dw_edma_get_default_irq_mode(chan);
>  
>  	dw_edma_device_terminate_all(dchan);
>  	dw_edma_device_synchronize(dchan);
>  
> -	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock) {
>  		chan->configured = false;
> +		/* Avoid a redundant write racing with shared-IRQ readers. */
> +		if (chan->irq_mode != default_mode)
> +			chan->irq_mode = default_mode;

[Severity: Medium]
Similarly, will this plain C write to chan->irq_mode also race with a lockless
read in a shared IRQ handler?

> +	}
>  
>  	vchan_free_chan_resources(&chan->vc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828163611.2691264-1-den@valinux.co.jp?part=2