Re: [PATCH v2 8/8] EDAC/aspeed: Add AST2700 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning IRQ_NONE for unhandled exclusive DRAMC interrupts relies on the spurious IRQ detector, which permanently disables ECC reporting.
--

--- Patch [8]: [PATCH v2 8/8] EDAC/aspeed: Add AST2700 support ---
commit 0d3da1d609b9b0868952fd656c60d988b1c32e40
Author: Ryan Chen <ryan_chen@aspeedtech.com>

EDAC/aspeed: Add AST2700 support

Add SDRAM ECC reporting for the Aspeed AST2700. Its DRAMC has a different
register layout, a split interrupt status/clear/mask scheme, DDR4/DDR5
memory and interrupt registers that are not key-protected.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index e88d9d2646de7..88b7f5ff37273 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c
[ ... ]
> @@ -180,6 +209,54 @@ static irqreturn_t aspeed_mcr_isr(int irq, void *arg)
[ ... ]
> +static irqreturn_t ast2700_dramc_isr(int irq, void *arg)
> +{
> +	u32 int_sts, ecc_sts, fail_addr;
> +	struct mem_ctl_info *mci = arg;
> +	struct aspeed_edac *priv;
> +	u8 rec_cnt, un_rec_cnt;
> +	phys_addr_t addr;
> +
> +	priv = mci->pvt_info;
> +
> +	scoped_guard(raw_spinlock, &priv->lock) {
> +		int_sts = readl(priv->regs + AST2700_INT_STS);
> +		if (!(int_sts & AST2700_INT_ECC))
> +			return IRQ_NONE;

[Severity: High]
If a non-ECC interrupt is left enabled by the bootloader in the mask
register, will returning IRQ_NONE here cause the kernel's spurious IRQ
detector to permanently disable the interrupt line?

Since the driver requests an exclusive, level-triggered interrupt, an
unhandled asserted line will cause an interrupt storm leading to a disabled
ECC error reporting mechanism.

> +
> +		ecc_sts = readl(priv->regs + AST2700_ECC_STS);
> +		fail_addr = readl(priv->regs + AST2700_ECC_FAIL_ADDR);
> +
> +		/* the interrupt registers are not key-protected; clear only ECC */
> +		writel(int_sts & AST2700_INT_ECC, priv->regs + AST2700_INT_CLR);
> +	}
[ ... ]
> +static void ast2700_set_irq(struct aspeed_edac *priv, bool enable)
> +{
> +	u32 val;
> +
> +	guard(raw_spinlock_irqsave)(&priv->lock);
> +
> +	/* interrupts are enabled by clearing their mask bits */
> +	val = readl(priv->regs + AST2700_INT_MASK);
> +	if (enable)
> +		val &= ~AST2700_INT_ECC;
> +	else
> +		val |= AST2700_INT_ECC;
> +
> +	writel(val, priv->regs + AST2700_INT_MASK);

[Severity: High]
Does this read-modify-write sequence leave non-ECC interrupts unmasked if
they were already enabled by the bootloader?

Taking full ownership of the mask register during initialization, perhaps by
masking all non-ECC sources explicitly instead of doing a read-modify-write,
might prevent the spurious interrupt issue mentioned above.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586@aspeedtech.com?part=8