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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Read-Modify-Write race condition causes lost hardware interrupts on a Write-0-to-Clear (W0C) register.
- [High] Returning IRQ_NONE for unhandled RTC interrupts causes a hard interrupt storm.
--

--- Patch [3]: [PATCH v2 3/8] rtc: abx80x: properly handle shared IRQs ---
commit 3fccf3aa21cff336c32ceacf1b3d453203dbb5b8
Author: Antoni Pokusinski <apokusinski01@gmail.com>

rtc: abx80x: properly handle shared IRQs

The abx80x RTC driver registers its interrupt handler with IRQF_SHARED,
but previously always returned IRQ_HANDLED and cleared the status register
even when no interrupt source from the RTC was active. This patch updates
the handler to only handle the interrupt when alarm or watchdog flags are
set, clearing only those handled bits. It returns IRQ_NONE when the
interrupt was not generated by the RTC.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index ca3af91c29977..fd9e887bb9c56 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -261,25 +261,32 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)

[ ... ]

> -	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));

[Severity: High]
Does this read-modify-write operation introduce a regression by creating a
race condition that could lose hardware interrupts?

The ABX8XX_REG_STATUS register has Write-0-to-Clear (W0C) semantics, meaning
writing 0 clears a bit and writing 1 preserves it. Because this writes
status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT), it writes 0 to any bit
that was 0 at the time of the earlier i2c_smbus_read_byte_data() call.

If a new status event is asserted by the hardware between the read and the
write, won't this write of 0 unintentionally clear it?

>  
> -	return IRQ_HANDLED;
> +	return handled;
>  }

[Severity: High]
Could returning IRQ_NONE here without clearing the status register cause a
regression where an interrupt storm occurs if an unhandled RTC interrupt fires?

Since abx80x_probe() does not initialize the interrupt mask register
(ABX8XX_REG_IRQ), a bootloader could leave an unknown interrupt enabled. 
If one of these unhandled interrupts fires, handled remains IRQ_NONE.

Because the register is only cleared when handled == IRQ_HANDLED, the
unhandled interrupt is never cleared from the RTC. Returning IRQ_NONE
would then cause the IRQ core to re-trigger the handler in an infinite
loop until the spurious IRQ detector permanently disables the shared IRQ
line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725145718.14733-1-apokusinski01@gmail.com?part=3