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

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@aspeedtech.com>

---
Changes in v4:
- Drop the Cc: stable tag.

Changes in v2:
- New patch.
---
 drivers/edac/aspeed_edac.c | 5 +++++
 1 file changed, 5 insertions(+)

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@aspeedtech.com>
> 
> ---
> 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.

- 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.

  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?

Thx.

> 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@aspeedtech.com>
> >
> > ---
> > 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