RE: [PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the mem_ctl_info on remove

[Ryan Chen]

> Subject: Re: [PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the
> mem_ctl_info on remove
> 
> On Wed, Sep 30, 2026 at 01:15:00PM +0800, Ryan Chen wrote:
> > The ECC interrupt is devm-managed, so it is only released after
> > .remove() has returned, and masking the controller does not wait for a
> > handler already running on another CPU. edac_mc_free() can therefore
> > free the mem_ctl_info the handler uses as its context while it is still running.
> >
> > Fix the ordering and synchronise by freeing the interrupt prior to
> > releasing related memory.
> >
> > Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC
> > driver")
> > Signed-off-by: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>
> >
> > ---
> > Changes in v4:
> > - Drop the Cc: stable tag.
> >
> > Changes in v2:
> > - New patch.
> > ---
> >  drivers/edac/aspeed_edac.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> > index 83d60414f89a..e05ebed5c2f2 100644
> > --- a/drivers/edac/aspeed_edac.c
> > +++ b/drivers/edac/aspeed_edac.c
> > @@ -359,11 +359,16 @@ static int aspeed_probe(struct platform_device
> > *pdev)  static void aspeed_remove(struct platform_device *pdev)  {
> >  	struct mem_ctl_info *mci;
> > +	int irq;
> >
> >  	/* disable interrupts */
> >  	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
> >  			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> >
> > +	irq = platform_get_irq(pdev, 0);
> > +	WARN_ON(irq < 0);
> > +	devm_free_irq(&pdev->dev, irq, platform_get_drvdata(pdev));
> 
> A couple of things come to mind when looking at this:
> 
> - if you have to do platform_get_irq(), then something in the design of this
>   driver is missing. Perhaps a private structure:
> 
>   struct mem_ctl_info.pvt_info
> 
>   which contains driver-specific stuff like the irq number and so on. Look at
>   the other EDAC drivers for examples.

Patch 7 does that - it moves the register base, the lock and the IRQ
number into a struct in mci->pvt_info, and aspeed_remove() uses
priv->irq from there. This patch sits ahead of it, while the driver
still keeps its state in file-scope variables, which is why it reached
for platform_get_irq().


> 
> - then, what is the point of this being a devm-managed IRQ if you have to
>   explicitly free it?
> 
>   By that logic, you might as well use the normal IRQ registration stuff.
> 

Agreed. I will update with following in V5.
The IRQ number goes next to the state the driver already
has and the handler is registered normally, so neither
platform_get_irq() nor devres is involved:

--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -36,6 +36,7 @@
 
 static struct regmap *aspeed_regmap;
+static int aspeed_irq;
 
@@ -333,11 +334,12 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	rc = devm_request_irq(&pdev->dev, irq, mcr_isr, IRQF_TRIGGER_HIGH,
-			      DRV_NAME, ctx);
+	rc = request_irq(irq, mcr_isr, IRQF_TRIGGER_HIGH, DRV_NAME, ctx);
 	if (rc)
 		return rc;
 
+	aspeed_irq = irq;
+
 	/* enable interrupts */
 	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
 			   ASPEED_MCR_INTR_CTRL_ENABLE,
@@ -359,6 +361,8 @@ static void aspeed_remove(struct platform_device *pdev)
 	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
 			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
 
+	free_irq(aspeed_irq, platform_get_drvdata(pdev));
+
 	/* free resources */
 	mci = edac_mc_del_mc(&pdev->dev);
 	if (mci)

config_irq() is the last thing aspeed_probe() does and it returns 0
straight afterwards, so there is no error path for devres to unwind in
the first place. Patch 7 then folds aspeed_irq into the private
structure along with the register base and the lock, and patches 7 and
8 use free_irq(priv->irq, mci).

>   But then looking at the other EDAC drivers, they all do devm_request_irq()
>   but only the DMC-520 one does free_irq().
> 
>   So that would mean that either all the other EDAC drivers need such a fix.
>   And they would all need proper auditing to make sure when they disable
> IRQs,
>   all IRQ handlers running have finished.
> 
>   Or, their hw can handle that and IRQs firing at driver remove time is not an
>   issue...
> 
>   Sounds to me like I need to do a EDAC subsystem TODO for all the driver
>   owners to go read and check...
> 
> So I'm presuming this is Sashiko reporting an issue and you're addressing it.
> Or is it a real issue that someone hits?

Sashiko, on v1 - it flagged the use-after-free as pre-existing and
suggested either freeing the interrupt before the mem_ctl_info or
dropping devm.
Nobody has hit it: I have not seen it and I have no report of one.

Regards
Ryan