Re: [PATCH] ata: pata_mpc52xx: convert to platform_get_irq()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Redundant error logging after `platform_get_irq()` breaks probe deferral silence.

Pre-existing issues:
- [High] Teardown inversion and double-free vulnerabilities due to manual cleanup of resources before `devm`-managed IRQ handler deregistration.
--

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

ata: pata_mpc52xx: convert to platform_get_irq()

Replace irq_of_parse_and_map() with platform_get_irq(), which returns
a negative errno on failure and integrates with the platform device
model. The corresponding irq_dispose_mapping() calls in the probe
error path and remove function are no longer needed.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..f8ecd58765dd3 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
[ ... ]
> @@ -730,10 +730,10 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	if ((prop) && (proplen >= 4))
>  		udma_mask = ATA_UDMA2 & ((1 << (*prop + 1)) - 1);
>  
> -	ata_irq = irq_of_parse_and_map(op->dev.of_node, 0);
> -	if (!ata_irq) {
> +	ata_irq = platform_get_irq(op, 0);
> +	if (ata_irq < 0) {
>  		dev_err(&op->dev, "error mapping irq\n");

[Severity: Medium]
Since platform_get_irq() already logs errors internally using dev_err_probe(),
which correctly silences -EPROBE_DEFER, does leaving this manual dev_err()
call in place cause redundant log spam during probe deferral?

> -		return -EINVAL;
> +		return ata_irq;
>  	}
>  
>  	/* Prepare our private structure */
[ ... ]
> @@ -796,7 +796,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(dmatsk);

[Severity: High]
This is a pre-existing issue, but does manually cleaning up the resources here
cause a use-after-free and teardown inversion?

Because the driver requests task_irq using devm_request_irq(), the interrupt
handler remains active until devres frees it after the probe error path
returns.

If bcom_ata_release(dmatsk) frees the memory while the devm-managed IRQ
handler is still registered, could an incoming interrupt access freed memory?

Additionally, disposing the IRQ mapping while the interrupt action is still
registered breaks the IRQ subsystem, as devm_free_irq() will later fail to
locate the descriptor.

Finally, since bcom_ata_release() internally calls bcom_task_free(), which
also calls irq_dispose_mapping(tsk->irq), does this sequence trigger a
double-free of the IRQ mapping?

>  err1:
> -	irq_dispose_mapping(ata_irq);
>  	return rv;
>  }
>  
> @@ -813,7 +812,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
>  	task_irq = bcom_get_task_irq(priv->dmatsk);
>  	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(priv->dmatsk);

[Severity: High]
This is a pre-existing issue, but does this pattern present the same
use-after-free, teardown inversion, and double-free risks as the probe error
path?

Since the IRQ handler is managed by devm, it is still active when this remove
function returns. Manually releasing the task resources and IRQ mapping before
devres unregisters the handler seems problematic.

> -	irq_dispose_mapping(priv->ata_irq);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP

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