Re: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes

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

Pre-existing issues:
- [Medium] The patch adds a mutex (`priv->lock`) to protect multi-step write sequences (like configuration key writes followed by register writes) but fails to acquire it during I2C read operations. Because interleaved I2C access resets the hardware configuration key on this RTC chip, concurrent unprotected reads can silently abort active write sequences in other threads.
--

--- Patch [3]: [PATCH v5 3/9] rtc: abx80x: add mutex protection for register writes ---
commit 22bce01eb8bed117145738b82a585f1daa2c3fdc
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

This patch introduces a mutex to serialize multi-step register writes 
on the ABX80X RTC, such as NVMEM transfers and configuration key writes.
It uses scoped guards to protect these sequences from race conditions.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index fdf9664ebb6f0..0c8d3416fd5a9 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -234,6 +237,8 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year - 100);
>  	buf[ABX8XX_REG_WD] = tm->tm_wday;
>  
> +	guard(mutex)(&priv->lock);
> +
>  	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_HTH,
>  					     sizeof(buf), buf);

[Severity: Medium]
This is a pre-existing issue, but do read operations like
abx80x_rtc_read_time() and abx80x_read_alarm() also need to take
priv->lock?

If a process reads the time or alarm concurrently with a configuration key
write sequence (like a sysfs store to the oscillator or autocalibration
attributes), could the read operation interleave and silently reset the
hardware unlock state?

This would cause the subsequent configuration write to fail, since interleaved
I2C access resets the hardware configuration key on this chip.

[ ... ]
> @@ -539,16 +559,18 @@ 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:
> -		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> -		if (status < 0)
> -			return status;
> +		scoped_guard(mutex, &priv->lock) {
> +			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> +			if (status < 0)
> +				return status;

[Severity: Medium]
This is a pre-existing issue, but does the RTC_VL_READ case just above
this block in abx80x_ioctl() also need to acquire the mutex?

Since the read case performs an unprotected I2C read, it could bypass
the lock and abort active locked write sequences in other threads.

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