[PATCHv4 00/15] dmaengine: fsldma: devm conversion, fixups, and cleanups

[PATCHv4 00/15] dmaengine: fsldma: devm conversion, fixups, and cleanups

[Rosen Penev]

- Kill the channel tasklet before removal to prevent a race with
  the IRQ handler.
- Check the return value of dma_async_device_register() instead
  of silently returning success.
- Replace the powerpc-specific I/O accessors with portable
  generic ones so the driver can be built on non-powerpc
  architectures.

Build-tested with LLVM=1 ARCH=powerpc allmodconfig

v4: address review comments
v3: even more sashiko fixes
v2: add extra fixes to satisfy sashiko

Rosen Penev (15):
  dmaengine: fsldma: kill tasklet before removing channel
  dmaengine: fsldma: drop desc_lock before invoking client callback
  dmaengine: fsldma: halt DMA engine before freeing resources
  dmaengine: fsldma: provide device_release callback
  dmaengine: fsldma: check dma_async_device_register() return value
  dmaengine: fsldma: fix probe error path not freeing IRQs
  dmaengine: fsldma: fix request_irqs unwind freeing unregistered IRQ
  dmaengine: fsldma: convert to platform_get_irq_optional()
  dmaengine: fsldma: use devm_kzalloc() to simplify code
  dmaengine: fsldma: use devm_platform_ioremap_resource()
  dmaengine: fsldma: convert channel allocation to devm_kzalloc()
  dmaengine: fsldma: use devm_of_iomap() to simplify code
  dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get
  dmaengine: fsldma: replace ppc-specific accessors with portable
    generic ones
  dmaengine: fsldma: fix kernel-doc param names to match function
    signatures

 drivers/dma/Kconfig  |   2 +-
 drivers/dma/fsldma.c | 258 ++++++++++++++++++++++---------------------
 drivers/dma/fsldma.h |  35 +++++-
 3 files changed, 168 insertions(+), 127 deletions(-)

--
2.54.0

[PATCHv4 01/15] dmaengine: fsldma: kill tasklet before removing channel

[Rosen Penev]

Add tasklet_kill() in fsl_dma_chan_remove() to prevent a race where the
tasklet is scheduled by the IRQ handler and runs after the channel has been
freed.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/fsldma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 22d62d958abd..0e2f84862261 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1205,6 +1205,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 
 static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 {
+	tasklet_kill(&chan->tasklet);
 	irq_dispose_mapping(chan->irq);
 	list_del(&chan->common.device_node);
 	iounmap(chan->regs);
-- 
2.54.0

Re: [PATCHv4 01/15] dmaengine: fsldma: kill tasklet before removing channel

[sashiko-bot]

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

Pre-existing issues:
- [High] fsldma_of_probe ignores the return value of dma_async_device_register.
- [High] Use-after-free read in fsl_dma_prep_memcpy error path due to freeing the list head during iteration.
- [High] Link descriptors are leaked if fsl_dma_tx_submit() is called while the device is suspended.
- [High] Deadlock caused by invoking client callbacks and dependencies with desc_lock held.
- [Medium] Incorrect loop unwind in fsldma_request_irqs leads to freeing an unallocated IRQ.
--

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

dmaengine: fsldma: kill tasklet before removing channel

Add tasklet_kill() in fsl_dma_chan_remove() to prevent a race where the
tasklet is scheduled by the IRQ handler and runs after the channel has been
freed.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..0e2f84862261c 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1205,6 +1205,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  
>  static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  {
> +	tasklet_kill(&chan->tasklet);
>  	irq_dispose_mapping(chan->irq);
>  	list_del(&chan->common.device_node);
>  	iounmap(chan->regs);

[Severity: Medium]
This is a pre-existing issue, but I noticed the error handling in
fsldma_request_irqs() can free an unallocated interrupt:

drivers/dma/fsldma.c:fsldma_request_irqs() {
...
out_unwind:
	for (/* none */; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;

		if (!chan->irq)
			continue;

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

If request_irq() fails for a channel at index i, the driver does not own
the interrupt for that index. Does starting the unwind loop at the same
index i trigger a kernel warning for trying to free an already-free
interrupt?

[Severity: High]
This is a pre-existing issue, but the return value of
dma_async_device_register() appears to be discarded during probe:

drivers/dma/fsldma.c:fsldma_of_probe() {
...
	dma_async_device_register(&fdev->common);
	return 0;
...
}

If dma_async_device_register() returns an error code like -ENOMEM, the
driver will falsely report a successful probe.

Could this leave the device partially initialized and cause a
use-after-free or id allocator free on an uninitialized id when
dma_async_device_unregister() is called during module unbind?

[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free
read in the error path of fsl_dma_prep_memcpy():

drivers/dma/fsldma.c:fsl_dma_prep_memcpy() {
...
fail:
	if (!first)
		return NULL;

	fsldma_free_desc_list_reverse(chan, &first->tx_list);
	return NULL;
}

When fsldma_free_desc_list_reverse() is called, it iterates through
tx_list:

drivers/dma/fsldma.c:fsldma_free_desc_list_reverse() {
...
	list_for_each_entry_safe_reverse(desc, _desc, list, node)
		fsl_dma_free_descriptor(chan, desc);
...
}

Since all descriptors, including first, are linked into first->tx_list,
when the first descriptor is freed, its embedded tx_list head is also
freed.

Does the safe iterator macro then read a freed next pointer when
evaluating the next element, resulting in a use-after-free read?

[Severity: High]
This is a pre-existing issue, but link descriptors seem to leak if a
transaction is submitted while the device is suspended:

drivers/dma/fsldma.c:fsl_dma_tx_submit() {
...
#ifdef CONFIG_PM
	if (unlikely(chan->pm_state != RUNNING)) {
		chan_dbg(chan, "cannot submit due to suspend\n");
		spin_unlock_bh(&chan->desc_lock);
		return -1;
	}
#endif
...
}

Since the descriptors were allocated by fsl_dma_prep_memcpy() and
ownership was handed off for submission, returning here without queueing
them leaves them orphaned.

Are these descriptors permanently leaked from the dma pool?

[Severity: High]
This is a pre-existing issue, but could invoking client callbacks while
holding desc_lock lead to a deadlock?

drivers/dma/fsldma.c:dma_do_tasklet() {
...
	spin_lock(&chan->desc_lock);
...
	fsldma_cleanup_descriptors(chan);

	spin_unlock(&chan->desc_lock);
...
}

fsldma_cleanup_descriptors() iterates over running descriptors and calls
fsldma_run_tx_complete_actions(), which invokes callbacks:

drivers/dma/fsldma.c:fsldma_run_tx_complete_actions() {
...
		dmaengine_desc_get_callback_invoke(txd, NULL);
...
}

If the client callback calls dmaengine_submit() or a dependent
transaction executes on the same channel, it will call
fsl_dma_tx_submit(), which attempts to acquire
spin_lock_bh(&chan->desc_lock).

Will this cause a self-deadlock since the lock is already held by the
tasklet?

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

[PATCHv4 02/15] dmaengine: fsldma: drop desc_lock before invoking client callback

[Rosen Penev]

fsldma_run_tx_complete_actions() calls dmaengine_desc_get_callback_invoke()
while still holding chan->desc_lock.  If the client submits a new
transaction from their completion callback, fsl_dma_tx_submit()
tries to acquire the same non-recursive spinlock, causing a
self-deadlock.

Fix by extracting the callback info under the lock, removing the
descriptor from ld_running, dropping the lock, then invoking the
callback and running dependencies outside the lock.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 0e2f84862261..455d21d738de 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -496,16 +496,19 @@ static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
 }
 
 /**
- * fsldma_run_tx_complete_actions - cleanup a single link descriptor
+ * fsldma_run_tx_complete_actions - unmap and extract callback from a descriptor
  * @chan: Freescale DMA channel
- * @desc: descriptor to cleanup and free
+ * @desc: descriptor to process
  * @cookie: Freescale DMA transaction identifier
+ * @cb: returned callback information
  *
- * This function is used on a descriptor which has been executed by the DMA
- * controller. It will run any callbacks, submit any dependencies.
+ * Unmap the descriptor if it has been submitted and extract its callback
+ * into @cb.  The caller must invoke the callback and run dependencies
+ * after releasing chan->desc_lock.
  */
 static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
-		struct fsl_desc_sw *desc, dma_cookie_t cookie)
+		struct fsl_desc_sw *desc, dma_cookie_t cookie,
+		struct dmaengine_desc_callback *cb)
 {
 	struct dma_async_tx_descriptor *txd = &desc->async_tx;
 	dma_cookie_t ret = cookie;
@@ -514,49 +517,14 @@ static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
 
 	if (txd->cookie > 0) {
 		ret = txd->cookie;
-
 		dma_descriptor_unmap(txd);
-		/* Run the link descriptor callback function */
-		dmaengine_desc_get_callback_invoke(txd, NULL);
 	}
 
-	/* Run any dependencies */
-	dma_run_dependencies(txd);
+	dmaengine_desc_get_callback(txd, cb);
 
 	return ret;
 }
 
-/**
- * fsldma_clean_running_descriptor - move the completed descriptor from
- * ld_running to ld_completed
- * @chan: Freescale DMA channel
- * @desc: the descriptor which is completed
- *
- * Free the descriptor directly if acked by async_tx api, or move it to
- * queue ld_completed.
- */
-static void fsldma_clean_running_descriptor(struct fsldma_chan *chan,
-		struct fsl_desc_sw *desc)
-{
-	/* Remove from the list of transactions */
-	list_del(&desc->node);
-
-	/*
-	 * the client is allowed to attach dependent operations
-	 * until 'ack' is set
-	 */
-	if (!async_tx_test_ack(&desc->async_tx)) {
-		/*
-		 * Move this descriptor to the list of descriptors which is
-		 * completed, but still awaiting the 'ack' bit to be set.
-		 */
-		list_add_tail(&desc->node, &chan->ld_completed);
-		return;
-	}
-
-	dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
-}
-
 /**
  * fsl_chan_xfer_ld_queue - transfer any pending transactions
  * @chan : Freescale DMA channel
@@ -635,22 +603,23 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
  */
 static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
 {
-	struct fsl_desc_sw *desc, *_desc;
+	struct fsl_desc_sw *desc;
 	dma_cookie_t cookie = 0;
 	dma_addr_t curr_phys = get_cdar(chan);
 	int seen_current = 0;
 
 	fsldma_clean_completed_descriptor(chan);
 
-	/* Run the callback for each descriptor, in order */
-	list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
-		/*
-		 * do not advance past the current descriptor loaded into the
-		 * hardware channel, subsequent descriptors are either in
-		 * process or have not been submitted
-		 */
-		if (seen_current)
-			break;
+	/*
+	 * Take descriptors one at a time from the front of the running
+	 * queue.  We re-read the list each iteration so that we don't
+	 * chase a stale next pointer across the lock-drop below.
+	 */
+	while (!seen_current && !list_empty(&chan->ld_running)) {
+		struct dmaengine_desc_callback cb;
+
+		desc = list_first_entry(&chan->ld_running,
+					struct fsl_desc_sw, node);
 
 		/*
 		 * stop the search if we reach the current descriptor and the
@@ -662,9 +631,42 @@ static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
 				break;
 		}
 
-		cookie = fsldma_run_tx_complete_actions(chan, desc, cookie);
+		cookie = fsldma_run_tx_complete_actions(chan, desc, cookie, &cb);
 
-		fsldma_clean_running_descriptor(chan, desc);
+		/*
+		 * Remove from the running list before dropping the lock so
+		 * that terminate_all cannot free this descriptor while we
+		 * call into the client below.
+		 */
+		list_del(&desc->node);
+
+		/*
+		 * Prevent dma_run_dependencies() from calling
+		 * fsl_chan_xfer_ld_queue() while we are not holding the
+		 * lock.  That would splice pending descriptors into
+		 * ld_running before they have been completed by hardware.
+		 * fsl_chan_xfer_ld_queue at the end of this function will
+		 * re-evaluate the situation.
+		 */
+		chan->idle = false;
+
+		/*
+		 * Drop the lock before invoking the client callback, since
+		 * the DMAengine API explicitly allows clients to submit new
+		 * transactions from their completion callback.  Otherwise
+		 * we self-deadlock on chan->desc_lock.
+		 */
+		spin_unlock(&chan->desc_lock);
+		dmaengine_desc_callback_invoke(&cb, NULL);
+		dma_run_dependencies(&desc->async_tx);
+		spin_lock(&chan->desc_lock);
+
+		chan->idle = true;
+
+		if (!async_tx_test_ack(&desc->async_tx))
+			list_add_tail(&desc->node, &chan->ld_completed);
+		else
+			dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
 	}
 
 	/*
-- 
2.54.0

Re: [PATCHv4 02/15] dmaengine: fsldma: drop desc_lock before invoking client callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditionally setting `chan->idle = true` inside `fsldma_cleanup_descriptors()` clobbers the active hardware busy state, causing active transfers to be overwritten.
- [High] Dropping the lock without reentrancy protection allows concurrent execution of `fsldma_cleanup_descriptors()`, breaking callback serialization and corrupting `completed_cookie`.
- [High] Dropping the channel lock during descriptor cleanup allows concurrent channel teardown to destroy the descriptor pool, leading to a Use-After-Free.
--

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

dmaengine: fsldma: drop desc_lock before invoking client callback

fsldma_run_tx_complete_actions() calls dmaengine_desc_get_callback_invoke()
while still holding chan->desc_lock.  If the client submits a new
transaction from their completion callback, fsl_dma_tx_submit()
tries to acquire the same non-recursive spinlock, causing a
self-deadlock.

Fix by extracting the callback info under the lock, removing the
descriptor from ld_running, dropping the lock, then invoking the
callback and running dependencies outside the lock.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..5c8a8fe25617d 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
[ ... ]
> @@ -635,22 +603,23 @@ static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
>  {
[ ... ]
> +	/*
> +	 * Take descriptors one at a time from the front of the running
> +	 * queue.  We re-read the list each iteration so that we don't
> +	 * chase a stale next pointer across the lock-drop below.
> +	 */
> +	while (!seen_current && !list_empty(&chan->ld_running)) {
[ ... ]
> +		/*
> +		 * stop the search if we reach the current descriptor and the
> +		 * channel is busy
> +		 */
> +		if (desc->async_tx.phys == curr_phys) {
> +			seen_current = 1;
> +			if (!dma_is_idle(chan))
> +				break;
> +		}
>  
[ ... ]
> +		/*
> +		 * Drop the lock before invoking the client callback, since
> +		 * the DMAengine API explicitly allows clients to submit new
> +		 * transactions from their completion callback.  Otherwise
> +		 * we self-deadlock on chan->desc_lock.
> +		 */
> +		spin_unlock(&chan->desc_lock);
> +		dmaengine_desc_callback_invoke(&cb, NULL);
> +		dma_run_dependencies(&desc->async_tx);
> +		spin_lock(&chan->desc_lock);

[Severity: High]
Does dropping the lock here without reentrancy protection allow concurrent
execution of fsldma_cleanup_descriptors()?

If another thread calls fsl_tx_status() concurrently, it could acquire
chan->desc_lock, enter fsldma_cleanup_descriptors(), and execute callbacks
simultaneously. This would break the DMAengine API's serialization guarantee.

> +
> +		chan->idle = true;

[Severity: High]
If the loop breaks out early because the current descriptor is busy
(!dma_is_idle(chan)), chan->idle is still set to true from the previous
iteration while the hardware is actively busy.

Will fsl_chan_xfer_ld_queue() called later in this function evaluate this
corrupted chan->idle state, skip its !chan->idle safety check, and forcefully
overwrite the active transaction by issuing a new DMA start command?

> +
> +		if (!async_tx_test_ack(&desc->async_tx))
> +			list_add_tail(&desc->node, &chan->ld_completed);
> +		else
> +			dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);

[Severity: High]
Could dropping the lock above allow fsl_dma_free_chan_resources() to execute
concurrently, destroying chan->desc_pool and setting it to NULL before we call
dma_pool_free() here?

>  	}
>  
>  	/*
>  	 * Start any pending transactions automatically
>  	 *
>  	 * In the ideal case, we keep the DMA controller busy while we go
>  	 * ahead and free the descriptors below.
>  	 */
>  	fsl_chan_xfer_ld_queue(chan);
>  
>  	if (cookie > 0)
>  		chan->common.completed_cookie = cookie;

[Severity: High]
If concurrent threads execute fsldma_cleanup_descriptors() due to the dropped
lock, can an older local cookie from one thread overwrite a newer one written
by another thread here?

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

[PATCHv4 03/15] dmaengine: fsldma: halt DMA engine before freeing resources

[Rosen Penev]

When a channel is released (fsl_dma_free_chan_resources) or the driver is
unbound (fsl_dma_chan_remove), the descriptor pool and channel resources
are freed without stopping the DMA hardware first.  An active transfer
could continue executing in the background, fetching descriptors or
writing data to physical memory pages that have already been freed.

Fix by calling dma_halt() in both paths before cleaning up, matching
the pattern already used in fsl_dma_device_terminate_all().

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/fsldma.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 455d21d738de..1ba10d065278 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -748,6 +748,7 @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
 
 	chan_dbg(chan, "free all channel resources\n");
 	spin_lock_bh(&chan->desc_lock);
+	dma_halt(chan);
 	fsldma_cleanup_descriptors(chan);
 	fsldma_free_desc_list(chan, &chan->ld_pending);
 	fsldma_free_desc_list(chan, &chan->ld_running);
@@ -1207,6 +1208,10 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 
 static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 {
+	spin_lock_bh(&chan->desc_lock);
+	dma_halt(chan);
+	spin_unlock_bh(&chan->desc_lock);
+
 	tasklet_kill(&chan->tasklet);
 	irq_dispose_mapping(chan->irq);
 	list_del(&chan->common.device_node);
-- 
2.54.0

[PATCHv4 04/15] dmaengine: fsldma: provide device_release callback

[Rosen Penev]

The DMA core requires drivers to set dma_device.device_release so that
the container structure is only freed after all references to it have
been dropped (see the comment above dma_async_device_register()).

This driver violated that contract: fdev was devm_kzalloc()'d with no
device_release callback.  If a client still held a channel reference
when the driver was unbound, dma_device_release() would eventually
run on freed memory, causing a use-after-free.

Fix by allocating fdev with kzalloc_obj(), adding
fsldma_device_release() to free it, and setting device_release.
fsldma_of_remove() now saves channel pointers and frees IRQs before
calling dma_async_device_unregister(), since fdev may be freed by
the release callback inside that call.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 1ba10d065278..43d817f6ded1 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1219,6 +1219,8 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 	kfree(chan);
 }
 
+static void fsldma_device_release(struct dma_device *dma_dev);
+
 static int fsldma_of_probe(struct platform_device *op)
 {
 	struct fsldma_device *fdev;
@@ -1257,6 +1259,7 @@ static int fsldma_of_probe(struct platform_device *op)
 	fdev->common.device_issue_pending = fsl_dma_memcpy_issue_pending;
 	fdev->common.device_config = fsl_dma_device_config;
 	fdev->common.device_terminate_all = fsl_dma_device_terminate_all;
+	fdev->common.device_release = fsldma_device_release;
 	fdev->common.dev = &op->dev;
 
 	fdev->common.src_addr_widths = FSL_DMA_BUSWIDTHS;
@@ -1316,19 +1319,33 @@ static int fsldma_of_probe(struct platform_device *op)
 	return err;
 }
 
+static void fsldma_device_release(struct dma_device *dma_dev)
+{
+	struct fsldma_device *fdev = container_of(dma_dev, struct fsldma_device,
+						  common);
+	kfree(fdev);
+}
+
 static void fsldma_of_remove(struct platform_device *op)
 {
-	struct fsldma_device *fdev;
+	struct fsldma_device *fdev = platform_get_drvdata(op);
+	struct fsldma_chan *chans[FSL_DMA_MAX_CHANS_PER_DEVICE];
 	unsigned int i;
 
-	fdev = platform_get_drvdata(op);
-	dma_async_device_unregister(&fdev->common);
+	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++)
+		chans[i] = fdev->chan[i];
 
 	fsldma_free_irqs(fdev);
 
+	/*
+	 * fdev may be freed by fsldma_device_release inside this call;
+	 * use saved copies of the channel pointers afterwards.
+	 */
+	dma_async_device_unregister(&fdev->common);
+
 	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
-		if (fdev->chan[i])
-			fsl_dma_chan_remove(fdev->chan[i]);
+		if (chans[i])
+			fsl_dma_chan_remove(chans[i]);
 	}
 	irq_dispose_mapping(fdev->irq);
 
-- 
2.54.0

Re: [PATCHv4 04/15] dmaengine: fsldma: provide device_release callback

[sashiko-bot]

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

New issues:
- [Critical] Use-After-Free (Read/Write) and Double-Free of `fdev` during device removal.
- [High] Teardown reordering causes a race condition leading to hung transfers.

Pre-existing issues:
- [Critical] Invoking client callbacks with the descriptor spinlock held causes a recursive deadlock.
- [High] DMA channels are unconditionally destroyed leading to potential Use-After-Free by active clients.
- [High] Missing tasklet cancellation leads to a Use-After-Free of the channel structure.
- [High] The error rollback path in fsldma_suspend_late() double-unlocks spinlocks.
--

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

dmaengine: fsldma: provide device_release callback

The DMA core requires drivers to set dma_device.device_release so that
the container structure is only freed after all references to it have
been dropped (see the comment above dma_async_device_register()).

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..06a6996a53229 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1211,6 +1211,8 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  	kfree(chan);
>  }

[Severity: High]
This is a pre-existing issue, but does this unconditionally destroy the
channel even if clients are still using it?

Since dma_async_device_unregister() doesn't enforce that clients drop all
channel references before it returns, clients might still hold pointers to
the freed chan structure. 

If they try to use it later, couldn't this cause a use-after-free?

[Severity: High]
This is also a pre-existing issue, but is it safe to free the channel without
killing the tasklet first?

The code frees the IRQ but does not call tasklet_kill(&chan->tasklet). 

If an interrupt fires and schedules a tasklet just before the IRQ is
disabled, the tasklet could execute concurrently or later and dereference
the freed chan structure in dma_do_tasklet().

>  
> +static void fsldma_device_release(struct dma_device *dma_dev);
> +

[ ... ]

> @@ -1308,19 +1311,33 @@ static int fsldma_of_probe(struct platform_device *op)
>  	return err;
>  }
>  
> +static void fsldma_device_release(struct dma_device *dma_dev)
> +{
> +	struct fsldma_device *fdev = container_of(dma_dev, struct fsldma_device,
> +						  common);
> +	kfree(fdev);
> +}
> +
>  static void fsldma_of_remove(struct platform_device *op)
>  {
> -	struct fsldma_device *fdev;
> +	struct fsldma_device *fdev = platform_get_drvdata(op);
> +	struct fsldma_chan *chans[FSL_DMA_MAX_CHANS_PER_DEVICE];
>  	unsigned int i;
>  
> -	fdev = platform_get_drvdata(op);
> -	dma_async_device_unregister(&fdev->common);
> +	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++)
> +		chans[i] = fdev->chan[i];
>  
>  	fsldma_free_irqs(fdev);

[Severity: High]
Does this reordering open a race condition that could hang transfers?

By calling fsldma_free_irqs(fdev) before dma_async_device_unregister(),
there is a window where the DMA device is still fully active and accepting
new transfers, but its interrupts have been disabled and freed. 

If a transaction completes during this window, will the completion interrupt
never fire?

>  
> +	/*
> +	 * fdev may be freed by fsldma_device_release inside this call;
> +	 * use saved copies of the channel pointers afterwards.
> +	 */
> +	dma_async_device_unregister(&fdev->common);
> +
>  	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> -		if (fdev->chan[i])
> -			fsl_dma_chan_remove(fdev->chan[i]);
> +		if (chans[i])
> +			fsl_dma_chan_remove(chans[i]);
>  	}
>  	irq_dispose_mapping(fdev->irq);
>  

[Severity: Critical]
Does this sequence result in a use-after-free and double-free of fdev?

If dma_async_device_unregister() drops the final reference and triggers
fsldma_device_release(), fdev is freed. 

However, the function then continues to access fdev indirectly by calling
fsl_dma_chan_remove(chans[i]), which modifies the list. It also reads
fdev->irq here to perform unmapping, and eventually explicitly calls
kfree(fdev) at the end of the function.

[Severity: High]
This is a pre-existing issue, but does fsldma_suspend_late() double-unlock
spinlocks in the error rollback path?

In fsldma_suspend_late(), the forward iteration acquires the descriptor
spinlock, updates the state, and then unlocks it on success. If it
encounters a busy channel, it jumps to the out label to roll back. 

However, the rollback loop unconditionally calls
spin_unlock_bh(&chan->desc_lock) for all previously processed channels,
even though their locks had already been successfully released in the
forward loop.

[Severity: Critical]
This is a pre-existing issue, but does invoking client callbacks with the
descriptor spinlock held cause a recursive deadlock?

fsldma_cleanup_descriptors() is called with chan->desc_lock held. It then
calls fsldma_run_tx_complete_actions(), which synchronously invokes the
client's completion callback. 

If the client submits a new DMA transaction from within this callback:

fsldma_run_tx_complete_actions()
   dmaengine_desc_get_callback_invoke()
      client_callback()
         fsl_dma_tx_submit()

Will fsl_dma_tx_submit() attempt to acquire the exact same chan->desc_lock
and result in an immediate deadlock?

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

[PATCHv4 05/15] dmaengine: fsldma: check dma_async_device_register() return value

[Rosen Penev]

Check the return value of dma_async_device_register() in the probe
path and propagate errors instead of silently returning success.
Previously, a registration failure would cause a NULL pointer
dereference in list_del_rcu() during remove when
dma_async_device_unregister() tried to remove the device's
global_node from a list it was never added to.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 43d817f6ded1..3009e1531292 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1303,7 +1303,11 @@ static int fsldma_of_probe(struct platform_device *op)
 		goto out_free_fdev;
 	}
 
-	dma_async_device_register(&fdev->common);
+	err = dma_async_device_register(&fdev->common);
+	if (err) {
+		dev_err(fdev->dev, "unable to register DMA device\n");
+		goto out_free_fdev;
+	}
 	return 0;
 
 out_free_fdev:
-- 
2.54.0

Re: [PATCHv4 05/15] dmaengine: fsldma: check dma_async_device_register() return value

[sashiko-bot]

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

New issues:
- [Critical] Missing `fsldma_free_irqs()` in the new `dma_async_device_register()` error path causes an IRQ leak and a critical Use-After-Free vulnerability.

Pre-existing issues:
- [High] `fsl_dma_chan_remove` frees the channel structure without killing its associated tasklet.
--

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

dmaengine: fsldma: check dma_async_device_register() return value

This commit adds a check for the return value of dma_async_device_register()
in the probe path. It propagates the error instead of silently succeeding,
fixing a null pointer dereference during removal on registration failure.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1292,7 +1292,11 @@ static int fsldma_of_probe(struct platform_device *op)
>  		goto out_free_fdev;
>  	}
>  
> -	dma_async_device_register(&fdev->common);
> +	err = dma_async_device_register(&fdev->common);
> +	if (err) {
> +		dev_err(fdev->dev, "unable to register DMA device\n");
> +		goto out_free_fdev;

[Severity: Critical]
If dma_async_device_register() fails here, we jump to out_free_fdev, but
what happens to the IRQs requested just prior in fsldma_request_irqs()?

The out_free_fdev cleanup path unmaps memory and frees the fdev structure,
but doesn't appear to call fsldma_free_irqs() or free_irq(). Will this leave
active IRQ handlers pointing to freed memory, leading to a use-after-free
if an interrupt fires on the shared line?

> +	}
>  	return 0;
>  
>  out_free_fdev:

[Severity: High]
This is a pre-existing issue, but does fsl_dma_chan_remove() need to kill
the channel's tasklet before freeing the channel structure?

In the cleanup loop immediately following this label, fsl_dma_chan_remove()
is called to tear down each channel. Looking at fsl_dma_chan_remove(), it
unmaps memory and frees the chan struct via kfree() without calling
tasklet_kill(&chan->tasklet).

If a scheduled tasklet executes concurrently or immediately after the
channel is freed, will the tasklet handler dereference the freed chan
pointer?

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

[PATCHv4 06/15] dmaengine: fsldma: fix probe error path not freeing IRQs

[Rosen Penev]

If dma_async_device_register() fails after fsldma_request_irqs()
succeeded, the error path jumped to out_free_fdev which only removed
channels but never freed the already-registered IRQs.  A subsequent
interrupt would access freed memory.

Fix by adding an out_free_irqs label that calls fsldma_free_irqs()
before falling through to the existing channel cleanup.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 3009e1531292..4475d50a94f5 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1306,10 +1306,12 @@ static int fsldma_of_probe(struct platform_device *op)
 	err = dma_async_device_register(&fdev->common);
 	if (err) {
 		dev_err(fdev->dev, "unable to register DMA device\n");
-		goto out_free_fdev;
+		goto out_free_irqs;
 	}
 	return 0;
 
+out_free_irqs:
+	fsldma_free_irqs(fdev);
 out_free_fdev:
 	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
 		if (fdev->chan[i])
-- 
2.54.0

[PATCHv4 07/15] dmaengine: fsldma: fix request_irqs unwind freeing unregistered IRQ

[Rosen Penev]

When fsldma_request_irqs() fails on a per-channel IRQ, the unwind
loop starts at the current index i, which calls free_irq() on the
IRQ that request_irq() just failed to register.  Decrement i before
the loop to skip the failed channel.

Bug introduced by commit 586f54672b33 ("dmaengine: fsldma: convert
to platform_get_irq_optional()").

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 4475d50a94f5..c04a7fbd2ed0 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1088,7 +1088,7 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
 	return 0;
 
 out_unwind:
-	for (/* none */; i >= 0; i--) {
+	for (i--; i >= 0; i--) {
 		chan = fdev->chan[i];
 		if (!chan)
 			continue;
-- 
2.54.0

Re: [PATCHv4 07/15] dmaengine: fsldma: fix request_irqs unwind freeing unregistered IRQ

[sashiko-bot]

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

Pre-existing issues:
- [High] The error cleanup path in `fsl_dma_prep_memcpy` causes a Use-After-Free read when using `fsldma_free_desc_list_reverse` to free a list whose head is embedded in one of the freed elements.
- [High] Missing `tasklet_kill()` during channel teardown leads to Use-After-Free if a tasklet is pending or running.
--

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

dmaengine: fsldma: fix request_irqs unwind freeing unregistered IRQ

When fsldma_request_irqs() fails on a per-channel IRQ, the unwind
loop starts at the current index i, which calls free_irq() on the
IRQ that request_irq() just failed to register. Decrement i before
the loop to skip the failed channel.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..285c5e284d425 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1085,7 +1085,7 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
>  	return 0;
>  
>  out_unwind:
> -	for (/* none */; i >= 0; i--) {
> +	for (i--; i >= 0; i--) {
>  		chan = fdev->chan[i];
>  		if (!chan)
>  			continue;

[Severity: High]
This is a pre-existing issue, but while reviewing the error handling I noticed
a potential use-after-free in drivers/dma/fsldma.c.

In fsl_dma_prep_memcpy(), if a descriptor allocation fails, the error path
cleans up by calling fsldma_free_desc_list_reverse() and passing the list
head embedded in the `first` descriptor:

	fsldma_free_desc_list_reverse(chan, &first->tx_list);

In fsldma_free_desc_list_reverse(), the traversal is done using:

	list_for_each_entry_safe_reverse(desc, _desc, list, node)
		fsl_dma_free_descriptor(chan, desc);

Since the list head `&first->tx_list` is embedded within the memory that will
be freed, does the list traversal macro's increment step cause a read from
freed memory when it evaluates the previous node?

[Severity: High]
This isn't a bug introduced by this patch, but I found a potential missing
cleanup in the channel teardown path in drivers/dma/fsldma.c.

In fsldma_chan_irq(), a tasklet is scheduled:

	tasklet_schedule(&chan->tasklet);

During driver unbind or error handling, fsl_dma_chan_remove() frees the DMA
channel:

	kfree(chan);

Does fsl_dma_chan_remove() need to call tasklet_kill(&chan->tasklet) before
freeing the channel memory? Even though fsldma_free_irqs() unregisters the
interrupt, if the tasklet is scheduled right before the IRQ is freed, could
the tasklet run after `chan` has been freed and cause a use-after-free?

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

[PATCHv4 08/15] dmaengine: fsldma: convert to platform_get_irq_optional()

[Rosen Penev]

Replace the per-controller irq_of_parse_and_map() call with
platform_get_irq_optional(). The controller IRQ is optional when absent
(-ENXIO) and the driver falls back to per-channel IRQs. Any other error is
treated as fatal. The corresponding irq_dispose_mapping() calls in the
probe error path and remove function are removed.

The per-channel IRQ mapping in fsl_dma_chan_probe() uses a child
device_node rather than the platform device's of_node, so it is not
converted here.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/fsldma.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index c04a7fbd2ed0..eba194d64105 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1213,7 +1213,6 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 	spin_unlock_bh(&chan->desc_lock);
 
 	tasklet_kill(&chan->tasklet);
-	irq_dispose_mapping(chan->irq);
 	list_del(&chan->common.device_node);
 	iounmap(chan->regs);
 	kfree(chan);
@@ -1248,7 +1247,14 @@ static int fsldma_of_probe(struct platform_device *op)
 	}
 
 	/* map the channel IRQ if it exists, but don't hookup the handler yet */
-	fdev->irq = irq_of_parse_and_map(op->dev.of_node, 0);
+	fdev->irq = platform_get_irq_optional(op, 0);
+	if (fdev->irq < 0) {
+		if (fdev->irq != -ENXIO) {
+			err = fdev->irq;
+			goto out_iounmap;
+		}
+		fdev->irq = 0;
+	}
 
 	dma_cap_set(DMA_MEMCPY, fdev->common.cap_mask);
 	dma_cap_set(DMA_SLAVE, fdev->common.cap_mask);
@@ -1317,7 +1323,7 @@ static int fsldma_of_probe(struct platform_device *op)
 		if (fdev->chan[i])
 			fsl_dma_chan_remove(fdev->chan[i]);
 	}
-	irq_dispose_mapping(fdev->irq);
+out_iounmap:
 	iounmap(fdev->regs);
 out_free:
 	kfree(fdev);
@@ -1353,7 +1359,6 @@ static void fsldma_of_remove(struct platform_device *op)
 		if (chans[i])
 			fsl_dma_chan_remove(chans[i]);
 	}
-	irq_dispose_mapping(fdev->irq);
 
 	iounmap(fdev->regs);
 	kfree(fdev);
-- 
2.54.0

[PATCHv4 09/15] dmaengine: fsldma: use devm_kzalloc() to simplify code

[Rosen Penev]

Convert fdev allocation from kzalloc_obj() to devm_kzalloc() to simplify
the probe error and remove paths by dropping the explicit kfree.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index eba194d64105..c3d2b24f8f07 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1222,29 +1222,25 @@ static void fsldma_device_release(struct dma_device *dma_dev);
 
 static int fsldma_of_probe(struct platform_device *op)
 {
+	struct device *dev = &op->dev;
 	struct fsldma_device *fdev;
 	struct device_node *child;
 	unsigned int i;
 	int err;
 
-	fdev = kzalloc_obj(*fdev);
-	if (!fdev) {
-		err = -ENOMEM;
-		goto out_return;
-	}
+	fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
+	if (!fdev)
+		return -ENOMEM;
 
-	fdev->dev = &op->dev;
+	fdev->dev = dev;
 	INIT_LIST_HEAD(&fdev->common.channels);
 	/* The DMA address bits supported for this device. */
 	fdev->addr_bits = (long)device_get_match_data(fdev->dev);
 
 	/* ioremap the registers for use */
 	fdev->regs = of_iomap(op->dev.of_node, 0);
-	if (!fdev->regs) {
-		dev_err(&op->dev, "unable to ioremap registers\n");
-		err = -ENOMEM;
-		goto out_free;
-	}
+	if (!fdev->regs)
+		return dev_err_probe(&op->dev, -ENOMEM, "unable to ioremap registers\n");
 
 	/* map the channel IRQ if it exists, but don't hookup the handler yet */
 	fdev->irq = platform_get_irq_optional(op, 0);
@@ -1325,9 +1321,6 @@ static int fsldma_of_probe(struct platform_device *op)
 	}
 out_iounmap:
 	iounmap(fdev->regs);
-out_free:
-	kfree(fdev);
-out_return:
 	return err;
 }
 
@@ -1361,7 +1354,6 @@ static void fsldma_of_remove(struct platform_device *op)
 	}
 
 	iounmap(fdev->regs);
-	kfree(fdev);
 }
 
 #ifdef CONFIG_PM
-- 
2.54.0

[PATCHv4 10/15] dmaengine: fsldma: use devm_platform_ioremap_resource()

[Rosen Penev]

Convert of_iomap() to devm_platform_ioremap_resource() to let the devm
framework handle unmapping. This allows removing the out_iounmap
label and the explicit iounmap() in both the probe error path and
the remove function.

The DGSR (fdev->regs) and per-channel registers (chan->regs) map
physically distinct regions in all supported variants
(EloPlus/Elo/Elo3), so there is no overlap risk.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index c3d2b24f8f07..e4a3315a7d9d 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1238,17 +1238,15 @@ static int fsldma_of_probe(struct platform_device *op)
 	fdev->addr_bits = (long)device_get_match_data(fdev->dev);
 
 	/* ioremap the registers for use */
-	fdev->regs = of_iomap(op->dev.of_node, 0);
-	if (!fdev->regs)
-		return dev_err_probe(&op->dev, -ENOMEM, "unable to ioremap registers\n");
+	fdev->regs = devm_platform_ioremap_resource(op, 0);
+	if (IS_ERR(fdev->regs))
+		return PTR_ERR(fdev->regs);
 
 	/* map the channel IRQ if it exists, but don't hookup the handler yet */
 	fdev->irq = platform_get_irq_optional(op, 0);
 	if (fdev->irq < 0) {
-		if (fdev->irq != -ENXIO) {
-			err = fdev->irq;
-			goto out_iounmap;
-		}
+		if (fdev->irq != -ENXIO)
+			return fdev->irq;
 		fdev->irq = 0;
 	}
 
@@ -1319,8 +1317,6 @@ static int fsldma_of_probe(struct platform_device *op)
 		if (fdev->chan[i])
 			fsl_dma_chan_remove(fdev->chan[i]);
 	}
-out_iounmap:
-	iounmap(fdev->regs);
 	return err;
 }
 
@@ -1352,8 +1348,6 @@ static void fsldma_of_remove(struct platform_device *op)
 		if (chans[i])
 			fsl_dma_chan_remove(chans[i]);
 	}
-
-	iounmap(fdev->regs);
 }
 
 #ifdef CONFIG_PM
-- 
2.54.0

[PATCHv4 11/15] dmaengine: fsldma: convert channel allocation to devm_kzalloc()

[Rosen Penev]

Convert fsl_dma_chan_probe from kzalloc_obj() to devm_kzalloc(), tying
the channel lifetime to the parent DMA device. Remove kfree(chan) in both
the probe error path and the remove function.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index e4a3315a7d9d..0df09789187d 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1114,11 +1114,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 	int err;
 
 	/* alloc channel */
-	chan = kzalloc_obj(*chan);
-	if (!chan) {
-		err = -ENOMEM;
-		goto out_return;
-	}
+	chan = devm_kzalloc(fdev->dev, sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
 
 	/* ioremap registers for use */
 	chan->regs = of_iomap(node, 0);
@@ -1200,9 +1198,6 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 
 out_iounmap_regs:
 	iounmap(chan->regs);
-out_free_chan:
-	kfree(chan);
-out_return:
 	return err;
 }
 
@@ -1215,7 +1210,6 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 	tasklet_kill(&chan->tasklet);
 	list_del(&chan->common.device_node);
 	iounmap(chan->regs);
-	kfree(chan);
 }
 
 static void fsldma_device_release(struct dma_device *dma_dev);
-- 
2.54.0

[PATCHv4 12/15] dmaengine: fsldma: use devm_of_iomap() to simplify code

[Rosen Penev]

Replace of_iomap() with devm_of_iomap() for per-channel register
mappings. This eliminates the iounmap calls in both the probe
error path and fsl_dma_chan_remove, and simplifies the error
handling by returning directly on failure.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 0df09789187d..dc70a6bf5723 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1119,18 +1119,13 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 		return -ENOMEM;
 
 	/* ioremap registers for use */
-	chan->regs = of_iomap(node, 0);
-	if (!chan->regs) {
-		dev_err(fdev->dev, "unable to ioremap registers\n");
-		err = -ENOMEM;
-		goto out_free_chan;
-	}
+	chan->regs = devm_of_iomap(fdev->dev, node, 0, NULL);
+	if (IS_ERR(chan->regs))
+		return dev_err_probe(fdev->dev, PTR_ERR(chan->regs), "unable to ioremap registers\n");
 
 	err = of_address_to_resource(node, 0, &res);
-	if (err) {
-		dev_err(fdev->dev, "unable to find 'reg' property\n");
-		goto out_iounmap_regs;
-	}
+	if (err)
+		return dev_err_probe(fdev->dev, err, "unable to find 'reg' property\n");
 
 	chan->feature = feature;
 	if (!fdev->feature)
@@ -1146,11 +1141,8 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 	chan->id = (res.start & 0xfff) < 0x300 ?
 		   ((res.start - 0x100) & 0xfff) >> 7 :
 		   ((res.start - 0x200) & 0xfff) >> 7;
-	if (chan->id >= FSL_DMA_MAX_CHANS_PER_DEVICE) {
-		dev_err(fdev->dev, "too many channels for device\n");
-		err = -EINVAL;
-		goto out_iounmap_regs;
-	}
+	if (chan->id >= FSL_DMA_MAX_CHANS_PER_DEVICE)
+		return dev_err_probe(fdev->dev, -EINVAL, "too many channels for device\n");
 
 	fdev->chan[chan->id] = chan;
 	tasklet_setup(&chan->tasklet, dma_do_tasklet);
@@ -1195,10 +1187,6 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 		 chan->irq ? chan->irq : fdev->irq);
 
 	return 0;
-
-out_iounmap_regs:
-	iounmap(chan->regs);
-	return err;
 }
 
 static void fsl_dma_chan_remove(struct fsldma_chan *chan)
@@ -1209,7 +1197,6 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
 
 	tasklet_kill(&chan->tasklet);
 	list_del(&chan->common.device_node);
-	iounmap(chan->regs);
 }
 
 static void fsldma_device_release(struct dma_device *dma_dev);
-- 
2.54.0

[PATCHv4 13/15] dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

[Rosen Penev]

Use of_irq_get() which returns a negative error code on failure
instead of silently returning 0. Split the IRQ validation check
in fsldma_request_irqs to handle three cases:

  - chan->irq < 0: propagate the error (e.g. -EPROBE_DEFER)
  - chan->irq == 0: IRQ not found, return -ENODEV
  - chan->irq > 0: valid IRQ, proceed

The fsldma_free_irqs() function's !chan->irq check is unchanged
since both 0 and negative values mean no IRQ to free.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index dc70a6bf5723..0ee3d719ae95 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1070,6 +1070,12 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
 		if (!chan)
 			continue;
 
+		if (chan->irq < 0) {
+			if (chan->irq != -EPROBE_DEFER)
+				chan_err(chan, "interrupts property missing in device tree\n");
+			ret = chan->irq;
+			goto out_unwind;
+		}
 		if (!chan->irq) {
 			chan_err(chan, "interrupts property missing in device tree\n");
 			ret = -ENODEV;
@@ -1093,7 +1099,7 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
 		if (!chan)
 			continue;
 
-		if (!chan->irq)
+		if (chan->irq <= 0)
 			continue;
 
 		free_irq(chan->irq, chan);
@@ -1178,7 +1184,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
 	dma_cookie_init(&chan->common);
 
 	/* find the IRQ line, if it exists in the device tree */
-	chan->irq = irq_of_parse_and_map(node, 0);
+	chan->irq = of_irq_get(node, 0);
 
 	/* Add the channel to DMA device channel list */
 	list_add_tail(&chan->common.device_node, &fdev->common.channels);
-- 
2.54.0

Re: [PATCHv4 13/15] dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

[sashiko-bot]

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

New issues:
- [Medium] Diagnostic print incorrectly evaluates negative error codes, preventing fallback to the controller IRQ.

Pre-existing issues:
- [High] The driver does not kill the channel tasklet before freeing the channel structure, risking a use-after-free.
- [High] The return value of dma_async_device_register() is ignored.
--

commit b6458c2f1ed9edcf2ad0905771c28bd59e93cd71
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

This commit replaces irq_of_parse_and_map() with of_irq_get() in the fsldma
driver. It also modifies the error handling in fsldma_request_irqs() to
properly propagate negative error codes, such as -EPROBE_DEFER, instead of
just checking for zero.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..b2259f8f69ab9 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c

[ ... ]

> @@ -1185,7 +1191,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  	dma_cookie_init(&chan->common);
>  
>  	/* find the IRQ line, if it exists in the device tree */
> -	chan->irq = irq_of_parse_and_map(node, 0);
> +	chan->irq = of_irq_get(node, 0);
>  
>  	/* Add the channel to DMA device channel list */
>  	list_add_tail(&chan->common.device_node, &fdev->common.channels);
>  
>  	dev_info(fdev->dev, "#%d (%s), irq %d\n", chan->id, compatible,
>  		 chan->irq ? chan->irq : fdev->irq);

[Severity: Medium]
Since chan->irq can now be a negative error code on failure, will the ternary
operator (chan->irq ? ...) evaluate to true? If so, does this mean the driver
will print the negative error code instead of falling back to the controller's
IRQ (fdev->irq) in the kernel log?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
should the driver kill the channel tasklet before freeing the channel
structure?

drivers/dma/fsldma.c:fsl_dma_chan_remove() {
	irq_dispose_mapping(chan->irq);
	list_del(&chan->common.device_node);
	iounmap(chan->regs);
	kfree(chan);
}

The driver schedules chan->tasklet in its interrupt handler, but during
device removal or probe error unwinding, fsl_dma_chan_remove() unmaps
I/O and frees chan without calling tasklet_kill(&chan->tasklet). Could
a tasklet scheduled just before teardown execute after chan has been
freed, causing a use-after-free?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
is it safe to ignore the return value of dma_async_device_register()?

drivers/dma/fsldma.c:fsldma_of_probe() {
    ...
	err = fsldma_request_irqs(fdev);
	if (err) {
		dev_err(fdev->dev, "unable to request IRQs\n");
		goto out_free_fdev;
	}

	dma_async_device_register(&fdev->common);
	return 0;

out_free_fdev:
    ...
}

If registration fails (e.g., due to memory allocation failure or
validation issues), fsldma_of_probe() will still return 0 (success).
This leaves the driver bound to an unregistered DMA device. When the
device is later removed, dma_async_device_unregister() will be called
on it, which might lead to undefined behavior.

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

[PATCHv4 14/15] dmaengine: fsldma: replace ppc-specific accessors with portable generic ones

[Rosen Penev]

- Convert remaining in_be32/in_le32 calls to FSL_DMA_IN macro
- Replace __ilog2 with generic ilog2 (pull in linux/log2.h)
- Add linux/io.h include
- Expand non-PPC accessor support from ARM-only to all architectures
- Guard 64-bit generic accessors with CONFIG_64BIT; provide
  emulation using 32-bit accessors on 32-bit platforms

Add COMPILE_TEST support as a result for extra compile coverage.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/Kconfig  |  2 +-
 drivers/dma/fsldma.c | 11 ++++++-----
 drivers/dma/fsldma.h | 35 ++++++++++++++++++++++++++++++++---
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 302021540d76..9b13e7aa31c7 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -206,7 +206,7 @@ config EP93XX_DMA
 
 config FSL_DMA
 	tristate "Freescale Elo series DMA support"
-	depends on FSL_SOC
+	depends on FSL_SOC || COMPILE_TEST
 	select DMA_ENGINE
 	select ASYNC_TX_ENABLE_CHANNEL_SWITCH
 	help
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 0ee3d719ae95..157db416eaaf 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -32,6 +32,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/log2.h>
 #include <linux/fsldma.h>
 #include "dmaengine.h"
 #include "fsldma.h"
@@ -266,7 +268,7 @@ static void fsl_chan_set_src_loop_size(struct fsldma_chan *chan, int size)
 	case 4:
 	case 8:
 		mode &= ~FSL_DMA_MR_SAHTS_MASK;
-		mode |= FSL_DMA_MR_SAHE | (__ilog2(size) << 14);
+		mode |= FSL_DMA_MR_SAHE | (ilog2(size) << 14);
 		break;
 	}
 
@@ -299,7 +301,7 @@ static void fsl_chan_set_dst_loop_size(struct fsldma_chan *chan, int size)
 	case 4:
 	case 8:
 		mode &= ~FSL_DMA_MR_DAHTS_MASK;
-		mode |= FSL_DMA_MR_DAHE | (__ilog2(size) << 16);
+		mode |= FSL_DMA_MR_DAHE | (ilog2(size) << 16);
 		break;
 	}
 
@@ -326,7 +328,7 @@ static void fsl_chan_set_request_count(struct fsldma_chan *chan, int size)
 
 	mode = get_mr(chan);
 	mode &= ~FSL_DMA_MR_BWC_MASK;
-	mode |= (__ilog2(size) << 24) & FSL_DMA_MR_BWC_MASK;
+	mode |= (ilog2(size) << 24) & FSL_DMA_MR_BWC_MASK;
 
 	set_mr(chan, mode);
 }
@@ -1007,8 +1009,7 @@ static irqreturn_t fsldma_ctrl_irq(int irq, void *data)
 	u32 gsr, mask;
 	int i;
 
-	gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs)
-						   : in_le32(fdev->regs);
+	gsr = FSL_DMA_IN(fdev, fdev->regs, 32);
 	mask = 0xff000000;
 	dev_dbg(fdev->dev, "IRQ: gsr 0x%.8x\n", gsr);
 
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index d7b7a3138b85..01f93123b233 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -232,17 +232,46 @@ static void fsl_iowrite64be(u64 val, u64 __iomem *addr)
 	out_be32((u32 __iomem *)addr + 1, (u32)val);
 }
 #endif
-#endif
-
-#if defined(CONFIG_ARM64) || defined(CONFIG_ARM)
+#else
 #define fsl_ioread32(p)		ioread32(p)
 #define fsl_ioread32be(p)	ioread32be(p)
 #define fsl_iowrite32(v, p)	iowrite32(v, p)
 #define fsl_iowrite32be(v, p)	iowrite32be(v, p)
+
+#ifdef CONFIG_64BIT
 #define fsl_ioread64(p)		ioread64(p)
 #define fsl_ioread64be(p)	ioread64be(p)
 #define fsl_iowrite64(v, p)	iowrite64(v, p)
 #define fsl_iowrite64be(v, p)	iowrite64be(v, p)
+#else
+static inline u64 fsl_ioread64(const u64 __iomem *addr)
+{
+	u32 val_lo = ioread32((u32 __iomem *)addr);
+	u32 val_hi = ioread32((u32 __iomem *)addr + 1);
+
+	return ((u64)val_hi << 32) + val_lo;
+}
+
+static inline void fsl_iowrite64(u64 val, u64 __iomem *addr)
+{
+	iowrite32(val >> 32, (u32 __iomem *)addr + 1);
+	iowrite32((u32)val, (u32 __iomem *)addr);
+}
+
+static inline u64 fsl_ioread64be(const u64 __iomem *addr)
+{
+	u32 val_hi = ioread32be((u32 __iomem *)addr);
+	u32 val_lo = ioread32be((u32 __iomem *)addr + 1);
+
+	return ((u64)val_hi << 32) + val_lo;
+}
+
+static inline void fsl_iowrite64be(u64 val, u64 __iomem *addr)
+{
+	iowrite32be(val >> 32, (u32 __iomem *)addr);
+	iowrite32be((u32)val, (u32 __iomem *)addr + 1);
+}
+#endif
 #endif
 
 #define FSL_DMA_IN(fsl_dma, addr, width)			\
-- 
2.54.0

[PATCHv4 15/15] dmaengine: fsldma: fix kernel-doc param names to match function signatures

[Rosen Penev]

Fix kernel-doc warnings where the documented parameter names
(@chan) no longer match the actual function signatures (@dchan),
and add the missing @cookie and @txstate parameters to
fsl_tx_status.

These are pre-existing mismatches that predate the recent
devm conversion series.

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

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 157db416eaaf..694c1b12bf2b 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -685,7 +685,7 @@ static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
 
 /**
  * fsl_dma_alloc_chan_resources - Allocate resources for DMA channel.
- * @chan : Freescale DMA channel
+ * @dchan : Freescale DMA channel
  *
  * This function will create a dma pool for descriptor allocation.
  *
@@ -742,7 +742,7 @@ static void fsldma_free_desc_list_reverse(struct fsldma_chan *chan,
 
 /**
  * fsl_dma_free_chan_resources - Free all resources of the channel.
- * @chan : Freescale DMA channel
+ * @dchan : Freescale DMA channel
  */
 static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
 {
@@ -878,7 +878,7 @@ static int fsl_dma_device_config(struct dma_chan *dchan,
 
 /**
  * fsl_dma_memcpy_issue_pending - Issue the DMA start command
- * @chan : Freescale DMA channel
+ * @dchan : Freescale DMA channel
  */
 static void fsl_dma_memcpy_issue_pending(struct dma_chan *dchan)
 {
@@ -891,7 +891,9 @@ static void fsl_dma_memcpy_issue_pending(struct dma_chan *dchan)
 
 /**
  * fsl_tx_status - Determine the DMA status
- * @chan : Freescale DMA channel
+ * @dchan : Freescale DMA channel
+ * @cookie : DMA transaction identifier
+ * @txstate : DMA transaction state
  */
 static enum dma_status fsl_tx_status(struct dma_chan *dchan,
 					dma_cookie_t cookie,
-- 
2.54.0