Re: [PATCH v2 4/8] rtc: abx80x: add irq to struct abx80x_priv

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The interrupt handler indiscriminately clears unrelated and newly arrived interrupts due to a read-modify-write race condition.
--

--- Patch [4]: [PATCH v2 4/8] rtc: abx80x: add irq to struct abx80x_priv ---
commit c9bf7d6c99deb7b828f721f449db552d3b1510a0
Author: Antoni Pokusinski <apokusinski01@gmail.com>

rtc: abx80x: add irq to struct abx80x_priv

This patch adds an irq field to the abx80x_priv struct to decouple the
interrupt number from the i2c_client. This enables the irq to be easily
accessed for both i2c and spi variants. It also updates the interrupt
handler to selectively clear status bits.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5486d9d0b1e5d..b83c789e213d6 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -261,35 +262,43 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
>  	struct i2c_client *client = dev_id;
>  	struct abx80x_priv *priv = i2c_get_clientdata(client);
>  	struct rtc_device *rtc = priv->rtc;
> +	irqreturn_t handled = IRQ_NONE;
>  	int status;
>  
>  	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
>  	if (status < 0)
> -		return IRQ_NONE;
> +		return handled;
>  
> -	if (status & ABX8XX_STATUS_AF)
> +	if (status & ABX8XX_STATUS_AF) {
>  		rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF);
> +		handled = IRQ_HANDLED;
> +	}
>  
>  	/*
>  	 * It is unclear if we'll get an interrupt before the external
>  	 * reset kicks in.
>  	 */
> -	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));

[Severity: High]
Does this status write introduce a race condition that could cause us to lose
newly arrived interrupts?

If a new interrupt (such as a watchdog or alarm event) triggers immediately
after the status register is read, its corresponding hardware bit becomes 1,
but it remains 0 in the local status variable.

When writing back status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT), this
writes 0 to the newly triggered interrupt's bit. Since this chip appears to
use write-0-to-clear semantics, writing 0 clears the new interrupt before it
can be handled, effectively losing the hardware event.

Would it be better to write ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT) directly
to ensure any unhandled bits remain untouched?

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

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