Re: [PATCH v2 16/19] dmaengine: dw-edma: Add engine recovery infrastructure

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

New issues:
- [High] Hardware interrupts are disabled via free_irq() before the hardware engine is stopped, risking an interrupt storm.
- [High] Passive stall detection will fail to trigger recovery for sleeping clients, leading to indefinite hangs.
- [High] Queueing work on a destroyed workqueue during driver removal leads to a kernel crash.
- [Low] Mixing goto-based cleanup labels with guard()/scoped_guard() declarations in the same function.

Pre-existing issues:
- [High] IRQs are requested before channels are fully initialized, leading to potential crashes from early interrupts.
--

--- Patch [16]: [PATCH v2 16/19] dmaengine: dw-edma: Add engine recovery infrastructure ---
commit 348674e8eed041726892cbce36b8d539a278c6d1
Author: Koichiro Den <[email protected]>
Subject: dmaengine: dw-edma: Add engine recovery infrastructure

This commit adds a worker to coordinate engine recovery in the dw-edma
driver. It gates publication and doorbells, waits for channels to stop,
and rebuilds linked-list contexts after resetting the engine.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 9305137f85f67..14cd0038dbd27 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> +static void dw_edma_engine_recovery_work(struct work_struct *work)
> +{
> +	struct dw_edma_engine_recovery *rec =
> +		container_of(work, struct dw_edma_engine_recovery, work);
> +	struct dw_edma *dw = rec->dw;
> +	const char *dir_name = str_write_read(rec->dir == EDMA_DIR_WRITE);
> +	u16 off = rec->dir == EDMA_DIR_WRITE ? 0 : dw->wr_ch_cnt;
> +	u16 cnt = rec->dir == EDMA_DIR_WRITE ? dw->wr_ch_cnt : dw->rd_ch_cnt;
> +	unsigned long timeout =
> +		jiffies + msecs_to_jiffies(DW_EDMA_ENGINE_QUIESCE_TIMEOUT_MS);
> +	struct dw_edma_chan *chan;
> +	bool configured_ll;
> +	bool busy;
> +	u16 i;
> +
> +	if (!dw_edma_engine_recovery_needed(rec)) {
> +		if (READ_ONCE(rec->active))
> +			dw_edma_engine_recovery_release(rec);
> +		return;
> +	}
> +
> +	WRITE_ONCE(rec->active, true);
> +
> +	/* Gate each channel before inspecting or resetting the direction. */
> +	for (i = 0; i < cnt; i++) {
> +		chan = &dw->chan[off + i];
> +		guard(spinlock_irqsave)(&chan->vc.lock);

[Severity: Low]
Does this code violate the cleanup subsystem guidelines by mixing goto-based
cleanup labels with scope-based guards?

The cleanup subsystem guide explicitly states that functions must not mix
goto-based cleanup labels with guard() or scoped_guard() declarations. This
function uses a goto out_release; label further down, while declaring
guard(spinlock_irqsave) here in the loop.

[ ... ]

> +		if (moving) {
> +			dev_warn_ratelimited(dw->chip->dev,
> +					     "%s engine quiesce timed out with transfers still progressing, retrying\n",
> +					     dir_name);
> +			queue_work(dw->wq, &rec->work);
> +			return;
> +		}
> +
> +		dev_warn(dw->chip->dev,
> +			 "%s engine reset with a channel frozen mid-element\n",
> +			 dir_name);
> +	}
> +
> +	if (!dw_edma_engine_recovery_needed(rec))
> +		goto out_release;

[ ... ]

> +	if (!dw->core->engine_reset(dw, rec->dir)) {
> +		/*
> +		 * Keep channels gated and do not re-enable the engine unless
> +		 * ENGINE_EN clears.
> +		 */
> +		if (++rec->fails >= DW_EDMA_ENGINE_RESET_MAX_FAILS) {
> +			dev_err(dw->chip->dev,
> +				"%s engine did not drain after %u attempts; leaving channels gated\n",
> +				dir_name, rec->fails);
> +			return;
> +		}
> +		queue_work(dw->wq, &rec->work);

[Severity: High]
Can passive stall detection fail to trigger this recovery for clients that
are sleeping?

The stall check relies on dw_edma_ll_stall_check() reading timeouts, but
it appears to only be invoked when dw_edma_core_ch_maybe_doorbell() is
called. If a hardware channel freezes and generates no further interrupts
while a client waits via wait_for_completion(), could the stall go
completely undetected because the status is never actively polled?

[ ... ]

> @@ -1640,6 +1905,13 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  
>  	raw_spin_lock_init(&dw->lock);
>  
> +	for (i = 0; i < ARRAY_SIZE(dw->eng_recovery); i++) {
> +		dw->eng_recovery[i].dw = dw;
> +		dw->eng_recovery[i].dir = i;
> +		INIT_WORK(&dw->eng_recovery[i].work,
> +			  dw_edma_engine_recovery_work);
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does dw_edma_probe() register interrupts
before the driver's internal channel state is initialized?

In dw_edma_probe(), the interrupt handler is registered by calling
dw_edma_irq_request() before the channel structures and work items are
fully initialized via dw_edma_channel_setup().

If a spurious or shared interrupt fires immediately after registration:

dw_edma_probe()
   dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);
   ...
   dw_edma_channel_setup(dw, wr_alloc, rd_alloc);

Could this attempt to access or queue work using zeroed memory, leading to
a kernel crash?

[ ... ]

> @@ -1738,11 +2010,6 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  	if (!dw)
>  		return -ENODEV;
>  
> -	if (chip->flags & DW_EDMA_CHIP_PARTIAL)
> -		err = dw_edma_core_quiesce(dw);
> -	else
> -		dw_edma_core_off(dw);
> -
>  	/* Free irqs */
>  	for (i = (dw->nr_irqs - 1); i >= 0; i--)
>  		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
> @@ -1751,6 +2018,16 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  	for (i = 0; i < dw->wr_ch_cnt + dw->rd_ch_cnt; i++)
>  		cancel_work_sync(&dw->chan[i].irq_work);
>  
> +	/* Prevent a running recovery worker from requeueing itself. */
> +	disable_work_sync(&dw->eng_recovery[0].work);
> +	disable_work_sync(&dw->eng_recovery[1].work);
> +
> +	/* A recovery worker can re-enable the engine, so stop it last. */
> +	if (chip->flags & DW_EDMA_CHIP_PARTIAL)
> +		err = dw_edma_core_quiesce(dw);
> +	else
> +		dw_edma_core_off(dw);
> +

[Severity: High]
Will moving dw_edma_core_off() after free_irq() risk an interrupt storm?

By tearing down the interrupt handler before stopping the hardware, the
hardware remains fully active while its handler is unregistered. If the
engine generates an interrupt, a shared interrupt handler would return
IRQ_NONE. Because the hardware interrupt is never cleared, won't the CPU
get stuck in an interrupt loop until the kernel disables the shared IRQ
line completely?

>  	destroy_workqueue(dw->wq);
>  
>  	/* Deregister eDMA device */

[Severity: High]
Could destroying the workqueue before unregistering the DMA device cause
a use-after-free crash?

Since destroy_workqueue() is called before dma_async_device_unregister(),
the device is still visible to clients. If a client software calls
dmaengine_tx_status() during driver unload, it can invoke
dw_edma_core_ch_maybe_doorbell(). With this patch, the doorbell logic now
eventually hits dw_edma_ll_stall_check(), which can call
queue_work(chan->dw->wq, &rec->work). 

Could this attempt to queue work on the already-destroyed workqueue?

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