Re: [PATCH v3 03/13] dmaengine: dw-edma: Add delegated channel request helpers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Delegating a channel to a remote host fails to mask its hardware interrupts locally, resulting in an unrecoverable infinite IRQ storm and permanent IRQ disablement.
- [High] Calling `dw_edma_release_delegated_chan` with `quiesce=true` forcefully disables the entire DMA direction on eDMA v0, disrupting all other active channels.
- [Medium] Lockless modification of shared hardware registers `int_mask`, `int_clear`, and `engine_en` in `dw_edma_v0_core_dir_off()` races with channel start.

Pre-existing issues:
- [High] `dw_edma_device_terminate_all()` accesses and modifies channel state locklessly, causing a TOCTOU data race with the interrupt handler.
--

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

dmaengine: dw-edma: Add delegated channel request helpers

Endpoint functions that expose endpoint-local DesignWare eDMA channels
to a remote host need to reserve exact hardware channels and hand
interrupt ownership to the remote side before publishing the channels.
This commit adds helpers to request hardware channels through DMAengine
and switch the interrupt routing after reservation. It also adds a
matching release helper that can optionally quiesce the channel.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 7a24248b84e94..ca0504eac1fca 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -1192,6 +1192,87 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> +static int dw_edma_delegate_chan(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +		return -EINVAL;
> +	if (chan->configured || chan->status != EDMA_ST_IDLE ||
> +	    chan->request != EDMA_REQ_NONE)
> +		return -EBUSY;
> +
> +	chan->irq_mode = DW_EDMA_CH_IRQ_REMOTE;

[Severity: Critical]
Does setting the software irq_mode to DW_EDMA_CH_IRQ_REMOTE without updating
the hardware interrupt mask leave the local hardware interrupts enabled?

If a channel was previously used locally, dw_edma_v0_core_start() clears its
interrupt mask bit in the hardware int_mask register.

When delegated to a remote host, the remote host programs and starts a
transfer, eventually asserting the DONE or ABORT interrupt line.

The local interrupt handler dw_edma_v0_core_handle_int() checks the channel:

dw_edma_v0_core_handle_int() {
        ...
        if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
                continue;
        ...
}

Because dw_edma_core_ch_ignore_irq() returns true for a remote-routed
channel, the handler skips clearing the interrupt and returns IRQ_NONE.

Can this result in an unrecoverable infinite IRQ storm and permanent IRQ
disablement for all active local DMA channels?

> +
> +	return 0;
> +}

[ ... ]

> +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 calling dw_edma_core_ch_quiesce() here destructively abort all other
active channels in the same direction on eDMA v0 hardware?

When dw_edma_core_ch_quiesce() is called on v0 hardware, it maps to
dw_edma_v0_core_dir_off():

dw_edma_v0_core_ch_quiesce() {
        ...
        dw_edma_v0_core_dir_off(chan->dw, chan->dir);
        ...
}

Which then forcefully zeros out the global engine_en register and masks
interrupts for the entire direction:

dw_edma_v0_core_dir_off() {
        ...
        SET_RW_32(dw, dir, engine_en, 0);
        ...
}

If other independent software clients are actively using different channels
in the same direction, will they be disrupted?

[Severity: Medium]
Is it safe to call dw_edma_core_ch_quiesce() without acquiring the dw->lock
spinlock?

dw_edma_v0_core_dir_off() performs lockless writes to shared direction-wide
registers like int_mask and engine_en:

dw_edma_v0_core_dir_off() {
        ...
        SET_RW_32(dw, dir, int_mask,
                  EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK);
        ...
}

Concurrently, another CPU could be running dw_edma_v0_core_start() for a
different channel in the same direction, which performs a read-modify-write
on the exact same int_mask register while holding dw->lock:

dw_edma_v0_core_start() {
        ...
        raw_spin_lock_irqsave(&dw->lock, flags);
        tmp = GET_RW_32(dw, chan->dir, int_mask);
        ...
        SET_RW_32(dw, chan->dir, int_mask, tmp);
        ...
}

Could this data race result in lost updates to the interrupt mask?

> +	chan->irq_mode = dw_edma_get_irq_mode(chan);
> +	dma_release_channel(dchan);
> +}
> +EXPORT_SYMBOL_GPL(dw_edma_release_delegated_chan);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_terminate_all()
evaluate and manipulate channel state locklessly?

dw_edma_device_terminate_all() {
        ...
        if (!chan->configured) {
                /* Do nothing */
        } else if (chan->status == EDMA_ST_PAUSE) {
        ...
}

Concurrently, dw_edma_done_interrupt() handles completed transfers and
manipulates the exact same fields while holding the chan->vc.lock:

dw_edma_done_interrupt() {
        ...
        spin_lock_irqsave(&chan->vc.lock, flags);
        vd = vchan_next_desc(&chan->vc);
        if (vd) {
                switch (chan->request) {
        ...
}

Can this lack of synchronization corrupt the DMA channel's state machine,
leading to leaked descriptors or failures to properly abort ongoing
transfers if terminate_all races with the interrupt handler?

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