Re: [PATCH v2 15/19] dmaengine: dw-edma: Add engine reset and enable operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dw_edma_v0_core_engine_reset() fails to handle HDMA compatibility mode, unconditionally accessing the unimplemented ENGINE_EN register and failing to properly stop channels.
--

--- Patch [15]: [PATCH v2 15/19] dmaengine: dw-edma: Add engine reset and enable operations ---
commit 480649ecac905454f1c85b3804e44856e4e73d24
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Add engine reset and enable operations

This commit adds separate engine_reset and engine_enable operations to allow
rebuilding LL channel contexts while the engine is disabled. Reset clears
ENGINE_EN and waits for it to read back zero, while enable restores the
direction and handles HDMA compatibility mode.

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c

[ ... ]

> @@ -220,6 +220,65 @@ static int dw_edma_v0_core_engine_disable(struct dw_edma *dw,

[ ... ]

> +static bool dw_edma_v0_core_engine_reset(struct dw_edma *dw,
> +					 enum dw_edma_dir dir)
> +{
> +	/*
> +	 * The databook says clearing ENGINE_EN resets the DMA logic while
> +	 * preserving configuration registers, but does not document drain
> +	 * semantics. On the tested integration, ENGINE_EN read back as zero
> +	 * only after outstanding transactions appeared to drain; the stalled
> +	 * channel then resumed after reset.
> +	 *
> +	 * If ENGINE_EN does not clear, do not re-enable the engine. On the
> +	 * tested integration, re-enabling after such a timeout wedged the
> +	 * controller and also blocked inbound BAR accesses. Return failure so
> +	 * the caller can keep channels gated and retry.
> +	 */
> +	if (dw_edma_v0_core_engine_disable(dw, dir)) {

[Severity: High]
Does this function need to handle HDMA compatibility mode similarly to how
it is handled in dw_edma_v0_core_engine_enable() and 
dw_edma_v0_core_dir_off()?

Since dw_edma_v0_core_engine_disable() unconditionally accesses the
ENGINE_EN register, wouldn't calling this in EDMA_MF_HDMA_COMPAT mode cause
a 200ms timeout in read_poll_timeout() since that register isn't 
implemented?

As noted in the driver comments, failing to clear this register might wedge
the controller and block inbound PCIe BAR accesses. Could this regression be
avoided by bypassing or handling the disable step correctly when in HDMA 
mode?

> +		dw_edma_v0_core_engine_dump(dw, dir);
> +		return false;
> +	}
> +
> +	/* Discard interrupt status belonging to the contexts being reset. */
> +	SET_RW_32(dw, dir, int_clear,
> +		  EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15