Re: [PATCH v6 4/6] dmaengine: dw-edma: Add delegated channel request helpers

On Tue, Aug 04, 2026 at 03:52:29AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Releasing a single delegated channel with `quiesce=true` on eDMA v0 hardware causes a data race on `engine_en` and incorrectly shuts down the entire DMA direction, disrupting all other local channels.
> --
> 
> --- Patch [4]: [PATCH v6 4/6] dmaengine: dw-edma: Add delegated channel request helpers ---
> commit 0987248b986738e4870e25b1f36d8c9f2eb69d8b
> Author: Koichiro Den <[email protected]>
> 
> dmaengine: dw-edma: Add delegated channel request helpers
> 
> This commit adds DesignWare eDMA-specific helpers to request and release
> hardware channels for delegation to a remote host. The release helper
> supports quiescing the channel before restoring default routing and
> releasing the DMAengine reservation.
> 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 1f893dc54c793..4c0d2bba755b5 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> [ ... ]
> > +void dw_edma_release_delegated_chan(struct dma_chan *dchan, bool quiesce)
> > +{
> > +	struct dw_edma_chan *chan;
> > +
> > +	if (!dchan)
> > +		return;
> > +
> > +	chan = dchan2dw_edma_chan(dchan);
> > +	if (quiesce && dw_edma_core_ch_quiesce(chan))
> 
> [Severity: High]
> Does releasing a delegated channel with quiesce set to true unintentionally
> disrupt all other active local channels in the same direction?
> 
> If an endpoint driver uses some channels locally while delegating others to a
> remote host, this call to dw_edma_core_ch_quiesce() appears to escalate to a
> direction-wide shutdown on eDMA v0 hardware:
> 
> drivers/dma/dw-edma/dw-edma-v0-core.c:dw_edma_v0_core_ch_quiesce() {
>     ...
>     return dw_edma_v0_core_dir_off(chan->dw, chan->dir);
> }
> 
> dw_edma_v0_core_dir_off() forcefully masks interrupts for all channels in the
> direction and locklessly clears the engine_en register via
> dw_edma_v0_core_engine_disable():
> 
> drivers/dma/dw-edma/dw-edma-v0-core.c:dw_edma_v0_core_engine_disable() {
>     ...
>     SET_RW_32(dw, dir, engine_en, 0);
> }
> 
> Since the per-channel delegation API does not enforce exclusive ownership of
> the direction, does this cause a data race and abort ongoing DMA transfers on
> unrelated active local channels?

That does not happen with the current consumer.

Patch 3 documents that the caller must own the whole provider sharing group and
stop peer programming before reclaim. pci_epf_dma follows it, ie. for eDMA
unroll and HDMA compatibility maps, it rejects partial delegation of a
direction. Native HDMA quiesce is per-channel.

A future consumer could miss this rule, yes. But adding group tracking to this
generic per-channel API feels too much. I think the documented caller
requirement is enough and keeps the design simple.

Best regards,
Koichiro

> 
> > +		dev_warn(chan->dw->chip->dev,
> > +			 "failed to quiesce delegated %s channel %u\n",
> > +			 chan->dir == EDMA_DIR_WRITE ? "write" : "read",
> > +			 chan->id);
> > +
> > +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> > +		chan->irq_mode = dw_edma_get_default_irq_mode(chan);
> > +
> > +	dma_release_channel(dchan);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4