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

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

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