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

[Koichiro Den]

On Mon, Jul 27, 2026 at 04:35:28PM -0400, Frank Li wrote:
> 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.

Thanks for pointing it out. I checked all event_lock critical sections again,
and yes, you're absolutely right. spinlock_t is enough because:
- the lock serializes interrupt status/LLP capture with an LL kick
  (doorbell). There is no point in ensuring the holder non-preemptible on
  PREEMPT_RT.
- dw_edma_record_irq() builds the new snapshot locally and publishes it only
  once it is fully formed.

> 
> > +
> > +	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,

That looks simpler. Thanks for the suggestion, I'll rework it that way.

> 
> 
> > +}
> > +
> >  /*
> >   * 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()

Sure. My mistake.

Thanks for the review.
Koichiro

> 
> 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
> >