[PATCH 0/3] dma: mv_xor: convert to devm resource management

[PATCH 0/3] dma: mv_xor: convert to devm resource management

[Rosen Penev]

Convert the mv_xor driver to use managed device resources (devm)
to simplify error handling and removal paths.

Patch 1 replaces the open-coded clock acquire/enable/disable/put
with devm_clk_get_optional_enabled, eliminating manual clock
cleanup in the probe error path.

Patch 2 adds the missing platform remove callback so that
channels, DMA devices, and IRQs are properly cleaned up on
driver unbind.

Patch 3 converts DMA pool allocation and IRQ requests to their
devm counterparts, allowing removal of the err_free_irq and
err_free_dma error labels.

Rosen Penev (3):
  dma: mv_xor: use devm_clk_get_optional_enabled
  dma: mv_xor: add missing platform remove function
  dma: mv_xor: use devm for dma pool and irq

 drivers/dma/mv_xor.c | 51 +++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

-- 
2.54.0

[PATCH 1/3] dma: mv_xor: use devm_clk_get_optional_enabled

[Rosen Penev]

Replace clk_get + clk_prepare_enable + clk_put with
devm_clk_get_optional_enabled. This eliminates the need for
manual clock cleanup in the probe error path.

It also fixes missing cleanup in a remove function.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 25ed61f1b089..a97fa0038652 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1359,12 +1359,9 @@ static int mv_xor_probe(struct platform_device *pdev)
 			mv_xor_conf_mbus_windows(xordev, dram);
 	}
 
-	/* Not all platforms can gate the clock, so it is not
-	 * an error if the clock does not exists.
-	 */
-	xordev->clk = clk_get(&pdev->dev, NULL);
-	if (!IS_ERR(xordev->clk))
-		clk_prepare_enable(xordev->clk);
+	xordev->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	if (IS_ERR(xordev->clk))
+		return PTR_ERR(xordev->clk);
 
 	/*
 	 * We don't want to have more than one channel per CPU in
@@ -1452,11 +1449,6 @@ static int mv_xor_probe(struct platform_device *pdev)
 				irq_dispose_mapping(xordev->channels[i]->irq);
 		}
 
-	if (!IS_ERR(xordev->clk)) {
-		clk_disable_unprepare(xordev->clk);
-		clk_put(xordev->clk);
-	}
-
 	return ret;
 }
 
-- 
2.54.0

Re: [PATCH 1/3] dma: mv_xor: use devm_clk_get_optional_enabled

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ tasklet `mv_chan->irq_tasklet` is never canceled, leading to a Use-After-Free.
--

commit afdff052af38a51a1e801381a05797da055168c9
Author: Rosen Penev <rosenp@gmail.com>

dma: mv_xor: use devm_clk_get_optional_enabled

Replace clk_get + clk_prepare_enable + clk_put with
devm_clk_get_optional_enabled. This eliminates the need for
manual clock cleanup in the probe error path.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
[ ... ]
> @@ -1452,11 +1449,6 @@ static int mv_xor_probe(struct platform_device *pdev)
>  				irq_dispose_mapping(xordev->channels[i]->irq);
>  		}
>  
> -	if (!IS_ERR(xordev->clk)) {
> -		clk_disable_unprepare(xordev->clk);
> -		clk_put(xordev->clk);
> -	}
> -

[Severity: High]
This is a pre-existing issue, but does mv_xor_channel_remove() leave a
tasklet pending when called from this probe error path?

If the probe fails and jumps to err_channel_add, the cleanup loop calls
mv_xor_channel_remove() which tears down the channel:

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
        dma_async_device_unregister(&mv_chan->dmadev);
        dma_unmap_single(...)
        dma_unmap_single(...)
        ...
        return 0;
}

However, it doesn't appear to call tasklet_kill() to cancel
mv_chan->irq_tasklet.

Since mv_chan is allocated with devres, returning from the probe error path
will automatically free the channel structure. If an interrupt fires and
schedules the tasklet during teardown, could it execute asynchronously after
devres has freed mv_chan, resulting in a use-after-free?

>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610065737.118211-1-rosenp@gmail.com?part=1

[PATCH 2/3] dma: mv_xor: add missing platform remove function

[Rosen Penev]

The driver was missing a remove callback, so channels, DMA
devices, and IRQs were never cleaned up on driver unbind.
Implement mv_xor_remove to undo probe, patterned after the
existing error path.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index a97fa0038652..3fc39cca7cbd 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1452,8 +1452,22 @@ static int mv_xor_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void mv_xor_remove(struct platform_device *pdev)
+{
+	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
+		if (xordev->channels[i]) {
+			mv_xor_channel_remove(xordev->channels[i]);
+			if (pdev->dev.of_node)
+				irq_dispose_mapping(xordev->channels[i]->irq);
+		}
+}
+
 static struct platform_driver mv_xor_driver = {
 	.probe		= mv_xor_probe,
+	.remove		= mv_xor_remove,
 	.suspend        = mv_xor_suspend,
 	.resume         = mv_xor_resume,
 	.driver		= {
-- 
2.54.0

Re: [PATCH 2/3] dma: mv_xor: add missing platform remove function

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing tasklet_kill() leads to a Use-After-Free during driver unbind.
- [High] Object lifetime mismatch between devres and the dmaengine framework causes use-after-free on driver unbind.
- [High] The driver unmaps DMA buffers and disables clocks during removal without explicitly stopping the DMA hardware.
- [High] A crafted or unexpectedly large Device Tree causes a heap buffer overflow in mv_xor_probe when xordev->xor_type == XOR_ARMADA_37XX.
- [High] Unsafe DMA buffer allocation and reversed DMA mapping directions for dummy buffers.
--

commit c4face3fac6884413550ece128f62a970eb9f8e3
Author: Rosen Penev <rosenp@gmail.com>

dma: mv_xor: add missing platform remove function

This commit introduces a remove callback for the mv_xor driver to clean up
channels and irq mappings upon driver unbind.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index a97fa0038652d..3fc39cca7cbd3 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1452,8 +1452,22 @@ static int mv_xor_probe(struct platform_device *pdev)
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but is there a potential array overflow in
mv_xor_probe() for Armada 37XX devices?

For XOR_ARMADA_37XX, max_channels is set to num_present_cpus() without capping
it to MV_XOR_MAX_CHANNELS (which is 2):

    if (xordev->xor_type == XOR_ARMADA_37XX)
        max_channels = num_present_cpus();

If the device tree provides more than two child nodes and there are more than
two CPUs, could the loop sequentially storing allocated channel pointers exceed
the bounds of the fixed-size xordev->channels array?

[Severity: High]
This is a pre-existing issue, but are the dummy_src and dummy_dst buffers in
struct mv_xor_chan safely aligned and mapped?

They are embedded in the struct without cache-line alignment, and their mapping
directions in mv_xor_channel_add() appear to be swapped:

    mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);

Since mv_xor_prep_dma_interrupt() uses dummy_src as the read source and
dummy_dst as the write destination, unmapping dummy_src with DMA_FROM_DEVICE
will invalidate the CPU cache.

Could this invalidation corrupt adjacent struct members sharing the same cache
line?
 
> +static void mv_xor_remove(struct platform_device *pdev)
> +{
> +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
> +		if (xordev->channels[i]) {
> +			mv_xor_channel_remove(xordev->channels[i]);

[Severity: High]
This isn't a bug introduced by this patch, but does mv_xor_channel_remove()
need to call tasklet_kill()?

When the driver unregisters the DMA device:

    dma_async_device_unregister(&mv_chan->dmadev);

it leaves pending tasklets capable of executing asynchronously.

Because the channel and DMA descriptor pool are freed by devres, could an
executing tasklet access freed memory if it runs after this cleanup?

[Severity: High]
This is a pre-existing issue, but does enabling driver unbind expose an
object lifetime mismatch?

The driver dynamically allocates mv_chan using devm_kzalloc(), which is freed
by devres when remove() completes.

Since dma_async_device_unregister() does not forcefully revoke client
references, could active clients holding a channel reference trigger a
use-after-free when they access or release the channel later?

[Severity: High]
This isn't a bug introduced by this patch, but does the hardware need to be
quiesced before tearing down the channel?

mv_xor_channel_remove() destructively unmaps dummy buffers via
dma_unmap_single(), and then devres disables the clock.

If a DMA transaction is currently in flight, could the hardware continue
writing to unmapped memory or hang the AXI bus?

> +			if (pdev->dev.of_node)
> +				irq_dispose_mapping(xordev->channels[i]->irq);
> +		}
> +}

[ ... ]

>  static struct platform_driver mv_xor_driver = {
>  	.probe		= mv_xor_probe,
> +	.remove		= mv_xor_remove,
>  	.suspend        = mv_xor_suspend,
>  	.resume         = mv_xor_resume,
>  	.driver		= {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610065737.118211-1-rosenp@gmail.com?part=2

[PATCH 3/3] dma: mv_xor: use devm for dma pool and irq

[Rosen Penev]

Replace dma_alloc_wc with dmam_alloc_attrs and request_irq
with devm_request_irq. This eliminates the need for
manual cleanup of the dma pool and irq in both the channel
remove function and the channel add error labels, removing
the err_free_irq and err_free_dma labels entirely.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 3fc39cca7cbd..2ac7f01155fe 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1013,8 +1013,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 
 	dma_async_device_unregister(&mv_chan->dmadev);
 
-	dma_free_wc(dev, MV_XOR_POOL_SIZE,
-			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
 	dma_unmap_single(dev, mv_chan->dummy_src_addr,
 			 MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
 	dma_unmap_single(dev, mv_chan->dummy_dst_addr,
@@ -1025,8 +1023,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 		list_del(&chan->device_node);
 	}
 
-	free_irq(mv_chan->irq, mv_chan);
-
 	return 0;
 }
 
@@ -1077,8 +1073,8 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	 * requires that we explicitly flush the writes
 	 */
 	mv_chan->dma_desc_pool_virt =
-	  dma_alloc_wc(&pdev->dev, MV_XOR_POOL_SIZE, &mv_chan->dma_desc_pool,
-		       GFP_KERNEL);
+	  dmam_alloc_attrs(&pdev->dev, MV_XOR_POOL_SIZE, &mv_chan->dma_desc_pool,
+			   GFP_KERNEL, DMA_ATTR_WRITE_COMBINE);
 	if (!mv_chan->dma_desc_pool_virt) {
 		ret = -ENOMEM;
 		goto err_unmap_dst;
@@ -1112,10 +1108,10 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	/* clear errors before enabling interrupts */
 	mv_chan_clear_err_status(mv_chan);
 
-	ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
+	ret = devm_request_irq(&pdev->dev, mv_chan->irq, mv_xor_interrupt_handler,
 			  0, dev_name(&pdev->dev), mv_chan);
 	if (ret)
-		goto err_free_dma;
+		goto err_unmap_dst;
 
 	mv_chan_unmask_interrupts(mv_chan);
 
@@ -1138,14 +1134,14 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 		ret = mv_chan_memcpy_self_test(mv_chan);
 		dev_dbg(&pdev->dev, "memcpy self test returned %d\n", ret);
 		if (ret)
-			goto err_free_irq;
+			goto err_unmap_dst;
 	}
 
 	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)) {
 		ret = mv_chan_xor_self_test(mv_chan);
 		dev_dbg(&pdev->dev, "xor self test returned %d\n", ret);
 		if (ret)
-			goto err_free_irq;
+			goto err_unmap_dst;
 	}
 
 	dev_info(&pdev->dev, "Marvell XOR (%s): ( %s%s%s)\n",
@@ -1156,15 +1152,10 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 
 	ret = dma_async_device_register(dma_dev);
 	if (ret)
-		goto err_free_irq;
+		goto err_unmap_dst;
 
 	return mv_chan;
 
-err_free_irq:
-	free_irq(mv_chan->irq, mv_chan);
-err_free_dma:
-	dma_free_wc(&pdev->dev, MV_XOR_POOL_SIZE,
-			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
 err_unmap_dst:
 	dma_unmap_single(dma_dev->dev, mv_chan->dummy_dst_addr,
 			 MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
-- 
2.54.0

Re: [PATCH 3/3] dma: mv_xor: use devm for dma pool and irq

[sashiko-bot]

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

New issues:
- [High] Calling `irq_dispose_mapping()` before `devm_request_irq()` teardown leaks the `irqaction` and causes a potential use-after-free.

Pre-existing issues:
- [High] Missing `tasklet_kill()` during channel teardown leads to a potential Use-After-Free.
- [High] The self-test functions fail to stop the DMA engine on timeout, leading to memory corruption.
- [High] The interrupt handler is registered and unmasked before the channel's spinlock and lists are initialized.
--

commit 37fd86f976ff0d9115db485708892518119901a0
Author: Rosen Penev <rosenp@gmail.com>

dma: mv_xor: use devm for dma pool and irq
    
Replace dma_alloc_wc with dmam_alloc_attrs and request_irq
with devm_request_irq. This eliminates the need for
manual cleanup of the dma pool and irq in both the channel
remove function and the channel add error labels, removing
the err_free_irq and err_free_dma labels entirely.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 3fc39cca7cbd3..2ac7f01155fe8 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1013,8 +1013,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  
>  	dma_async_device_unregister(&mv_chan->dmadev);
>  
> -	dma_free_wc(dev, MV_XOR_POOL_SIZE,
> -			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
>  	dma_unmap_single(dev, mv_chan->dummy_src_addr,
>  			 MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
>  	dma_unmap_single(dev, mv_chan->dummy_dst_addr,
> @@ -1025,8 +1023,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  		list_del(&chan->device_node);
>  	}
>  
> -	free_irq(mv_chan->irq, mv_chan);
> -

[Severity: High]
This is a pre-existing issue, but does this function also need a call to
tasklet_kill(&mv_chan->irq_tasklet) before returning?

The mv_chan structure is allocated via devm_kzalloc() and will be freed by
devres upon device removal or probe failure. If the tasklet is scheduled by an
interrupt just before removal, it might execute asynchronously after the
mv_chan memory has been freed, causing a use-after-free panic.

>  	return 0;
>  }
>  

[ ... ]

> @@ -1112,10 +1108,10 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  	/* clear errors before enabling interrupts */
>  	mv_chan_clear_err_status(mv_chan);
>  
> -	ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
> +	ret = devm_request_irq(&pdev->dev, mv_chan->irq, mv_xor_interrupt_handler,
>  			  0, dev_name(&pdev->dev), mv_chan);

[Severity: High]
By switching to devm_request_irq() here, could we cause an irqaction leak and
potential use-after-free during device unbind or probe error?

There are manual irq_dispose_mapping() calls in the error paths of
mv_xor_probe() and inside mv_xor_remove() which were not converted:

drivers/dma/mv_xor.c:mv_xor_remove() {
    ...
    for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
        if (xordev->channels[i]) {
            mv_xor_channel_remove(xordev->channels[i]);
            if (pdev->dev.of_node)
                irq_dispose_mapping(xordev->channels[i]->irq);
        }
}

Calling irq_dispose_mapping() will unmap and delete the IRQ descriptor from
the sparse IRQ tree. When the devres cleanup later attempts to free the
devm-managed IRQ, irq_to_desc() will return NULL, and free_irq() will
return without actually freeing the struct irqaction memory.

[Severity: High]
This is a pre-existing issue, but is it safe to request and unmask
the interrupt before the channel's lock and lists are initialized?

The interrupt is requested here, and unmasked shortly after:

	mv_chan_unmask_interrupts(mv_chan);

However, the spinlock and lists are initialized further down:

	spin_lock_init(&mv_chan->lock);
	INIT_LIST_HEAD(&mv_chan->chain);

If an interrupt fires immediately (e.g. from a previous bind/unbind or a shared
IRQ), the handler will schedule the tasklet, which then attempts to acquire the
uninitialized spinlock and access the uninitialized lists in
mv_chan_slot_cleanup(), potentially causing a crash.

>  	if (ret)
> -		goto err_free_dma;
> +		goto err_unmap_dst;
>  
>  	mv_chan_unmask_interrupts(mv_chan);
>  
> @@ -1138,14 +1134,14 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  		ret = mv_chan_memcpy_self_test(mv_chan);
>  		dev_dbg(&pdev->dev, "memcpy self test returned %d\n", ret);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_unmap_dst;

[Severity: High]
This is a pre-existing issue, but if the DMA self-test times out, are we missing
a hardware abort before freeing the destination buffer?

In mv_chan_memcpy_self_test() and mv_chan_xor_self_test(), if the DMA hardware
times out, the function jumps to the free_resources label:

drivers/dma/mv_xor.c:mv_chan_memcpy_self_test() {
    ...
	if (mv_xor_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
		dev_err(dma_chan->device->dev,
			"Self-test copy timed out, disabling\n");
		err = -ENODEV;
		goto free_resources;
	}
    ...
free_resources:
	dmaengine_unmap_put(unmap);
	mv_xor_free_chan_resources(dma_chan);
out:
	kfree(src);
	kfree(dest);
	return err;
}

The code unmaps the DMA buffers and frees the destination memory via kfree(),
but never attempts to halt or reset the underlying hardware channel. If the
delayed hardware completes the transfer later, will it write the DMA results
directly into the freed kernel memory pages?

>  	}
>  
>  	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)) {
>  		ret = mv_chan_xor_self_test(mv_chan);
>  		dev_dbg(&pdev->dev, "xor self test returned %d\n", ret);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_unmap_dst;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610065737.118211-1-rosenp@gmail.com?part=3