Re: [PATCHv2 3/3] EDAC/fsl_ddr: restore MC error registers on probe failure

[Rosen Penev]

On Fri, Jul 31, 2026 at 7:57 AM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> On Thu, Jul 30, 2026 at 06:21:30PM -0700, Rosen Penev wrote:
> > fsl_mc_err_probe() clears FSL_MC_ERR_DISABLE and, in EDAC_OPSTATE_INT
> > mode, sets FSL_MC_ERR_INT_EN to enable error reporting. If a later
> > initialization step fails and jumps to the err/err2 labels, the cleanup
> > only frees driver memory and does not restore these hardware registers.
> >
> > This leaves the memory controller configured to generate interrupts while
> > no handler is successfully registered, which can cause an unhandled
> > interrupt storm on the shared IRQ line. Restore FSL_MC_ERR_DISABLE,
> > FSL_MC_ERR_INT_EN and the SBE threshold in both error paths.
> >
> > Do not pre-initialize orig_ddr_err_disable/orig_ddr_err_sbe to 0; the
> > err path must only restore registers that were actually modified.
> > Split the error labels so early failures (before the first hardware
> > write at line ~~580) jump to err_free, which does cleanup without
> > touching hardware registers. Save orig_ddr_err_sbe alongside
> > orig_ddr_err_disable so the err label correctly restores both
> > registers, avoiding stale bootloader state clobbers.
> >
> > Built for arm64 (defconfig + CONFIG_EDAC_FSL_DDR) with LLVM=1;
> > drivers/edac/fsl_ddr_edac.o compiles cleanly.
>
> You can put this after ---, this is redunctant, every patch must pass
> build.
Will remove.
>
> Frank
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> > Assisted-by: opencode:hy3-free
> > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index c505fb268474..139277c4e2f0 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -577,23 +577,22 @@ int fsl_mc_err_probe(struct platform_device *op)
> >       pdata->orig_ddr_err_disable = ddr_in32(pdata, FSL_MC_ERR_DISABLE);
> >       ddr_out32(pdata, FSL_MC_ERR_DISABLE, 0);
> >
> > +     /* store the original SBE threshold */
> > +     pdata->orig_ddr_err_sbe = ddr_in32(pdata, FSL_MC_ERR_SBE) & 0xff0000;
> > +
> >       /* clear all error bits */
> >       ddr_out32(pdata, FSL_MC_ERR_DETECT, ~0);
> >
> >       res = edac_mc_add_mc_with_groups(mci, fsl_ddr_dev_groups);
> >       if (res) {
> >               edac_dbg(3, "failed edac_mc_add_mc()\n");
> > -             goto err;
> > +             goto err1;
> >       }
> >
> >       if (edac_op_state == EDAC_OPSTATE_INT) {
> >               ddr_out32(pdata, FSL_MC_ERR_INT_EN,
> >                         DDR_EIE_MBEE | DDR_EIE_SBEE);
> >
> > -             /* store the original error management threshold */
> > -             pdata->orig_ddr_err_sbe = ddr_in32(pdata,
> > -                                                FSL_MC_ERR_SBE) & 0xff0000;
> > -
> >               /* set threshold to 1 error per interrupt */
> >               ddr_out32(pdata, FSL_MC_ERR_SBE, 0x10000);
> >
> > @@ -621,6 +620,11 @@ int fsl_mc_err_probe(struct platform_device *op)
> >
> >  err2:
> >       edac_mc_del_mc(&op->dev);
> > +     ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
> > +err1:
> > +     ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > +               pdata->orig_ddr_err_disable);
> > +     ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);
> >  err:
> >       edac_mc_free(mci);
> >       return res;
> > --
> > 2.55.0
> >
> >