[PATCHv2 0/9] dma: mv_xor: convert to devm resource management

[PATCHv2 0/9] 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 5 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 6 adds the missing platform remove callback so that
channels, DMA devices, and IRQs are properly cleaned up on
driver unbind.

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

v2: add more sashiko fixes.

Rosen Penev (9):
  dmaengine: mv_xor: initialize chan state before requesting IRQ
  dmaengine: mv_xor: fix use-after-free in probe error path
  dmaengine: mv_xor: bound maximum channels for Armada 37xx
  dmaengine: mv_xor: abort channel before freeing resources on timeout
  dmaengine: mv_xor: use devm_clk_get_optional_enabled
  dmaengine: mv_xor: switch to of_irq_get()
  dmaengine: mv_xor: use devm for dma pool and irq
  dmaengine: mv_xor: allocate dummy buffers with dmam_alloc_coherent
  dmaengine: mv_xor: add missing platform remove function

 drivers/dma/mv_xor.c | 161 +++++++++++++++++++++----------------------
 drivers/dma/mv_xor.h |   4 +-
 2 files changed, 81 insertions(+), 84 deletions(-)

--
2.54.0

[PATCHv2 1/9] dmaengine: mv_xor: initialize chan state before requesting IRQ

[Rosen Penev]

In mv_xor_channel_add(), the IRQ is requested and unmasked before the
channel's spinlock, descriptor lists, and cookie state are initialized.
If an interrupt fires immediately (e.g. from a shared IRQ or previous
bind/unbind cycle), the handler schedules the tasklet, which then
accesses the uninitialized spinlock and lists in mv_chan_slot_cleanup(),
resulting in undefined behavior.

Fix by moving spin_lock_init(), INIT_LIST_HEAD(), dma_cookie_init(),
and tasklet_setup() to immediately follow the basic struct field
initialization, before any DMA mappings or IRQ registration.

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

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 25ed61f1b089..93a8e9f7c529 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1054,6 +1054,18 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	dma_dev->dev = &pdev->dev;
 	mv_chan->xordev = xordev;
 
+	spin_lock_init(&mv_chan->lock);
+	INIT_LIST_HEAD(&mv_chan->chain);
+	INIT_LIST_HEAD(&mv_chan->completed_slots);
+	INIT_LIST_HEAD(&mv_chan->free_slots);
+	INIT_LIST_HEAD(&mv_chan->allocated_slots);
+	mv_chan->dmachan.device = dma_dev;
+	dma_cookie_init(&mv_chan->dmachan);
+
+	mv_chan->mmr_base = xordev->xor_base;
+	mv_chan->mmr_high_base = xordev->xor_high_base;
+	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);
+
 	/*
 	 * These source and destination dummy buffers are used to implement
 	 * a DMA_INTERRUPT operation as a minimum-sized XOR operation.
@@ -1105,10 +1117,6 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 		dma_dev->device_prep_dma_xor = mv_xor_prep_dma_xor;
 	}
 
-	mv_chan->mmr_base = xordev->xor_base;
-	mv_chan->mmr_high_base = xordev->xor_high_base;
-	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);
-
 	/* clear errors before enabling interrupts */
 	mv_chan_clear_err_status(mv_chan);
 
@@ -1124,14 +1132,6 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	else
 		mv_chan_set_mode(mv_chan, XOR_OPERATION_MODE_XOR);
 
-	spin_lock_init(&mv_chan->lock);
-	INIT_LIST_HEAD(&mv_chan->chain);
-	INIT_LIST_HEAD(&mv_chan->completed_slots);
-	INIT_LIST_HEAD(&mv_chan->free_slots);
-	INIT_LIST_HEAD(&mv_chan->allocated_slots);
-	mv_chan->dmachan.device = dma_dev;
-	dma_cookie_init(&mv_chan->dmachan);
-
 	list_add_tail(&mv_chan->dmachan.device_node, &dma_dev->channels);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
-- 
2.54.0

Re: [PATCHv2 1/9] dmaengine: mv_xor: initialize chan state before requesting IRQ

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `tasklet_kill()` on error paths and during channel removal can lead to a use-after-free of the channel structure.
- [High] Unprotected concurrent access to shared MBUS window hardware registers and `xordev` arrays across multiple DMA channels.
- [High] DMA descriptor slots are permanently leaked if `mv_xor_add_io_win()` fails during DMA prep.
- [High] Missing `dma_wmb()` before updating the hardware chain descriptor.
- [High] Cache-line tearing risk due to unaligned embedded DMA buffers.
--

commit ea8e635be2ff110a83818007cc43ae217683e3c9
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: mv_xor: initialize chan state before requesting IRQ

This commit fixes a potential race condition in the mv_xor driver by
initializing the channel's spinlock, lists, and tasklet before registering
the IRQ. This prevents a scenario where an immediately firing interrupt
could access uninitialized structures and cause undefined behavior.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..93a8e9f7c529a 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1054,6 +1054,18 @@ mv_xor_channel_add(struct mv_xor_device *xordev,

[ ... ]

> +	spin_lock_init(&mv_chan->lock);
> +	INIT_LIST_HEAD(&mv_chan->chain);
> +	INIT_LIST_HEAD(&mv_chan->completed_slots);
> +	INIT_LIST_HEAD(&mv_chan->free_slots);
> +	INIT_LIST_HEAD(&mv_chan->allocated_slots);
> +	mv_chan->dmachan.device = dma_dev;
> +	dma_cookie_init(&mv_chan->dmachan);
> +
> +	mv_chan->mmr_base = xordev->xor_base;
> +	mv_chan->mmr_high_base = xordev->xor_high_base;
> +	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);

[Severity: High]
This isn't a bug introduced by this patch, but since you are moving the
tasklet setup here, could this trigger a cache-line tearing issue?

Looking at struct mv_xor_chan in drivers/dma/mv_xor.h, dummy_src and
dummy_dst are embedded as 128-byte character arrays without cacheline
alignment annotations.

Shortly after this tasklet_setup(), dummy_src is mapped with
DMA_FROM_DEVICE:

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

Since the array shares a cache line with adjacent struct fields like
irq_tasklet and op_in_desc which are initialized before the mapping, won't
the DMA cache invalidation destroy any recent CPU writes to these fields?

[ ... ]

>  	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
>  		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;
>  	}

[ ... ]

> err_free_irq:
> 	free_irq(mv_chan->irq, mv_chan);
> err_free_dma:

[Severity: High]
This is a pre-existing issue, but does the error path here miss a
tasklet_kill() call?

If the probe fails after tasklet_setup() and request_irq(), the error
path frees the IRQ but leaves a pending tasklet that might execute and use
the soon-to-be freed channel structure. Should tasklet_kill() be called
during channel removal and in this error path?


While reviewing this driver, I also noticed a few other pre-existing issues
in drivers/dma/mv_xor.c that might need attention:

[Severity: High]
This is a pre-existing issue, but is there a missing lock when accessing
the shared MBUS window registers?

In mv_xor_add_io_win(), the function reads WINDOW_BAR_ENABLE(0), searches
for a free window index, and updates both xordev->win_start[i] and the
hardware registers:

drivers/dma/mv_xor.c:mv_xor_add_io_win() {
        ...
        win_enable = readl(base + WINDOW_BAR_ENABLE(0));
        i = ffs(~win_enable) - 1;
        ...
        xordev->win_start[i] = addr;
        xordev->win_end[i] = addr + size;
        ...
}

Since xordev is shared across multiple XOR channels and the DMA engine
framework allows concurrent device_prep_dma_* calls, can concurrent
operations pick the same window index and overwrite each other's target
mappings?

[Severity: High]
This is a pre-existing issue, but does mv_xor_prep_dma_xor() leak DMA
descriptor slots if mv_xor_add_io_win() fails?

If mv_xor_add_io_win() fails, the function returns NULL directly:

drivers/dma/mv_xor.c:mv_xor_prep_dma_xor() {
        ...
        ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
        if (ret)
                return NULL;
        ...
}

Since the descriptor slot was previously allocated and moved to
mv_chan->allocated_slots, returning NULL here without returning the
descriptor to mv_chan->free_slots appears to permanently leak the slot.
Could this eventually exhaust the channel's descriptor pool?

[Severity: High]
This isn't a bug introduced by this patch, but is a memory barrier required
before updating the hardware chain descriptor?

In mv_xor_tx_submit(), when appending a new descriptor to an active chain,
the code modifies the previous descriptor in write-combined memory:

drivers/dma/mv_xor.c:mv_xor_tx_submit() {
        ...
        /* fix up the hardware chain */
        mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
        ...
}

Without a dma_wmb() before this assignment, could the hardware read the
updated phy_next_desc before the new descriptor's contents are flushed
from the CPU write-combine buffer?

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

[PATCHv2 2/9] dmaengine: mv_xor: fix use-after-free in probe error path

[Rosen Penev]

mv_xor_channel_remove() does not call tasklet_kill() to cancel
mv_chan->irq_tasklet.  In the probe error path (err_channel_add) the
channel structure is devm-allocated, so it is freed automatically when
the probe function returns.  If an interrupt fires and schedules the
tasklet during teardown, it can execute after devres has freed mv_chan,
resulting in a use-after-free.

Fix this by masking hardware interrupts on the channel and then calling
tasklet_kill() at the start of mv_xor_channel_remove(), ensuring no
new interrupts can schedule the tasklet and any already-running
instance has completed before the rest of the channel is torn down.

Assisted-by: opencode:big-pickle
Fixes: a6b4a9d2c106 ("dma: mv_xor: split initialization/cleanup of XOR channels")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 93a8e9f7c529..ef29e8be1db6 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -106,6 +106,14 @@ static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
 	writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
 }
 
+static void mv_chan_mask_interrupts(struct mv_xor_chan *chan)
+{
+	u32 val = readl_relaxed(XOR_INTR_MASK(chan));
+
+	val &= ~(XOR_INTR_MASK_VALUE << (chan->idx * 16));
+	writel_relaxed(val, XOR_INTR_MASK(chan));
+}
+
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)
 {
 	u32 val = readl_relaxed(XOR_INTR_MASK(chan));
@@ -1011,6 +1019,9 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 	struct dma_chan *chan, *_chan;
 	struct device *dev = mv_chan->dmadev.dev;
 
+	mv_chan_mask_interrupts(mv_chan);
+	tasklet_kill(&mv_chan->irq_tasklet);
+
 	dma_async_device_unregister(&mv_chan->dmadev);
 
 	dma_free_wc(dev, MV_XOR_POOL_SIZE,
-- 
2.54.0

Re: [PATCHv2 2/9] dmaengine: mv_xor: fix use-after-free in probe error path

[sashiko-bot]

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

New issues:
- [High] Calling `tasklet_kill()` before `free_irq()` in `mv_xor_channel_remove()` leaves a race window where an already-executing interrupt handler can reschedule the tasklet, resulting in a use-after-free.

Pre-existing issues:
- [High] Missing `tasklet_kill()` in the `mv_xor_channel_add()` error path leaves a pending tasklet that can access freed memory if initialization fails.
--

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

dmaengine: mv_xor: fix use-after-free in probe error path

This commit attempts to fix a use-after-free in the probe error path by
masking hardware interrupts and calling tasklet_kill() at the start of
mv_xor_channel_remove(). This is intended to ensure no new interrupts can
schedule the tasklet during teardown.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..8e8902bbda9ac 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
[ ... ]
> @@ -1011,6 +1019,9 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  	struct dma_chan *chan, *_chan;
>  	struct device *dev = mv_chan->dmadev.dev;
>  
> +	mv_chan_mask_interrupts(mv_chan);
> +	tasklet_kill(&mv_chan->irq_tasklet);

[Severity: High]
Could calling tasklet_kill() here, before free_irq(), leave a race window?

While mv_chan_mask_interrupts() prevents new interrupts from being raised by
the hardware, it does not stop an interrupt handler that is already
executing on another CPU.

If tasklet_kill() runs concurrently while the ISR is executing but before
it calls tasklet_schedule(), tasklet_kill() will see the tasklet is neither
scheduled nor running, and return immediately. The ISR will then schedule
the tasklet.

Later in mv_xor_channel_remove():

	free_irq(mv_chan->irq, mv_chan);

free_irq() waits for the ISR to finish but does not cancel the newly
scheduled tasklet. When the mv_chan structure is subsequently freed via
devres, might the pending tasklet execute on freed memory?

Should tasklet_kill() be called after free_irq() ensures no ISR is running?

> +
>  	dma_async_device_unregister(&mv_chan->dmadev);
>  
>  	dma_free_wc(dev, MV_XOR_POOL_SIZE,

[Severity: High]
This is a pre-existing issue, but does the mv_xor_channel_add() error path
also need a tasklet_kill() call?

During mv_xor_channel_add(), the self-test functions submit DMA transactions
that trigger real hardware interrupts, scheduling mv_chan->irq_tasklet.

If a self-test times out or dma_async_device_register() fails, the code
jumps to the err_free_irq label, which calls free_irq() but omits
tasklet_kill().

Since the mv_chan structure is devm-allocated, it will be automatically
freed when the probe function returns an error. Could the pending tasklet
then execute on freed memory?

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

[PATCHv2 3/9] dmaengine: mv_xor: bound maximum channels for Armada 37xx

[Rosen Penev]

For XOR_ARMADA_37XX the driver set max_channels = num_present_cpus()
without bounding it by MV_XOR_MAX_CHANNELS (2).  On a system with
more than 2 CPUs this lets the probe loop write past the end of the
xordev->channels[] array when the DT describes enough child nodes.
Add the missing min_t() guard.

Assisted-by: opencode:big-pickle
Fixes: ac5f0f3f863e ("dmaengine: mv_xor: add support for Armada 3700 SoC")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index ef29e8be1db6..588af337afe3 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1387,7 +1387,8 @@ static int mv_xor_probe(struct platform_device *pdev)
 	 */
 	max_engines = num_present_cpus();
 	if (xordev->xor_type == XOR_ARMADA_37XX)
-		max_channels =	num_present_cpus();
+		max_channels = min_t(unsigned int, MV_XOR_MAX_CHANNELS,
+				     num_present_cpus());
 	else
 		max_channels = min_t(unsigned int,
 				     MV_XOR_MAX_CHANNELS,
-- 
2.54.0

Re: [PATCHv2 3/9] dmaengine: mv_xor: bound maximum channels for Armada 37xx

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `clk_prepare_enable()` is ignored, leading to potential warnings and hardware hangs if the clock fails to enable.
- [High] The driver fails to kill the scheduled `irq_tasklet` on error and teardown paths, leading to a Use-After-Free.
- [High] In `mv_xor_channel_remove()`, DMA memory is freed before the interrupt handler is disabled, creating a race condition leading to Use-After-Free.
- [High] The driver lacks a `.remove` callback and does not set `.suppress_bind_attrs = true`, causing severe resource leaks and use-after-free on sysfs unbind.
- [High] The IRQ handler is requested and unmasked before the channel's spinlock and lists are initialized, resulting in a race condition causing a NULL pointer dereference.
- [High] The inline dummy buffers (`dummy_src`, `dummy_dst`) share cachelines with struct members and their DMA mapping directions are reversed.
--

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

dmaengine: mv_xor: bound maximum channels for Armada 37xx

This commit adds a missing bounds check for the max_channels calculation
on Armada 37xx. It ensures max_channels does not exceed MV_XOR_MAX_CHANNELS
based on num_present_cpus(), preventing the probe loop from writing past
the end of the xordev->channels[] array.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..68f0880ad441f 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
[ ... ]
> @@ -1376,7 +1376,8 @@ static int mv_xor_probe(struct platform_device *pdev)
>  	 */
>  	max_engines = num_present_cpus();
>  	if (xordev->xor_type == XOR_ARMADA_37XX)
> -		max_channels =	num_present_cpus();
> +		max_channels = min_t(unsigned int, MV_XOR_MAX_CHANNELS,
> +				     num_present_cpus());
>  	else
>  		max_channels = min_t(unsigned int,
>  				     MV_XOR_MAX_CHANNELS,

While reviewing this patch, I noticed a few pre-existing issues in the
surrounding code that might need attention.

[Severity: High]
This isn't a bug introduced by this patch, but looking at mv_xor_probe()
the return value of clk_prepare_enable() appears to be ignored:

    if (!IS_ERR(xordev->clk))
        clk_prepare_enable(xordev->clk);

If the clock fails to enable, would the driver continue probing and
subsequently hang the hardware bus during I/O accesses?

Additionally, the error path unconditionally executes clk_disable_unprepare():

    if (!IS_ERR(xordev->clk)) {
        clk_disable_unprepare(xordev->clk);
        clk_put(xordev->clk);
    }

Could this trigger a state imbalance warning in the clock core if the clock
was never successfully enabled?

[Severity: High]
This is a pre-existing issue, but does mv_xor_channel_remove() miss killing
the irq_tasklet before freeing the channel?

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
    ...
    free_irq(mv_chan->irq, mv_chan);

    return 0;
}

Could a pending tasklet run after mv_chan is freed by devres, leading to
a use-after-free? It looks like the err_free_irq path in mv_xor_channel_add()
has a similar pattern and might also need a tasklet_kill() before returning.

[Severity: High]
This is a pre-existing issue, but is there a race condition in
mv_xor_channel_remove() when freeing DMA memory?

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
    ...
    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,
                     MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
    ...
    free_irq(mv_chan->irq, mv_chan);
}

Since dma_free_wc() and dma_unmap_single() are called before free_irq(), could
an interrupt fire during this window and schedule the tasklet to access the
freed descriptor buffers?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver lack a
.remove callback and omit .suppress_bind_attrs = true?

drivers/dma/mv_xor.c:
static struct platform_driver mv_xor_driver = {
    .probe          = mv_xor_probe,
    .suspend        = mv_xor_suspend,
    .resume         = mv_xor_resume,
    .driver         = {
        .name           = MV_XOR_NAME,
        .of_match_table = mv_xor_dt_ids,
    },
};

If the root user manually unbinds the device via sysfs, would this leak DMA
device registrations and hardware interrupts? Could a subsequent interrupt
access the devm-freed mv_chan and cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but does requesting and unmasking the
interrupt before initializing the channel's lock and lists risk a NULL
pointer dereference?

drivers/dma/mv_xor.c:mv_xor_channel_add() {
    ...
    ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
                      0, dev_name(&pdev->dev), mv_chan);
    if (ret)
        goto err_free_dma;

    mv_chan_unmask_interrupts(mv_chan);

    ...

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

If an interrupt fires immediately after being unmasked, wouldn't the tasklet
attempt to acquire an uninitialized spinlock and iterate over the
uninitialized mv_chan->chain list?

[Severity: High]
This isn't a bug introduced by this patch, but do the inline dummy buffers
share cachelines with adjacent struct members?

drivers/dma/mv_xor.h:struct mv_xor_chan {
    ...
    int                     op_in_desc;
    char                    dummy_src[MV_XOR_MIN_BYTE_COUNT];
    char                    dummy_dst[MV_XOR_MIN_BYTE_COUNT];
    dma_addr_t              dummy_src_addr, dummy_dst_addr;
}

In mv_xor_channel_add(), the mapping directions appear to be reversed:

    mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
    ...
    mv_chan->dummy_dst_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_dst, MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);

Could mapping dummy_src as DMA_FROM_DEVICE cause the DMA API to perform
a cache invalidation that clobbers dirty, unflushed CPU writes to adjacent
members like op_in_desc?

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

[PATCHv2 4/9] dmaengine: mv_xor: abort channel before freeing resources on timeout

[Rosen Penev]

In mv_chan_memcpy_self_test() and mv_chan_xor_self_test(), if the DMA
operation times out (status check returns !DMA_COMPLETE), the code jumps
to free_resources which unmaps DMA buffers, frees the descriptor pool,
and releases the source/destination pages without ever stopping the
hardware channel.  The DMA engine may still be executing and could DMA
into the freed destination pages, corrupting kernel memory.

Add mv_chan_disable() which masks interrupts and clears the activation
register to abort any in-flight operation.  Call it in the self-test
error paths before freeing resources, and also at the top of
mv_xor_free_chan_resources() to protect clients that release the
channel while DMA is active.

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

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 588af337afe3..44a7d4f7fb0d 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -169,6 +169,12 @@ static void mv_chan_activate(struct mv_xor_chan *chan)
 	writel(BIT(0), XOR_ACTIVATION(chan));
 }
 
+static void mv_chan_disable(struct mv_xor_chan *chan)
+{
+	mv_chan_mask_interrupts(chan);
+	writel_relaxed(0, XOR_ACTIVATION(chan));
+}
+
 static char mv_chan_is_busy(struct mv_xor_chan *chan)
 {
 	u32 state = readl_relaxed(XOR_ACTIVATION(chan));
@@ -638,6 +644,8 @@ static void mv_xor_free_chan_resources(struct dma_chan *chan)
 	struct mv_xor_desc_slot *iter, *_iter;
 	int in_use_descs = 0;
 
+	mv_chan_disable(mv_chan);
+
 	spin_lock_bh(&mv_chan->lock);
 
 	mv_chan_slot_cleanup(mv_chan);
@@ -854,7 +862,7 @@ static int mv_chan_memcpy_self_test(struct mv_xor_chan *mv_chan)
 		dev_err(dma_chan->device->dev,
 			"Self-test copy timed out, disabling\n");
 		err = -ENODEV;
-		goto free_resources;
+		goto free_chan;
 	}
 
 	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
@@ -863,9 +871,11 @@ static int mv_chan_memcpy_self_test(struct mv_xor_chan *mv_chan)
 		dev_err(dma_chan->device->dev,
 			"Self-test copy failed compare, disabling\n");
 		err = -ENODEV;
-		goto free_resources;
+		goto free_chan;
 	}
 
+free_chan:
+	mv_chan_disable(mv_chan);
 free_resources:
 	dmaengine_unmap_put(unmap);
 	mv_xor_free_chan_resources(dma_chan);
@@ -987,7 +997,7 @@ mv_chan_xor_self_test(struct mv_xor_chan *mv_chan)
 		dev_err(dma_chan->device->dev,
 			"Self-test xor timed out, disabling\n");
 		err = -ENODEV;
-		goto free_resources;
+		goto free_chan;
 	}
 
 	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
@@ -999,10 +1009,12 @@ mv_chan_xor_self_test(struct mv_xor_chan *mv_chan)
 				"Self-test xor failed compare, disabling. index %d, data %x, expected %x\n",
 				i, ptr[i], cmp_word);
 			err = -ENODEV;
-			goto free_resources;
+			goto free_chan;
 		}
 	}
 
+free_chan:
+	mv_chan_disable(mv_chan);
 free_resources:
 	dmaengine_unmap_put(unmap);
 	mv_xor_free_chan_resources(dma_chan);
@@ -1020,6 +1032,7 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 	struct device *dev = mv_chan->dmadev.dev;
 
 	mv_chan_mask_interrupts(mv_chan);
+	mv_chan_disable(mv_chan);
 	tasklet_kill(&mv_chan->irq_tasklet);
 
 	dma_async_device_unregister(&mv_chan->dmadev);
-- 
2.54.0

[PATCHv2 5/9] dmaengine: 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.

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 44a7d4f7fb0d..d9403172ef59 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1383,12 +1383,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
@@ -1477,11 +1474,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: [PATCHv2 5/9] dmaengine: mv_xor: use devm_clk_get_optional_enabled

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing tasklet_kill during channel teardown leads to Use-After-Free.
- [High] Missing .remove callback without suppress_bind_attrs leads to sysfs unbind Use-After-Free.
--

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

dmaengine: 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
> index 25ed61f1b0899..a97fa0038652d 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);

[Severity: High]
This is a pre-existing issue, but does moving to a devres-managed clock
further expose a missing .remove callback?

The platform driver mv_xor_driver lacks a .remove callback and does not set
suppress_bind_attrs = true. This allows the driver to be unbound via sysfs.

Because there is no .remove callback, the driver core performs no teardown
on unbind, leaving DMA channels registered and the interrupt active since
request_irq() is used rather than devm_request_irq().

Devres will then automatically free the mv_chan memory, unmap IO memory,
and (with this patch) disable the clock. Any subsequent interrupt or DMA
request will access the freed mv_chan memory and unmapped registers,
causing a use-after-free and system panic.

> +	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);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but could a pending tasklet run
after devres frees the channel memory during probe failure?

In mv_xor_channel_add() in drivers/dma/mv_xor.c, an IRQ tasklet is
initialized. If probe fails (for instance, if a self-test times out while
an interrupt fires concurrently), the error path here calls
mv_xor_channel_remove(), which uses free_irq() to disable the interrupt.

However, neither mv_xor_channel_remove() nor the err_free_irq path in
mv_xor_channel_add() calls tasklet_kill(). Because mv_chan is allocated
via devm_kzalloc(), devres will free the memory shortly after probe returns.

If the scheduled tasklet executes after the memory is freed, it will
dereference mv_chan, leading to a use-after-free.

> -	if (!IS_ERR(xordev->clk)) {
> -		clk_disable_unprepare(xordev->clk);
> -		clk_put(xordev->clk);
> -	}
> -
>  	return ret;
>  }

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

[PATCHv2 6/9] dmaengine: mv_xor: switch to of_irq_get()

[Rosen Penev]

Replace irq_of_parse_and_map() + irq_dispose_mapping() with of_irq_get(),
which maps the IRQ and registers a devres action to automatically dispose
the mapping on driver removal or probe failure.  This allows dropping the
explicit irq_dispose_mapping() calls in the probe error path and remove
function.

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

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index d9403172ef59..eefc8f22bec6 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1423,9 +1423,9 @@ static int mv_xor_probe(struct platform_device *pdev)
 			dma_cap_set(DMA_XOR, cap_mask);
 			dma_cap_set(DMA_INTERRUPT, cap_mask);
 
-			irq = irq_of_parse_and_map(np, 0);
-			if (!irq) {
-				ret = -ENODEV;
+			irq = of_irq_get(np, 0);
+			if (irq < 0) {
+				ret = irq;
 				goto err_channel_add;
 			}
 
@@ -1433,7 +1433,6 @@ static int mv_xor_probe(struct platform_device *pdev)
 						  cap_mask, irq);
 			if (IS_ERR(chan)) {
 				ret = PTR_ERR(chan);
-				irq_dispose_mapping(irq);
 				goto err_channel_add;
 			}
 
@@ -1468,11 +1467,8 @@ static int mv_xor_probe(struct platform_device *pdev)
 
 err_channel_add:
 	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
-		if (xordev->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);
-		}
 
 	return ret;
 }
-- 
2.54.0

[PATCHv2 7/9] dmaengine: 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 eefc8f22bec6..0c159b9e9216 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1037,8 +1037,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,
@@ -1049,8 +1047,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;
 }
 
@@ -1113,8 +1109,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;
@@ -1144,10 +1140,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);
 
@@ -1162,14 +1158,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",
@@ -1180,15 +1176,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: [PATCHv2 7/9] dmaengine: mv_xor: use devm for dma pool and irq

[sashiko-bot]

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

New issues:
- [High] Resource lifecycle violation: calling irq_dispose_mapping() before deferred devm_request_irq() cleanup leaks irqaction and risks interrupt storms.

Pre-existing issues:
- [High] Missing tasklet_kill() during channel teardown leads to potential Use-After-Free.
--

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

dmaengine: 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 25ed61f1b0899..2176be478c86b 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c

[ ... ]

> @@ -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;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the driver never calls
tasklet_kill(&mv_chan->irq_tasklet) during device teardown here in
mv_xor_channel_remove().

If an interrupt fires and schedules the tasklet during a probe failure,
could the tasklet execute after devres has freed the mv_chan structure,
resulting in a Use-After-Free?

This risk might be exacerbated by removing the synchronous free_irq() call
here, which previously helped silence interrupts earlier in the teardown
process.

[ ... ]

> @@ -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]
By deferring the interrupt teardown to devres, could this lead to an issue
where the IRQ mapping is disposed before devm can execute free_irq()?

Looking at mv_xor_probe() which calls mv_xor_channel_add(), if a channel
fails to add, it explicitly calls irq_dispose_mapping() on the error path:

drivers/dma/mv_xor.c:mv_xor_probe() {
    ...
    if (IS_ERR(chan)) {
        ret = PTR_ERR(chan);
        irq_dispose_mapping(irq);
        goto err_channel_add;
    }
    ...
}

It also disposes the mapping for previously successful channels during cleanup:

drivers/dma/mv_xor.c:mv_xor_probe() {
    ...
err_channel_add:
    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);
        }
    }
    ...
}

Since devres runs after probe() completes, this destroys the IRQ descriptor
while the interrupt is still actively requested. When devres later invokes
free_irq(), it might fail to find the descriptor, resulting in a leak of
the irqaction structure.

Additionally, since the error path no longer calls free_irq() synchronously,
does this leave the interrupt unmasked during probe teardown, risking
interrupt storms?

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

[PATCHv2 8/9] dmaengine: mv_xor: allocate dummy buffers with dmam_alloc_coherent

[Rosen Penev]

Replace the streaming DMA mappings for the dummy interrupt-operation
buffers with coherent allocations.  The embedded char arrays in the
channel struct shared cachelines with other members, so dma_map_single
could corrupt adjacent data during cache maintenance.  These buffers
are never touched by the CPU, so coherent memory is the correct choice.

The old DMA directions were also reversed: dummy_src is read by the
XOR engine (should be DMA_TO_DEVICE) and dummy_dst is written by it
(should be DMA_FROM_DEVICE).  Coherent allocations are semantically
directionless, sidestepping the issue entirely.

With dmam_alloc_coherent managing the lifetime the old dma_unmap_single
calls and the error-path labels in mv_xor_channel_add are no longer
needed.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 50 ++++++++++++++------------------------------
 drivers/dma/mv_xor.h |  4 ++--
 2 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 0c159b9e9216..255df2dd9c71 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1029,7 +1029,6 @@ mv_chan_xor_self_test(struct mv_xor_chan *mv_chan)
 static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 {
 	struct dma_chan *chan, *_chan;
-	struct device *dev = mv_chan->dmadev.dev;
 
 	mv_chan_mask_interrupts(mv_chan);
 	mv_chan_disable(mv_chan);
@@ -1037,11 +1036,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
 
 	dma_async_device_unregister(&mv_chan->dmadev);
 
-	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,
-			 MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
-
 	list_for_each_entry_safe(chan, _chan, &mv_chan->dmadev.channels,
 				 device_node) {
 		list_del(&chan->device_node);
@@ -1055,9 +1049,9 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 		   struct platform_device *pdev,
 		   int idx, dma_cap_mask_t cap_mask, int irq)
 {
-	int ret = 0;
 	struct mv_xor_chan *mv_chan;
 	struct dma_device *dma_dev;
+	int ret;
 
 	mv_chan = devm_kzalloc(&pdev->dev, sizeof(*mv_chan), GFP_KERNEL);
 	if (!mv_chan)
@@ -1089,19 +1083,18 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	/*
 	 * These source and destination dummy buffers are used to implement
 	 * a DMA_INTERRUPT operation as a minimum-sized XOR operation.
-	 * Hence, we only need to map the buffers at initialization-time.
+	 * Hence, we only need to allocate the buffers at initialization-time.
+	 * The XOR engine reads from dummy_src and writes to dummy_dst.
 	 */
-	mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
-		mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
-	if (dma_mapping_error(dma_dev->dev, mv_chan->dummy_src_addr))
+	mv_chan->dummy_src = dmam_alloc_coherent(&pdev->dev, MV_XOR_MIN_BYTE_COUNT,
+						  &mv_chan->dummy_src_addr, GFP_KERNEL);
+	if (!mv_chan->dummy_src)
 		return ERR_PTR(-ENOMEM);
 
-	mv_chan->dummy_dst_addr = dma_map_single(dma_dev->dev,
-		mv_chan->dummy_dst, MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
-	if (dma_mapping_error(dma_dev->dev, mv_chan->dummy_dst_addr)) {
-		ret = -ENOMEM;
-		goto err_unmap_src;
-	}
+	mv_chan->dummy_dst = dmam_alloc_coherent(&pdev->dev, MV_XOR_MIN_BYTE_COUNT,
+						  &mv_chan->dummy_dst_addr, GFP_KERNEL);
+	if (!mv_chan->dummy_dst)
+		return ERR_PTR(-ENOMEM);
 
 
 	/* allocate coherent memory for hardware descriptors
@@ -1111,10 +1104,8 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	mv_chan->dma_desc_pool_virt =
 	  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;
-	}
+	if (!mv_chan->dma_desc_pool_virt)
+		return ERR_PTR(-ENOMEM);
 
 	/* discover transaction capabilities from the platform data */
 	dma_dev->cap_mask = cap_mask;
@@ -1143,7 +1134,7 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	ret = devm_request_irq(&pdev->dev, mv_chan->irq, mv_xor_interrupt_handler,
 			  0, dev_name(&pdev->dev), mv_chan);
 	if (ret)
-		goto err_unmap_dst;
+		return ERR_PTR(ret);
 
 	mv_chan_unmask_interrupts(mv_chan);
 
@@ -1158,14 +1149,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_unmap_dst;
+			return ERR_PTR(ret);
 	}
 
 	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_unmap_dst;
+			return ERR_PTR(ret);
 	}
 
 	dev_info(&pdev->dev, "Marvell XOR (%s): ( %s%s%s)\n",
@@ -1176,18 +1167,9 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 
 	ret = dma_async_device_register(dma_dev);
 	if (ret)
-		goto err_unmap_dst;
+		return ERR_PTR(ret);
 
 	return mv_chan;
-
-err_unmap_dst:
-	dma_unmap_single(dma_dev->dev, mv_chan->dummy_dst_addr,
-			 MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
-err_unmap_src:
-	dma_unmap_single(dma_dev->dev, mv_chan->dummy_src_addr,
-			 MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
-
-	return ERR_PTR(ret);
 }
 
 static void
diff --git a/drivers/dma/mv_xor.h b/drivers/dma/mv_xor.h
index c87cefd38a07..666c72e457d6 100644
--- a/drivers/dma/mv_xor.h
+++ b/drivers/dma/mv_xor.h
@@ -120,8 +120,8 @@ struct mv_xor_chan {
 	int			slots_allocated;
 	struct tasklet_struct	irq_tasklet;
 	int                     op_in_desc;
-	char			dummy_src[MV_XOR_MIN_BYTE_COUNT];
-	char			dummy_dst[MV_XOR_MIN_BYTE_COUNT];
+	void			*dummy_src;
+	void			*dummy_dst;
 	dma_addr_t		dummy_src_addr, dummy_dst_addr;
 	u32                     saved_config_reg, saved_int_mask_reg;
 
-- 
2.54.0

[PATCHv2 9/9] dmaengine: 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 255df2dd9c71..85cb77022144 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1446,8 +1446,19 @@ 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]);
+}
+
 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: [PATCHv2 9/9] dmaengine: mv_xor: add missing platform remove function

[sashiko-bot]

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

New issues:
- [High] Resource leaks in the newly introduced mv_xor_remove function: missing clk_disable_unprepare, clk_put, and irq_dispose_mapping.

Pre-existing issues:
- [High] Incorrect teardown sequence in mv_xor_channel_remove leads to Use-After-Free due to delayed free_irq and missing tasklet_kill.
--

commit 9ff45f7ddd6e175cd19bc1dcad7a0c33d39a5914
Author: Rosen Penev <rosenp@gmail.com>
dmaengine: mv_xor: add missing platform remove function

The driver was missing a remove callback, so channels, DMA devices, and IRQs
were never cleaned up on driver unbind. This patch implements mv_xor_remove
to undo probe operations.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..6065daec69ba9 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1460,8 +1460,19 @@ 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]);

[Severity: High]
Are we missing the teardown of the clock and IRQ mappings here?

Looking at the error path in mv_xor_probe(), we have explicit cleanup
for these resources:

drivers/dma/mv_xor.c:mv_xor_probe() {
...
err_channel_add:
    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);
        }

    if (!IS_ERR(xordev->clk)) {
        clk_disable_unprepare(xordev->clk);
        clk_put(xordev->clk);
    }
...
}

Does omitting clk_disable_unprepare(), clk_put(), and irq_dispose_mapping()
in mv_xor_remove() leak the clock reference and IRQ descriptors when the
driver is unbound?

[Severity: High]
This is a pre-existing issue, but calling mv_xor_channel_remove() here during
unbind exposes a teardown sequence problem.

In mv_xor_channel_remove(), dma_free_wc() is called before free_irq(), and
there is no tasklet_kill() for the irq_tasklet:

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
        ...
        dma_free_wc(dev, MV_XOR_POOL_SIZE,
                    mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
        ...
        free_irq(mv_chan->irq, mv_chan);

        return 0;
}

If an interrupt fires after dma_free_wc() but before free_irq(), the handler
can schedule the irq_tasklet which will access the freed descriptor pool.

Furthermore, without tasklet_kill(), a pending tasklet could execute after
mv_chan is freed by devres, resulting in a use-after-free.

Could this unbind sequence trigger these race conditions?

> +}
> +
>  static struct platform_driver mv_xor_driver = {
>  	.probe		= mv_xor_probe,
> +	.remove		= mv_xor_remove,

[ ... ]

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