Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

On Fri, Jul 31, 2026 at 7:53 AM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
> > >
> > > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > > We need to tear down the irq before edac_mc_free() to avoid a use after
> > > > free on teardown as request_irq uses mci as dev_id.
> > > >
> > > > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > > > ---
> > > >  drivers/edac/fsl_ddr_edac.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > > index f4cc89b26954..f1ddd1f47f2a 100644
> > > > --- a/drivers/edac/fsl_ddr_edac.c
> > > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > > @@ -601,7 +601,7 @@ int fsl_mc_err_probe(struct platform_device *op)
> > > >
> > > >               /* register interrupts */
> > > >               pdata->irq = platform_get_irq(op, 0);
> > > > -             res = devm_request_irq(&op->dev, pdata->irq,
> > > > +             res = request_irq(pdata->irq,
> > > >                                      fsl_mc_isr,
> > > >                                      IRQF_SHARED,
> > > >                                      "[EDAC] MC err", mci);
> > > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
> > > >
> > > >       if (edac_op_state == EDAC_OPSTATE_INT) {
> > > >               ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > > > +             free_irq(pdata->irq, mci);
> > >
> > > here already disable hardware irq. Just call
> > > synchronize_irq() to make sure pending irq handled.
> > AI disagrees with this suggestion:
> >
> > The reviewer's suggestion to just call synchronize_irq() instead of
> > free_irq() is problematic for two reasons:
> > 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> > the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> > this device from asserting the IRQ line, but other devices sharing the
> > same IRQ line can still trigger it. Without free_irq, the handler
> > remains in the shared action list and can be called with a dangling
> > mci pointer after edac_mc_free() — the very UAF this patch aims to
> > fix.
> > 2. free_irq already synchronizes: free_irq() internally calls
> > synchronize_irq() after removing the action from the descriptor. An
> > explicit synchronize_irq() before free_irq() would be redundant.
>
> You post too quick, I have not seen this mail yet.
>
> devm_free_irq() here. and add comment why need explicit call it.
Needing to explicitly call devm_free_irq kind of defeats the purpose of devm.

Explicit ordering in this case makes more sense to me, especially with
the ddr_out32 calls.

There's actually a second benefit to the non devm version. The devm
version prints an error message on failure. The normal one does not.
The significance is that the driver prints its own message.
>
> Frank
>
> > >
> > > Frank
> > >
> > > >       }
> > > >
> > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > --
> > > > 2.55.0
> > > >
> > > >