Re: [PATCH v2 01/13] dmaengine: ste_dma40: Fix failed start cleanup

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

New issues:
- [Medium] Out-of-order descriptor completion corrupts dmaengine cookie tracking.
- [Medium] Failed cyclic descriptors are moved to the `done` list, causing a permanent tasklet soft-lock.

Pre-existing issues:
- [High] The return value of pm_runtime_get_sync() is ignored, risking hardware access on a powered-off device.
--

--- Patch [1]: [PATCH v2 01/13] dmaengine: ste_dma40: Fix failed start cleanup ---
commit b33a3592aafc5e2714045326450fe12077ee3161
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix failed start cleanup

This commit addresses a bug where d40_queue_start() returned NULL on start
failure without unwinding the transfer state or clearing the busy flag. It
introduces an error path to complete the descriptor through the tasklet and
balance the runtime PM reference.

Closes: https://lore.kernel.org/dmaengine/[email protected]/

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -1531,8 +1531,15 @@ static struct d40_desc *d40_queue_start(struct d40_chan *d40c)
>  	if (d40d != NULL) {
>  		if (!d40c->busy) {
>  			d40c->busy = true;
>  			pm_runtime_get_sync(d40c->base->dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_get_sync() here?

If the power domain fails to resume, the driver might proceed with hardware
register accesses like d40_desc_load() and d40_start() while the device is
powered off, which could trigger a synchronous external abort.

[ ... ]
>  
>  		/* Start dma job */
>  		err = d40_start(d40c);
>  
> -		if (err)
> -			return NULL;
> +		if (err) {
> +			d40_desc_remove(d40d);
> +			d40_desc_done(d40c, d40d);

[Severity: Medium]
Does calling d40_desc_done() here break dmaengine cookie tracking by
completing descriptors out-of-order?

If dma_tc_handle() calls d40_queue_start() and this new descriptor fails to
start, it is placed into the done list. Then dma_tc_handle() proceeds to
place the successfully completed old descriptor into the done list. Since it
is a FIFO queue, the newly failed descriptor gets processed before the old
successful one, which could cause the channel's completed cookie to go
backwards.

[Severity: Medium]
Also, if the descriptor is cyclic, could moving it to the done list upon
failure cause a permanent tasklet soft-lock?

The dma_tasklet() deliberately skips removing cyclic descriptors from the
done list:

dma_tasklet() {
        ...
        if (!d40d->cyclic) {
                if (async_tx_test_ack(&d40d->txd)) {
                        d40_desc_remove(d40d);
        ...
}

If a failed cyclic descriptor is forced into the done list, it looks like it
would never be dequeued, causing the tasklet to infinitely loop and starve
other DMA jobs.

> +			d40c->pending_tx++;
> +			d40c->busy = false;
> +			pm_runtime_put_autosuspend(d40c->base->dev);
> +			tasklet_schedule(&d40c->tasklet);
> +			return ERR_PTR(err);
> +		}
>  	}
>  
>  	return d40d;

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