dmaengine: bestcomm: use devm_platform_get_and_ioremap_resource() to simplify code
PATCH 1/1 dmaengine: bestcomm: use devm_platform_get_and_ioremap_resource() to simplify code
Replace the open-coded resource lookup, request_mem_region, ioremap and the manual iounmap/release_mem_region cleanup in probe/remove with the managed devm_platform_get_and_ioremap_resource() helper. This removes the now-unused error-unmap/release paths and simplifies probing.
Fix a resource size mismatch between allocating and freeing where request_mem_region() and release_mem_region() hopefully use the same size but is not guarenteed.
---
v4: fix subject
v3: reword again
v2: remove Sashiko generated description. Reword description.
drivers/dma/bestcomm/bestcomm.c | 45 ++++++++------------------------- 1 file changed, 10 insertions(+), 35 deletions(-)
drivers/dma/bestcomm/bestcomm.c +10 -35
@@ -13,7 +13,6 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/of.h> -#include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/platform_device.h> #include <asm/io.h>
@@ -365,13 +364,19 @@ bcom_engine_cleanup(void)static int mpc52xx_bcom_probe (struct platform_device *op) {struct device_node *ofn_sram; -struct resource res_bcom; +struct resource *res_bcom; +void __iomem *regs;int rv;/* Inform user we're ok so far */ printk (KERN_INFO "DMA: MPC52xx BestComm driver\n" ); +/* Get, reserve & map io */ + regs =devm_platform_get_and_ioremap_resource (op,0 , &res_bcom); +if (IS_ERR (regs)) +return PTR_ERR (regs); +/* Get the bestcomm node */ of_node_get (op->dev.of_node);
@@ -402,35 +407,13 @@ static int mpc52xx_bcom_probe(struct platform_device *op)/* Save the node */ bcom_eng->ofnode = op->dev.of_node; -/* Get, reserve & map io */ -if (of_address_to_resource (op->dev.of_node,0 , &res_bcom)) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't get resource\n"); - rv = -EINVAL ; - goto error_sramclean; - } - -if (!request_mem_region (res_bcom.start,resource_size (&res_bcom), -DRIVER_NAME )) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't request registers region\n"); - rv = -EBUSY ; - goto error_sramclean; - } - - bcom_eng->regs_base = res_bcom.start; - bcom_eng->regs =ioremap (res_bcom.start,sizeof (struct mpc52xx_sdma )); -if (!bcom_eng->regs) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't map registers\n"); - rv = -ENOMEM ; - goto error_release; - } + bcom_eng->regs = regs; + bcom_eng->regs_base = res_bcom->start;/* Now, do the real init */ rv =bcom_engine_init ();if (rv) - goto error_unmap; + goto error_sramclean;/* Done ! */ printk(KERN_INFO "DMA: MPC52xx BestComm engine @%08lx ok !\n",
@@ -439,10 +422,6 @@ static int mpc52xx_bcom_probe(struct platform_device *op)return 0 ;/* Error path */ -error_unmap: -iounmap (bcom_eng->regs); -error_release: -release_mem_region (res_bcom.start,sizeof (struct mpc52xx_sdma )); error_sramclean:kfree (bcom_eng);bcom_sram_cleanup ();
@@ -463,10 +442,6 @@ static void mpc52xx_bcom_remove(struct platform_device *op)/* Cleanup SRAM */ bcom_sram_cleanup (); -/* Release regs */ -iounmap (bcom_eng->regs); -release_mem_region (bcom_eng->regs_base,sizeof (struct mpc52xx_sdma )); -/* Release the node */ of_node_put (bcom_eng->ofnode);
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Dangling Pointer / Use-After-Free in error path --
commit fa3bb7a565a9593e1a87078d2bf750faaf572e6d
Author: Rosen Penev <[email protected]>
dmaengine: bestcomm: use devm_platform_get_and_ioremap_resource() to simplify code
Replace the open-coded resource lookup, request_mem_region, ioremap and the manual iounmap/release_mem_region cleanup in probe/remove with the managed devm_platform_get_and_ioremap_resource() helper. This removes the now-unused error-unmap/release paths and simplifies probing.
Quoted 4 lines
drivers/dma/bestcomm/bestcomm.c +0
-0
[ ... ]
drivers/dma/bestcomm/bestcomm.c static int mpc52xx_bcom_probe(struct platform_device *op) · line 426
Quoted 10 lines
drivers/dma/bestcomm/bestcomm.c +0
-4@@ -439,10 +422,6 @@ static int mpc52xx_bcom_probe(struct platform_device *op)return 0 ;/* Error path */ -error_unmap: -iounmap (bcom_eng->regs); -error_release: -release_mem_region (res_bcom.start,sizeof (struct mpc52xx_sdma )); error_sramclean:kfree (bcom_eng);
[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave a dangling pointer?
When mpc52xx_bcom_probe() fails after allocating memory for the globally visible bcom_eng pointer, it frees the memory but doesn't reset bcom_eng to NULL.
If probe fails (e.g., during bcom_engine_init()), the global bcom_eng will point to freed memory. If dependent drivers load later and call functions like bcom_task_alloc(), wouldn't they dereference this dangling pointer?
Quoted 3 lines
drivers/dma/bestcomm/bestcomm.c +0
-0bcom_sram_cleanup ();
error_ofput:
of_node_put(op->dev.of_node);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 11 lines · Rosen Penev
Replace the open-coded resource lookup,
request_mem_region, ioremap and the manual iounmap/release_mem_region cleanup in probe/remove with the manageddevm_platform_get_and_ioremap_resource()helper. This removes the now-unused error-unmap/release paths and simplifies probing.Fix a resource size mismatch between allocating and freeing where
request_mem_region()andrelease_mem_region()hopefully use the same size but is not guarenteed.Signed-off-by Rosen Penev <[email protected]>---
Missed v4 at subject.
Quoted 103 lines
v4: fix subject
v3: reword again
v2: remove Sashiko generated description. Reword description.drivers/dma/bestcomm/bestcomm.c | 45 ++++++++------------------------- 1 file changed, 10 insertions(+), 35 deletions(-)drivers/dma/bestcomm/bestcomm.c +10
-36@@ -13,7 +13,6 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/of.h> -#include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/platform_device.h> #include <asm/io.h> @@ -365,13 +364,19 @@ bcom_engine_cleanup(void)static int mpc52xx_bcom_probe (struct platform_device *op) {struct device_node *ofn_sram; -struct resource res_bcom; +struct resource *res_bcom; +void __iomem *regs;int rv;/* Inform user we're ok so far */ printk (KERN_INFO "DMA: MPC52xx BestComm driver\n" ); +/* Get, reserve & map io */ + regs =devm_platform_get_and_ioremap_resource (op,0 , &res_bcom); +if (IS_ERR (regs)) +return PTR_ERR (regs); +/* Get the bestcomm node */ of_node_get (op->dev.of_node);@@ -402,35 +407,13 @@ static int mpc52xx_bcom_probe(struct platform_device *op)/* Save the node */ bcom_eng->ofnode = op->dev.of_node; -/* Get, reserve & map io */ -if (of_address_to_resource (op->dev.of_node,0 , &res_bcom)) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't get resource\n"); - rv = -EINVAL ; - goto error_sramclean; - } - -if (!request_mem_region (res_bcom.start,resource_size (&res_bcom), -DRIVER_NAME )) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't request registers region\n"); - rv = -EBUSY ; - goto error_sramclean; - } - - bcom_eng->regs_base = res_bcom.start; - bcom_eng->regs =ioremap (res_bcom.start,sizeof (struct mpc52xx_sdma )); -if (!bcom_eng->regs) { - printk(KERN_ERR DRIVER_NAME ": " - "Can't map registers\n"); - rv = -ENOMEM ; - goto error_release; - } + bcom_eng->regs = regs; + bcom_eng->regs_base = res_bcom->start;/* Now, do the real init */ rv =bcom_engine_init ();if (rv) - goto error_unmap; + goto error_sramclean;/* Done ! */ printk(KERN_INFO "DMA: MPC52xx BestComm engine @%08lx ok !\n",@@ -439,10 +422,6 @@ static int mpc52xx_bcom_probe(struct platform_device *op)return 0 ;/* Error path */ -error_unmap: -iounmap (bcom_eng->regs); -error_release: -release_mem_region (res_bcom.start,sizeof (struct mpc52xx_sdma )); error_sramclean:kfree (bcom_eng);bcom_sram_cleanup ();@@ -463,10 +442,6 @@ static void mpc52xx_bcom_remove(struct platform_device *op)/* Cleanup SRAM */ bcom_sram_cleanup (); -/* Release regs */ -iounmap (bcom_eng->regs); -release_mem_region (bcom_eng->regs_base,sizeof (struct mpc52xx_sdma )); -/* Release the node */ of_node_put (bcom_eng->ofnode); --2.55.0
Vinod Koul
maintainerQuoted 5 lines · Rosen Penev
Replace the open-coded resource lookup,
request_mem_region, ioremap and the manual iounmap/release_mem_region cleanup in probe/remove with the manageddevm_platform_get_and_ioremap_resource()helper. This removes the now-unused error-unmap/release paths and simplifies probing.
Quoted 7 lines
Fix a resource size mismatch between allocating and freeing where
request_mem_region()andrelease_mem_region()hopefully use the same size but is not guarenteed.Signed-off-by Rosen Penev <[email protected]>---
v4: fix subject
Where is v4 in the subject..
Nevertheless this fails for me, can you please rebase
Signature · 1 line
~Vinod
Quoted 10 lines · Rosen Penev
Replace the open-coded resource lookup,
request_mem_region, ioremap and the manual iounmap/release_mem_region cleanup in probe/remove with the manageddevm_platform_get_and_ioremap_resource()helper. This removes the now-unused error-unmap/release paths and simplifies probing.Fix a resource size mismatch between allocating and freeing where
request_mem_region()andrelease_mem_region()hopefully use the same size but is not guarenteed.[...]
Applied, thanks!
[1/1] dmaengine: bestcomm: use devm_platform_get_and_ioremap_resource() to simplify code
commit: 0a8dda0a15d3926422d286567f945a05328a4ac6
Best regards,
Signature · 1 line
~Vinod