| Message ID | 20260811194034.28937-4-apokusinski01@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: abx80x: add support for abx81x | expand |
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. [ ... ] > @@ -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.
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
diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c index fdf9664ebb6f..0c8d3416fd5a 100644 --- a/drivers/rtc/rtc-abx80x.c +++ b/drivers/rtc/rtc-abx80x.c @@ -15,6 +15,7 @@ #include <linux/i2c.h> #include <linux/kstrtox.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/of.h> #include <linux/rtc.h> #include <linux/watchdog.h> @@ -127,6 +128,7 @@ struct abx80x_priv { struct rtc_device *rtc; struct i2c_client *client; struct watchdog_device wdog; + struct mutex lock; }; static int abx80x_write_config_key(struct i2c_client *client, u8 key) @@ -219,6 +221,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm) static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct i2c_client *client = to_i2c_client(dev); + struct abx80x_priv *priv = i2c_get_clientdata(client); unsigned char buf[8]; int err, flags; @@ -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); if (err < 0) { @@ -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; @@ -317,6 +324,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t) static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t) { struct i2c_client *client = to_i2c_client(dev); + struct abx80x_priv *priv = i2c_get_clientdata(client); u8 alarm[6]; int err; @@ -330,6 +338,8 @@ static int abx80x_set_alarm(struct device *dev, struct rtc_wkalrm *t) alarm[4] = bin2bcd(t->time.tm_mday); alarm[5] = bin2bcd(t->time.tm_mon + 1); + guard(mutex)(&priv->lock); + err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_AHTH, sizeof(alarm), alarm); if (err < 0) { @@ -352,6 +362,7 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, int autocalibration) { struct i2c_client *client = to_i2c_client(dev); + struct abx80x_priv *priv = i2c_get_clientdata(client); int retval, flags = 0; if ((autocalibration != 0) && (autocalibration != 1024) && @@ -360,6 +371,8 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, return -EINVAL; } + guard(mutex)(&priv->lock); + flags = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC); if (flags < 0) return flags; @@ -443,6 +456,7 @@ static ssize_t oscillator_store(struct device *dev, const char *buf, size_t count) { struct i2c_client *client = to_i2c_client(dev->parent); + struct abx80x_priv *priv = i2c_get_clientdata(client); int retval, flags, rc_mode = 0; if (strncmp(buf, "rc", 2) == 0) { @@ -454,6 +468,8 @@ static ssize_t oscillator_store(struct device *dev, return -EINVAL; } + guard(mutex)(&priv->lock); + flags = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC); if (flags < 0) return flags; @@ -511,8 +527,11 @@ static const struct attribute_group rtc_calib_attr_group = { static int abx80x_alarm_irq_enable(struct device *dev, unsigned int enabled) { struct i2c_client *client = to_i2c_client(dev); + struct abx80x_priv *priv = i2c_get_clientdata(client); int err; + guard(mutex)(&priv->lock); + if (enabled) err = i2c_smbus_write_byte_data(client, ABX8XX_REG_IRQ, (ABX8XX_IRQ_IM_1_4 | @@ -526,6 +545,7 @@ static int abx80x_alarm_irq_enable(struct device *dev, unsigned int enabled) static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) { struct i2c_client *client = to_i2c_client(dev); + struct abx80x_priv *priv = i2c_get_clientdata(client); int status, tmp; switch (cmd) { @@ -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; - status &= ~ABX8XX_STATUS_BLF; + status &= ~ABX8XX_STATUS_BLF; - tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, - status); - if (tmp < 0) - return tmp; + tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, + status); + if (tmp < 0) + return tmp; + } return 0; @@ -617,6 +639,8 @@ static int __abx80x_wdog_set_timeout(struct watchdog_device *wdog, struct abx80x_priv *priv = watchdog_get_drvdata(wdog); u8 val = ABX8XX_WDT_WDS | timeout_bits(timeout); + guard(mutex)(&priv->lock); + /* * Writing any timeout to the WDT register resets the watchdog timer. * Writing 0 disables it. @@ -701,6 +725,8 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset, len = min(lower + bytes, (size_t)ABX8XX_SRAM_WIN_SIZE) - lower; len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX); + guard(mutex)(&priv->lock); + ret = i2c_smbus_write_byte_data(priv->client, ABX8XX_REG_EXTRAM, extram); if (ret) @@ -908,6 +934,9 @@ static int abx80x_probe(struct i2c_client *client) priv->rtc->ops = &abx80x_rtc_ops; priv->client = client; + err = devm_mutex_init(&client->dev, &priv->lock); + if (err) + return err; i2c_set_clientdata(client, priv);
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. Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com> --- drivers/rtc/rtc-abx80x.c | 45 +++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-)