dmaengine: fsl_raid: check fsl_re_chan_probe() return value
PATCH v8 1/1 dmaengine: fsl_raid: check fsl_re_chan_probe() return value
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.
---
v8: handle channel unwinding in fsl_re_probe
v7: handle differently based on sashiko review
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 | 67 ++++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 19 deletions(-)
drivers/dma/fsl_raid.c +48 -19
@@ -687,7 +687,6 @@ static int fsl_re_chan_probe(struct platform_device *ofdev, goto err_free; } - re_priv->re_jrs[q] = chan; chan->chan.device = dma_dev; chan->chan.private = chan; chan->dev = chandev;
@@ -741,6 +740,11 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,/* Enable RE/CHAN */ out_be32 (&chan->jrregs->jr_command,FSL_RE_ENABLE ); +/* Only record the channel once it is fully initialized, so the +* cleanup paths never touch a partially-probed ring. +*/ + re_priv->re_jrs[q] = chan; +return 0 ; err_free_1:
@@ -750,6 +754,17 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,return ret; } +static void fsl_re_remove_chan (struct fsl_re_chan *chan) +{ +tasklet_kill (&chan->irqtask); + +dma_pool_free (chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr, + chan->inb_phys_addr); + +dma_pool_free (chan->re_dev->hw_desc_pool, chan->oub_ring_virt_addr, + chan->oub_phys_addr); +} +/* Probe function for RAID Engine */ static int fsl_re_probe (struct platform_device *ofdev) {
@@ -759,7 +774,7 @@ static int fsl_re_probe(struct platform_device *ofdev)u32 off;u8 ridx =0 ;struct dma_device *dma_dev; -int rc; +int rc, i;struct device *dev = &ofdev->dev;/* IOMAP the entire RAID Engine region */
@@ -832,33 +847,46 @@ static int fsl_re_probe(struct platform_device *ofdev) rc =of_property_read_u32 (np,"reg" , &off);if (rc) {dev_err (dev,"Reg property not found in JQ node\n" ); -return -ENODEV ; + rc = -ENODEV ; + goto err_unwind; }/* 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 (rc) { -fsl_re_chan_probe (ofdev, child, ridx++, off); - re_priv->total_chans++; +if (!of_device_is_compatible (child,"fsl,raideng-v1.0-job-ring" )) +continue ; + +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 =fsl_re_chan_probe (ofdev, child, ridx, off); +if (rc) +dev_err (dev,"job ring %d probe failed: %d\n" , ridx, rc); + re_priv->total_chans++; + ridx++; } } -dma_async_device_register (dma_dev); + rc =dma_async_device_register (dma_dev); +if (rc) + goto err_unwind;return 0 ; -} - -static void fsl_re_remove_chan (struct fsl_re_chan *chan) -{ -tasklet_kill (&chan->irqtask); -dma_pool_free (chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr, - chan->inb_phys_addr); +err_unwind: +/* +* Tear down any channels that were successfully probed before the +* error. fsl_re_chan_probe() already frees its own resources on +* failure, so only fully-initialized channels in re_jrs[] need +* cleanup here; otherwise their IRQs and DMA ring memory leak. +*/ +for (i =0 ; i < re_priv->total_chans; i++) +if (re_priv->re_jrs[i]) +fsl_re_remove_chan (re_priv->re_jrs[i]); -dma_pool_free (chan->re_dev->hw_desc_pool, chan->oub_ring_virt_addr, - chan->oub_phys_addr); +return rc; }static void fsl_re_remove (struct platform_device *ofdev)
@@ -872,7 +900,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);
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] dmaengine: fsl_raid: unconditional total_chans increment contradicts commit --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: fsl_raid: unconditional total_chans increment contradicts commit --
commit 208ae247c55a1b6bf4d94121e4104ab723b71bae
Author: Rosen Penev <[email protected]>
dmaengine: fsl_raid: check fsl_re_chan_probe() return value
The commit adds a check for fsl_re_chan_probe() failure in fsl_re_probe() to prevent leaving uninitialized array elements in re_jrs[]. It also adds a cleanup path to correctly tear down channels if an error occurs.
Quoted 4 lines
drivers/dma/fsl_raid.c +0
-0
[ ... ]
Quoted 1 line
drivers/dma/fsl_raid.c +0
-0@@ -824,33 +839,46 @@ static int fsl_re_probe(struct platform_device *ofdev)
[ ... ]
Quoted 12 lines
drivers/dma/fsl_raid.c +5
-5/* 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 (rc) { -fsl_re_chan_probe (ofdev, child, ridx++, off); - re_priv->total_chans++; +if (!of_device_is_compatible (child,"fsl,raideng-v1.0-job-ring" )) +continue ; + +if (ridx >=FSL_RE_MAX_CHANS ) { +dev_warn (dev,"too many job rings, max %d\n" ,FSL_RE_MAX_CHANS );
drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 869
Quoted 10 lines
drivers/dma/fsl_raid.c +8
-0+of_node_put (child); +break ; } + + rc =fsl_re_chan_probe (ofdev, child, ridx, off); +if (rc) +dev_err (dev,"job ring %d probe failed: %d\n" , ridx, rc); + re_priv->total_chans++; + ridx++; }
[Severity: Medium]
The commit message states "only count successfully probed channels", but does this code still unconditionally increment the counters?
If fsl_re_chan_probe() fails and returns a non-zero rc, it logs an error but then falls through to increment re_priv->total_chans and ridx since there is no continue statement. This advances the counters even for failed probes, wasting slots in the fixed-size re_jrs array. Should this include a continue when rc is non-zero?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1