Re: [PATCH v4 3/9] dmaengine: dw-edma: Serialize abort state updates

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing lock in dw_edma_device_pause and dw_edma_device_resume causes Data Race and Use-After-Free.
- [High] Permanent channel stall when resuming from PAUSE due to bypassed descriptor completion.
- [High] Unexpected hardware abort leaves queued descriptors stranded indefinitely.
--

--- Patch [3]: [PATCH v4 3/9] dmaengine: dw-edma: Serialize abort state updates ---
commit 4837da5eb731c888cd532d74c75c719d7336551e
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Serialize abort state updates

This commit fixes a race condition where issue_pending() could observe an
inconsistent state. It extends the critical section in
dw_edma_abort_interrupt() to cover the channel request and status updates.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 972c7ea18d46b..daf70df59e077 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -736,9 +736,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);
>  	}
> -	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  	chan->request = EDMA_REQ_NONE;
>  	chan->status = EDMA_ST_IDLE;

[Severity: High]
This is a pre-existing issue, but does an unexpected hardware abort leave
queued descriptors stranded indefinitely here?

On a hardware abort (not requested by software), dw_edma_abort_interrupt()
fetches the single active descriptor, completes it with DMA_TRANS_ABORTED,
and unconditionally transitions the channel to EDMA_ST_IDLE.

Could this ignore any queued descriptors remaining in vc->desc_issued?
Since the hardware is now marked IDLE without processing the remaining
queue, it seems these descriptors are permanently stranded until the
client initiates a new dmaengine_issue_pending() call.

> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing lock in
dw_edma_device_pause() and dw_edma_device_resume() that could cause a data
race and use-after-free?

The dmaengine API allows clients to call dmaengine_resume() and
dmaengine_pause() without driver-level locks. In dw_edma_device_pause()
and dw_edma_device_resume(), the state is modified locklessly:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_pause() {
    ...
    chan->request = EDMA_REQ_PAUSE;
    ...
}

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
    chan->status = EDMA_ST_BUSY;
    dw_edma_start_transfer(chan);
    ...
}

If dw_edma_device_resume() is called concurrently with an interrupt handled
on another CPU, could both CPUs execute dw_edma_start_transfer() on the
same descriptor?

Both would access the same child chunk, pass it to dw_edma_core_start(),
and free it, which could result in list corruption and a use-after-free:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_start_transfer() {
    ...
    list_del(&child->list);
    kfree(child);
    ...
}


[Severity: High]
This is a pre-existing issue, but can the channel stall permanently when
resuming from PAUSE due to bypassed descriptor completion?

When the final chunk of a transfer finishes, its chunks_alloc drops to 0.
If EDMA_REQ_PAUSE is handled in this interrupt, the switch in
dw_edma_done_interrupt() bypasses the completion logic, leaving the fully
finished descriptor in the issued list:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt() {
    ...
        case EDMA_REQ_PAUSE:
            chan->request = EDMA_REQ_NONE;
            chan->status = EDMA_ST_PAUSE;
            break;
    ...
}

Later, dw_edma_device_resume() sets the state to BUSY unconditionally:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
    chan->status = EDMA_ST_BUSY;
    dw_edma_start_transfer(chan);
    ...
}

Because the chunk list is empty, dw_edma_start_transfer() fails to start
the hardware and returns 0. Could this mismatched software state leave
the channel permanently deadlocked in a BUSY state?

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