Re: [PATCH v6 07/10] dmaengine: dw-edma: Add non_ll_start() callback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The DMA engine control callbacks (dw_edma_device_resume, dw_edma_device_pause, and dw_edma_device_terminate_all) modify channel state locklessly and perform unsafe virt-dma list operations without holding the required virtual channel lock.
--

--- Patch [7]: [PATCH v6 07/10] dmaengine: dw-edma: Add non_ll_start() callback ---
commit 659e1f56ed5af4e60c5ded05c2fdc117d6a334f9
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add non_ll_start() callback

Add a non_ll_start() callback and move the common non-linked-list channel
handling into the EDMA core so it can be shared by both the EDMA and HDMA.
Prepare for the upcoming reorganization of the burst and chunk structures.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index bab4d49c92feb..e18d6e827c2c9 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h

[ ... ]

> @@ -201,7 +202,16 @@ dw_edma_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  static inline
>  void dw_edma_core_start(struct dw_edma *dw, struct dw_edma_chunk *chunk, bool first)
>  {
> -	dw->core->start(chunk, first);
> +	if (chunk->chan->non_ll) {
> +		struct dw_edma_burst *child;
> +
> +		child = list_first_entry_or_null(&chunk->burst->list,
> +						 struct dw_edma_burst, list);
> +		if (child)
> +			dw->core->non_ll_start(chunk->chan, child);
> +	} else {
> +		dw->core->start(chunk, first);
> +	}
>  }

[Severity: High]
This is a pre-existing issue, but while refactoring the start routines, are
there some missing virt-dma lock protections in the broader EDMA core?

For instance, in drivers/dma/dw-edma/dw-edma-core.c, dw_edma_device_resume()
seems to call dw_edma_start_transfer() without holding the virtual channel
lock:

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

Because dw_edma_start_transfer() pops elements via vchan_next_desc(), could
this lead to list corruption on the virt-dma list when a client driver resumes
the engine?

Similarly, does dw_edma_device_pause() modify the channel request state
locklessly?

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

Could this cause a data race with the interrupt handler
dw_edma_done_interrupt() which runs concurrently?

Finally, in dw_edma_device_terminate_all() the state is also updated
locklessly:

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

Does this fail to call vchan_get_all_descriptors() to clean up pending
virt-dma lists? Could this cause descriptors to leak or mistakenly execute
later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-edma_ll-v6-0-1471d278b73a@nxp.com?part=7