[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()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver ignores negative error codes from `platform_get_irq()` (such as `-EPROBE_DEFER`) and incorrectly shadows them with `-ENODEV`, breaking deferred probing.
- [High] Hardware interrupts are enabled before `request_irq()` and left active if `request_irq()` fails, potentially causing an unhandled interrupt storm.
--

--- Patch [1]: [PATCHv2 2/3] EDAC/fsl_ddr: use non devm for request_irq() ---
commit 3e29959b7ca66cc613f9380e3c367c87f8883fe5
Author: Rosen Penev <rosenp@gmail.com>

EDAC/fsl_ddr: use non devm for request_irq()
    
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.

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index e4eaec0aa81d5..878d4a86a3c9c 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -623,7 +623,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);

[Severity: High]
This is a pre-existing issue, but are we properly handling deferred probing
here?

If platform_get_irq() returns a negative error code like -EPROBE_DEFER,
this code passes the negative value directly to request_irq() which expects an
unsigned int. When request_irq() inevitably fails with -EINVAL, the driver
jumps to the err2 label and hardcodes the error return to -ENODEV:

    if (res < 0) {
        pr_err("%s: Unable to request irq %d for FSL DDR DRAM ERR\n",
               __func__, pdata->irq);
        res = -ENODEV;
        goto err2;
    }

Can this cause the driver to permanently fail initialization on systems where
the interrupt controller is probed after this device?

[Severity: High]
This isn't a bug introduced by this patch, but might the error path fail to
roll back the hardware state if request_irq() fails?

Earlier in fsl_mc_err_probe(), hardware interrupts are enabled:

    if (edac_op_state == EDAC_OPSTATE_INT) {
        ddr_out32(pdata, FSL_MC_ERR_INT_EN,
                  DDR_EIE_MBEE | DDR_EIE_SBEE);

If request_irq() fails, the driver jumps to err2 and eventually returns, but
it doesn't disable the interrupts in the FSL_MC_ERR_INT_EN register:

err2:
    edac_mc_del_mc(&op->dev);
err:
    devres_release_group(&op->dev, fsl_mc_err_probe);
    edac_mc_free(mci);
    return res;

Could this leave the device in a state where it generates an unhandled
interrupt storm, potentially prompting the IRQ core to disable the shared
interrupt line entirely?

> @@ -661,6 +661,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,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731012130.1212640-3-rosenp@gmail.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Possible use-after-free in the interrupt handler during module removal / driver unbind.
- [High] Hardware interrupts are enabled before the IRQ handler is successfully registered, risking an unhandled interrupt storm if the IRQ request fails.
--

--- Patch [1]: [PATCHv2 1/3] EDAC/fsl_ddr: use devm_platform_ioremap_resource() ---
commit 6f9b8e9cc4f802944da43deab8c11651f32e407d
Author: Rosen Penev <rosenp@gmail.com>

EDAC/fsl_ddr: use devm_platform_ioremap_resource()

This commit replaces the open-coded of_address_to_resource() and
devm_ioremap() sequences with devm_platform_ioremap_resource(). It
also removes the explicit devres group management as devm is already
used.

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index e4eaec0aa81d5..4236e06d568e9 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
[ ... ]
> @@ -638,7 +615,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");
>  
> @@ -647,7 +623,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);

[Severity: High]
This isn't a bug introduced by this patch, but since we are relying on
devm APIs here, is there a risk of a use-after-free during driver unbind?

During module removal, fsl_mc_err_remove() manually frees the mci structure
via edac_mc_free(mci). However, the shared interrupt handler fsl_mc_isr()
is managed by devm_request_irq() and remains active until the driver core
cleans up devm resources strictly after remove() returns.

If another device sharing the same IRQ line triggers an interrupt in this
window, the kernel will execute fsl_mc_isr(), which unconditionally
dereferences the freed mci pointer:

fsl_mc_isr(..., void *dev_id)
{
        struct mem_ctl_info *mci = dev_id;
        struct fsl_mc_pdata *pdata = mci->pvt_info;
        ...
}

Should the IRQ be explicitly freed in remove(), or registered with a non-devm
API to ensure the handler is unregistered before mci is freed?

[Severity: High]
This is also a pre-existing issue, but does this error path leave the hardware
interrupt enabled if devm_request_irq() fails?

Earlier in fsl_mc_err_probe(), the hardware interrupt is explicitly enabled:

        ddr_out32(pdata, FSL_MC_ERR_INT_EN, DDR_EIE_MBEE | DDR_EIE_SBEE);

If devm_request_irq() fails (for example, due to -EPROBE_DEFER or resource
constraints), the failure path jumps here to err2, which does not disable
the hardware interrupt. If a memory error occurs and the controller asserts
the IRQ line, the lack of a registered handler might cause an unhandled
interrupt storm.

Should the error path disable the interrupt before returning?

>  	return res;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731012130.1212640-2-rosenp@gmail.com?part=1