[PATCH v4 0/9] dmaengine: dw-edma: Fixes and interrupt-path groundwork

[PATCH v4 0/9] dmaengine: dw-edma: Fixes and interrupt-path groundwork

[Koichiro Den]

Hi,

This series collects dw-edma fixes and interrupt-path groundwork, mostly
split from v1 of the "Support dynamic LL appends" series.

Most v4 changes address review feedback from Frank and Sashiko. I also
collected Frank's Reviewed-by tags.

  v3: https://lore.kernel.org/r/20260715175740.1110404-1-den@valinux.co.jp/
  v2: https://lore.kernel.org/r/20260710080903.2392888-1-den@valinux.co.jp/
 (v1: https://lore.kernel.org/r/20260615154111.2174161-1-den@valinux.co.jp/)

Rebased on the latest dmaengine/fixes:
commit 867621ba2030 ("dmaengine: qcom: bam_dma: Fix command element mask
                      field for BAM v1.6.0+")

Best regards,
Koichiro


Koichiro Den (9):
  dmaengine: dw-edma: Fix HDMA channel status register access
  dmaengine: dw-edma: Terminate all descriptors without callbacks
  dmaengine: dw-edma: Serialize abort state updates
  dmaengine: dw-edma: Complete descriptors before pausing
  dmaengine: dw-edma: Serialize channel state checks
  dmaengine: dw-edma: Clear stale requests on termination
  dmaengine: dw-edma-pcie: Drop redundant pci_free_irq_vectors()
  dmaengine: dw-edma: Snapshot the v0 interrupt status once per handler
    pass
  dmaengine: dw-edma: Defer channel IRQ handling to workqueue

 drivers/dma/dw-edma/dw-edma-core.c    | 193 ++++++++++++++++++++++----
 drivers/dma/dw-edma/dw-edma-core.h    |  11 ++
 drivers/dma/dw-edma/dw-edma-pcie.c    |   3 -
 drivers/dma/dw-edma/dw-edma-v0-core.c |  28 ++--
 drivers/dma/dw-edma/dw-hdma-v0-core.c |   2 +-
 5 files changed, 188 insertions(+), 49 deletions(-)

-- 
2.51.0

[PATCH v4 1/9] dmaengine: dw-edma: Fix HDMA channel status register access

[Koichiro Den]

GET_CH_32() takes the direction before the channel ID, but
dw_hdma_v0_core_ch_status() passed them in the opposite order. This can
make the status callback read another HDMA channel status register.

Use the same argument order as the other HDMA register accesses.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - No changes.

 drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 632abb8b481c..2beec876b184 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -79,7 +79,7 @@ static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
 	u32 tmp;
 
 	tmp = FIELD_GET(HDMA_V0_CH_STATUS_MASK,
-			GET_CH_32(dw, chan->id, chan->dir, ch_stat));
+			GET_CH_32(dw, chan->dir, chan->id, ch_stat));
 
 	if (tmp == 1)
 		return DMA_IN_PROGRESS;
-- 
2.51.0

[PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks

[Koichiro Den]

The DMA Engine client documentation says in the "Terminate APIs" section
of Documentation/driver-api/dmaengine/client.rst:

"No callback functions will be called for any incomplete transfers."

dw-edma instead calls vchan_cookie_complete() when a deferred STOP reaches
the interrupt handler. This schedules a callback for the active descriptor
and leaves other issued or submitted descriptors queued. A late callback
after dmaengine_terminate_sync() can dereference client state that has
already been freed, while leftover descriptors may later restart into
reused buffers or leak.

Move all issued and submitted descriptors to the terminated list whenever
termination completes. For a pending STOP, do this from both the DONE and
ABORT paths. Complete their cookies in order without scheduling callbacks.

A STOP can remain pending until the running transfer raises an
interrupt. Make device_synchronize() wait for such a pending STOP to
complete before releasing terminated descriptors. Reuse it from
free_chan_resources(), then release the remaining virt-dma resources.
Sleep instead of busy-polling while waiting, and warn if the existing
timeout expires.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Use fsleep(). (Frank)
  - Use guard()/scoped_guard() for the new locking. (Frank)
  - Keep chan->configured across device_synchronize() and clear it in
    free_chan_resources() instead. (Sashiko)
  - Wait only while a deferred STOP remains pending.

 drivers/dma/dw-edma/dw-edma-core.c | 90 +++++++++++++++++++++++++-----
 1 file changed, 76 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 89a4c498a17b..972c7ea18d46 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/dmaengine.h>
@@ -201,6 +202,35 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
 	return 1;
 }
 
+static void dw_edma_terminate_vdesc(struct virt_dma_desc *vd)
+{
+	list_del(&vd->node);
+	dma_cookie_complete(&vd->tx);
+	vchan_terminate_vdesc(vd);
+}
+
+static void dw_edma_terminate_vdesc_list(struct list_head *head)
+{
+	struct virt_dma_desc *vd, *_vd;
+
+	list_for_each_entry_safe(vd, _vd, head, node)
+		dw_edma_terminate_vdesc(vd);
+}
+
+/* Must be called with vc.lock held. */
+static void dw_edma_terminate_all_descs(struct dw_edma_chan *chan)
+{
+	/*
+	 * This order must not be reversed. Cookies are assigned when
+	 * descriptors are submitted, so desc_issued contains older cookies
+	 * than desc_submitted. Completing desc_submitted first could move
+	 * chan->vc.chan.completed_cookie backwards when desc_issued is
+	 * terminated afterwards.
+	 */
+	dw_edma_terminate_vdesc_list(&chan->vc.desc_issued);
+	dw_edma_terminate_vdesc_list(&chan->vc.desc_submitted);
+}
+
 static void dw_edma_device_caps(struct dma_chan *dchan,
 				struct dma_slave_caps *caps)
 {
@@ -308,20 +338,22 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 	int err = 0;
 
+	guard(spinlock_irqsave)(&chan->vc.lock);
+
 	if (!chan->configured) {
-		/* Do nothing */
+		dw_edma_terminate_all_descs(chan);
 	} else if (chan->status == EDMA_ST_PAUSE) {
+		dw_edma_terminate_all_descs(chan);
 		chan->status = EDMA_ST_IDLE;
-		chan->configured = false;
 	} else if (chan->status == EDMA_ST_IDLE) {
-		chan->configured = false;
+		dw_edma_terminate_all_descs(chan);
 	} else if (dw_edma_core_ch_status(chan) == DMA_COMPLETE) {
 		/*
 		 * The channel is in a false BUSY state, probably didn't
 		 * receive or lost an interrupt
 		 */
+		dw_edma_terminate_all_descs(chan);
 		chan->status = EDMA_ST_IDLE;
-		chan->configured = false;
 	} else if (chan->request > EDMA_REQ_PAUSE) {
 		err = -EPERM;
 	} else {
@@ -673,8 +705,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
 			break;
 
 		case EDMA_REQ_STOP:
-			list_del(&vd->node);
-			vchan_cookie_complete(vd);
+			dw_edma_terminate_all_descs(chan);
 			chan->request = EDMA_REQ_NONE;
 			chan->status = EDMA_ST_IDLE;
 			break;
@@ -698,7 +729,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
 
 	spin_lock_irqsave(&chan->vc.lock, flags);
 	vd = vchan_next_desc(&chan->vc);
-	if (vd) {
+	if (vd && chan->request == EDMA_REQ_STOP) {
+		dw_edma_terminate_all_descs(chan);
+	} else if (vd) {
 		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
 		list_del(&vd->node);
 		vchan_cookie_complete(vd);
@@ -856,21 +889,49 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
 	return 0;
 }
 
-static void dw_edma_free_chan_resources(struct dma_chan *dchan)
+static void dw_edma_wait_termination(struct dma_chan *dchan)
 {
+	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
-	int ret;
+	bool stopping;
 
+	/*
+	 * A STOP may be deferred to a later interrupt while the channel is still
+	 * running. Wait until that handler completes the termination.
+	 */
 	while (time_before(jiffies, timeout)) {
-		ret = dw_edma_device_terminate_all(dchan);
-		if (!ret)
-			break;
+		scoped_guard(spinlock_irqsave, &chan->vc.lock)
+			stopping = chan->request == EDMA_REQ_STOP;
 
-		if (time_after_eq(jiffies, timeout))
+		if (!stopping)
 			return;
 
-		cpu_relax();
+		fsleep(1000);
 	}
+
+	dev_warn(chan->dw->chip->dev,
+		 "timeout waiting for channel termination\n");
+}
+
+static void dw_edma_device_synchronize(struct dma_chan *dchan)
+{
+	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
+
+	dw_edma_wait_termination(dchan);
+	vchan_synchronize(&chan->vc);
+}
+
+static void dw_edma_free_chan_resources(struct dma_chan *dchan)
+{
+	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
+
+	dw_edma_device_terminate_all(dchan);
+	dw_edma_device_synchronize(dchan);
+
+	scoped_guard(spinlock_irqsave, &chan->vc.lock)
+		chan->configured = false;
+
+	vchan_free_chan_resources(&chan->vc);
 }
 
 static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
@@ -968,6 +1029,7 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
 	dma->device_pause = dw_edma_device_pause;
 	dma->device_resume = dw_edma_device_resume;
 	dma->device_terminate_all = dw_edma_device_terminate_all;
+	dma->device_synchronize = dw_edma_device_synchronize;
 	dma->device_issue_pending = dw_edma_device_issue_pending;
 	dma->device_tx_status = dw_edma_device_tx_status;
 	dma->device_prep_slave_sg = dw_edma_device_prep_slave_sg;
-- 
2.51.0

Re: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks

[sashiko-bot]

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

New issues:
- [High] If channel termination times out, the channel state becomes permanently wedged in BUSY, rendering it unusable.

Pre-existing issues:
- [High] Pre-existing missing locking in `dw_edma_device_pause()` causes data races with interrupt handlers.
--

--- Patch [2]: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks ---
commit 9db3cc9880b9ba6f4fe534fd17ede7df5d2a503f
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Terminate all descriptors without callbacks

This commit moves all issued and submitted descriptors to the terminated
list when termination completes. It also introduces a wait for pending
STOPs before releasing resources to prevent late callbacks from
dereferencing freed client state.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -673,8 +705,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>  
>  		case EDMA_REQ_STOP:
> -			list_del(&vd->node);
> -			vchan_cookie_complete(vd);
> +			dw_edma_terminate_all_descs(chan);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;

[Severity: High]
This is a pre-existing issue, but can dw_edma_device_pause() race with
the interrupt handler and wedge the channel?

dw_edma_device_pause() modifies chan->request and chan->status locklessly.

If it races with dw_edma_done_interrupt() (which holds the lock and sets
chan->status = EDMA_ST_IDLE here), dw_edma_device_pause() can concurrently
set chan->request = EDMA_REQ_PAUSE on an IDLE channel.

This leaves the channel in a state where dw_edma_device_resume() returns
-EPERM because it expects EDMA_ST_PAUSE, and dw_edma_device_issue_pending()
ignores it because chan->request is not EDMA_REQ_NONE, permanently
breaking the channel.

[ ... ]

> +static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	dw_edma_device_terminate_all(dchan);
> +	dw_edma_device_synchronize(dchan);
> +
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +		chan->configured = false;
> +
> +	vchan_free_chan_resources(&chan->vc);
>  }

[Severity: High]
If dw_edma_wait_termination() (called via dw_edma_device_synchronize)
times out, does the channel become permanently wedged in a BUSY state?

If the hardware fails to respond within 5 seconds, the timeout returns and
vchan_free_chan_resources() clears all descriptors.

If the hardware eventually finishes and raises a deferred DONE interrupt,
dw_edma_done_interrupt() is called. Because the descriptors were already
freed, vchan_next_desc() returns NULL.

Since the state reset logic (setting chan->request = EDMA_REQ_NONE and
chan->status = EDMA_ST_IDLE) is nested inside the if (vd) check in
dw_edma_done_interrupt(), it is completely skipped. This leaves the channel
stuck in EDMA_ST_BUSY and causes future dw_edma_alloc_chan_resources()
calls to fail with -EBUSY.

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

Re: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks

[Frank Li]

On Fri, Jul 17, 2026 at 12:24:56AM +0900, Koichiro Den wrote:
> The DMA Engine client documentation says in the "Terminate APIs" section
> of Documentation/driver-api/dmaengine/client.rst:
>
> "No callback functions will be called for any incomplete transfers."
>
> dw-edma instead calls vchan_cookie_complete() when a deferred STOP reaches
> the interrupt handler. This schedules a callback for the active descriptor
> and leaves other issued or submitted descriptors queued. A late callback
> after dmaengine_terminate_sync() can dereference client state that has
> already been freed, while leftover descriptors may later restart into
> reused buffers or leak.
>
> Move all issued and submitted descriptors to the terminated list whenever
> termination completes. For a pending STOP, do this from both the DONE and
> ABORT paths. Complete their cookies in order without scheduling callbacks.
>
> A STOP can remain pending until the running transfer raises an
> interrupt. Make device_synchronize() wait for such a pending STOP to
> complete before releasing terminated descriptors. Reuse it from
> free_chan_resources(), then release the remaining virt-dma resources.
> Sleep instead of busy-polling while waiting, and warn if the existing
> timeout expires.
>
> Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v4:
>   - Use fsleep(). (Frank)
>   - Use guard()/scoped_guard() for the new locking. (Frank)
>   - Keep chan->configured across device_synchronize() and clear it in
>     free_chan_resources() instead. (Sashiko)
>   - Wait only while a deferred STOP remains pending.
>
>  drivers/dma/dw-edma/dw-edma-core.c | 90 +++++++++++++++++++++++++-----
>  1 file changed, 76 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 89a4c498a17b..972c7ea18d46 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/dmaengine.h>
> @@ -201,6 +202,35 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  	return 1;
>  }
>
> +static void dw_edma_terminate_vdesc(struct virt_dma_desc *vd)
> +{
> +	list_del(&vd->node);
> +	dma_cookie_complete(&vd->tx);
> +	vchan_terminate_vdesc(vd);
> +}
> +
> +static void dw_edma_terminate_vdesc_list(struct list_head *head)
> +{
> +	struct virt_dma_desc *vd, *_vd;
> +
> +	list_for_each_entry_safe(vd, _vd, head, node)
> +		dw_edma_terminate_vdesc(vd);
> +}
> +
> +/* Must be called with vc.lock held. */
> +static void dw_edma_terminate_all_descs(struct dw_edma_chan *chan)
> +{
> +	/*
> +	 * This order must not be reversed. Cookies are assigned when
> +	 * descriptors are submitted, so desc_issued contains older cookies
> +	 * than desc_submitted. Completing desc_submitted first could move
> +	 * chan->vc.chan.completed_cookie backwards when desc_issued is
> +	 * terminated afterwards.
> +	 */
> +	dw_edma_terminate_vdesc_list(&chan->vc.desc_issued);
> +	dw_edma_terminate_vdesc_list(&chan->vc.desc_submitted);
> +}
> +
>  static void dw_edma_device_caps(struct dma_chan *dchan,
>  				struct dma_slave_caps *caps)
>  {
> @@ -308,20 +338,22 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>  	int err = 0;
>
> +	guard(spinlock_irqsave)(&chan->vc.lock);
> +
>  	if (!chan->configured) {
> -		/* Do nothing */
> +		dw_edma_terminate_all_descs(chan);
>  	} else if (chan->status == EDMA_ST_PAUSE) {
> +		dw_edma_terminate_all_descs(chan);
>  		chan->status = EDMA_ST_IDLE;
> -		chan->configured = false;
>  	} else if (chan->status == EDMA_ST_IDLE) {
> -		chan->configured = false;
> +		dw_edma_terminate_all_descs(chan);
>  	} else if (dw_edma_core_ch_status(chan) == DMA_COMPLETE) {
>  		/*
>  		 * The channel is in a false BUSY state, probably didn't
>  		 * receive or lost an interrupt
>  		 */
> +		dw_edma_terminate_all_descs(chan);
>  		chan->status = EDMA_ST_IDLE;
> -		chan->configured = false;
>  	} else if (chan->request > EDMA_REQ_PAUSE) {
>  		err = -EPERM;
>  	} else {
> @@ -673,8 +705,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>
>  		case EDMA_REQ_STOP:
> -			list_del(&vd->node);
> -			vchan_cookie_complete(vd);
> +			dw_edma_terminate_all_descs(chan);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;
> @@ -698,7 +729,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
>
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  	vd = vchan_next_desc(&chan->vc);
> -	if (vd) {
> +	if (vd && chan->request == EDMA_REQ_STOP) {
> +		dw_edma_terminate_all_descs(chan);
> +	} else if (vd) {
>  		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);
> @@ -856,21 +889,49 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
>  	return 0;
>  }
>
> -static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +static void dw_edma_wait_termination(struct dma_chan *dchan)
>  {
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>  	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> -	int ret;
> +	bool stopping;
>
> +	/*
> +	 * A STOP may be deferred to a later interrupt while the channel is still
> +	 * running. Wait until that handler completes the termination.
> +	 */
>  	while (time_before(jiffies, timeout)) {
> -		ret = dw_edma_device_terminate_all(dchan);
> -		if (!ret)
> -			break;
> +		scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +			stopping = chan->request == EDMA_REQ_STOP;
>
> -		if (time_after_eq(jiffies, timeout))
> +		if (!stopping)
>  			return;
>
> -		cpu_relax();
> +		fsleep(1000);
>  	}
> +
> +	dev_warn(chan->dw->chip->dev,
> +		 "timeout waiting for channel termination\n");
> +}
> +
> +static void dw_edma_device_synchronize(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	dw_edma_wait_termination(dchan);
> +	vchan_synchronize(&chan->vc);
> +}
> +
> +static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	dw_edma_device_terminate_all(dchan);
> +	dw_edma_device_synchronize(dchan);
> +
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +		chan->configured = false;
> +
> +	vchan_free_chan_resources(&chan->vc);
>  }
>
>  static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> @@ -968,6 +1029,7 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
>  	dma->device_pause = dw_edma_device_pause;
>  	dma->device_resume = dw_edma_device_resume;
>  	dma->device_terminate_all = dw_edma_device_terminate_all;
> +	dma->device_synchronize = dw_edma_device_synchronize;
>  	dma->device_issue_pending = dw_edma_device_issue_pending;
>  	dma->device_tx_status = dw_edma_device_tx_status;
>  	dma->device_prep_slave_sg = dw_edma_device_prep_slave_sg;
> --
> 2.51.0
>

[PATCH v4 3/9] dmaengine: dw-edma: Serialize abort state updates

[Koichiro Den]

dw_edma_abort_interrupt() drops vc.lock before changing request and
status. issue_pending() can acquire the lock in that small window,
observe the old busy state, and skip starting queued descriptors. Then
the abort handler overwrites the channel status as idle, leaving the new
descriptors stranded for good.

Keep descriptor completion and the state transition in the same critical
section.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Collect Frank's Reviewed-by tag; no code changes.

 drivers/dma/dw-edma/dw-edma-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 972c7ea18d46..daf70df59e07 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -736,9 +736,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
 		list_del(&vd->node);
 		vchan_cookie_complete(vd);
 	}
-	spin_unlock_irqrestore(&chan->vc.lock, flags);
 	chan->request = EDMA_REQ_NONE;
 	chan->status = EDMA_ST_IDLE;
+	spin_unlock_irqrestore(&chan->vc.lock, flags);
 }
 
 static void dw_edma_emul_irq_ack(struct irq_data *d)
-- 
2.51.0

Re: [PATCH v4 3/9] dmaengine: dw-edma: Serialize abort state updates

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing lock in dw_edma_device_pause and dw_edma_device_resume causes Data Race and Use-After-Free.
- [High] Permanent channel stall when resuming from PAUSE due to bypassed descriptor completion.
- [High] Unexpected hardware abort leaves queued descriptors stranded indefinitely.
--

--- Patch [3]: [PATCH v4 3/9] dmaengine: dw-edma: Serialize abort state updates ---
commit 4837da5eb731c888cd532d74c75c719d7336551e
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Serialize abort state updates

This commit fixes a race condition where issue_pending() could observe an
inconsistent state. It extends the critical section in
dw_edma_abort_interrupt() to cover the channel request and status updates.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 972c7ea18d46b..daf70df59e077 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -736,9 +736,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);
>  	}
> -	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  	chan->request = EDMA_REQ_NONE;
>  	chan->status = EDMA_ST_IDLE;

[Severity: High]
This is a pre-existing issue, but does an unexpected hardware abort leave
queued descriptors stranded indefinitely here?

On a hardware abort (not requested by software), dw_edma_abort_interrupt()
fetches the single active descriptor, completes it with DMA_TRANS_ABORTED,
and unconditionally transitions the channel to EDMA_ST_IDLE.

Could this ignore any queued descriptors remaining in vc->desc_issued?
Since the hardware is now marked IDLE without processing the remaining
queue, it seems these descriptors are permanently stranded until the
client initiates a new dmaengine_issue_pending() call.

> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing lock in
dw_edma_device_pause() and dw_edma_device_resume() that could cause a data
race and use-after-free?

The dmaengine API allows clients to call dmaengine_resume() and
dmaengine_pause() without driver-level locks. In dw_edma_device_pause()
and dw_edma_device_resume(), the state is modified locklessly:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_pause() {
    ...
    chan->request = EDMA_REQ_PAUSE;
    ...
}

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
    chan->status = EDMA_ST_BUSY;
    dw_edma_start_transfer(chan);
    ...
}

If dw_edma_device_resume() is called concurrently with an interrupt handled
on another CPU, could both CPUs execute dw_edma_start_transfer() on the
same descriptor?

Both would access the same child chunk, pass it to dw_edma_core_start(),
and free it, which could result in list corruption and a use-after-free:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_start_transfer() {
    ...
    list_del(&child->list);
    kfree(child);
    ...
}


[Severity: High]
This is a pre-existing issue, but can the channel stall permanently when
resuming from PAUSE due to bypassed descriptor completion?

When the final chunk of a transfer finishes, its chunks_alloc drops to 0.
If EDMA_REQ_PAUSE is handled in this interrupt, the switch in
dw_edma_done_interrupt() bypasses the completion logic, leaving the fully
finished descriptor in the issued list:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt() {
    ...
        case EDMA_REQ_PAUSE:
            chan->request = EDMA_REQ_NONE;
            chan->status = EDMA_ST_PAUSE;
            break;
    ...
}

Later, dw_edma_device_resume() sets the state to BUSY unconditionally:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
    chan->status = EDMA_ST_BUSY;
    dw_edma_start_transfer(chan);
    ...
}

Because the chunk list is empty, dw_edma_start_transfer() fails to start
the hardware and returns 0. Could this mismatched software state leave
the channel permanently deadlocked in a BUSY state?

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

[PATCH v4 4/9] dmaengine: dw-edma: Complete descriptors before pausing

[Koichiro Den]

If PAUSE is requested while the final chunk of a descriptor is in
flight, the DONE interrupt takes the PAUSE path without checking whether
the descriptor has been depleted. The depleted descriptor remains on the
issued list and the channel enters EDMA_ST_PAUSE.

On resume, dw_edma_start_transfer() finds the descriptor but no chunk to
start and returns 0. The caller ignores that result and leaves the
channel stuck in EDMA_ST_BUSY with no transfer running.

Check for descriptor completion before acknowledging PAUSE. If there is
no work to start on resume, leave the channel idle. Also ignore DONE
interrupts while the channel is paused so a stale or repeated interrupt
cannot change its state or start queued work.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Collect Frank's Reviewed-by tag; no code changes.

 drivers/dma/dw-edma/dw-edma-core.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index daf70df59e07..ac40a832a9c7 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -327,7 +327,8 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
 		err = -EPERM;
 	} else {
 		chan->status = EDMA_ST_BUSY;
-		dw_edma_start_transfer(chan);
+		if (!dw_edma_start_transfer(chan))
+			chan->status = EDMA_ST_IDLE;
 	}
 
 	return err;
@@ -687,10 +688,16 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
 	unsigned long flags;
 
 	spin_lock_irqsave(&chan->vc.lock, flags);
+	if (chan->status == EDMA_ST_PAUSE) {
+		spin_unlock_irqrestore(&chan->vc.lock, flags);
+		return;
+	}
+
 	vd = vchan_next_desc(&chan->vc);
 	if (vd) {
 		switch (chan->request) {
 		case EDMA_REQ_NONE:
+		case EDMA_REQ_PAUSE:
 			desc = vd2dw_edma_desc(vd);
 			if (!desc->chunks_alloc) {
 				dw_hdma_set_callback_result(vd,
@@ -699,6 +706,12 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
 				vchan_cookie_complete(vd);
 			}
 
+			if (chan->request == EDMA_REQ_PAUSE) {
+				chan->request = EDMA_REQ_NONE;
+				chan->status = EDMA_ST_PAUSE;
+				break;
+			}
+
 			/* Continue transferring if there are remaining chunks or issued requests.
 			 */
 			chan->status = dw_edma_start_transfer(chan) ? EDMA_ST_BUSY : EDMA_ST_IDLE;
@@ -710,11 +723,6 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
 			chan->status = EDMA_ST_IDLE;
 			break;
 
-		case EDMA_REQ_PAUSE:
-			chan->request = EDMA_REQ_NONE;
-			chan->status = EDMA_ST_PAUSE;
-			break;
-
 		default:
 			break;
 		}
-- 
2.51.0

Re: [PATCH v4 4/9] dmaengine: dw-edma: Complete descriptors before pausing

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but remains unfixed: dw_edma_device_pause() and dw_edma_device_resume() modify shared state and access virt-dma lists without holding the required chan->vc.lock.
--

--- Patch [4]: [PATCH v4 4/9] dmaengine: dw-edma: Complete descriptors before pausing ---
commit ae2fd62162735ab1efcac8f4a660ab9c1e877d64
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Complete descriptors before pausing

This commit ensures that the DMA engine checks for descriptor completion
before acknowledging a pause request. If there is no work to start on
resume, it leaves the channel idle. It also ignores done interrupts while
the channel is paused so a stale or repeated interrupt cannot change its
state or start queued work.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index daf70df59e077..ac40a832a9c76 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -327,7 +327,8 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
>  		err = -EPERM;
>  	} else {
>  		chan->status = EDMA_ST_BUSY;
> -		dw_edma_start_transfer(chan);
> +		if (!dw_edma_start_transfer(chan))
> +			chan->status = EDMA_ST_IDLE;

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_resume() (and
dw_edma_device_pause()) modify shared state and access virt-dma lists
without holding the required chan->vc.lock?

For dw_edma_device_resume(), calling dw_edma_start_transfer() locklessly
iterates over chan->vc.desc_issued via vchan_next_desc() and mutates chunk
structures.

Could this cause list corruption or lockdep splats if run concurrently with
the interrupt handler or dmaengine_terminate_all()?

Similarly, in dw_edma_device_pause(), can a time-of-check to time-of-use
race occur if it evaluates chan->status == EDMA_ST_BUSY locklessly?

If dw_edma_done_interrupt() concurrently completes the last descriptor and
transitions the state to EDMA_ST_IDLE, the pause function could then
overwrite chan->request = EDMA_REQ_PAUSE. 

Could this leave the channel in an invalid state (status == EDMA_ST_IDLE,
request == EDMA_REQ_PAUSE) where it cannot be resumed (since resume expects
EDMA_ST_PAUSE) and cannot process new descriptors?

>  	}
>  
>  	return err;

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

[PATCH v4 5/9] dmaengine: dw-edma: Serialize channel state checks

[Koichiro Den]

pause() and resume() read and update channel state without holding vc.lock,
while the interrupt handlers update the same state under it. Take the same
lock around those state checks so that request, status, and configured stay
consistent.

For example, pause() can observe EDMA_ST_BUSY right before the interrupt
handler completes the final descriptor and moves the channel to
EDMA_ST_IDLE, and then record EDMA_REQ_PAUSE on an already idle channel. No
further interrupt will acknowledge the request, and since issue_pending()
requires EDMA_REQ_NONE, the channel is wedged for good: terminate_all()
leaves the stale request behind, so even reconfiguring the channel does not
recover it.

issue_pending() already runs under vc.lock, but it tests configured before
taking it. Move that test under the lock as well, so configured, request,
and status are evaluated as one channel-state snapshot.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Use guard() where lock/unlock is newly added. (Frank)

 drivers/dma/dw-edma/dw-edma-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index ac40a832a9c7..64ace6b9ebdc 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -302,6 +302,8 @@ static int dw_edma_device_pause(struct dma_chan *dchan)
 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 	int err = 0;
 
+	guard(spinlock_irqsave)(&chan->vc.lock);
+
 	if (!chan->configured)
 		err = -EPERM;
 	else if (chan->status != EDMA_ST_BUSY)
@@ -319,6 +321,8 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 	int err = 0;
 
+	guard(spinlock_irqsave)(&chan->vc.lock);
+
 	if (!chan->configured) {
 		err = -EPERM;
 	} else if (chan->status != EDMA_ST_PAUSE) {
@@ -369,11 +373,9 @@ static void dw_edma_device_issue_pending(struct dma_chan *dchan)
 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 	unsigned long flags;
 
-	if (!chan->configured)
-		return;
-
 	spin_lock_irqsave(&chan->vc.lock, flags);
-	if (vchan_issue_pending(&chan->vc) && chan->request == EDMA_REQ_NONE &&
+	if (chan->configured && vchan_issue_pending(&chan->vc) &&
+	    chan->request == EDMA_REQ_NONE &&
 	    chan->status == EDMA_ST_IDLE) {
 		chan->status = EDMA_ST_BUSY;
 		dw_edma_start_transfer(chan);
-- 
2.51.0

[PATCH v4 6/9] dmaengine: dw-edma: Clear stale requests on termination

[Koichiro Den]

terminate_all() can finish immediately when the channel is unconfigured,
paused, idle, or already stopped in hardware. A pending PAUSE request can
survive these paths and block issue_pending() even after termination.

Clear the request whenever termination leaves the channel idle. A running
channel keeps its STOP request until the interrupt handler consumes it.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Collect Frank's Reviewed-by tag and adjust context for the preceding
    patch.

 drivers/dma/dw-edma/dw-edma-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 64ace6b9ebdc..b76f0aeb647d 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -364,6 +364,8 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
 	} else {
 		chan->request = EDMA_REQ_STOP;
 	}
+	if (chan->status == EDMA_ST_IDLE)
+		chan->request = EDMA_REQ_NONE;
 
 	return err;
 }
-- 
2.51.0

[PATCH v4 7/9] dmaengine: dw-edma-pcie: Drop redundant pci_free_irq_vectors()

[Koichiro Den]

dw_edma_pcie enables the PCI device with pcim_enable_device(), so IRQ
vectors allocated by pci_alloc_irq_vectors() are released by
pcim_msi_release() on device release. The driver should not call
pci_free_irq_vectors() manually.

Drop the redundant remove-time cleanup and rely on the managed PCI
device lifetime instead, as documented by commit 03e4905402ae ("PCI/MSI:
Clarify pci_free_irq_vectors() usage for managed devices").

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - No changes.

 drivers/dma/dw-edma/dw-edma-pcie.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
index 791c46e8ae4c..5e81a433a957 100644
--- a/drivers/dma/dw-edma/dw-edma-pcie.c
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -555,9 +555,6 @@ static void dw_edma_pcie_remove(struct pci_dev *pdev)
 	err = dw_edma_remove(chip);
 	if (err)
 		pci_warn(pdev, "can't remove device properly: %d\n", err);
-
-	/* Freeing IRQs */
-	pci_free_irq_vectors(pdev);
 }
 
 static const struct pci_device_id dw_edma_pcie_id_table[] = {
-- 
2.51.0

[PATCH v4 8/9] dmaengine: dw-edma: Snapshot the v0 interrupt status once per handler pass

[Koichiro Den]

The v0 interrupt handler reads the interrupt status register twice per
invocation, once through the DONE accessor and once through the ABORT
accessor, although both fields live in the same 32-bit register. On
remote setups (dw-edma-pcie) each read is a non-posted round trip across
the PCIe link costing on the order of a microsecond, and with one
completion interrupt per element the duplicate adds up. As an example,
profiling the R-Car S4 remote path put the handler at ~7us per
invocation, dominated by such reads.

Read the register once and derive the DONE and ABORT views from the
snapshot. No abort is lost to this because the pass only clears status
bits it observed, so an abort raised after the snapshot keeps its status
and its own interrupt delivery brings it to the next pass. An abort on
an observed channel cannot race the clear either. Software can restart
the halted channel only after abort() runs, and abort() is called after
dw_edma_v0_core_clear_abort_int().

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - No changes.

 drivers/dma/dw-edma/dw-edma-v0-core.c | 28 +++++++++++++--------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index cfdd6463252e..6befae2c95ac 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -218,18 +218,6 @@ static void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan)
 		  FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id)));
 }
 
-static u32 dw_edma_v0_core_status_done_int(struct dw_edma *dw, enum dw_edma_dir dir)
-{
-	return FIELD_GET(EDMA_V0_DONE_INT_MASK,
-			 GET_RW_32(dw, dir, int_status));
-}
-
-static u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir)
-{
-	return FIELD_GET(EDMA_V0_ABORT_INT_MASK,
-			 GET_RW_32(dw, dir, int_status));
-}
-
 static irqreturn_t
 dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
 			   dw_edma_handler_t done, dw_edma_handler_t abort)
@@ -239,7 +227,7 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
 	irqreturn_t ret = IRQ_NONE;
 	struct dw_edma_chan *chan;
 	unsigned long off;
-	u32 mask;
+	u32 mask, sts;
 
 	if (dir == EDMA_DIR_WRITE) {
 		total = dw->wr_ch_cnt;
@@ -251,7 +239,17 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
 		mask = dw_irq->rd_mask;
 	}
 
-	val = dw_edma_v0_core_status_done_int(dw, dir);
+	/*
+	 * DONE and ABORT status share one register, and on remote setups
+	 * every read is a non-posted round trip across the PCIe link. Take
+	 * one snapshot and derive both views from it. An abort raised
+	 * after the snapshot is deferred, not lost: only bits observed in
+	 * the snapshot are ever cleared below, so its status remains set and
+	 * triggers another handler pass.
+	 */
+	sts = GET_RW_32(dw, dir, int_status);
+
+	val = FIELD_GET(EDMA_V0_DONE_INT_MASK, sts);
 	val &= mask;
 	for_each_set_bit(pos, &val, total) {
 		chan = &dw->chan[pos + off];
@@ -262,7 +260,7 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
 		ret = IRQ_HANDLED;
 	}
 
-	val = dw_edma_v0_core_status_abort_int(dw, dir);
+	val = FIELD_GET(EDMA_V0_ABORT_INT_MASK, sts);
 	val &= mask;
 	for_each_set_bit(pos, &val, total) {
 		chan = &dw->chan[pos + off];
-- 
2.51.0

[PATCH v4 9/9] dmaengine: dw-edma: Defer channel IRQ handling to workqueue

[Koichiro Den]

On some SoCs (e.g. R-Car S4) the endpoint-side eDMA raises a single
fixed SPI that is hardwired to CPU0 and covers every read and write
channel. Handling channel events directly in that hard IRQ context
serializes the completion processing of all channels on one CPU:
descriptor recycling and refill, client callbacks (the vchan tasklet
runs on the scheduling CPU) and the doorbell writes all funnel through
CPU0, while the handler additionally spins on each channel's vc.lock.
Especially under heavy multichannel load, this contention becomes a
performance bottleneck.

Keep the hard IRQ handler minimal: clear the status, dispatch channel
events, and defer per-channel processing to work items. A work item per
channel preserves ordering while allowing different channels to run in
parallel on any CPU.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - Collect Frank's Reviewed-by tag; no code changes.

 drivers/dma/dw-edma/dw-edma-core.c | 69 +++++++++++++++++++++++++++---
 drivers/dma/dw-edma/dw-edma-core.h | 11 +++++
 2 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index b76f0aeb647d..49fddad131c7 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -30,6 +30,11 @@ struct dw_edma_desc *vd2dw_edma_desc(struct virt_dma_desc *vd)
 	return container_of(vd, struct dw_edma_desc, vd);
 }
 
+enum dw_edma_irq_event {
+	DW_EDMA_IRQ_DONE	= BIT(0),
+	DW_EDMA_IRQ_ABORT	= BIT(1),
+};
+
 static inline
 u64 dw_edma_get_pci_address(struct dw_edma_chan *chan, phys_addr_t cpu_addr)
 {
@@ -753,6 +758,39 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
 }
 
+static void dw_edma_irq_work(struct work_struct *work)
+{
+	struct dw_edma_chan *chan = container_of(work, struct dw_edma_chan,
+						 irq_work);
+	unsigned int events;
+
+	do {
+		events = atomic_xchg(&chan->irq_pending, 0);
+
+		if (events & DW_EDMA_IRQ_DONE)
+			dw_edma_done_interrupt(chan);
+		if (events & DW_EDMA_IRQ_ABORT)
+			dw_edma_abort_interrupt(chan);
+	} while (atomic_read(&chan->irq_pending));
+}
+
+static void dw_edma_queue_irq_work(struct dw_edma_chan *chan,
+				   enum dw_edma_irq_event event)
+{
+	atomic_or(event, &chan->irq_pending);
+	queue_work(chan->dw->wq, &chan->irq_work);
+}
+
+static void dw_edma_done_interrupt_deferred(struct dw_edma_chan *chan)
+{
+	dw_edma_queue_irq_work(chan, DW_EDMA_IRQ_DONE);
+}
+
+static void dw_edma_abort_interrupt_deferred(struct dw_edma_chan *chan)
+{
+	dw_edma_queue_irq_work(chan, DW_EDMA_IRQ_ABORT);
+}
+
 static void dw_edma_emul_irq_ack(struct irq_data *d)
 {
 	struct dw_edma *dw = irq_data_get_irq_chip_data(d);
@@ -847,8 +885,8 @@ static inline irqreturn_t dw_edma_interrupt_write_inner(int irq, void *data)
 	struct dw_edma_irq *dw_irq = data;
 
 	return dw_edma_core_handle_int(dw_irq, EDMA_DIR_WRITE,
-				       dw_edma_done_interrupt,
-				       dw_edma_abort_interrupt);
+				       dw_edma_done_interrupt_deferred,
+				       dw_edma_abort_interrupt_deferred);
 }
 
 static inline irqreturn_t dw_edma_interrupt_read_inner(int irq, void *data)
@@ -856,8 +894,8 @@ static inline irqreturn_t dw_edma_interrupt_read_inner(int irq, void *data)
 	struct dw_edma_irq *dw_irq = data;
 
 	return dw_edma_core_handle_int(dw_irq, EDMA_DIR_READ,
-				       dw_edma_done_interrupt,
-				       dw_edma_abort_interrupt);
+				       dw_edma_done_interrupt_deferred,
+				       dw_edma_abort_interrupt_deferred);
 }
 
 static inline irqreturn_t dw_edma_interrupt_write(int irq, void *data)
@@ -930,6 +968,8 @@ static void dw_edma_device_synchronize(struct dma_chan *dchan)
 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
 
 	dw_edma_wait_termination(dchan);
+	cancel_work_sync(&chan->irq_work);
+	atomic_set(&chan->irq_pending, 0);
 	vchan_synchronize(&chan->vc);
 }
 
@@ -977,6 +1017,8 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
 		chan->configured = false;
 		chan->request = EDMA_REQ_NONE;
 		chan->status = EDMA_ST_IDLE;
+		INIT_WORK(&chan->irq_work, dw_edma_irq_work);
+		atomic_set(&chan->irq_pending, 0);
 
 		if (chan->dir == EDMA_DIR_WRITE)
 			chan->ll_max = (chip->ll_region_wr[chan->id].sz / EDMA_LL_SZ);
@@ -1190,10 +1232,21 @@ int dw_edma_probe(struct dw_edma_chip *chip)
 	/* Disable eDMA, only to establish the ideal initial conditions */
 	dw_edma_core_off(dw);
 
+	/*
+	 * Deferred IRQ works are queued from the hard IRQ handlers, so the
+	 * workqueue must exist before any IRQ is requested.
+	 */
+	dw->wq = alloc_workqueue("dw-edma:%s", WQ_UNBOUND | WQ_HIGHPRI, 0,
+				 dev_name(chip->dev));
+	if (!dw->wq)
+		return -ENOMEM;
+
 	/* Request IRQs */
 	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);
-	if (err)
+	if (err) {
+		destroy_workqueue(dw->wq);
 		return err;
+	}
 
 	/* Allocate a dedicated virtual IRQ for interrupt-emulation doorbells */
 	err = dw_edma_emul_irq_alloc(dw);
@@ -1216,6 +1269,7 @@ int dw_edma_probe(struct dw_edma_chip *chip)
 	for (i = (dw->nr_irqs - 1); i >= 0; i--)
 		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
 	dw_edma_emul_irq_free(dw);
+	destroy_workqueue(dw->wq);
 
 	return err;
 }
@@ -1240,6 +1294,11 @@ int dw_edma_remove(struct dw_edma_chip *chip)
 		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
 	dw_edma_emul_irq_free(dw);
 
+	for (i = 0; i < dw->wr_ch_cnt + dw->rd_ch_cnt; i++)
+		cancel_work_sync(&dw->chan[i].irq_work);
+
+	destroy_workqueue(dw->wq);
+
 	/* Deregister eDMA device */
 	dma_async_device_unregister(&dw->dma);
 	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
index 6474cacf7195..7f6301572fa4 100644
--- a/drivers/dma/dw-edma/dw-edma-core.h
+++ b/drivers/dma/dw-edma/dw-edma-core.h
@@ -9,8 +9,10 @@
 #ifndef _DW_EDMA_CORE_H
 #define _DW_EDMA_CORE_H
 
+#include <linux/atomic.h>
 #include <linux/msi.h>
 #include <linux/dma/edma.h>
+#include <linux/workqueue.h>
 
 #include "../virt-dma.h"
 
@@ -87,6 +89,9 @@ struct dw_edma_chan {
 
 	struct dma_slave_config		config;
 	bool				non_ll;
+
+	struct work_struct		irq_work;
+	atomic_t			irq_pending;
 };
 
 struct dw_edma_irq {
@@ -109,6 +114,12 @@ struct dw_edma {
 
 	struct dw_edma_chan		*chan;
 
+	/*
+	 * WQ_HIGHPRI keeps completion processing responsive under heavy load;
+	 * WQ_UNBOUND lets different channels run on different CPUs.
+	 */
+	struct workqueue_struct		*wq;
+
 	raw_spinlock_t			lock;		/* Protect v0 shared registers */
 
 	struct dw_edma_chip             *chip;
-- 
2.51.0

Re: [PATCH v4 9/9] dmaengine: dw-edma: Defer channel IRQ handling to workqueue

[sashiko-bot]

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

Pre-existing issues:
- [High] Interrupts are requested before critical channel structures are fully initialized, leading to possible NULL pointer dereferences or uninitialized lock acquisitions on early or shared interrupts.
--

--- Patch [9]: [PATCH v4 9/9] dmaengine: dw-edma: Defer channel IRQ handling to workqueue ---
commit 9d2ebede7ce8b472f3d4f1a246b418b61a57de7a
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Defer channel IRQ handling to workqueue

This commit shifts channel IRQ handling from hard IRQ context to workqueues.
It keeps the hard IRQ handler minimal to clear status and dispatch channel
events, while deferring per-channel processing to work items to improve
concurrency.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -1190,10 +1232,21 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	/* Disable eDMA, only to establish the ideal initial conditions */
>  	dw_edma_core_off(dw);
>  
> +	/*
> +	 * Deferred IRQ works are queued from the hard IRQ handlers, so the
> +	 * workqueue must exist before any IRQ is requested.
> +	 */
> +	dw->wq = alloc_workqueue("dw-edma:%s", WQ_UNBOUND | WQ_HIGHPRI, 0,
> +				 dev_name(chip->dev));
> +	if (!dw->wq)
> +		return -ENOMEM;
> +
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);

[Severity: High]
This is a pre-existing issue, but requesting interrupts here before critical
channel structures are initialized can lead to a NULL pointer dereference.

Since dw_edma_irq_request() uses IRQF_SHARED, an interrupt from another
device sharing the line could fire immediately.

If that happens before dw_edma_channel_setup() completes, the handler
(like dw_edma_interrupt_emulated()) will unconditionally dereference the
uninitialized dw_irq->dw pointer.

Additionally, the deferred work items might attempt to acquire chan->vc.lock
before vchan_init() has been called in dw_edma_channel_setup().

Does the initialization order need to be adjusted so that structures are fully
setup before interrupts are requested?

> -	if (err)
> +	if (err) {
> +		destroy_workqueue(dw->wq);
>  		return err;
> +	}
>  
>  	/* Allocate a dedicated virtual IRQ for interrupt-emulation doorbells */
>  	err = dw_edma_emul_irq_alloc(dw);

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