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

[Niklas Cassel]

On Mon, Jun 08, 2026 at 02:47:15PM -0700, Rosen Penev wrote:
> The BestComm task IRQ handler accesses priv->dmatsk, which is freed
> by bcom_ata_release().  With devm_request_irq(), the handler remains
> registered until devres unwinds after the error/remove function
> returns, creating a window where the handler can access freed memory.
> 
> Switch to request_irq() / free_irq() so that the interrupt is
> unregistered before bcom_ata_release() tears down the task.
> 
> Assisted-by: opencode:big-pickle
> Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>  drivers/ata/pata_mpc52xx.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62..9af0aa89a5c6 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;
>  
> @@ -779,7 +779,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	rv = mpc52xx_ata_hw_init(priv);
>  	if (rv) {
>  		dev_err(&op->dev, "error initializing hardware\n");
> -		goto err2;
> +		goto err_free_irq;
>  	}
>  
>  	/* Register ourselves to libata */
> @@ -787,13 +787,15 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  				  mwdma_mask, udma_mask);
>  	if (rv) {
>  		dev_err(&op->dev, "error registering with ATA layer\n");
> -		goto err2;
> +		goto err_free_irq;
>  	}
>  
>  	return 0;
>  
> - err2:
> + err_free_irq:
> +	free_irq(task_irq, priv);
>  	irq_dispose_mapping(task_irq);
> + err_free_task:
>  	bcom_ata_release(dmatsk);
>   err1:
>  	irq_dispose_mapping(ata_irq);
> @@ -811,6 +813,7 @@ static void mpc52xx_ata_remove(struct platform_device *op)
>  
>  	/* Clean up DMA */
>  	task_irq = bcom_get_task_irq(priv->dmatsk);
> +	free_irq(task_irq, priv);
>  	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(priv->dmatsk);
>  	irq_dispose_mapping(priv->ata_irq);
> -- 
> 2.54.0
> 

+ Andy

Andy, since you gave a talk about this recently.
Surely there must be a better way than to migrate away from the device
managed APIs.

Do you have some suggestion?


Kind regards,
Niklas