Re: [PATCH v2 20/64] dmaengine: xilinx-dpdma: kill vchan BH on remove

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

Pre-existing issues:
- [High] Premature clock disable and delayed IRQ free expose the driver to synchronous external aborts during teardown and probe error paths.
- [High] Hardware registers are accessed during probe before the AXI clock is prepared and enabled.
- [High] Concurrent modification of virt-dma `desc_issued` list due to missing `chan->vchan.lock`.
--

--- Patch [20]: [PATCH v2 20/64] dmaengine: xilinx-dpdma: kill vchan BH on remove ---
commit b6ee3342e2955cfec74b9591a7918040bbe546f8
Author: Allen Pais <[email protected]>

dmaengine: xilinx-dpdma: kill vchan BH on remove

virt-dma now dispatches completion callbacks through per-channel BH
work. Cancel that work when removing a channel, while retaining the
driver's separate error-handling tasklet.

> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index d9a3542c4531c..2ec82064fd2ed 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -1685,6 +1685,7 @@ static void xilinx_dpdma_chan_remove(struct xilinx_dpdma_chan *chan)
>  		return;
>  
>  	tasklet_kill(&chan->err_task);
> +	dma_chan_kill_bh(&chan->vchan.chan);
>  	list_del(&chan->vchan.chan.device_node);
>  }

[Severity: High]
This is a pre-existing issue, but does the driver disable the clock too
early during teardown and probe error paths?

Looking at xilinx_dpdma_remove(), the AXI clock is disabled before calling
xilinx_dpdma_chan_remove() to kill the tasklets and BHs:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_remove() {
...
	clk_disable_unprepare(xdev->axi_clk);

	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
		xilinx_dpdma_chan_remove(xdev->chan[i]);
...
}

Similarly, in the xilinx_dpdma_probe() error path, the clock is disabled
before tasklets are killed and before the shared IRQ is freed:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_probe() {
...
error_dma_async:
	clk_disable_unprepare(xdev->axi_clk);
error:
	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
		xilinx_dpdma_chan_remove(xdev->chan[i]);

	free_irq(xdev->irq, xdev);
...
}

Could this cause synchronous external aborts if an active shared IRQ handler,
tasklet, or BH attempts to access hardware registers while the clock is
disabled?

[Severity: High]
This is a pre-existing issue, but are hardware registers accessed during
probe before the AXI clock is prepared and enabled?

In xilinx_dpdma_probe(), dpdma_hw_init() is called early to disable
interrupts and clear statuses, which accesses hardware registers:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_probe() {
...
	dpdma_hw_init(xdev);

	xdev->irq = platform_get_irq(pdev, 0);
...
}

However, the clock is enabled much later in the probe sequence:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_probe() {
...
	ret = clk_prepare_enable(xdev->axi_clk);
	if (ret) {
...
}

Will this cause a synchronous external abort on device probe if the
bootloader hasn't already enabled the clock?

[Severity: High]
This is a pre-existing issue, but does xilinx_dpdma_chan_handle_err()
modify the virt-dma desc_issued list without the correct lock?

It appears to modify chan->vchan.desc_issued without holding
chan->vchan.lock:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_chan_handle_err() {
...
	if (!chan->desc.pending &&
	    list_empty(&chan->vchan.desc_issued)) {
		active->error = true;
		list_add_tail(&active->vdesc.node,
			      &chan->vchan.desc_issued);
...
}

The virt-dma layer expects chan->vchan.lock to protect the desc_issued list,
as seen in xilinx_dpdma_terminate_all() where chan->vchan.lock is taken:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_terminate_all() {
...
	spin_lock_irqsave(&chan->vchan.lock, flags);
	vchan_get_all_descriptors(&chan->vchan, &descriptors);
	spin_unlock_irqrestore(&chan->vchan.lock, flags);
...
}

Could this concurrent modification lead to linked list corruption if a
hardware channel error occurs while a client calls
dmaengine_terminate_async()?

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