Re: [PATCH v2 31/64] dmaengine: pl330: route callbacks via channel BH

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

New issues:
- [High] Calling dma_chan_init_bh() in the dynamic channel allocation path triggers a kernel WARN_ON when a channel is freed and subsequently re-allocated.

Pre-existing issues:
- [High] Concurrent channel error processing can schedule the bottom-half work after the channel thread is set to NULL during channel release, leading to a NULL pointer dereference.
- [High] The `desc_pool` list in the DMA controller structure is modified without holding `pool_lock` in `pl330_tasklet`, leading to linked list corruption.
- [High] Calling `pl330_tasklet` directly from `pl330_issue_pending` causes completed DMA callbacks to run synchronously in the caller's context, leading to deadlocks.
--

--- Patch [31]: [PATCH v2 31/64] dmaengine: pl330: route callbacks via channel BH ---
commit 03f651c688fce11fe6da583209843570b5a94019
Author: Allen Pais <[email protected]>

dmaengine: pl330: route callbacks via channel BH

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

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 25ba84b187041..f4940bd1ab849 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);
> +			dma_chan_schedule_bh(&pch->chan);
>  		}
>  	}
>  }
>  
> -static void pl330_tasklet(struct tasklet_struct *t)
> +static void pl330_tasklet(struct dma_chan *chan)
>  {
> -	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;

[Severity: High]
This is a pre-existing issue, but can this function crash due to a NULL
pointer dereference if it races with a concurrent channel release?

When pl330_free_chan_resources() kills the bottom-half, it sets pch->thread
to NULL:

pl330_free_chan_resources()
    dma_chan_kill_bh(&pch->chan);
    ...
    pl330_release_channel(pch->thread);
    pch->thread = NULL;

If a concurrent DMAC tasklet calls dma_pl330_rqcb(), it unconditionally
queues the bottom-half again:

dma_pl330_rqcb()
    dma_chan_schedule_bh(&pch->chan);

Since dma_chan_schedule_bh() is not blocked after kill_bh, the workqueue
could execute pl330_tasklet() on the freed channel. Later in this function,
we acquire a lock via the thread pointer:

pl330_tasklet()
    spin_lock(&pch->thread->dmac->lock);

Would this dereference the NULL thread pointer?

[Severity: High]
This is also a pre-existing issue, but does moving descriptors to the global
desc_pool here corrupt the linked list?

Later in pl330_tasklet(), completed non-cyclic descriptors are moved back to
the global pool:

pl330_tasklet()
    list_move_tail(&desc->node, &pch->dmac->desc_pool);

The pch->dmac->desc_pool list is shared across the controller and normally
protected by pool_lock, but pl330_tasklet() only holds the channel-specific
pch->lock here. Could concurrent modifications to desc_pool from multiple
channels or allocations cause list corruption?

[ ... ]
> @@ -2173,7 +2171,7 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  		return -ENOMEM;
>  	}
>  
> -	tasklet_setup(&pch->task, pl330_tasklet);
> +	dma_chan_init_bh(&pch->chan, pl330_tasklet);
>  
>  	spin_unlock_irqrestore(&pl330->lock, flags);

[Severity: High]
Does initializing the bottom-half work structure here trigger a warning on
subsequent channel allocations?

pl330_alloc_chan_resources() is called each time a DMA client dynamically
requests a channel. When the client releases the channel, dma_chan_kill_bh()
flushes the workqueue but does not clear the bh_work_initialized flag.

If a client requests this same channel again, calling dma_chan_init_bh()
will hit the WARN_ON(chan->bh_work_initialized) guard in the DMA engine core.
Should this initialization be moved to driver probe time instead?

[ ... ]
> @@ -2490,7 +2488,7 @@ static void pl330_issue_pending(struct dma_chan *chan)
>  	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
>  	spin_unlock_irqrestore(&pch->lock, flags);
>  
> -	pl330_tasklet(&pch->task);
> +	pl330_tasklet(&pch->chan);
>  }

[Severity: High]
This is a pre-existing issue, but does calling the bottom-half worker
directly from the issue_pending path risk deadlocks?

Clients of the DMA engine API typically call dma_async_issue_pending()
while holding their own spinlocks. By invoking pl330_tasklet() synchronously
here:

pl330_issue_pending()
    pl330_tasklet(&pch->chan);

If any descriptors were marked as DONE by a concurrent IRQ before the
scheduled bottom-half could run, pl330_tasklet() will immediately process
them and invoke their registered client DMA callbacks inline.

If the client callback attempts to acquire the same spinlock the client
already holds, will this cause an AB-BA or AA deadlock?

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