Re: [PATCH v3 2/8] rtc: abx80x: add mutex protection for register writes

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Build failure due to jumping over a `__cleanup__` variable declaration in a switch statement.

Pre-existing issues:
- [High] Shared IRQ handler unconditionally returns IRQ_HANDLED and clears the status register, risking interrupt storms and breaking battery low detection.
- [High] Missing mutex protection on I2C reads allows them to interleave with and silently break multi-step configuration key write sequences.
--

--- Patch [2]: [PATCH v3 2/8] rtc: abx80x: add mutex protection for register writes ---
commit d3bc95e6c97e9f4f1dbb461f4986e984a4ccd25a
Author: Antoni Pokusinski <apokusinski01@gmail.com>

rtc: abx80x: add mutex protection for register writes

The ABX80X RTC driver performs multi-step register operations such as
NVMEM transfers or register writes preceded by a configuration key
write. Add a mutex to serialize all the register writes to protect
these sequences against race conditions.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5486d9d0b1e5d..8d791b9ec7d21 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -219,6 +221,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)

[Severity: High]
This is a pre-existing issue, but should I2C reads like those performed in
abx80x_rtc_read_time() also be protected by the new mutex?

If an unprotected I2C read interleaves with a protected multi-step write
(such as abx80x_rtc_set_autocalibration()), it can reset the configuration
key on the hardware. This would cause the target register write to be
silently ignored.

>  static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
[ ... ]
> @@ -263,6 +268,8 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
>  	struct rtc_device *rtc = priv->rtc;
>  	int status;
>  
> +	guard(mutex)(&priv->lock);
> +
>  	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
>  	if (status < 0)
>  		return IRQ_NONE;

[Severity: High]
This isn't a bug introduced by this patch, but does this interrupt handler
unconditionally clear the status register and return IRQ_HANDLED later in
the function?

If a shared device or spurious interrupt triggers abx80x_handle_irq(),
returning IRQ_HANDLED for unrelated interrupts prevents the core IRQ
subsystem from disabling a stuck shared line.

Additionally, writing 0 unconditionally clears all flags, including
ABX8XX_STATUS_BLF, which prevents userspace from detecting a low battery.

[ ... ]
> @@ -539,6 +559,8 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
>  		return put_user(tmp, (unsigned int __user *)arg);
>  
>  	case RTC_VL_CLR:
> +		guard(mutex)(&priv->lock);
> +

[Severity: Low]
Does this cause a build failure?

The guard macro declares a cleanup variable inside the switch statement
without a surrounding compound block. Any other ioctl command jumping to the
default label will bypass the variable's initialization, which GCC usually
flags as a hard error.

>  		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
>  		if (status < 0)
>  			return status;

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