Re: [PATCH 6/7] EDAC/aspeed: Abstract SoC differences behind chip data

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

Pre-existing issues:
- [High] The `mci` structure is freed before the devm-managed interrupt handler is unregistered, leading to a Use-After-Free.
- [High] Lockdep splat due to acquiring `priv->lock` with interrupts enabled.
--

--- Patch [6]: [PATCH 6/7] EDAC/aspeed: Abstract SoC differences behind chip data ---
commit 95a7cd8a39c90d2b48dfabaca16f91703e3c872b
Author: Ryan Chen <[email protected]>

EDAC/aspeed: Abstract SoC differences behind chip data

The driver hard-codes the AST2400/2500/2600 register layout, ECC and
DRAM-type bits, memory types and write-protection key. Abstract these
SoC-specific details behind a per-SoC struct aspeed_edac_chip selected by
the compatible, and move the per-instance state (register base, lock)
into mci->pvt_info instead of globals.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 7bd552ee9a612..91df5d2df5f11 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c
[ ... ]
> @@ -213,12 +249,12 @@ static int init_csrows(struct mem_ctl_info *mci)
>  	nr_pages = resource_size(&r) >> PAGE_SHIFT;
>  	csrow->last_page = csrow->first_page + nr_pages - 1;
>  
> -	scoped_guard(raw_spinlock, &aspeed_lock)
> -		reg04 = readl(aspeed_regs + ASPEED_MCR_CONF);
> -	dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
> +	scoped_guard(raw_spinlock, &priv->lock)
> +		conf = readl(priv->regs + priv->chip->conf_reg);

[Severity: High]
This isn't a bug introduced by this patch, but could this lock acquisition
cause a lockdep violation?

This lock is acquired in hardirq context by aspeed_mcr_isr(). By using
scoped_guard(raw_spinlock, ...) here in init_csrows(), the lock is acquired
with interrupts enabled in process context.

Does acquiring the same lock both in hardirq context and with interrupts
enabled trigger a lockdep warning?

> +	type = field_get(priv->chip->conf_dram_type, conf);
>  
>  	dimm = csrow->channels[0]->dimm;
> -	dimm->mtype = dram_type;
> +	dimm->mtype = priv->chip->dram_type[type];
>  	dimm->edac_mode = EDAC_SECDED;
[ ... ]
> @@ -311,17 +356,36 @@ static void aspeed_remove(struct platform_device *pdev)
>  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
>  
>  	/* disable interrupts */
> -	aspeed_set_irq(false);
> +	aspeed_set_irq(mci, false);
>  
>  	/* free resources */
>  	edac_mc_del_mc(&pdev->dev);
>  	edac_mc_free(mci);

[Severity: High]
This is a pre-existing issue, but does this devres-managed IRQ ordering cause
a use-after-free here?

Because the interrupt is registered with devm_request_irq() during probe, it
is not actually unregistered and synchronized until the devres cleanup phase,
which happens after aspeed_remove() returns.

If an interrupt handler is running concurrently on another CPU during removal,
could it dereference mci and mci->pvt_info after edac_mc_free(mci) has freed
them?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6