Re: [PATCH v3 11/24] dmaengine: dw-edma: Serialize LL event capture with channel kicks

[Frank Li]

On Tue, Jul 28, 2026 at 02:03:10AM +0900, Koichiro Den wrote:
> The hard IRQ path clears interrupt status before deferred handling takes
> vc.lock. If a doorbell starts a new hardware run in between, the
> deferred handler can apply an event from the previous run.
>
> Use the event lock to serialize the interrupt status read and clear,
> snapshot recording, and LL doorbell writes. Do not write another
> doorbell while a snapshot is pending. A ring reset discards the snapshot
> under the same lock. When events coalesce, keep STOP over PROGRESS.
>
> Changing chan->request alone neither changes the ring nor writes a
> doorbell, so it does not invalidate the snapshot. Preserve it across
> request changes. A later patch uses PROGRESS and STOP to decide whether
> a pending STOP or PAUSE request can finish.
>
> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> ---
> Changes in v3:
>   - New patch to serialize IRQ event capture with LL doorbell writes and
>     defer doorbells while an LL event is pending. (Sashiko)
>   - Add explicit PROGRESS and STOP types for deferred LL snapshots.
>     (Frank)
>
>  drivers/dma/dw-edma/dw-edma-core.c    | 101 +++++++++++++++++++++++++-
>  drivers/dma/dw-edma/dw-edma-core.h    |  37 ++++++++++
>  drivers/dma/dw-edma/dw-edma-v0-core.c |  64 ++++++++--------
>  drivers/dma/dw-edma/dw-hdma-v0-core.c |  32 +++++---
>  4 files changed, 187 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index e3d0559206d8..3970b066dca2 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -33,7 +33,8 @@ struct dw_edma_desc *vd2dw_edma_desc(struct virt_dma_desc *vd)
>
>  enum dw_edma_deferred_event {
>  	DW_EDMA_DEFERRED_DONE	= BIT(0),
> -	DW_EDMA_DEFERRED_ABORT	= BIT(1),
> +	DW_EDMA_DEFERRED_LL	= BIT(1),
> +	DW_EDMA_DEFERRED_ABORT	= BIT(2),
>  };
>
>  static inline
> @@ -74,6 +75,52 @@ static void vchan_free_desc(struct virt_dma_desc *vdesc)
>  	kfree(vd2dw_edma_desc(vdesc));
>  }
>
> +static void dw_edma_ll_snapshot_discard_locked(struct dw_edma_chan *chan)
> +{
> +	lockdep_assert_held(dw_edma_event_lock(chan));
> +
> +	chan->ll_irq.event = DW_EDMA_LL_EVENT_NONE;
> +}
> +
> +static void dw_edma_ll_snapshot_discard(struct dw_edma_chan *chan)
> +{
> +	guard(raw_spinlock_irqsave)(dw_edma_event_lock(chan));
> +
> +	dw_edma_ll_snapshot_discard_locked(chan);
> +}
> +
> +/* Must be called with vc.lock held. */
> +static bool
> +dw_edma_ll_snapshot_take(struct dw_edma_chan *chan,
> +			 struct dw_edma_ll_snapshot *snapshot)
> +{
> +	guard(raw_spinlock_irqsave)(dw_edma_event_lock(chan));

Not sure previous why use raw_spinlock, suppose it is wrong. should be
spin_lock.

> +
> +	if (chan->ll_irq.event == DW_EDMA_LL_EVENT_NONE)
> +		return false;
> +
> +	*snapshot = chan->ll_irq;
> +	dw_edma_ll_snapshot_discard_locked(chan);
> +
> +	return true;
> +}
> +
...
>
> +/*
> + * Lock ordering:
> + *
> + *   chan->vc.lock -> dw_edma_event_lock(chan)
> + *
> + * Interrupt providers invoke the dw_edma_handler_t callback with the event
> + * lock held. The callback must not take vc.lock.
> + */
> +static inline raw_spinlock_t *dw_edma_event_lock(struct dw_edma_chan *chan)
> +{
> +	if (chan->dw->core->event_scope == DW_EDMA_EVENT_PER_DIR)
> +		return &chan->dw->event_lock[chan->dir];
> +
> +	return &chan->event_lock;

can we change chan:: event_lock to pointer,

when DW_EDMA_EVENT_PER_DIR

	chan->event_lock -> chan->dw->event_lock[chan->dir]

else
	chan->event_lock -> create a lock when register HDMA channel.

So needn't DW_EDMA_EVENT_PER_DIR,


> +}
> +
>  /*
>   * Return the current LL entry index. A negative value means that the channel
>   * context is not initialized or was lost after a link reset.
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index b3098eb83442..43e33a400a3e 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -339,8 +339,8 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  			   dw_edma_handler_t handler)
>  {
>  	struct dw_edma *dw = dw_irq->dw;
> -	unsigned long total, pos, val;
> -	irqreturn_t ret = IRQ_NONE;
> +	u8 events[EDMA_V0_MAX_NR_CH] = {};
> +	unsigned long total, pos, val, active = 0;
>  	struct dw_edma_chan *chan;
>  	unsigned long off;
>  	unsigned long *mask;
> @@ -356,45 +356,45 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  		mask = dw_irq->rd_mask;
>  	}
>
> -	/*
> -	 * 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];
> +	scoped_guard(raw_spinlock_irqsave, &dw->event_lock[dir]) {

use helper dw_edma_event_lock()

Frank
> +		/*
> +		 * 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.
> +		 */
> +		sts = GET_RW_32(dw, dir, int_status);
>
> -		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> -			continue;
> +		val = FIELD_GET(EDMA_V0_DONE_INT_MASK, sts);
> +		val &= *mask;
> +		for_each_set_bit(pos, &val, total) {
> +			chan = &dw->chan[pos + off];
>
> -		dw_edma_v0_core_clear_done_int(chan);
> -		handler(chan, DW_EDMA_IRQ_DONE);
> +			if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> +				continue;
>
> -		ret = IRQ_HANDLED;
> -	}
> +			events[pos] |= DW_EDMA_IRQ_DONE;
> +			active |= BIT(pos);
> +			dw_edma_v0_core_clear_done_int(chan);
> +		}
>
> -	val = FIELD_GET(EDMA_V0_ABORT_INT_MASK, sts);
> -	val &= *mask;
> -	for_each_set_bit(pos, &val, total) {
> -		chan = &dw->chan[pos + off];
> +		val = FIELD_GET(EDMA_V0_ABORT_INT_MASK, sts);
> +		val &= *mask;
> +		for_each_set_bit(pos, &val, total) {
> +			chan = &dw->chan[pos + off];
>
> -		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> -			continue;
> +			if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> +				continue;
>
> -		dw_edma_v0_core_clear_abort_int(chan);
> -		handler(chan, DW_EDMA_IRQ_ABORT);
> +			events[pos] |= DW_EDMA_IRQ_ABORT;
> +			active |= BIT(pos);
> +			dw_edma_v0_core_clear_abort_int(chan);
> +		}
>
> -		ret = IRQ_HANDLED;
> +		for_each_set_bit(pos, &active, total)
> +			handler(&dw->chan[pos + off], events[pos]);
>  	}
>
> -	return ret;
> +	return active ? IRQ_HANDLED : IRQ_NONE;
>  }
>
>  static void dw_edma_v0_write_ll_data(struct dw_edma_chan *chan, int i,
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 64a07fbc5be3..bbf9ee0392f8 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -177,10 +177,12 @@ dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  			   dw_edma_handler_t handler)
>  {
>  	struct dw_edma *dw = dw_irq->dw;
> -	unsigned long total, pos, val;
> +	unsigned int events;
> +	unsigned long total, pos;
>  	irqreturn_t ret = IRQ_NONE;
>  	struct dw_edma_chan *chan;
>  	unsigned long off, *mask;
> +	u32 val;
>
>  	if (dir == EDMA_DIR_WRITE) {
>  		total = dw->wr_ch_cnt;
> @@ -197,20 +199,28 @@ dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
>  			continue;
>
> -		val = dw_hdma_v0_core_status_int(chan);
> -		if (FIELD_GET(HDMA_V0_STOP_INT_MASK, val)) {
> -			dw_hdma_v0_core_clear_done_int(chan);
> -			handler(chan, DW_EDMA_IRQ_STOP);
> +		events = 0;
> +		scoped_guard(raw_spinlock_irqsave, dw_edma_event_lock(chan)) {
> +			val = dw_hdma_v0_core_status_int(chan);
>
> -			ret = IRQ_HANDLED;
> -		}
> +			if (FIELD_GET(HDMA_V0_STOP_INT_MASK, val)) {
> +				events |= DW_EDMA_IRQ_STOP;
> +				dw_hdma_v0_core_clear_done_int(chan);
> +			}
>
> -		if (FIELD_GET(HDMA_V0_ABORT_INT_MASK, val)) {
> -			dw_hdma_v0_core_clear_abort_int(chan);
> -			handler(chan, DW_EDMA_IRQ_ABORT);
> +			if (FIELD_GET(HDMA_V0_ABORT_INT_MASK, val)) {
> +				events |= DW_EDMA_IRQ_ABORT;
> +				dw_hdma_v0_core_clear_abort_int(chan);
> +			}
>
> -			ret = IRQ_HANDLED;
> +			if (events)
> +				handler(chan, events);
>  		}
> +
> +		if (!events)
> +			continue;
> +
> +		ret = IRQ_HANDLED;
>  	}
>
>  	return ret;
> --
> 2.51.0
>