dmaengine: ppc4xx: convert irq_of_parse_and_map to platform_get_irq

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

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.

Quoted 8 lines

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

index 279a431ccae38..99aee726644e0 100644
@@ -3865,28 +3865,24 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				     int *initcode)
 {
 	struct platform_device *ofdev;

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev, · line 3873

Quoted 10 lines

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

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

Quoted 6 lines

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

 			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 {

[ ... ]

drivers/dma/ppc4xx/adma.c static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev, · line 3983

Quoted 2 lines

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

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

drivers/dma/ppc4xx/adma.c static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev, · line 3989

Quoted 9 lines

drivers/dma/ppc4xx/adma.c +1 -3

 	}
 	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.

drivers/dma/ppc4xx/adma.c static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev, · line 3991

Quoted 3 lines

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

-		}
 	}
 }

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