Re: [PATCH v2 19/64] dmaengine: xilinx-dma: use dma_chan BH instead of tasklets

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

Pre-existing issues:
- [High] request_irq() is called before channel initialization is complete, creating a race condition if an early interrupt fires.
- [High] Uninitialized channel access in xilinx_mcdma_irq_handler during probe.
- [High] Recursive spinlock deadlock when running dependencies on the same DMA channel.
--

--- Patch [19]: [PATCH v2 19/64] dmaengine: xilinx-dma: use dma_chan BH instead of tasklets ---
commit 2b1e6b23fed3e56575f2e8169196ee83cbca26e8
Author: Allen Pais <[email protected]>

dmaengine: xilinx-dma: use dma_chan BH instead of tasklets

This patch replaces the per-channel tasklet with the shared dma_chan BH
helper. The handler continues to run in softirq context while dmaengine
owns the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 404235c173538..bf9f22ed12659 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
[ ... ]
> @@ -1115,11 +1113,12 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
>  
>  /**
>   * xilinx_dma_do_tasklet - Schedule completion tasklet
> - * @t: Pointer to the Xilinx DMA channel structure
> + * @c: Pointer to the Xilinx DMA channel structure
>   */
> -static void xilinx_dma_do_tasklet(struct tasklet_struct *t)
> +static void xilinx_dma_do_tasklet(struct dma_chan *c)
>  {
> -	struct xilinx_dma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct xilinx_dma_chan *chan = container_of(c,
> +			struct xilinx_dma_chan, common);
>  
>  	xilinx_dma_chan_desc_cleanup(chan);
>  }

[Severity: High]
This is a pre-existing issue, but can this lead to a recursive spinlock
deadlock? 

Looking at xilinx_dma_chan_desc_cleanup(), the channel lock is acquired
before running dependencies:

xilinx_dma_chan_desc_cleanup() {
    ...
    spin_lock_irqsave(&chan->lock, flags);
    ...
    dma_run_dependencies(&desc->async_tx);
    ...
}

If a dependent transaction belongs to the same channel, dma_run_dependencies()
will invoke xilinx_dma_tx_submit(), which unconditionally attempts to acquire
the exact same chan->lock. Since standard spinlocks are not recursive, would
this cause the CPU to deadlock?

[ ... ]
> @@ -1898,7 +1897,7 @@ static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data)
>  		spin_unlock(&chan->lock);
>  	}
>  
> -	tasklet_hi_schedule(&chan->tasklet);
> +	dma_chan_schedule_bh(&chan->common);
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but could this handler dereference a NULL
pointer during device probe?

If a shared interrupt fires while the driver is still probing channels,
xilinx_mcdma_irq_handler() might compute a chan_offset for a channel that
has not been probed yet:

xilinx_mcdma_irq_handler() {
    ...
    chan = chan->xdev->chan[chan_offset];
    status = dma_ctrl_read(chan, XILINX_MCDMA_CHAN_SR_OFFSET(chan->tdest));
    ...
}

If the channel array entry is still NULL, would dma_ctrl_read() trigger a
crash?

[ ... ]
> @@ -3075,8 +3074,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>  			str_enabled_disabled(chan->has_sg));
>  	}
>  
> -	/* Initialize the tasklet */
> -	tasklet_setup(&chan->tasklet, xilinx_dma_do_tasklet);
> +	dma_chan_init_bh(&chan->common, xilinx_dma_do_tasklet);

[Severity: High]
This is a pre-existing issue, but does calling request_irq() earlier in
xilinx_dma_chan_probe() create a race condition here?

In xilinx_dma_chan_probe(), request_irq() is called with IRQF_SHARED
before chan->start_transfer and the bottom-half work are initialized:

xilinx_dma_chan_probe() {
    ...
    err = request_irq(chan->irq, xdev->dma_config->irq_handler,
                      IRQF_SHARED, "xilinx-dma-controller", chan);
    ...
    chan->start_transfer = xilinx_dma_start_transfer;
    ...
    dma_chan_init_bh(&chan->common, xilinx_dma_do_tasklet);
    ...
}

If an interrupt fires on the shared line immediately after request_irq()
succeeds, the handler could try to invoke chan->start_transfer() or
dma_chan_schedule_bh() while they are still uninitialized. Should the IRQ
registration be moved after the channel is fully initialized?

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