> From: Cédric Le Goater <[email protected]> > Sent: Tuesday, June 30, 2026 11:50 AM > To: Jian Zhang <[email protected]>; Peter Maydell > <[email protected]>; Steven Lee <[email protected]>; Troy > Lee <[email protected]>; Jamin Lin <[email protected]>; Kane > Chen <[email protected]>; Andrew Jeffery > <[email protected]>; Joel Stanley <[email protected]>; > [email protected]; [email protected] > Subject: Re: [PATCH] hw/intc/aspeed: Drop stale pending interrupts > > > On 6/12/26 08:08, Jian Zhang wrote: > > The Aspeed INTC records an interrupt source in the pending bitmap when > > the source is masked or another status bit is still being handled. > > When the guest later clears the status register, the model promotes > > all saved pending bits back to status unconditionally. > > > > This is not correct for level-triggered sources. A source can > > deassert while another source connected to the same OR gate keeps the > > aggregated INTC line asserted. Promoting the stale bit later makes > > the guest demux a child interrupt whose device status has already been > cleared. > > > > This is visible on AST2700 I2C, where the I2C buses are aggregated > > through INTCIO GICINT194 before reaching the GIC. A stale I2C source > > bit can be promoted back to the INTCIO status register, causing Linux > > to run the corresponding I2C ISR with an empty I2C interrupt status > > register. > > For example, the Linux aspeed-i2c debug ring shows a transfer that > > first receives a valid status interrupt, then receives a spurious ISR > > with both isr and raw status equal to zero. The zero-status ISR > > clears the saved command error and the transfer completes with ret=0: > > > > event=start isr=0x00000000 raw=0x00000000 cmd_err=0 msgs_idx=0 > > event=isr isr=0x00010011 raw=0x00010011 cmd_err=0 msgs_idx=0 > > event=isr isr=0x00000000 raw=0x00000000 cmd_err=1 msgs_idx=1 > > event=complete ret=0 cmd_err=0 msgs_idx=1 > > > > A normal command can then be reported as zero transferred messages, > > which is converted to -EIO by Linux i2c_smbus_xfer_emulated(). The > > race is more likely when multiple I2C buses are accessed concurrently. > > > > Drop pending bits that no longer correspond to an asserted and enabled > > source before they can be promoted back to status. > > > > Signed-off-by: Jian Zhang <[email protected]> > > > This looks correct to me. > > Jamin, could you take a look please ? > > Thanks, > > C. >
Reviewed-by: Jamin Lin <[email protected]> Thanks, Jamin > > > > --- > > hw/intc/aspeed_intc.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index > > 5a36fff5204..ab80c239bc1 100644 > > --- a/hw/intc/aspeed_intc.c > > +++ b/hw/intc/aspeed_intc.c > > @@ -107,6 +107,27 @@ static const AspeedINTCIRQ > *aspeed_intc_get_irq(AspeedINTCClass *aic, > > g_assert_not_reached(); > > } > > > > +static uint32_t aspeed_intc_orgate_levels(AspeedINTCState *s, int > > +inpin_idx) { > > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s); > > + uint32_t levels = 0; > > + int i; > > + > > + for (i = 0; i < aic->num_lines && i < 32; i++) { > > + if (s->orgates[inpin_idx].levels[i]) { > > + levels |= BIT(i); > > + } > > + } > > + > > + return levels; > > +} > > + > > +static void aspeed_intc_drop_stale_pending(AspeedINTCState *s, int > > +inpin_idx) { > > + s->pending[inpin_idx] &= aspeed_intc_orgate_levels(s, inpin_idx) & > > + s->enable[inpin_idx]; } > > + > > /* > > * Update the state of an interrupt controller pin by setting > > * the specified output pin to the given level. > > @@ -231,6 +252,8 @@ static void aspeed_intc_set_irq(void *opaque, int irq, > int level) > > trace_aspeed_intc_set_irq(name, inpin_idx, level); > > enable = s->enable[inpin_idx]; > > > > + aspeed_intc_drop_stale_pending(s, inpin_idx); > > + > > if (!level) { > > return; > > } > > @@ -343,6 +366,7 @@ static void > aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset, > > /* All source ISR execution are done */ > > if (!s->regs[reg]) { > > trace_aspeed_intc_all_isr_done(name, inpin_idx); > > + aspeed_intc_drop_stale_pending(s, inpin_idx); > > if (s->pending[inpin_idx]) { > > /* > > * handle pending source interrupt @@ -402,6 +426,7 @@ > > static void aspeed_intc_status_handler_multi_outpins(AspeedINTCState *s, > > /* All source ISR executions are done from a specific bit */ > > if (data & BIT(i)) { > > trace_aspeed_intc_all_isr_done_bit(name, inpin_idx, i); > > + aspeed_intc_drop_stale_pending(s, inpin_idx); > > if (s->pending[inpin_idx] & BIT(i)) { > > /* > > * Handle pending source interrupt.
