Re: [PATCH v3 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `vc.lock` acquisition in `dw_edma_device_resume`, `dw_edma_device_pause`, and `dw_edma_device_terminate_all` leads to severe race conditions with the interrupt handler.
- [High] Missing endianness conversions for DMA descriptors on DW_EDMA_CHIP_LOCAL configurations.
--

commit 4cc66a09b65be2e8496de1853db67b385be4c5bb
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

Reusing ll_region.sz as the transfer size is misleading because
ll_region.sz represents the memory size of the EDMA link list, not the
amount of data to be transferred.

Add a new xfer_sz field to explicitly indicate the total transfer size
of a chunk.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 1fec1b52e3d47..53469c8c8b82e 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -192,7 +192,7 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  		return 0;
>  
>  	dw_edma_core_start(dw, child, !desc->xfer_sz);
> -	desc->xfer_sz += child->ll_region.sz;
> +	desc->xfer_sz += child->xfer_sz;

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition
involving dw_edma_start_transfer() and the DMA control functions.

Does dw_edma_device_resume() safely handle concurrent accesses with the
interrupt handler?

It appears dw_edma_device_resume() calls dw_edma_start_transfer()
locklessly, which eventually invokes vchan_next_desc(). However,
vchan_next_desc() explicitly requires vc.lock to be held.

Similar unlocked modifications occur in dw_edma_device_pause(), where
chan->request is modified without locks:

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

Can this race with dw_edma_done_interrupt() (which accesses these fields
under vc.lock) and lead to list corruption or lockdep warnings?

>  	dw_edma_free_burst(child);
>  	list_del(&child->list);
>  	kfree(child);

[ ... ]

[Severity: High]
This is a pre-existing issue, but are there missing endianness conversions
for DMA descriptors on DW_EDMA_CHIP_LOCAL configurations?

In drivers/dma/dw-edma/dw-edma-v0-core.c:dw_edma_v0_write_ll_data():

    lli->transfer_size = size;
    lli->sar.reg = sar;
    lli->dar.reg = dar;
    dma_wmb();
    lli->control = control;

These native assignments write host-endian values. On Big-Endian systems
using local eDMA controllers, will the Little-Endian eDMA hardware
misinterpret addresses, lengths, and control bits?

Similar native assignments also appear in dw_edma_v0_write_ll_link() and
dw_hdma_v0_write_ll_data().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-edma_ll-v3-0-877aa463740c@nxp.com?part=2