Re: [PATCH v3 29/34] dmaengine: pl330: route callbacks via channel BH

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

New issues:
- [High] Calling dmaengine_init_bh() dynamically inside pl330_alloc_chan_resources() causes a kernel WARN_ON when a channel is freed and requested again.

Pre-existing issues:
- [High] Re-issuing a DMA request from within a completion callback causes an unbalanced PM runtime counter, potentially suspending the device mid-transfer.
--

--- Patch [29]: [PATCH v3 29/34] dmaengine: pl330: route callbacks via channel BH ---
commit 1ceab77643ee2c8f72930ed84f7872c29a5db235
Author: Allen Pais <[email protected]>

dmaengine: pl330: route callbacks via channel BH

Convert the per-channel tasklet to dmaengine_*_bh while keeping the
controller tasklet for non-callback events.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 25ba84b187041..1b6069ba0d5e3 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
[ ... ]
> @@ -2060,14 +2057,15 @@ static inline void fill_queue(struct dma_pl330_chan *pch)
>  			desc->status = DONE;
>  			dev_err(pch->dmac->ddma.dev, "%s:%d Bad Desc(%d)\n",
>  					__func__, __LINE__, desc->txd.cookie);
> -			tasklet_schedule(&pch->task);
> +			dmaengine_schedule_bh(&pch->chan);
>  		}
>  	}
>  }
>  
> -static void pl330_tasklet(struct tasklet_struct *t)
> +static void pl330_tasklet(struct dma_chan *chan)
>  {

[Severity: High]
This is a pre-existing issue, but can the PM runtime counter become
unbalanced if a client re-issues a transfer from within the completion
callback?

In pl330_tasklet(), the local power_down variable is set to true if the
work_list is empty:

pl330_tasklet()
    if (list_empty(&pch->work_list)) {
        power_down = true;

The lock is then dropped before invoking the client callback:

        dmaengine_desc_callback_invoke(&cb, NULL);

If the client submits a new transfer and calls dma_async_issue_pending()
from this callback, it invokes pl330_issue_pending() which calls
pm_runtime_get_sync() and starts the transfer.

When the callback returns, the outer tasklet completes and unconditionally
calls pm_runtime_put_autosuspend():

    if (power_down)
        pm_runtime_put_autosuspend(pch->dmac->ddma.dev);

Since the local power_down is still true, wouldn't this incorrectly drop
the newly acquired PM reference, potentially causing the hardware to
autosuspend while the new transfer is actively running?

> -	struct dma_pl330_chan *pch = from_tasklet(pch, t, task);
> +	struct dma_pl330_chan *pch = container_of(chan, struct dma_pl330_chan,
> +						  chan);
>  	struct dma_pl330_desc *desc, *_dt;
>  	unsigned long flags;
>  	bool power_down = false;
[ ... ]
> @@ -2173,7 +2171,7 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  		return -ENOMEM;
>  	}
>  
> -	tasklet_setup(&pch->task, pl330_tasklet);
> +	dmaengine_init_bh(&pch->chan, pl330_tasklet);
>  
>  	spin_unlock_irqrestore(&pl330->lock, flags);

[Severity: High]
When a client driver requests a DMA channel, pl330_alloc_chan_resources()
calls dmaengine_init_bh(), which sets chan->bh_work_initialized = true.

If the client later frees the channel via pl330_free_chan_resources(),
dmaengine_kill_bh() is called, but this does not clear the initialized flag.

If any client requests the same channel again, wouldn't calling
dmaengine_init_bh() here trip the WARN_ON(chan->bh_work_initialized)
check in dmaengine_init_bh()?

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