[PATCH] dmaengine: ppc4xx: use devm_platform_ioremap_resource()

[PATCH] dmaengine: ppc4xx: use devm_platform_ioremap_resource()

[Rosen Penev]

Replace the open-coded sequence of of_address_to_resource(),
request_mem_region(), and ioremap() with devm_platform_ioremap_resource().
This eliminates error-path cleanup for both the memory region and the
ioremap, and lets the devm framework handle automatic release on probe
failure or device removal.

The two separate initcodes PPC_ADMA_INIT_MEMRES and PPC_ADMA_INIT_MEMREG
are collapsed into PPC_ADMA_INIT_MEMRES since the combined call covers
both steps.

Also emove unused PPC_ADMA_INIT_MEMREG enum value

The PPC_ADMA_INIT_MEMREG error code is no longer used after converting
to devm_platform_ioremap_resource().  Remove it from the enum and the
corresponding error string.

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

diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index bcc54d584e2b..9decdd04a51e 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -37,7 +37,6 @@
 enum ppc_adma_init_code {
 	PPC_ADMA_INIT_OK = 0,
 	PPC_ADMA_INIT_MEMRES,
-	PPC_ADMA_INIT_MEMREG,
 	PPC_ADMA_INIT_ALLOC,
 	PPC_ADMA_INIT_COHERENT,
 	PPC_ADMA_INIT_CHANNEL,
@@ -49,7 +48,6 @@ enum ppc_adma_init_code {
 static char *ppc_adma_errors[] = {
 	[PPC_ADMA_INIT_OK] = "ok",
 	[PPC_ADMA_INIT_MEMRES] = "failed to get memory resource",
-	[PPC_ADMA_INIT_MEMREG] = "failed to request memory region",
 	[PPC_ADMA_INIT_ALLOC] = "failed to allocate memory for adev "
 				"structure",
 	[PPC_ADMA_INIT_COHERENT] = "failed to allocate coherent memory for "
@@ -4003,7 +4001,6 @@ static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev,
 static int ppc440spe_adma_probe(struct platform_device *ofdev)
 {
 	struct device_node *np = ofdev->dev.of_node;
-	struct resource res;
 	struct ppc440spe_adma_device *adev;
 	struct ppc440spe_adma_chan *chan;
 	struct ppc_dma_chan_ref *ref, *_ref;
@@ -4046,28 +4043,12 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
 		pool_size <<= 2;
 	}
 
-	if (of_address_to_resource(np, 0, &res)) {
-		dev_err(&ofdev->dev, "failed to get memory resource\n");
-		initcode = PPC_ADMA_INIT_MEMRES;
-		ret = -ENODEV;
-		goto out;
-	}
-
-	if (!request_mem_region(res.start, resource_size(&res),
-				dev_driver_string(&ofdev->dev))) {
-		dev_err(&ofdev->dev, "failed to request memory region %pR\n",
-			&res);
-		initcode = PPC_ADMA_INIT_MEMREG;
-		ret = -EBUSY;
-		goto out;
-	}
-
 	/* create a device */
 	adev = kzalloc_obj(*adev);
 	if (!adev) {
 		initcode = PPC_ADMA_INIT_ALLOC;
 		ret = -ENOMEM;
-		goto err_adev_alloc;
+		goto out;
 	}
 
 	adev->id = id;
@@ -4087,10 +4068,10 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
 	dev_dbg(&ofdev->dev, "allocated descriptor pool virt 0x%p phys 0x%llx\n",
 		adev->dma_desc_pool_virt, (u64)adev->dma_desc_pool);
 
-	regs = ioremap(res.start, resource_size(&res));
-	if (!regs) {
-		dev_err(&ofdev->dev, "failed to ioremap regs!\n");
-		ret = -ENOMEM;
+	regs = devm_platform_ioremap_resource(ofdev, 0);
+	if (IS_ERR(regs)) {
+		ret = PTR_ERR(regs);
+		initcode = PPC_ADMA_INIT_MEMRES;
 		goto err_regs_alloc;
 	}
 
@@ -4127,7 +4108,7 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
 	if (!chan) {
 		initcode = PPC_ADMA_INIT_CHANNEL;
 		ret = -ENOMEM;
-		goto err_chan_alloc;
+		goto err_regs_alloc;
 	}
 
 	spin_lock_init(&chan->lock);
@@ -4206,19 +4187,12 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
 	}
 err_page_alloc:
 	kfree(chan);
-err_chan_alloc:
-	if (adev->id == PPC440SPE_XOR_ID)
-		iounmap(adev->xor_reg);
-	else
-		iounmap(adev->dma_reg);
 err_regs_alloc:
 	dma_free_coherent(adev->dev, adev->pool_size,
 			  adev->dma_desc_pool_virt,
 			  adev->dma_desc_pool);
 err_dma_alloc:
 	kfree(adev);
-err_adev_alloc:
-	release_mem_region(res.start, resource_size(&res));
 out:
 	if (id < PPC440SPE_ADMA_ENGINES_NUM)
 		ppc440spe_adma_devices[id] = initcode;
@@ -4232,8 +4206,6 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
 static void ppc440spe_adma_remove(struct platform_device *ofdev)
 {
 	struct ppc440spe_adma_device *adev = platform_get_drvdata(ofdev);
-	struct device_node *np = ofdev->dev.of_node;
-	struct resource res;
 	struct dma_chan *chan, *_chan;
 	struct ppc_dma_chan_ref *ref, *_ref;
 	struct ppc440spe_adma_chan *ppc440spe_chan;
@@ -4270,12 +4242,6 @@ static void ppc440spe_adma_remove(struct platform_device *ofdev)
 
 	dma_free_coherent(adev->dev, adev->pool_size,
 			  adev->dma_desc_pool_virt, adev->dma_desc_pool);
-	if (adev->id == PPC440SPE_XOR_ID)
-		iounmap(adev->xor_reg);
-	else
-		iounmap(adev->dma_reg);
-	of_address_to_resource(np, 0, &res);
-	release_mem_region(res.start, resource_size(&res));
 	kfree(adev);
 }
 
-- 
2.55.0

Re: [PATCH] dmaengine: ppc4xx: use devm_platform_ioremap_resource()

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling `dma_free_coherent()` with a NULL `adev->dev` pointer in the error path if `devm_platform_ioremap_resource()` fails.
- [High] Missing tasklet_kill on the probe error path leads to Use-After-Free of the chan structure.
- [High] Missing free_irq() for the error IRQ if subsequent allocations fail during setup.
- [High] Missing check for dma_map_page() failure.
--

commit ed3114dad8f9fbd2d3d5f92d786754a5f161c4f0
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: ppc4xx: use devm_platform_ioremap_resource()

This patch replaces the open-coded sequence of of_address_to_resource(),
request_mem_region(), and ioremap() with devm_platform_ioremap_resource().
It also removes unused error codes and simplifies error paths since the
devm framework handles automatic release.

> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index 279a431ccae38..8e6a162396234 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
[ ... ]
> @@ -4087,10 +4068,10 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	dev_dbg(&ofdev->dev, "allocated descriptor pool virt 0x%p phys 0x%llx\n",
>  		adev->dma_desc_pool_virt, (u64)adev->dma_desc_pool);
>  
> -	regs = ioremap(res.start, resource_size(&res));
> -	if (!regs) {
> -		dev_err(&ofdev->dev, "failed to ioremap regs!\n");
> -		ret = -ENOMEM;
> +	regs = devm_platform_ioremap_resource(ofdev, 0);
> +	if (IS_ERR(regs)) {
> +		ret = PTR_ERR(regs);
> +		initcode = PPC_ADMA_INIT_MEMRES;
>  		goto err_regs_alloc;
>  	}

[Severity: High]
This is a pre-existing issue, but does jumping to err_regs_alloc here cause a
NULL pointer dereference?

adev is allocated earlier with kzalloc_obj(), which zero-initializes the memory
and leaves adev->dev as NULL. adev->dev is only assigned after the register
mapping succeeds.

If devm_platform_ioremap_resource() fails, execution jumps to the
err_regs_alloc label:

err_regs_alloc:
	dma_free_coherent(adev->dev, adev->pool_size,
			  adev->dma_desc_pool_virt,
			  adev->dma_desc_pool);

Could passing this NULL adev->dev pointer to dma_free_coherent() cause a kernel
panic during device probe failure?

[ ... ]
> @@ -4206,19 +4187,12 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	}
>  err_page_alloc:
>  	kfree(chan);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a tasklet
to run after the chan structure is freed here?

During probe, the driver initializes a tasklet and successfully registers the
hardware IRQ handler via ppc440spe_adma_setup_irqs(). From this point, an
interrupt can fire and schedule the tasklet asynchronously.

If the subsequent dma_async_device_register() fails, the code jumps to the
err_dev_reg error path, which calls ppc440spe_adma_release_irqs(). This frees
the IRQ, but it doesn't appear to call tasklet_kill(&chan->irq_tasklet) to
synchronize with or cancel any pending or running tasklets.

When execution falls through to err_page_alloc, chan is freed via kfree(chan).
Will a running or pending tasklet then access the freed chan memory, leading to
a use-after-free?

> -err_chan_alloc:
> -	if (adev->id == PPC440SPE_XOR_ID)
> -		iounmap(adev->xor_reg);
> -	else
> -		iounmap(adev->dma_reg);
>  err_regs_alloc:
>  	dma_free_coherent(adev->dev, adev->pool_size,
>  			  adev->dma_desc_pool_virt,
>  			  adev->dma_desc_pool);
[ ... ]

In addition to the changes in the diff, a few pre-existing issues were noticed
in this file during review:

[Severity: High]
This is a pre-existing issue, but should the return value of dma_map_page() be
checked for errors?

In ppc440spe_adma_probe():

	chan->pdest = dma_map_page(&ofdev->dev, chan->pdest_page, 0,
				   PAGE_SIZE, DMA_BIDIRECTIONAL);
	chan->qdest = dma_map_page(&ofdev->dev, chan->qdest_page, 0,
				   PAGE_SIZE, DMA_BIDIRECTIONAL);

If the system is under heavy memory or IOMMU pressure and dma_map_page() fails,
it will return an invalid DMA address. Since this address is never verified
with dma_mapping_error(), could the hardware be programmed with an invalid
address, resulting in memory corruption or IOMMU faults?

[Severity: High]
This is a pre-existing issue, but are we leaking the error IRQ in the
ppc440spe_adma_setup_irqs() error path?

If adev->err_irq > 0, the error IRQ is successfully requested using
request_irq(). Later in the function, if of_find_compatible_node() or
of_iomap() fails, the code jumps to err_req2:

err_req2:
	free_irq(adev->irq, chan);
err_req1:
	irq_dispose_mapping(adev->irq);
err_irq_map:
	if (adev->err_irq > 0) {

The err_req2 block calls free_irq(adev->irq, chan) for the primary IRQ, but
it appears to miss calling free_irq(adev->err_irq, chan) for the error IRQ.

If the probe function subsequently fails and frees the chan structure, could a
firing error interrupt execute the handler with the freed chan pointer, leading
to a use-after-free?

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

Re: [PATCH] dmaengine: ppc4xx: use devm_platform_ioremap_resource()

[Frank Li]

On Thu, Sep 10, 2026 at 02:20:18PM -0700, Rosen Penev wrote:
> Replace the open-coded sequence of of_address_to_resource(),
> request_mem_region(), and ioremap() with devm_platform_ioremap_resource().
> This eliminates error-path cleanup for both the memory region and the
> ioremap

, and lets the devm framework handle automatic release on probe
> failure or device removal.

cut this.

Frank
>
> The two separate initcodes PPC_ADMA_INIT_MEMRES and PPC_ADMA_INIT_MEMREG
> are collapsed into PPC_ADMA_INIT_MEMRES since the combined call covers
> both steps.
>
> Also emove unused PPC_ADMA_INIT_MEMREG enum value
>
> The PPC_ADMA_INIT_MEMREG error code is no longer used after converting
> to devm_platform_ioremap_resource().  Remove it from the enum and the
> corresponding error string.
>
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/dma/ppc4xx/adma.c | 46 +++++----------------------------------
>  1 file changed, 6 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index bcc54d584e2b..9decdd04a51e 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
> @@ -37,7 +37,6 @@
>  enum ppc_adma_init_code {
>  	PPC_ADMA_INIT_OK = 0,
>  	PPC_ADMA_INIT_MEMRES,
> -	PPC_ADMA_INIT_MEMREG,
>  	PPC_ADMA_INIT_ALLOC,
>  	PPC_ADMA_INIT_COHERENT,
>  	PPC_ADMA_INIT_CHANNEL,
> @@ -49,7 +48,6 @@ enum ppc_adma_init_code {
>  static char *ppc_adma_errors[] = {
>  	[PPC_ADMA_INIT_OK] = "ok",
>  	[PPC_ADMA_INIT_MEMRES] = "failed to get memory resource",
> -	[PPC_ADMA_INIT_MEMREG] = "failed to request memory region",
>  	[PPC_ADMA_INIT_ALLOC] = "failed to allocate memory for adev "
>  				"structure",
>  	[PPC_ADMA_INIT_COHERENT] = "failed to allocate coherent memory for "
> @@ -4003,7 +4001,6 @@ static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev,
>  static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  {
>  	struct device_node *np = ofdev->dev.of_node;
> -	struct resource res;
>  	struct ppc440spe_adma_device *adev;
>  	struct ppc440spe_adma_chan *chan;
>  	struct ppc_dma_chan_ref *ref, *_ref;
> @@ -4046,28 +4043,12 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  		pool_size <<= 2;
>  	}
>
> -	if (of_address_to_resource(np, 0, &res)) {
> -		dev_err(&ofdev->dev, "failed to get memory resource\n");
> -		initcode = PPC_ADMA_INIT_MEMRES;
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -
> -	if (!request_mem_region(res.start, resource_size(&res),
> -				dev_driver_string(&ofdev->dev))) {
> -		dev_err(&ofdev->dev, "failed to request memory region %pR\n",
> -			&res);
> -		initcode = PPC_ADMA_INIT_MEMREG;
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
>  	/* create a device */
>  	adev = kzalloc_obj(*adev);
>  	if (!adev) {
>  		initcode = PPC_ADMA_INIT_ALLOC;
>  		ret = -ENOMEM;
> -		goto err_adev_alloc;
> +		goto out;
>  	}
>
>  	adev->id = id;
> @@ -4087,10 +4068,10 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	dev_dbg(&ofdev->dev, "allocated descriptor pool virt 0x%p phys 0x%llx\n",
>  		adev->dma_desc_pool_virt, (u64)adev->dma_desc_pool);
>
> -	regs = ioremap(res.start, resource_size(&res));
> -	if (!regs) {
> -		dev_err(&ofdev->dev, "failed to ioremap regs!\n");
> -		ret = -ENOMEM;
> +	regs = devm_platform_ioremap_resource(ofdev, 0);
> +	if (IS_ERR(regs)) {
> +		ret = PTR_ERR(regs);
> +		initcode = PPC_ADMA_INIT_MEMRES;
>  		goto err_regs_alloc;
>  	}
>
> @@ -4127,7 +4108,7 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	if (!chan) {
>  		initcode = PPC_ADMA_INIT_CHANNEL;
>  		ret = -ENOMEM;
> -		goto err_chan_alloc;
> +		goto err_regs_alloc;
>  	}
>
>  	spin_lock_init(&chan->lock);
> @@ -4206,19 +4187,12 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	}
>  err_page_alloc:
>  	kfree(chan);
> -err_chan_alloc:
> -	if (adev->id == PPC440SPE_XOR_ID)
> -		iounmap(adev->xor_reg);
> -	else
> -		iounmap(adev->dma_reg);
>  err_regs_alloc:
>  	dma_free_coherent(adev->dev, adev->pool_size,
>  			  adev->dma_desc_pool_virt,
>  			  adev->dma_desc_pool);
>  err_dma_alloc:
>  	kfree(adev);
> -err_adev_alloc:
> -	release_mem_region(res.start, resource_size(&res));
>  out:
>  	if (id < PPC440SPE_ADMA_ENGINES_NUM)
>  		ppc440spe_adma_devices[id] = initcode;
> @@ -4232,8 +4206,6 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  static void ppc440spe_adma_remove(struct platform_device *ofdev)
>  {
>  	struct ppc440spe_adma_device *adev = platform_get_drvdata(ofdev);
> -	struct device_node *np = ofdev->dev.of_node;
> -	struct resource res;
>  	struct dma_chan *chan, *_chan;
>  	struct ppc_dma_chan_ref *ref, *_ref;
>  	struct ppc440spe_adma_chan *ppc440spe_chan;
> @@ -4270,12 +4242,6 @@ static void ppc440spe_adma_remove(struct platform_device *ofdev)
>
>  	dma_free_coherent(adev->dev, adev->pool_size,
>  			  adev->dma_desc_pool_virt, adev->dma_desc_pool);
> -	if (adev->id == PPC440SPE_XOR_ID)
> -		iounmap(adev->xor_reg);
> -	else
> -		iounmap(adev->dma_reg);
> -	of_address_to_resource(np, 0, &res);
> -	release_mem_region(res.start, resource_size(&res));
>  	kfree(adev);
>  }
>
> --
> 2.55.0
>