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

[Antoni Pokusinski <apokusinski01@gmail.com>]

On Tue, Aug 11, 2026 at 07:56:55PM +0000, sashiko-bot@kernel.org wrote:
> 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 <apokusinski01@gmail.com>
> 
> 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.
> 
> [ ... ]
Not true, reading the registers does not cause a configuration key reset
> > @@ -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.
>
As above, the read does cause configuration key reset.
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260811194034.28937-1-apokusinski01@gmail.com?part=3