[PATCH] dmaengine: altera-msgdma: fail probe when reset times out

[PATCH] dmaengine: altera-msgdma: fail probe when reset times out

[Pengpeng Hou]

msgdma_probe() resets the controller before publishing the DMA device
and OF DMA provider, but msgdma_reset() only logs a timeout and then
continues to enable the controller and mark it idle.

If the reset bit never clears, the driver can still register a DMA
engine backed by a controller that did not leave reset. Return the
readl_poll_timeout() error from msgdma_reset() and abort probe on reset
failure.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/dma/altera-msgdma.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index b46999c81df0..f60a4e86a246 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -466,7 +466,7 @@ static int msgdma_dma_config(struct dma_chan *dchan,
 	return 0;
 }
 
-static void msgdma_reset(struct msgdma_device *mdev)
+static int msgdma_reset(struct msgdma_device *mdev)
 {
 	u32 val;
 	int ret;
@@ -478,8 +478,10 @@ static void msgdma_reset(struct msgdma_device *mdev)
 	ret = readl_poll_timeout(mdev->csr + MSGDMA_CSR_STATUS, val,
 				 (val & MSGDMA_CSR_STAT_RESETTING) == 0,
 				 1, 10000);
-	if (ret)
+	if (ret) {
 		dev_err(mdev->dev, "DMA channel did not reset\n");
+		return ret;
+	}
 
 	/* Clear all status bits */
 	iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + MSGDMA_CSR_STATUS);
@@ -489,6 +491,7 @@ static void msgdma_reset(struct msgdma_device *mdev)
 		  MSGDMA_CSR_CTL_GLOBAL_INTR, mdev->csr + MSGDMA_CSR_CONTROL);
 
 	mdev->idle = true;
+	return 0;
 };
 
 static void msgdma_copy_one(struct msgdma_device *mdev,
@@ -896,7 +899,9 @@ static int msgdma_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	msgdma_reset(mdev);
+	ret = msgdma_reset(mdev);
+	if (ret)
+		goto fail;
 
 	ret = dma_async_device_register(dma_dev);
 	if (ret)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] dmaengine: altera-msgdma: fail probe when reset times out

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing dma_async_device_unregister() on probe error path leads to Use-After-Free
- [High] IRQ handler requested before data structures are initialized, risking NULL pointer dereference
--

commit 4e931dfaccbe922ddc5a4a0f0f0b3dd9e532e207
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

dmaengine: altera-msgdma: fail probe when reset times out

This commit updates msgdma_reset() to return an error code when the
controller fails to reset. It also modifies msgdma_probe() to abort the
probe process if the reset times out, preventing the registration of a DMA
engine backed by a stalled controller.

> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> --- a/drivers/dma/altera-msgdma.c
> +++ b/drivers/dma/altera-msgdma.c
[ ... ]
> @@ -896,7 +899,9 @@ static int msgdma_probe(struct platform_device *pdev)
>  		goto fail;
>  	}
>  
> -	msgdma_reset(mdev);
> +	ret = msgdma_reset(mdev);
> +	if (ret)
> +		goto fail;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential memory
issue in the error handling path just after this code. If
of_dma_controller_register() fails later in the probe, the code branches to
the fail label:

msgdma_probe() {
    ...
    ret = dma_async_device_register(dma_dev);
    if (ret)
        goto fail;

    ret = of_dma_controller_register(...);
    if (ret == -EINVAL)
        ...
    else if (ret && ret != -ENODEV)
        goto fail;
    ...
fail:
    msgdma_dev_remove(mdev);
    return ret;
}

Does msgdma_dev_remove() need to call dma_async_device_unregister() to clean
up the registered device?

Since mdev is allocated with devm_kzalloc(), the dma_device structure is
freed when the probe fails. Will leaving the device registered leave a
dangling pointer in the global dma_device_list leading to a use-after-free?

[Severity: High]
This is also a pre-existing issue, but looking at the earlier setup in
msgdma_probe(), could an interrupt fire before the required structures are
fully initialized?

msgdma_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, mdev->irq, msgdma_irq_handler, ...);
    ...
    tasklet_setup(&mdev->irq_tasklet, msgdma_tasklet);
    dma_cookie_init(&mdev->dmachan);
    spin_lock_init(&mdev->lock);
    INIT_LIST_HEAD(&mdev->active_list);
    INIT_LIST_HEAD(&mdev->pending_list);
    ...
}

If the hardware is already asserting an interrupt during boot (for example,
left over from a bootloader), will the IRQ handler access uninitialized
structures like mdev->pending_list and trigger a NULL pointer dereference?

Should the interrupt be requested only after these data structures are safely
initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623060554.13523-1-pengpeng@iscas.ac.cn?part=1