Re: [PATCH v3 02/13] dmaengine: dw-edma: Add core quiesce operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The v0 eDMA `ch_quiesce` operation globally disables the DMA engine and clobbers interrupt states for all channels in the direction.
- [High] `dw_edma_v0_core_dir_off` modifies the shared direction-wide `int_mask` register without holding `dw->lock`, causing a data race.
--

commit d7eb32d006bf71fa3372ef97f6fa793fa851c5c8
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Add core quiesce operations

This adds core operations to quiesce only the resources represented by a
dw-edma instance. It implements per-channel and per-direction quiesce
operations for v0 eDMA and HDMA compatible register layouts.

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 1781ba4f022ec..316d8c94eff9c 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c

[ ... ]

> @@ -160,6 +160,15 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch,
>  	readl_ch(dw, dir, ch, &(__dw_ch_regs(dw, dir, ch)->name))
>  
>  /* eDMA management callbacks */
> +static void dw_edma_v0_core_dir_off(struct dw_edma *dw, enum dw_edma_dir dir)
> +{
> +	SET_RW_32(dw, dir, int_mask,
> +		  EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK);

[Severity: High]
Does this modification of the shared int_mask register need to be
protected by dw->lock?

In dw_edma_v0_core_start(), modifications to int_mask are protected by
raw_spin_lock_irqsave(&dw->lock, flags). However,
dw_edma_v0_core_dir_off() directly overwrites the entire int_mask
register without acquiring dw->lock.

Could this cause a data race where a concurrent read-modify-write cycle
in dw_edma_v0_core_start() clobbers the mask state configured here,
leaving the interrupt configuration inconsistent?

> +	SET_RW_32(dw, dir, int_clear,
> +		  EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK);
> +	SET_RW_32(dw, dir, engine_en, 0);
> +}

[ ... ]

> +static void dw_edma_v0_core_ch_quiesce(struct dw_edma_chan *chan)
> +{
> +	dw_edma_v0_core_dir_off(chan->dw, chan->dir);
> +}

[Severity: High]
Will this globally disable the DMA engine and clobber interrupt states
for all active channels in this direction?

The .ch_quiesce operation is intended as a per-channel action. However,
dw_edma_v0_core_ch_quiesce() wraps dw_edma_v0_core_dir_off(), which
writes 0 to engine_en and masks all interrupts for the entire direction.

If a device has multiple active channels in the same direction, would
releasing one delegated channel halt and silently drop pending interrupts
for all other active channels in that direction?

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