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

[Rosen Penev]

On Tue, Jun 9, 2026 at 1:36 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> 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.
Note the fixes tag. The purpose is to get this backported.
>
> 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.
Doesn't matter much. task_irq gets requested, not ata_irq.
>
> 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()
Done in later patch in this series.
> - 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
>
>