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

On Fri, Jul 31, 2026 at 11:47=E2=80=AFAM Frank Li <[email protected]> wr=
ote:
>
> On Fri, Jul 31, 2026 at 10:38:53AM -0700, Rosen Penev wrote:
> > On Fri, Jul 31, 2026 at 7:53=E2=80=AFAM Frank Li <[email protected]>=
 wrote:
> > >
> > > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > > On Thu, Jul 30, 2026 at 11:49=E2=80=AFAM Frank Li <[email protected]=
.com> 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 u=
se after
> > > > > > free on teardown as request_irq uses mci as dev_id.
> > > > > >
> > > > > > Signed-off-by: Rosen Penev <[email protected]>
> > > > > > ---
> > > > > >  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 =3D platform_get_irq(op, 0);
> > > > > > -             res =3D devm_request_irq(&op->dev, pdata->irq,
> > > > > > +             res =3D request_irq(pdata->irq,
> > > > > >                                      fsl_mc_isr,
> > > > > >                                      IRQF_SHARED,
> > > > > >                                      "[EDAC] MC err", mci);
> > > > > > @@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_devi=
ce *op)
> > > > > >
> > > > > >       if (edac_op_state =3D=3D 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. Disablin=
g
> > > > 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() =E2=80=94 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.
>
> It may need goto if add logic after request_irq(), I want to make probe
> code clean enough.
>
> Error message doesn't mattter, we can remove it.
>
> I suggest use other method, let use devm add action to make free irq befo=
re
> free mci.
ACK. A couple of other drivers suffer from the same issue.
>
> Frank
>
> > >
> > > Frank
> > >
> > > > >
> > > > > Frank
> > > > >
> > > > > >       }
> > > > > >
> > > > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > > > --
> > > > > > 2.55.0
> > > > > >
> > > > > >