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

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.

Frank
>
> Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <[email protected]>
> ---
>  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
>
>