[PATCHv6] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[PATCHv6] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[Rosen Penev]

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and
unconditionally increments total_chans. When a channel fails to probe
(for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet
total_chans still advances, so fsl_re_remove_chan() later dereferences the
NULL pointer during device removal.

Check return value and only count successfully probed channels, and guard
fsl_re_remove() against NULL entries.

Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v6: increment ridx before continue;
 v5: put some stuff on one line for easier readability.
 v4: use break;
 v3: fix sashiko review
 v2: fix description
 drivers/dma/fsl_raid.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 9cc289f7129e..f059ff2e37f8 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -836,18 +836,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
-			rc = of_device_is_compatible(child,
-					     "fsl,raideng-v1.0-job-ring");
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
+			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
+			if (!rc) {
+				ridx++;
+				continue;
+			}
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
 			if (rc) {
-				fsl_re_chan_probe(ofdev, child, ridx++, off);
-				re_priv->total_chans++;
+				dev_err(dev, "job ring %d probe failed: %d\n", ridx, rc);
+				ridx++;
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)
@@ -872,7 +884,8 @@ static void fsl_re_remove(struct platform_device *ofdev)
 
 	/* Cleanup chan related memory areas */
 	for (i = 0; i < re_priv->total_chans; i++)
-		fsl_re_remove_chan(re_priv->re_jrs[i]);
+		if (re_priv->re_jrs[i])
+			fsl_re_remove_chan(re_priv->re_jrs[i]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
-- 
2.55.0

Re: [PATCHv6] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: fsl_raid: decoupling total_chans from ridx leaks channels on remove
- [Medium] dmaengine: fsl_raid: unconditionally incrementing ridx for incompatible nodes exhausts slots
--

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

dmaengine: fsl_raid: check fsl_re_chan_probe() return value
    
This commit modifies fsl_re_probe to check the return value of
fsl_re_chan_probe and only increment total_chans on success. It also adds
NULL checks during channel removal in fsl_re_remove to prevent dereferencing
uninitialized slots.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..83856d554b12b 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -828,18 +828,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		}
>  		/* Find out the Job Rings present under each JQ */
>  		for_each_child_of_node(np, child) {
> -			rc = of_device_is_compatible(child,
> -					     "fsl,raideng-v1.0-job-ring");
> +			if (ridx >= FSL_RE_MAX_CHANS) {
> +				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
> +				of_node_put(child);
> +				break;
> +			}
> +
> +			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
> +			if (!rc) {
> +				ridx++;
> +				continue;
> +			}

[Severity: Medium]
When iterating child nodes, if a node is not compatible, does this code
incorrectly increment ridx?

This could waste limited job ring capacity (FSL_RE_MAX_CHANS) by advancing the
internal logical index for child nodes that are not job rings.

> +
> +			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
>  			if (rc) {
> -				fsl_re_chan_probe(ofdev, child, ridx++, off);
> -				re_priv->total_chans++;
> +				dev_err(dev, "job ring %d probe failed: %d\n", ridx, rc);
> +				ridx++;
> +				continue;
>  			}
> +			ridx++;
> +			re_priv->total_chans++;

[Severity: High]
If any child node fails to probe or is skipped as incompatible, ridx is
incremented but total_chans is not. Does this create a persistent mismatch
between the array index and the channel count?

A subsequent successful probe would store the channel in re_priv->re_jrs at
an index greater than or equal to total_chans.

[ ... ]
> @@ -864,7 +876,8 @@ static void fsl_re_remove(struct platform_device *ofdev)
>  
>  	/* Cleanup chan related memory areas */
>  	for (i = 0; i < re_priv->total_chans; i++)
> -		fsl_re_remove_chan(re_priv->re_jrs[i]);
> +		if (re_priv->re_jrs[i])
> +			fsl_re_remove_chan(re_priv->re_jrs[i]);

[Severity: High]
Does this cleanup loop miss successfully populated channels stored at indices
greater than or equal to total_chans?

Since total_chans is used as the exclusive upper bound, successfully probed
channels placed at higher indices (due to previous skipped or failed nodes)
could be bypassed and leaked during device removal.

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

Re: [PATCHv6] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[Frank Li]

On Wed, Sep 16, 2026 at 04:02:28PM -0700, Rosen Penev wrote:
> fsl_re_probe() ignores the return value of fsl_re_chan_probe() and
> unconditionally increments total_chans. When a channel fails to probe
> (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet
> total_chans still advances, so fsl_re_remove_chan() later dereferences the
> NULL pointer during device removal.
>
> Check return value and only count successfully probed channels, and guard
> fsl_re_remove() against NULL entries.
>
> Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
> Assisted-by: LLM
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v6: increment ridx before continue;
>  v5: put some stuff on one line for easier readability.
>  v4: use break;
>  v3: fix sashiko review
>  v2: fix description
>  drivers/dma/fsl_raid.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 9cc289f7129e..f059ff2e37f8 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -836,18 +836,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		}
>  		/* Find out the Job Rings present under each JQ */
>  		for_each_child_of_node(np, child) {

look like you need rebase, latest code

use for_each_compatible_node_scoped()

Frank
> -			rc = of_device_is_compatible(child,
> -					     "fsl,raideng-v1.0-job-ring");
> +			if (ridx >= FSL_RE_MAX_CHANS) {
> +				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
> +				of_node_put(child);
> +				break;
> +			}
> +
> +			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
> +			if (!rc) {
> +				ridx++;
> +				continue;
> +			}
> +
> +			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
>  			if (rc) {
> -				fsl_re_chan_probe(ofdev, child, ridx++, off);
> -				re_priv->total_chans++;
> +				dev_err(dev, "job ring %d probe failed: %d\n", ridx, rc);
> +				ridx++;
> +				continue;
>  			}
> +			ridx++;
> +			re_priv->total_chans++;


>  		}
>  	}
>
> -	dma_async_device_register(dma_dev);
> -
> -	return 0;
> +	return dma_async_device_register(dma_dev);
>  }
>
>  static void fsl_re_remove_chan(struct fsl_re_chan *chan)
> @@ -872,7 +884,8 @@ static void fsl_re_remove(struct platform_device *ofdev)
>
>  	/* Cleanup chan related memory areas */
>  	for (i = 0; i < re_priv->total_chans; i++)
> -		fsl_re_remove_chan(re_priv->re_jrs[i]);
> +		if (re_priv->re_jrs[i])
> +			fsl_re_remove_chan(re_priv->re_jrs[i]);
>
>  	/* Unregister the driver */
>  	dma_async_device_unregister(&re_priv->dma_dev);
> --
> 2.55.0
>