Re: [PATCH v5 9/9] rtc: abx80x: add support for ABX81X

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

Pre-existing issues:
- [High] Read-modify-write (RMW) operations on the W0C (write-zero-to-clear) status register cause lost interrupts.
--

--- Patch [9]: [PATCH v5 9/9] rtc: abx80x: add support for ABX81X ---
commit de21e840f75a86e42ad746c99077b6acf5f3e1fb
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add support for ABX81X
    
Abracon ABX81X is a family of RTCs with SPI interface featuring
the same functionalities as the ABX80X I2C clocks.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 08b22c0676995..7145de876b50b 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * A driver for the I2C members of the Abracon AB x8xx RTC family,
> - * and compatible: AB 1805 and AB 0805
> + * A driver for the I2C and SPI members of the Abracon AB x8xx RTC family,
> + * and compatible: AB 1805, AB 0805, AB 1815 and AB 0815

[ ... ]

While reviewing this patch, I noticed a pre-existing issue in the driver
that might cause lost interrupts.

drivers/rtc/rtc-abx80x.c:abx80x_handle_irq() {
[ ... ]
	if (handled == IRQ_HANDLED)
		regmap_write(priv->regmap, ABX8XX_REG_STATUS,
			     status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but does this write inadvertently
clear newly-asserted interrupts?

The ABX8XX_REG_STATUS register uses a 'Write 0 to Clear' (W0C) mechanism,
where writing 0 clears a bit and writing 1 preserves it. Because the code
bitwise-ANDs the previously read status, it writes 0 to any bit that was
0 during the read.

If a hardware event asserts a flag between the read and this write, the
write of 0 will clear the new event before it is handled.

[ ... ]

drivers/rtc/rtc-abx80x.c:abx80x_ioctl() {
[ ... ]
	case RTC_VL_CLR:
		scoped_guard(mutex, &priv->lock) {
			err = regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS,
						 ABX8XX_STATUS_BLF, 0);
		}
[ ... ]
}

[Severity: High]
This is also a pre-existing issue, but does this read-modify-write operation
expose all other W0C flags to the exact same race condition?

Using regmap_update_bits to clear the BLF bit performs a read-modify-write
and writes 0 to all other unasserted bits, which could cause them to be lost
if they were asserted right after the read phase.

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