Re: [PATCH] ata: Remove redundant dev_err()

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

Pre-existing issues:
- [High] Double Free of IRQ Mapping
- [High] Memory Leak of irqaction (Devres Conflict)
--

--- Patch [1]: [PATCH] ata: Remove redundant dev_err() ---
commit c11d795c8457981d78fb45735e5126724a48c0df
Author: Pan Chuang <[email protected]>

ata: Remove redundant dev_err()

Since devm_request_irq() automatically logs error messages on failure,
this commit removes the redundant driver-specific dev_err() call from
the probe path.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..1739743427cfc 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -769,10 +769,8 @@ 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);
> -	if (rv) {
> -		dev_err(&op->dev, "error requesting DMA IRQ\n");
> +	if (rv)
>  		goto err2;

[Severity: High]
This is a pre-existing issue, but I noticed a potential double free of the
IRQ mapping and a memory leak related to this error path.

If devm_request_irq() fails, the code jumps to the err2 label:

 err2:
	irq_dispose_mapping(task_irq);
	bcom_ata_release(dmatsk);

Does this cause a double free of the IRQ mapping? The bcom_ata_release()
function internally calls bcom_task_free(), which also unconditionally calls
irq_dispose_mapping(tsk->irq) on the same task_irq.

Additionally, can these manual irq_dispose_mapping() calls in the err1 and err2
paths (and similarly in mpc52xx_ata_remove()) cause the irqaction structures
to leak?

Because devm_request_irq() is used, the devres cleanup runs after the probe
function returns. If the IRQ mapping is already manually disposed of here,
irq_dispose_mapping() removes the descriptor. When devres later invokes
free_irq(), it will find a NULL descriptor and return early, skipping the
kfree() for the irqaction structure.

> -	}
>  	priv->dmatsk = dmatsk;
>  
>  	/* Init the hw */

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