Re: [PATCH 1/4] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The IRQ handler `mpc52xx_ata_task_irq` is registered and unmasked before the driver's private state (`priv->dmatsk`) is fully initialized, creating a window where an immediate interrupt could trigger a NULL pointer dereference.
--

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

ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

This patch replaces devm_request_irq with request_irq for the BestComm
task IRQ to ensure the interrupt is unregistered before bcom_ata_release()
tears down the task. This fixes a race condition during device removal or
error paths where the handler could access freed memory.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..9af0aa89a5c66 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -767,11 +767,11 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	}
>  
>  	task_irq = bcom_get_task_irq(dmatsk);
> -	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
> -				"ATA task", priv);
> +	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
> +			  "ATA task", priv);
>  	if (rv) {
>  		dev_err(&op->dev, "error requesting DMA IRQ\n");
> -		goto err2;
> +		goto err_free_task;
>  	}
>  	priv->dmatsk = dmatsk;

[Severity: High]
This is a pre-existing issue, but could this ordering cause a NULL pointer
dereference if an interrupt fires immediately after registration?

In mpc52xx_ata_probe(), the IRQ is requested and enabled before the DMA
task pointer is assigned to the driver's private state. Since priv is
allocated with devm_kzalloc(), priv->dmatsk is NULL during the
request_irq() call.

If a pending or spurious interrupt fires before the priv->dmatsk assignment
completes, the handler is invoked and dereferences the uninitialized pointer:

mpc52xx_ata_task_irq()
    while (bcom_buffer_done(priv->dmatsk))
        bcom_retrieve_buffer(priv->dmatsk, NULL, NULL);

Should priv->dmatsk be assigned to the private structure before registering
the interrupt handler to avoid this window?

[ ... ]

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