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

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 <[email protected]>

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/[email protected]?part=1