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

[Andy Shevchenko]

On Tue, Jun 09, 2026 at 09:20:35AM +0200, Niklas Cassel wrote:
> 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.

...

> >  	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");

Side note: In case we leave devm_request_irq(), this message is a dup
and may be removed.

> > -		goto err2;
> > +		goto err_free_task;

Now, devm_*() should be never followed by goto. If we want to keep devm
approach, the original code has more issues.

> >  	}

...

> 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.

Thank for Cc'ing me.

> Do you have some suggestion?

This patch either way (are we going to devm fully or rollback to non-devm)
is half-baked.

What I may suggest here is to extend this patch to cover the second
problematic place (*) and make it as a fix for backporting.

After that one may carefully go and finish converting probe to devm_*().

*) https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/ata/pata_mpc52xx.c#L733
The call of irq_of_parse_and_map() should be moved after devm_kzalloc().
So, this fix should do what it does + the above.

P.S.
Someone may tide up this driver a lot, but this is out of the scope of
this patch. FWIW, the brief list of potential refactoring patches
based on probe (in unsorted order):
- use devm_platform_ioremap_resource()
- moving towards device_property_read_*()
- using dev_err_probe()
- use temporary variable to hold struct device pointer
- use fwnode_irq_get()
- apply multipliers from units.h and time.h (like HZ_PER_MHZ)
- moving to a new struct dev_pm_ops and pm_sleep_ptr() macro

-- 
With Best Regards,
Andy Shevchenko