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

[sashiko-bot@kernel.org]

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