Re: [PATCH] dmaengine: altera-msgdma: initialize state before requesting IRQ

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing dma_async_device_unregister() on the of_dma_controller_register() error path causes a use-after-free in the DMA framework.
- [High] IRQ is requested before the hardware is properly reset and quiesced, leaving a race window that can cause an interrupt storm or concurrent hardware access during reset.
- [High] Chained DMA transactions are stalled forever because msgdma_start_transfer() moves all pending transactions to active_list but only submits the first one to the hardware.
- [High] Hardware interrupts are enabled at probe but never disabled in the error path or during module removal, risking an IRQ storm.
--

commit 7387a0714cc7c9797e08cd9d4ea5b72695584890
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

dmaengine: altera-msgdma: initialize state before requesting IRQ

msgdma_probe() registers the IRQ before initializing the tasklet, lock,
and descriptor lists used by the interrupt handler. A pending interrupt
can access incomplete state during probe.

> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> index e23e5b441a24e..412f4bce1d4aa 100644
> --- a/drivers/dma/altera-msgdma.c
> +++ b/drivers/dma/altera-msgdma.c
> @@ -847,11 +847,6 @@ static int msgdma_probe(struct platform_device *pdev)
>  	if (mdev->irq < 0)
>  		return -ENXIO;
>  
> -	ret = devm_request_irq(&pdev->dev, mdev->irq, msgdma_irq_handler,
> -			       0, dev_name(&pdev->dev), mdev);
> -	if (ret)
> -		return ret;
> -
>  	tasklet_setup(&mdev->irq_tasklet, msgdma_tasklet);
>  
>  	dma_cookie_init(&mdev->dmachan);
> @@ -863,6 +858,11 @@ static int msgdma_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&mdev->done_list);
>  	INIT_LIST_HEAD(&mdev->free_list);
>  
> +	ret = devm_request_irq(&pdev->dev, mdev->irq, msgdma_irq_handler,
> +			       0, dev_name(&pdev->dev), mdev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
This isn't a bug introduced by this patch, but the IRQ is still being
requested before msgdma_reset() is called to quiesce the hardware.

If the bootloader left the device active, can it fire an interrupt
immediately upon registration?

If this happens, the interrupt handler might access hardware registers and
schedule the tasklet while the probe routine concurrently writes to reset
bits in msgdma_reset(), potentially causing bus hangs or an IRQ storm.
Should the hardware be quiesced before registering the IRQ?

[Severity: High]
This is a pre-existing issue, but does the probe error path leak the async
DMA device if registration fails?

If of_dma_controller_register() fails, the code jumps to the cleanup path:

drivers/dma/altera-msgdma.c:msgdma_probe() {
	...
	} else if (ret && ret != -ENODEV) {
		goto fail;
	}
	...
fail:
	msgdma_dev_remove(mdev);
	return ret;
}

However, msgdma_dev_remove() omits deregistering the async device:

drivers/dma/altera-msgdma.c:msgdma_dev_remove() {
	if (!mdev)
		return;

	devm_free_irq(mdev->dev, mdev->irq, mdev);
	tasklet_kill(&mdev->irq_tasklet);
	list_del(&mdev->dmachan.device_node);
}

Because mdev is allocated with devm_kzalloc(), it gets freed upon probe
failure, leaving the dmaengine subsystem with pointers to a freed
dma_device. Could this result in a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but are hardware interrupts left
enabled if probe fails or the module is removed?

msgdma_reset() explicitly enables interrupts on the hardware:

drivers/dma/altera-msgdma.c:msgdma_reset() {
	...
	iowrite32(MSGDMA_CSR_CTL_STOP_ON_ERR | MSGDMA_CSR_CTL_STOP_ON_EARLY |
		  MSGDMA_CSR_CTL_GLOBAL_INTR, mdev->csr + MSGDMA_CSR_CONTROL);
	...
}

If an error occurs after resetting (e.g., during dma_async_device_register),
the cleanup path calls msgdma_dev_remove(), which frees the OS IRQ handler
but fails to write back to MSGDMA_CSR_CONTROL to disable the hardware-level
interrupts.

Could this leave the hardware capable of asserting the IRQ line without a
registered handler, leading to an unhandled IRQ storm?

[Severity: High]
This is a pre-existing issue, but are chained DMA transactions stalled
indefinitely when using this driver?

When msgdma_start_transfer() is called, it moves all pending transactions to
the active list, but only submits the first one to the hardware:

drivers/dma/altera-msgdma.c:msgdma_start_transfer() {
	...
	list_splice_tail_init(&mdev->pending_list, &mdev->active_list);
	msgdma_copy_desc_to_fifo(mdev, desc);
	...
}

Once the hardware finishes the first transaction, the interrupt handler fires
and calls msgdma_start_transfer() to initiate the next transfer. However, it
then checks the pending list instead of the active list:

drivers/dma/altera-msgdma.c:msgdma_start_transfer() {
	desc = list_first_entry_or_null(&mdev->pending_list,
					struct msgdma_sw_desc, node);
	if (!desc)
		return;
	...
}

Since the pending list is empty from the prior splice, does it abandon the
remaining transactions in the active list and stall the pipeline?

>  	dma_dev = &mdev->dmadev;
>  
>  	/* Set DMA capabilities */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830053308.2190597-1-runyu.xiao@seu.edu.cn?part=1