dmaengine: ppc4xx: use devm_platform_ioremap_resource()

PATCH Superseded Rosen Penev 3 participants 3 messages Sep 10 – Sep 11 dmaengine

Share & tools

Rosen Penev

author

2026-09-10 21:20 UTC #0 lore raw

PATCH 1/1 dmaengine: ppc4xx: use devm_platform_ioremap_resource()

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 <[email protected]>

---
 drivers/dma/ppc4xx/adma.c | 46 +++++----------------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

drivers/dma/ppc4xx/adma.c +6 ~~-40~~

index bcc54d584e2b..9decdd04a51e 100644
@@ -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);
 }

Signature · 1 line

2.55.0

[email protected]

AI review

2026-09-10 21:29 UTC #1 lore raw

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 <[email protected]>
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.

Quoted 4 lines

drivers/dma/ppc4xx/adma.c +0 -0
index 279a431ccae38..8e6a162396234 100644

[ ... ]

Quoted 4 lines

drivers/dma/ppc4xx/adma.c +0 -0

@@ -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);

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4076

Quoted 10 lines

drivers/dma/ppc4xx/adma.c +4 -4

-	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?

[ ... ]

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4189

Quoted 4 lines

drivers/dma/ppc4xx/adma.c +0 -0

@@ -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?

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4193

Quoted 9 lines

drivers/dma/ppc4xx/adma.c +0 -5

-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?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Frank Li

2026-09-11 15:58 UTC #2 lore raw

In reply to Rosen Penev

Quoted 4 lines · 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

Quoted 1 line

failure or device removal.

cut this.

Frank

Quoted 144 lines

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 <[email protected]>

---
 drivers/dma/ppc4xx/adma.c | 46 +++++----------------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

drivers/dma/ppc4xx/adma.c +6 ~~-41~~

index bcc54d584e2b..9decdd04a51e 100644
@@ -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