Re: [PATCH v4 4/9] rtc: abx80x: properly handle shared IRQs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe read-modify-write on the W0C status register clears unhandled and newly asserted interrupts.
- [High] The shared interrupt handler incorrectly claims interrupts without verifying if the alarm interrupt is enabled, risking interrupt storms or lost IRQs for other devices.
--

--- Patch [4]: [PATCH v4 4/9] rtc: abx80x: properly handle shared IRQs ---
commit 973a5c08e9e5b0f99ad16fb217339890d12578f8
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: properly handle shared IRQs

The abx80x RTC driver registers its interrupt handler with IRQF_SHARED,
but the handler always returned IRQ_HANDLED and cleared the status
register even when no interrupt source from the RTC was active.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 0c8d3416fd5a9..485d1ea08030d 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -266,27 +272,34 @@ 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;
>  
>  	guard(mutex)(&priv->lock);
>  
>  	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;
> +	}

[Severity: High]
This is a pre-existing issue, but since this patch aims to properly handle
shared IRQs, should this also check if the Alarm Interrupt Enable (AIE)
bit is set?

In many RTCs, the Alarm Flag (AF) is set upon a match regardless of whether
the interrupt is actually enabled. If AIE is disabled but AF is set, could
this incorrectly assume the RTC generated the interrupt, claim it from
other devices sharing the IRQ line, and emit a spurious RTC alarm event?

>  
>  	/*
>  	 * 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]
This isn't a bug introduced by this patch, as the original code wrote 0 to
clear all bits, but does this new read-modify-write operation still leave a
race condition for write-0-to-clear (W0C) registers?

If an unhandled interrupt asserts between reading ABX8XX_REG_STATUS and
writing it back, wouldn't writing back the read status (where the new
interrupt's bit was 0) write a 0 to that newly asserted bit, inadvertently
clearing it before it can be handled?

Additionally, does status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT)
unconditionally write 0 to the WDT bit, even if only the AF bit was
handled?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4