Re: [PATCH] EDAC: fsl_ddr: use devm_platform_ioremap_resource()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware registers altered during initialization are not restored on probe error paths.
- [High] Use-after-free of `mci` and `pdata` in the shared interrupt handler during device removal.
--

--- Patch [1]: [PATCH] EDAC: fsl_ddr: use devm_platform_ioremap_resource() ---
commit 95d86798c0c6ff810ee7e3129ccd0c259af42fed
Author: Rosen Penev <rosenp@gmail.com>

EDAC: fsl_ddr: use devm_platform_ioremap_resource()
    
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().

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index e4eaec0aa81d5..f19ccef942c02 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c

[ ... ]

> @@ -532,28 +535,7 @@ 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;

[Severity: High]
This is a pre-existing issue, but since we are modifying error paths in
fsl_mc_err_probe(), does the error handling here fail to restore hardware
registers?

Later in the probe function, hardware state is saved and error interrupts
are enabled by clearing FSL_MC_ERR_DISABLE and setting FSL_MC_ERR_INT_EN.

If a subsequent initialization step fails and jumps to the err or err2
labels:

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

These paths clean up driver memory but do not restore the modified hardware
registers. Since the memory controller is left configured to generate
interrupts but no handler is successfully registered, could a subsequent
memory error trigger an unhandled interrupt storm on the shared IRQ line?

> -	}
> -
> -	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");

[Severity: High]
This isn't a bug introduced by this patch, but could there be a
use-after-free race condition during device removal?

In fsl_mc_err_probe(), the interrupt is registered as shared using the
managed API devm_request_irq(), passing mci as the context data.

During device removal:

drivers/edac/fsl_ddr_edac.c:fsl_mc_err_remove() {
    ...
    edac_mc_del_mc(&op->dev);
    edac_mc_free(mci);
}

Since devres_release_all() runs after fsl_mc_err_remove() completes, the
shared IRQ remains active while mci is explicitly freed here.

If another device sharing the IRQ line triggers an interrupt during this
window, fsl_mc_isr() will dereference the freed mci and pdata pointers.
Should the IRQ be freed explicitly before freeing mci, or should mci be
managed by devres as well?

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