Re: [PATCH 5/7] EDAC/aspeed: Replace regmap with direct register access

[sashiko-bot@kernel.org]

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

New issues:
- [High] Deadlock and lockdep splat due to acquiring the global `aspeed_lock` in process context without disabling interrupts.
- [High] Writing back a stale cached register value restores previously cleared interrupt counters, risking an interrupt storm.
- [Medium] Unbounded scheduling latency caused by calling `dev_dbg()` within a `raw_spinlock` critical section.
- [Medium] Bypassed lock annotation and unguarded hardware access via local pointer alias.

Pre-existing issues:
- [High] Missing IRQ synchronization in `aspeed_remove()` leads to a Use-After-Free of the `mci` structure.
--

--- Patch [5]: [PATCH 5/7] EDAC/aspeed: Replace regmap with direct register access ---
commit 4694a0001d51ca262e162c7af74ae2a61ea5c379
Author: Ryan Chen <ryan_chen@aspeedtech.com>
Date:   Wed Aug 12 05:48:29 2026 +0000

    EDAC/aspeed: Replace regmap with direct register access
    
    The driver instantiates its own regmap purely as an MMIO wrapper: it has
    no register cache, uses custom reg_read/reg_write callbacks, and is not
    shared as a syscon with other drivers. So it brings nothing here beyond
    the spinlock that regmap takes around each access when fast_io is set.
    
    Drop the regmap and access the registers directly with readl()/writel()
    under an explicit raw spinlock, held across the whole read-modify-write so
    the controller is unlocked once around the grouped writes rather than on
    every register write. The lock is a raw_spinlock_t because the ECC
    interrupt handler runs in hardirq context, where under PREEMPT_RT a
    sleeping spinlock could not be acquired.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 8bfeb21d3204a..7bd552ee9a612 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c

[ ... ]

> @@ -147,12 +100,24 @@ static irqreturn_t mcr_isr(int irq, void *arg)
>  {
>  	struct mem_ctl_info *mci = arg;
>  	u32 rec_addr, un_rec_addr;
> -	u32 reg50, reg5c, reg58;
> -	u8  rec_cnt, un_rec_cnt;
> -
> -	regmap_read(aspeed_regmap, ASPEED_MCR_INTR_CTRL, &reg50);
> -	dev_dbg(mci->pdev, "received edac interrupt w/ mcr register 50: 0x%x\n",
> -		reg50);
> +	u8 rec_cnt, un_rec_cnt;
> +	u32 reg50;
> +
> +	scoped_guard(raw_spinlock, &aspeed_lock) {
> +		reg50 = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
> +		dev_dbg(mci->pdev, "received edac interrupt w/ mcr register 50: 0x%x\n",
> +			reg50);

[Severity: Medium]
Is it safe to call dev_dbg() while holding a raw_spinlock?

Printk-family functions can take complex internal locks or cause heavy console
I/O. Since the lock was changed to a raw_spinlock_t to allow the handler to
run in hardirq context under PREEMPT_RT, could calling dev_dbg() here cause
massive latency spikes?

> +		un_rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_UNREC);
> +		rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_REC);
> +
> +		/* clearing the counters needs a set-then-clear of CLEAR */
> +		writel(ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
> +		writel(reg50 | ASPEED_MCR_INTR_CTRL_CLEAR,
> +		       aspeed_regs + ASPEED_MCR_INTR_CTRL);
> +		writel(reg50 & ~ASPEED_MCR_INTR_CTRL_CLEAR,
> +		       aspeed_regs + ASPEED_MCR_INTR_CTRL);

[Severity: High]
Will this second write restore the previously cleared error counters?

The hardware clears error counters when the CLEAR bit is written to 1. The
original regmap sequence read the hardware twice, so it correctly wrote back 0
for the counters.

Here, we reuse the initial cached read (reg50), which contains the non-zero
counters. If the counter bits are R/W, does writing back (reg50 & ~CLEAR)
restore the counters to their previous non-zero values and immediately
retrigger the interrupt?

[ ... ]

> @@ -246,7 +213,8 @@ 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;
>  
> -	regmap_read(aspeed_regmap, ASPEED_MCR_CONF, &reg04);
> +	scoped_guard(raw_spinlock, &aspeed_lock)
> +		reg04 = readl(aspeed_regs + ASPEED_MCR_CONF);

[Severity: High]
Can this cause a deadlock and lockdep splat when the interrupt fires?

The global aspeed_lock is acquired in hardirq context by mcr_isr(). However,
init_csrows() acquires this lock in process context using
scoped_guard(raw_spinlock), which leaves local interrupts enabled.

If an interrupt fires while the process is holding the lock, wouldn't the
ISR spin forever waiting for the lock on the same CPU? Should this use the
irqsave variant instead?

[ ... ]

> @@ -274,13 +241,11 @@ static int aspeed_probe(struct platform_device *pdev)
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
>  
> -	aspeed_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> -					 &aspeed_regmap_config);
> -	if (IS_ERR(aspeed_regmap))
> -		return PTR_ERR(aspeed_regmap);
> +	scoped_guard(raw_spinlock, &aspeed_lock)
> +		aspeed_regs = regs;

[Severity: High]
Similar to init_csrows() above, does this need to use the irqsave variant to
avoid deadlocking if the interrupt fires?

>  
>  	/* bail out if ECC mode is not configured */
> -	regmap_read(aspeed_regmap, ASPEED_MCR_CONF, &reg04);
> +	reg04 = readl(regs + ASPEED_MCR_CONF);

[Severity: Medium]
Does this bypass the lock annotation and unguarded hardware access checks?

The commit introduces a __guarded_by(&aspeed_lock) annotation on aspeed_regs.
However, accessing ASPEED_MCR_CONF using the unannotated local regs pointer
without holding the lock circumvents the intended static analysis safety check
and drops the synchronization.

[ ... ]

> @@ -346,8 +311,7 @@ static void aspeed_remove(struct platform_device *pdev)
>  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
>  
>  	/* disable interrupts */
> -	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
> -			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> +	aspeed_set_irq(false);
>  
>  	/* free resources */
>  	edac_mc_del_mc(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does missing IRQ synchronization in
aspeed_remove() lead to a use-after-free of the mci structure?

The driver uses devm_request_irq(), meaning the interrupt is unregistered
and synchronized automatically by devres only after aspeed_remove() returns.
Although aspeed_set_irq(false) disables interrupts at the hardware level,
it does not wait for an already-running ISR to complete.

Could edac_mc_free(mci) (called immediately after edac_mc_del_mc) free the
structure while mcr_isr() is concurrently executing on another CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-edac-v1-0-03992edea297@aspeedtech.com?part=5