[PATCHv2 0/3] EDAC/fsl_ddr: some cleanups

[PATCHv2 0/3] EDAC/fsl_ddr: some cleanups

[Rosen Penev]

First one is a cleanup for devm_platform_ioremap_resource() and some bugs.

Others are issues flagged by sashiko.

Rosen Penev (3):
  EDAC/fsl_ddr: use devm_platform_ioremap_resource()
  EDAC/fsl_ddr: use non devm for request_irq()
  EDAC/fsl_ddr: restore MC error registers on probe failure

 drivers/edac/fsl_ddr_edac.c | 56 ++++++++++++-------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

-- 
2.55.0

[PATCHv2 1/3] EDAC/fsl_ddr: use devm_platform_ioremap_resource()

[Rosen Penev]

Replace the open-coded of_address_to_resource() plus devm_request_mem_region()
and devm_ioremap() sequence with devm_platform_ioremap_resource(), which folds
the resource lookup, region reservation and mapping into one step and returns
an ERR_PTR checked with IS_ERR()/PTR_ERR().

Behaviorally equivalent with respect to region reservation: the driver
already reserved the region, so the non-overlapping reg requirement was
already satisfied. Drop the now-unused linux/of_address.h include.

Remove devres stuff. Not needed. devm is already used.

Fix a mistaken return code of -ENOMEM when PTR_ERR should be used.

Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/fsl_ddr_edac.c | 39 +++++++------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 2a545e5812e7..3745eec67335 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -22,7 +22,6 @@
 #include <linux/gfp.h>
 
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include "edac_module.h"
 #include "fsl_ddr_edac.h"
 
@@ -495,13 +494,14 @@ int fsl_mc_err_probe(struct platform_device *op)
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[2];
 	struct fsl_mc_pdata *pdata;
-	struct resource r;
+	void __iomem *mc_vbase;
 	u32 ecc_en_mask;
 	u32 sdram_ctl;
 	int res;
 
-	if (!devres_open_group(&op->dev, fsl_mc_err_probe, GFP_KERNEL))
-		return -ENOMEM;
+	mc_vbase = devm_platform_ioremap_resource(op, 0);
+	if (IS_ERR(mc_vbase))
+		return PTR_ERR(mc_vbase);
 
 	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
 	layers[0].size = 4;
@@ -511,10 +511,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 	layers[1].is_virt_csrow = false;
 	mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
 			    sizeof(*pdata));
-	if (!mci) {
-		devres_release_group(&op->dev, fsl_mc_err_probe);
+	if (!mci)
 		return -ENOMEM;
-	}
 
 	pdata = mci->pvt_info;
 	pdata->name = "fsl_mc_err";
@@ -531,33 +529,12 @@ int fsl_mc_err_probe(struct platform_device *op)
 	 * Default is big endian.
 	 */
 	pdata->little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
-
-	res = of_address_to_resource(op->dev.of_node, 0, &r);
-	if (res) {
-		pr_err("%s: Unable to get resource for MC err regs\n",
-		       __func__);
-		goto err;
-	}
-
-	if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
-				     pdata->name)) {
-		pr_err("%s: Error while requesting mem region\n",
-		       __func__);
-		res = -EBUSY;
-		goto err;
-	}
-
-	pdata->mc_vbase = devm_ioremap(&op->dev, r.start, resource_size(&r));
-	if (!pdata->mc_vbase) {
-		pr_err("%s: Unable to setup MC err regs\n", __func__);
-		res = -ENOMEM;
-		goto err;
-	}
+	pdata->mc_vbase = mc_vbase;
 
 	if (pdata->flag == TYPE_IMX9) {
 		pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
 		if (IS_ERR(pdata->inject_vbase)) {
-			res = -ENOMEM;
+			res = PTR_ERR(pdata->inject_vbase);
 			goto err;
 		}
 	}
@@ -637,7 +614,6 @@ int fsl_mc_err_probe(struct platform_device *op)
 		       pdata->irq);
 	}
 
-	devres_remove_group(&op->dev, fsl_mc_err_probe);
 	edac_dbg(3, "success\n");
 	pr_info(EDAC_MOD_STR " MC err registered\n");
 
@@ -646,7 +622,6 @@ int fsl_mc_err_probe(struct platform_device *op)
 err2:
 	edac_mc_del_mc(&op->dev);
 err:
-	devres_release_group(&op->dev, fsl_mc_err_probe);
 	edac_mc_free(mci);
 	return res;
 }
-- 
2.55.0

[PATCHv2 2/3] EDAC/fsl_ddr: use non devm for request_irq()

[Rosen Penev]

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@gmail.com>
---
 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 3745eec67335..c505fb268474 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -599,7 +599,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);
@@ -635,6 +635,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);
 	}
 
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
-- 
2.55.0

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

[Rosen Penev]

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.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 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

Re: [PATCHv2 2/3] EDAC/fsl_ddr: use non devm for request_irq()

[Frank Li]

On Thu, Jul 30, 2026 at 06:21:29PM -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@gmail.com>
> ---
>  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 3745eec67335..c505fb268474 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -599,7 +599,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);
> @@ -635,6 +635,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);

I asked at v1.

it already disable hardware irq request,

synchronize_irq() should be enought to make sure finish current irq handle.

Frank

>  	}
>
>  	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> --
> 2.55.0
>
>

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

[Frank Li]

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 <rosenp@gmail.com>
> ---
>  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
>
>

Re: [PATCHv2 1/3] EDAC/fsl_ddr: use devm_platform_ioremap_resource()

[Frank Li]

On Thu, Jul 30, 2026 at 06:21:28PM -0700, Rosen Penev wrote:
> Replace the open-coded of_address_to_resource() plus devm_request_mem_region()
> and devm_ioremap() sequence with devm_platform_ioremap_resource(), which folds
> the resource lookup, region reservation and mapping into one step and returns
> an ERR_PTR checked with IS_ERR()/PTR_ERR().
>
> Behaviorally equivalent with respect to region reservation: the driver
> already reserved the region, so the non-overlapping reg requirement was
> already satisfied. Drop the now-unused linux/of_address.h include.
>
> Remove devres stuff. Not needed. devm is already used.
>
> Fix a mistaken return code of -ENOMEM when PTR_ERR should be used.
>
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Need change log here.

You missed my review-by and tested-by tags. If you delete it by some reason
like big changes, you should said here explicit.

Frank

>  drivers/edac/fsl_ddr_edac.c | 39 +++++++------------------------------
>  1 file changed, 7 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 2a545e5812e7..3745eec67335 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -22,7 +22,6 @@
>  #include <linux/gfp.h>
>
>  #include <linux/of.h>
> -#include <linux/of_address.h>
>  #include "edac_module.h"
>  #include "fsl_ddr_edac.h"
>
> @@ -495,13 +494,14 @@ int fsl_mc_err_probe(struct platform_device *op)
>  	struct mem_ctl_info *mci;
>  	struct edac_mc_layer layers[2];
>  	struct fsl_mc_pdata *pdata;
> -	struct resource r;
> +	void __iomem *mc_vbase;
>  	u32 ecc_en_mask;
>  	u32 sdram_ctl;
>  	int res;
>
> -	if (!devres_open_group(&op->dev, fsl_mc_err_probe, GFP_KERNEL))
> -		return -ENOMEM;
> +	mc_vbase = devm_platform_ioremap_resource(op, 0);
> +	if (IS_ERR(mc_vbase))
> +		return PTR_ERR(mc_vbase);
>
>  	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
>  	layers[0].size = 4;
> @@ -511,10 +511,8 @@ int fsl_mc_err_probe(struct platform_device *op)
>  	layers[1].is_virt_csrow = false;
>  	mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
>  			    sizeof(*pdata));
> -	if (!mci) {
> -		devres_release_group(&op->dev, fsl_mc_err_probe);
> +	if (!mci)
>  		return -ENOMEM;
> -	}
>
>  	pdata = mci->pvt_info;
>  	pdata->name = "fsl_mc_err";
> @@ -531,33 +529,12 @@ int fsl_mc_err_probe(struct platform_device *op)
>  	 * Default is big endian.
>  	 */
>  	pdata->little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
> -
> -	res = of_address_to_resource(op->dev.of_node, 0, &r);
> -	if (res) {
> -		pr_err("%s: Unable to get resource for MC err regs\n",
> -		       __func__);
> -		goto err;
> -	}
> -
> -	if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
> -				     pdata->name)) {
> -		pr_err("%s: Error while requesting mem region\n",
> -		       __func__);
> -		res = -EBUSY;
> -		goto err;
> -	}
> -
> -	pdata->mc_vbase = devm_ioremap(&op->dev, r.start, resource_size(&r));
> -	if (!pdata->mc_vbase) {
> -		pr_err("%s: Unable to setup MC err regs\n", __func__);
> -		res = -ENOMEM;
> -		goto err;
> -	}
> +	pdata->mc_vbase = mc_vbase;
>
>  	if (pdata->flag == TYPE_IMX9) {
>  		pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
>  		if (IS_ERR(pdata->inject_vbase)) {
> -			res = -ENOMEM;
> +			res = PTR_ERR(pdata->inject_vbase);
>  			goto err;
>  		}
>  	}
> @@ -637,7 +614,6 @@ int fsl_mc_err_probe(struct platform_device *op)
>  		       pdata->irq);
>  	}
>
> -	devres_remove_group(&op->dev, fsl_mc_err_probe);
>  	edac_dbg(3, "success\n");
>  	pr_info(EDAC_MOD_STR " MC err registered\n");
>
> @@ -646,7 +622,6 @@ int fsl_mc_err_probe(struct platform_device *op)
>  err2:
>  	edac_mc_del_mc(&op->dev);
>  err:
> -	devres_release_group(&op->dev, fsl_mc_err_probe);
>  	edac_mc_free(mci);
>  	return res;
>  }
> --
> 2.55.0
>
>

Re: [PATCHv2 1/3] EDAC/fsl_ddr: use devm_platform_ioremap_resource()

[Rosen Penev]

On Fri, Jul 31, 2026 at 8:05 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Thu, Jul 30, 2026 at 06:21:28PM -0700, Rosen Penev wrote:
> > Replace the open-coded of_address_to_resource() plus devm_request_mem_region()
> > and devm_ioremap() sequence with devm_platform_ioremap_resource(), which folds
> > the resource lookup, region reservation and mapping into one step and returns
> > an ERR_PTR checked with IS_ERR()/PTR_ERR().
> >
> > Behaviorally equivalent with respect to region reservation: the driver
> > already reserved the region, so the non-overlapping reg requirement was
> > already satisfied. Drop the now-unused linux/of_address.h include.
> >
> > Remove devres stuff. Not needed. devm is already used.
> >
> > Fix a mistaken return code of -ENOMEM when PTR_ERR should be used.
> >
> > Assisted-by: opencode:hy3-free
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
>
> Need change log here.
>
> You missed my review-by and tested-by tags. If you delete it by some reason
> like big changes, you should said here explicit.
I changed some stuff yeah. I had them added but removed just in case.
>
> Frank
>
> >  drivers/edac/fsl_ddr_edac.c | 39 +++++++------------------------------
> >  1 file changed, 7 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 2a545e5812e7..3745eec67335 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -22,7 +22,6 @@
> >  #include <linux/gfp.h>
> >
> >  #include <linux/of.h>
> > -#include <linux/of_address.h>
> >  #include "edac_module.h"
> >  #include "fsl_ddr_edac.h"
> >
> > @@ -495,13 +494,14 @@ int fsl_mc_err_probe(struct platform_device *op)
> >       struct mem_ctl_info *mci;
> >       struct edac_mc_layer layers[2];
> >       struct fsl_mc_pdata *pdata;
> > -     struct resource r;
> > +     void __iomem *mc_vbase;
> >       u32 ecc_en_mask;
> >       u32 sdram_ctl;
> >       int res;
> >
> > -     if (!devres_open_group(&op->dev, fsl_mc_err_probe, GFP_KERNEL))
> > -             return -ENOMEM;
> > +     mc_vbase = devm_platform_ioremap_resource(op, 0);
> > +     if (IS_ERR(mc_vbase))
> > +             return PTR_ERR(mc_vbase);
> >
> >       layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> >       layers[0].size = 4;
> > @@ -511,10 +511,8 @@ int fsl_mc_err_probe(struct platform_device *op)
> >       layers[1].is_virt_csrow = false;
> >       mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
> >                           sizeof(*pdata));
> > -     if (!mci) {
> > -             devres_release_group(&op->dev, fsl_mc_err_probe);
> > +     if (!mci)
> >               return -ENOMEM;
> > -     }
> >
> >       pdata = mci->pvt_info;
> >       pdata->name = "fsl_mc_err";
> > @@ -531,33 +529,12 @@ int fsl_mc_err_probe(struct platform_device *op)
> >        * Default is big endian.
> >        */
> >       pdata->little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
> > -
> > -     res = of_address_to_resource(op->dev.of_node, 0, &r);
> > -     if (res) {
> > -             pr_err("%s: Unable to get resource for MC err regs\n",
> > -                    __func__);
> > -             goto err;
> > -     }
> > -
> > -     if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
> > -                                  pdata->name)) {
> > -             pr_err("%s: Error while requesting mem region\n",
> > -                    __func__);
> > -             res = -EBUSY;
> > -             goto err;
> > -     }
> > -
> > -     pdata->mc_vbase = devm_ioremap(&op->dev, r.start, resource_size(&r));
> > -     if (!pdata->mc_vbase) {
> > -             pr_err("%s: Unable to setup MC err regs\n", __func__);
> > -             res = -ENOMEM;
> > -             goto err;
> > -     }
> > +     pdata->mc_vbase = mc_vbase;
> >
> >       if (pdata->flag == TYPE_IMX9) {
> >               pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
> >               if (IS_ERR(pdata->inject_vbase)) {
> > -                     res = -ENOMEM;
> > +                     res = PTR_ERR(pdata->inject_vbase);
> >                       goto err;
> >               }
> >       }
> > @@ -637,7 +614,6 @@ int fsl_mc_err_probe(struct platform_device *op)
> >                      pdata->irq);
> >       }
> >
> > -     devres_remove_group(&op->dev, fsl_mc_err_probe);
> >       edac_dbg(3, "success\n");
> >       pr_info(EDAC_MOD_STR " MC err registered\n");
> >
> > @@ -646,7 +622,6 @@ int fsl_mc_err_probe(struct platform_device *op)
> >  err2:
> >       edac_mc_del_mc(&op->dev);
> >  err:
> > -     devres_release_group(&op->dev, fsl_mc_err_probe);
> >       edac_mc_free(mci);
> >       return res;
> >  }
> > --
> > 2.55.0
> >
> >

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@oss.nxp.com> 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@gmail.com>
> > ---
> >  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
> >
> >