[PATCHv2] dmaengine: ppc4xx: convert irq_of_parse_and_map to platform_get_irq

[PATCHv2] dmaengine: ppc4xx: convert irq_of_parse_and_map to platform_get_irq

[Rosen Penev]

Replace irq_of_parse_and_map() with platform_get_irq(), which is the
preferred way to obtain IRQ resources from platform devices.  This
eliminates the corresponding irq_dispose_mapping() calls since the
framework manages the mapping.

While here, fix a latent bug in the err_req2 error path: the error IRQ
was not freed when a subsequent step (I2O setup) failed.

The struct device_node *np declaration is moved to the scope where
it is still needed (I2O register lookup).

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: use _optional
 drivers/dma/ppc4xx/adma.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index 279a431ccae3..99aee726644e 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -3865,28 +3865,24 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				     int *initcode)
 {
 	struct platform_device *ofdev;
-	struct device_node *np;
 	int ret;
 
 	ofdev = container_of(adev->dev, struct platform_device, dev);
-	np = ofdev->dev.of_node;
 	if (adev->id != PPC440SPE_XOR_ID) {
-		adev->err_irq = irq_of_parse_and_map(np, 1);
-		if (!adev->err_irq) {
+		adev->err_irq = platform_get_irq_optional(ofdev, 1);
+		if (adev->err_irq < 0) {
 			dev_warn(adev->dev, "no err irq resource?\n");
 			*initcode = PPC_ADMA_INIT_IRQ2;
-			adev->err_irq = -ENXIO;
 		} else
 			atomic_inc(&ppc440spe_adma_err_irq_ref);
 	} else {
 		adev->err_irq = -ENXIO;
 	}
 
-	adev->irq = irq_of_parse_and_map(np, 0);
-	if (!adev->irq) {
-		dev_err(adev->dev, "no irq resource\n");
+	adev->irq = platform_get_irq(ofdev, 0);
+	if (adev->irq < 0) {
 		*initcode = PPC_ADMA_INIT_IRQ1;
-		ret = -ENXIO;
+		ret = adev->irq;
 		goto err_irq_map;
 	}
 	dev_dbg(adev->dev, "irq %d, err irq %d\n",
@@ -3899,7 +3895,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 			adev->irq);
 		*initcode = PPC_ADMA_INIT_IRQ1;
 		ret = -EIO;
-		goto err_req1;
+		goto err_irq_map;
 	}
 
 	/* only DMA engines have a separate error IRQ
@@ -3917,7 +3913,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				adev->err_irq);
 			*initcode = PPC_ADMA_INIT_IRQ2;
 			ret = -EIO;
-			goto err_req2;
+			goto err_req1;
 		}
 	}
 
@@ -3927,6 +3923,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 			    XOR_IE_ICIE_BIT | XOR_IE_RPTIE_BIT,
 			    &adev->xor_reg->ier);
 	} else {
+		struct device_node *np;
 		u32 mask, enable;
 
 		np = of_find_compatible_node(NULL, NULL, "ibm,i2o-440spe");
@@ -3956,14 +3953,13 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 	return 0;
 
 err_req2:
-	free_irq(adev->irq, chan);
+	if (adev->err_irq > 0)
+		free_irq(adev->err_irq, chan);
 err_req1:
-	irq_dispose_mapping(adev->irq);
+	free_irq(adev->irq, chan);
 err_irq_map:
-	if (adev->err_irq > 0) {
-		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref))
-			irq_dispose_mapping(adev->err_irq);
-	}
+	if (adev->err_irq > 0)
+		atomic_dec(&ppc440spe_adma_err_irq_ref);
 	return ret;
 }
 
@@ -3987,13 +3983,10 @@ static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev,
 		iowrite32(mask, &adev->i2o_reg->iopim);
 	}
 	free_irq(adev->irq, chan);
-	irq_dispose_mapping(adev->irq);
 	if (adev->err_irq > 0) {
 		free_irq(adev->err_irq, chan);
-		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref)) {
-			irq_dispose_mapping(adev->err_irq);
+		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref))
 			iounmap(adev->i2o_reg);
-		}
 	}
 }
 
-- 
2.55.0

Re: [PATCHv2] dmaengine: ppc4xx: convert irq_of_parse_and_map to platform_get_irq

[sashiko-bot]

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

New issues:
- [Medium] Failure to properly handle -EPROBE_DEFER from platform_get_irq_optional(), breaking probe deferral for the error IRQ.
- [Medium] Inconsistent validation of `adev->err_irq` leads to a leaked atomic reference count if IRQ 0 is returned.

Pre-existing issues:
- [High] Pre-existing memory leak of `adev->i2o_reg` virtual mappings for the DMA engines on driver unbind.
- [High] Unprotected read-modify-write on the shared I2O IOPIM hardware register.
--

commit 50a4484470a83649ae6d7a02d361e752aed3ddeb
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: ppc4xx: convert irq_of_parse_and_map to platform_get_irq

This patch replaces irq_of_parse_and_map() with platform_get_irq(), which
is the preferred way to obtain IRQ resources from platform devices, allowing
the framework to manage the mapping. It also fixes a latent bug in the error
path where the error IRQ was not freed if I2O setup failed. The device_node
np declaration is moved to a tighter scope where it is still needed.

> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index 279a431ccae38..99aee726644e0 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
> @@ -3865,28 +3865,24 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
>  				     int *initcode)
>  {
>  	struct platform_device *ofdev;
> -	struct device_node *np;
>  	int ret;
>  
>  	ofdev = container_of(adev->dev, struct platform_device, dev);
> -	np = ofdev->dev.of_node;
>  	if (adev->id != PPC440SPE_XOR_ID) {
> -		adev->err_irq = irq_of_parse_and_map(np, 1);
> -		if (!adev->err_irq) {
> +		adev->err_irq = platform_get_irq_optional(ofdev, 1);
> +		if (adev->err_irq < 0) {

[Severity: Medium]
Will this silently swallow the -EPROBE_DEFER error code?

If the interrupt controller is not yet ready, platform_get_irq_optional()
will return -EPROBE_DEFER. Treating all negative codes here as missing
resources prevents the driver from correctly deferring its probe.

[Severity: Medium]
Does this inconsistent validation of adev->err_irq cause a leaked atomic
reference count if IRQ 0 is returned?

If platform_get_irq_optional() returns 0, this setup path treats it as a
valid IRQ and evaluates to false, executing the else branch and incrementing
the atomic refcount.

However, as seen further down in the diff during release, the teardown path
uses if (adev->err_irq > 0) which would evaluate to false, skipping the
atomic_dec entirely and leaking the reference count.

>  			dev_warn(adev->dev, "no err irq resource?\n");
>  			*initcode = PPC_ADMA_INIT_IRQ2;
> -			adev->err_irq = -ENXIO;
>  		} else
>  			atomic_inc(&ppc440spe_adma_err_irq_ref);
>  	} else {

[ ... ]

> @@ -3987,13 +3983,10 @@ static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev,
>  		iowrite32(mask, &adev->i2o_reg->iopim);

[Severity: High]
This is a pre-existing issue, but could this lockless read-modify-write
on the shared I2O IOPIM register cause data corruption?

Both DMA0 and DMA1 engines map the exact same ibm,i2o-440spe device tree node
and access the shared physical iopim register during setup and teardown:

ppc440spe_adma_setup_irqs() {
    ...
    mask = ioread32(&adev->i2o_reg->iopim) & enable;
    iowrite32(mask, &adev->i2o_reg->iopim);
    ...
}

If one engine accesses ppc440spe_adma_setup_irqs() or
ppc440spe_adma_release_irqs() concurrently with the other, the lockless
ioread32() and iowrite32() sequence can race, causing one engine's updates
to silently clobber the other's and permanently break interrupts.

>  	}
>  	free_irq(adev->irq, chan);
> -	irq_dispose_mapping(adev->irq);
>  	if (adev->err_irq > 0) {
>  		free_irq(adev->err_irq, chan);
> -		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref)) {
> -			irq_dispose_mapping(adev->err_irq);
> +		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref))
>  			iounmap(adev->i2o_reg);

[Severity: High]
This is a pre-existing issue, but does tying the cleanup of independent
per-device allocations to a shared global refcount leak the other virtual
mappings?

Both DMA0 and DMA1 engines independently call of_iomap() in
ppc440spe_adma_setup_irqs(), allocating unique virtual mappings:

ppc440spe_adma_setup_irqs() {
    ...
    adev->i2o_reg = of_iomap(np, 0);
    ...
}

Because iounmap() is gated behind this shared global refcount, it only
reaches zero for the last engine released. The mapping for the first released
engine is permanently leaked. Furthermore, if err_irq < 0, both unmaps are
bypassed entirely.

> -		}
>  	}
>  }

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