Re: [PATCH v3 3/8] rtc: abx80x: properly handle shared IRQs

On Fri, Jul 31, 2026 at 07:05:22PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Returning IRQ_NONE without masking or clearing unhandled RTC interrupts can cause an interrupt storm on the shared IRQ line.
> --
> 
> --- Patch [3]: [PATCH v3 3/8] rtc: abx80x: properly handle shared IRQs ---
> commit e06155ed4aeb662277df57a2324e881c6d065e84
> Author: Antoni Pokusinski <[email protected]>
> 
> rtc: abx80x: properly handle shared IRQs
> 
> This commit modifies the abx80x interrupt handler to only claim the IRQ and
> clear the status register if the alarm or watchdog flags are explicitly set.
> It aims to prevent claiming interrupts from other devices sharing the same
> IRQ line.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> 
> [ ... ]
> 
> > @@ -266,27 +266,34 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> 
> [ ... ]
> 
> > -	if (status & ABX8XX_STATUS_WDT)
> > +	if (status & ABX8XX_STATUS_WDT) {
> >  		dev_alert(&client->dev, "watchdog timeout interrupt.\n");
> > +		handled = IRQ_HANDLED;
> > +	}
> >  
> > -	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0);
> > +	if (handled == IRQ_HANDLED)
> > +		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
> > +					  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
> >  
> > -	return IRQ_HANDLED;
> > +	return handled;
> 
> [Severity: High]
> If the RTC hardware asserts an interrupt for a reason other than AF or WDT
> (for example, a timer or century interrupt left enabled by a bootloader),
> will this cause an interrupt storm?
> 
> Because handled remains IRQ_NONE, the handler skips the i2c_smbus_write_byte_data()
> call and returns IRQ_NONE without clearing the unhandled RTC status bits.
> 
> Since the unhandled RTC interrupt is never cleared, the IRQ line will remain
> asserted. This causes the kernel to immediately re-enter the handler until
> the spurious IRQ detector steps in and completely disables the shared IRQ line.
> 
> Could we check if the status register indicates the interrupt originated
> from this RTC, clear those bits, and return IRQ_HANDLED?
Ok so it turns out that neither the current nor the newly proposed interrupt
handling is correct.

The current approach is that we write 0 to REG_STATUS so we clear
e.g. the BL flag which prevents the userspace from detecting a low
battery. Also, IRQ_HANDLED is returned unconditionally which prevents
handling a potential interrupt from another shared device.

The new approach that I've added in this v3 is not ok neither
since (as sashiko points out) any interrupt other than AF or WDT may be
enabled e.g. by the bootloader and in this case we'd get an interrupt
storm.

In v4 I'll improve the new solution with explicitly disabling the
interrupts that are not used in the probe function. That is:
* disable EX1, EX2, Timer, BL interrupts in REG_IRQ
* disable "oscillator fail" and "autocalibration fail" interrupts in REG_OSC
> 
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3