| Message ID | 20260811194034.28937-8-apokusinski01@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: abx80x: add support for abx81x | expand |
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [High] The patch replaces read-modify-write sequences with `regmap_write_bits()` after writing the hardware configuration key, breaking the write-protection unlock sequence. Pre-existing issues: - [High] The probe function attempts to clear interrupt enable bits (`ABX8XX_OSC_ACIE` and `ABX8XX_OSC_OFIE`) in the protected Oscillator Control Register (`ABX8XX_REG_OSC`) without first writing the required configuration key (`ABX8XX_CFG_KEY_OSC`). -- --- Patch [7]: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers --- commit 6808e36654fbc0a72e9b540a05e0f2557530aa65 Author: Antoni Pokusinski <apokusinski01@gmail.com> rtc: abx80x: replace read-modify-write pattern with regmap helpers This commit replaces read-modify-write sequences with regmap_update_bits() and regmap_write_bits() to simplify register updates. > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c > index 5d41882f3c704..ab88af43b4fa0 100644 > --- a/drivers/rtc/rtc-abx80x.c > +++ b/drivers/rtc/rtc-abx80x.c [ ... ] > @@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, > flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512); > } > > + guard(mutex)(&priv->lock); > + > /* Unlock write access to Oscillator Control Register */ > if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0) > return -EIO; > > - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); > - > - return retval; > + return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, > + ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512, > + flags); > } [Severity: High] Does this change break the configuration key unlock sequence? The configuration key unlock only allows a single I2C write operation. Because regmap_write_bits() internally performs a read-modify-write sequence, it initiates an I2C read transaction before the actual write. Will this inserted read consume the unlock key and cause the subsequent data write to be silently rejected by the hardware? The original code explicitly read the register before unlocking it to avoid this sequence. [ ... ] > @@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev, > > guard(mutex)(&priv->lock); > > - retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags); > - if (retval < 0) > - return retval; > - > - if (rc_mode == 0) > - flags &= ~(ABX8XX_OSC_OSEL); > - else > - flags |= (ABX8XX_OSC_OSEL); > - > /* Unlock write access on Oscillator Control register */ > if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0) > return -EIO; > > - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); > - if (retval < 0) { > + retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL, > + rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL)); > + if (retval < 0) > dev_err(dev, "Failed to write Oscillator Control register\n"); > - return retval; > - } > > return retval ? retval : count; > } [Severity: High] Will this regmap_write_bits() call also fail for the same reason? Since it follows abx80x_write_config_key(), the implicit read operation will likely consume the unlock key before the write occurs. [ ... ] > @@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client) > if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0) > return -EIO; > > - err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL, > - data | ABX8XX_OUT_CTRL_EXDS); > + /* > + * Avoid extra power leakage. The RV1805 uses smaller > + * 10pin package and the EXTI input is not present. > + * Disable it to avoid leakage. > + */ > + err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL, > + ABX8XX_OUT_CTRL_EXDS, > + ABX8XX_OUT_CTRL_EXDS); > if (err < 0) { > dev_err(&client->dev, > "Unable to write output control register\n"); [Severity: High] Similar to the above, does the implicit read inside regmap_write_bits() consume the configuration key unlock for the MISC register before the actual write happens? [ ... ] > - err = regmap_read(regmap, ABX8XX_REG_OSC, &data); > + err = regmap_update_bits(regmap, ABX8XX_REG_IRQ, > + ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E | > + ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0); > if (err < 0) { > - dev_err(&client->dev, "Unable to read Oscillator Control register\n"); > + dev_err(&client->dev, "Unable to update irq register\n"); > return -EIO; > } > > - err = regmap_write(regmap, ABX8XX_REG_OSC, > - data & ~(ABX8XX_OSC_ACIE | > - ABX8XX_OSC_OFIE)); > + err = regmap_update_bits(regmap, ABX8XX_REG_OSC, > + ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0); > if (err < 0) { > - dev_err(&client->dev, "Unable to write Oscillator Control register\n"); > + dev_err(&client->dev, "Unable to update Oscillator Control register\n"); > return -EIO; > } [Severity: High] This is a pre-existing issue, but does this register update require unlocking first? It appears the probe function attempts to clear the interrupt enable bits in the Oscillator Control Register without first writing the required configuration key (ABX8XX_CFG_KEY_OSC). If the configuration key is not written first, will the hardware silently ignore this write, leaving interrupts enabled if they were previously set by the bootloader? If an unhandled interrupt (like Autocalibration or Oscillator Failure) fires, could it cause an interrupt storm? The IRQ handler abx80x_handle_irq() only checks for AF and WDT flags. Since the IRQ is requested with IRQF_SHARED, an unhandled interrupt might result in the kernel disabling the shared IRQ line and breaking unrelated devices.
On Tue, Aug 11, 2026 at 07:56:53PM +0000, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > > New issues: > - [High] The patch replaces read-modify-write sequences with `regmap_write_bits()` after writing the hardware configuration key, breaking the write-protection unlock sequence. > > Pre-existing issues: > - [High] The probe function attempts to clear interrupt enable bits (`ABX8XX_OSC_ACIE` and `ABX8XX_OSC_OFIE`) in the protected Oscillator Control Register (`ABX8XX_REG_OSC`) without first writing the required configuration key (`ABX8XX_CFG_KEY_OSC`). > -- > > --- Patch [7]: [PATCH v5 7/9] rtc: abx80x: replace read-modify-write pattern with regmap helpers --- > commit 6808e36654fbc0a72e9b540a05e0f2557530aa65 > Author: Antoni Pokusinski <apokusinski01@gmail.com> > > rtc: abx80x: replace read-modify-write pattern with regmap helpers > > This commit replaces read-modify-write sequences with regmap_update_bits() > and regmap_write_bits() to simplify register updates. > > > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c > > index 5d41882f3c704..ab88af43b4fa0 100644 > > --- a/drivers/rtc/rtc-abx80x.c > > +++ b/drivers/rtc/rtc-abx80x.c > > [ ... ] > > > @@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, > > flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512); > > } > > > > + guard(mutex)(&priv->lock); > > + > > /* Unlock write access to Oscillator Control Register */ > > if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0) > > return -EIO; > > > > - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); > > - > > - return retval; > > + return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, > > + ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512, > > + flags); > > } > > [Severity: High] > Does this change break the configuration key unlock sequence? > > The configuration key unlock only allows a single I2C write operation. Because > regmap_write_bits() internally performs a read-modify-write sequence, it > initiates an I2C read transaction before the actual write. Will this inserted > read consume the unlock key and cause the subsequent data write to be silently > rejected by the hardware? > > The original code explicitly read the register before unlocking it to avoid > this sequence. > > [ ... ] > No it won't because reads do not cause configuration key reset. > > @@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev, > > > > guard(mutex)(&priv->lock); > > > > - retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags); > > - if (retval < 0) > > - return retval; > > - > > - if (rc_mode == 0) > > - flags &= ~(ABX8XX_OSC_OSEL); > > - else > > - flags |= (ABX8XX_OSC_OSEL); > > - > > /* Unlock write access on Oscillator Control register */ > > if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0) > > return -EIO; > > > > - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); > > - if (retval < 0) { > > + retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL, > > + rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL)); > > + if (retval < 0) > > dev_err(dev, "Failed to write Oscillator Control register\n"); > > - return retval; > > - } > > > > return retval ? retval : count; > > } > > [Severity: High] > Will this regmap_write_bits() call also fail for the same reason? > > Since it follows abx80x_write_config_key(), the implicit read operation will > likely consume the unlock key before the write occurs. > > [ ... ] > No, same reason as above > > @@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client) > > if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0) > > return -EIO; > > > > - err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL, > > - data | ABX8XX_OUT_CTRL_EXDS); > > + /* > > + * Avoid extra power leakage. The RV1805 uses smaller > > + * 10pin package and the EXTI input is not present. > > + * Disable it to avoid leakage. > > + */ > > + err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL, > > + ABX8XX_OUT_CTRL_EXDS, > > + ABX8XX_OUT_CTRL_EXDS); > > if (err < 0) { > > dev_err(&client->dev, > > "Unable to write output control register\n"); > > [Severity: High] > Similar to the above, does the implicit read inside regmap_write_bits() > consume the configuration key unlock for the MISC register before the actual > write happens? > > [ ... ] > No, same reason as above > > - err = regmap_read(regmap, ABX8XX_REG_OSC, &data); > > + err = regmap_update_bits(regmap, ABX8XX_REG_IRQ, > > + ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E | > > + ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0); > > if (err < 0) { > > - dev_err(&client->dev, "Unable to read Oscillator Control register\n"); > > + dev_err(&client->dev, "Unable to update irq register\n"); > > return -EIO; > > } > > > > - err = regmap_write(regmap, ABX8XX_REG_OSC, > > - data & ~(ABX8XX_OSC_ACIE | > > - ABX8XX_OSC_OFIE)); > > + err = regmap_update_bits(regmap, ABX8XX_REG_OSC, > > + ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0); > > if (err < 0) { > > - dev_err(&client->dev, "Unable to write Oscillator Control register\n"); > > + dev_err(&client->dev, "Unable to update Oscillator Control register\n"); > > return -EIO; > > } > > [Severity: High] > This is a pre-existing issue, but does this register update require unlocking > first? > > It appears the probe function attempts to clear the interrupt enable bits in the > Oscillator Control Register without first writing the required configuration key > (ABX8XX_CFG_KEY_OSC). > > If the configuration key is not written first, will the hardware silently ignore > this write, leaving interrupts enabled if they were previously set by the > bootloader? > Yes, will be fixed in v6 > If an unhandled interrupt (like Autocalibration or Oscillator Failure) fires, > could it cause an interrupt storm? The IRQ handler abx80x_handle_irq() only > checks for AF and WDT flags. Since the IRQ is requested with IRQF_SHARED, an > unhandled interrupt might result in the kernel disabling the shared IRQ line > and breaking unrelated devices. > > -- > Sashiko AI review ยท https://sashiko.dev/#/patchset/20260811194034.28937-1-apokusinski01@gmail.com?part=7
diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c index 5d41882f3c70..ab88af43b4fa 100644 --- a/drivers/rtc/rtc-abx80x.c +++ b/drivers/rtc/rtc-abx80x.c @@ -232,7 +232,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct abx80x_priv *priv = dev_get_drvdata(dev); unsigned char buf[8]; - int err, flags; + int err; if (tm->tm_year < 100) return -EINVAL; @@ -256,18 +256,11 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm) } /* Clear the OF bit of Oscillator Status Register */ - err = regmap_read(priv->regmap, ABX8XX_REG_OSS, &flags); + err = regmap_update_bits(priv->regmap, ABX8XX_REG_OSS, ABX8XX_OSS_OF, 0); if (err < 0) - return err; - - err = regmap_write(priv->regmap, ABX8XX_REG_OSS, - flags & ~ABX8XX_OSS_OF); - if (err < 0) { dev_err(dev, "Unable to write oscillator status register\n"); - return err; - } - return 0; + return err; } static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) @@ -378,7 +371,6 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, { struct abx80x_priv *priv = dev_get_drvdata(dev); unsigned int flags = 0; - int retval; if ((autocalibration != 0) && (autocalibration != 1024) && (autocalibration != 512)) { @@ -386,12 +378,6 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, return -EINVAL; } - guard(mutex)(&priv->lock); - - retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags); - if (retval < 0) - return retval; - if (autocalibration == 0) { flags &= ~(ABX8XX_OSC_ACAL_512 | ABX8XX_OSC_ACAL_1024); } else if (autocalibration == 1024) { @@ -403,13 +389,15 @@ static int abx80x_rtc_set_autocalibration(struct device *dev, flags |= (ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512); } + guard(mutex)(&priv->lock); + /* Unlock write access to Oscillator Control Register */ if (abx80x_write_config_key(dev, ABX8XX_CFG_KEY_OSC) < 0) return -EIO; - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); - - return retval; + return regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, + ABX8XX_OSC_ACAL_1024 | ABX8XX_OSC_ACAL_512, + flags); } static int abx80x_rtc_get_autocalibration(struct device *dev) @@ -473,7 +461,6 @@ static ssize_t oscillator_store(struct device *dev, { struct abx80x_priv *priv = dev_get_drvdata(dev->parent); int retval, rc_mode = 0; - unsigned int flags; if (strncmp(buf, "rc", 2) == 0) { rc_mode = 1; @@ -486,24 +473,14 @@ static ssize_t oscillator_store(struct device *dev, guard(mutex)(&priv->lock); - retval = regmap_read(priv->regmap, ABX8XX_REG_OSC, &flags); - if (retval < 0) - return retval; - - if (rc_mode == 0) - flags &= ~(ABX8XX_OSC_OSEL); - else - flags |= (ABX8XX_OSC_OSEL); - /* Unlock write access on Oscillator Control register */ if (abx80x_write_config_key(dev->parent, ABX8XX_CFG_KEY_OSC) < 0) return -EIO; - retval = regmap_write(priv->regmap, ABX8XX_REG_OSC, flags); - if (retval < 0) { + retval = regmap_write_bits(priv->regmap, ABX8XX_REG_OSC, ABX8XX_OSC_OSEL, + rc_mode == 0 ? 0 : (ABX8XX_OSC_OSEL)); + if (retval < 0) dev_err(dev, "Failed to write Oscillator Control register\n"); - return retval; - } return retval ? retval : count; } @@ -573,18 +550,11 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) case RTC_VL_CLR: scoped_guard(mutex, &priv->lock) { - err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status); - if (err < 0) - return err; - - status &= ~ABX8XX_STATUS_BLF; - - err = regmap_write(priv->regmap, ABX8XX_REG_STATUS, status); - if (err < 0) - return err; + err = regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS, + ABX8XX_STATUS_BLF, 0); } - return 0; + return err; default: return -ENOIOCTLCMD; @@ -841,7 +811,6 @@ static int abx80x_probe(struct i2c_client *client) struct device_node *np = client->dev.of_node; struct abx80x_priv *priv; int i, err, trickle_cfg = -EINVAL; - unsigned int data; char buf[7]; unsigned int part = (uintptr_t)i2c_get_match_data(client); unsigned int partnumber; @@ -891,14 +860,9 @@ static int abx80x_probe(struct i2c_client *client) dev_info(&client->dev, "model %04x, revision %u.%u, lot %x, wafer %x, uid %x\n", partnumber, majrev, minrev, lot, wafer, uid); - err = regmap_read(regmap, ABX8XX_REG_CTRL1, &data); - if (err < 0) { - dev_err(&client->dev, "Unable to read control register\n"); - return -EIO; - } - - err = regmap_write(regmap, ABX8XX_REG_CTRL1, - (data & ~(ABX8XX_CTRL_12_24 | ABX8XX_CTRL_ARST)) | ABX8XX_CTRL_WRITE); + err = regmap_update_bits(regmap, ABX8XX_REG_CTRL1, + ABX8XX_CTRL_12_24 | ABX8XX_CTRL_ARST | ABX8XX_CTRL_WRITE, + ABX8XX_CTRL_WRITE); if (err < 0) { dev_err(&client->dev, "Unable to write control register\n"); return -EIO; @@ -912,30 +876,10 @@ static int abx80x_probe(struct i2c_client *client) * register is set. RV-1805-C3 datasheet indicates that * the bit should be cleared in section 11h - Control2. */ - err = regmap_read(regmap, ABX8XX_REG_CTRL2, &data); - if (err < 0) { - dev_err(&client->dev, - "Unable to read control2 register\n"); - return -EIO; - } - - err = regmap_write(regmap, ABX8XX_REG_CTRL2, - data & ~ABX8XX_CTRL2_RSVD); - if (err < 0) { - dev_err(&client->dev, - "Unable to write control2 register\n"); - return -EIO; - } - - /* - * Avoid extra power leakage. The RV1805 uses smaller - * 10pin package and the EXTI input is not present. - * Disable it to avoid leakage. - */ - err = regmap_read(regmap, ABX8XX_REG_OUT_CTRL, &data); + err = regmap_update_bits(regmap, ABX8XX_REG_CTRL2, + ABX8XX_CTRL2_RSVD, 0); if (err < 0) { - dev_err(&client->dev, - "Unable to read output control register\n"); + dev_err(&client->dev, "Unable to write control2 register\n"); return -EIO; } @@ -946,8 +890,14 @@ static int abx80x_probe(struct i2c_client *client) if (abx80x_write_config_key(&client->dev, ABX8XX_CFG_KEY_MISC) < 0) return -EIO; - err = regmap_write(regmap, ABX8XX_REG_OUT_CTRL, - data | ABX8XX_OUT_CTRL_EXDS); + /* + * Avoid extra power leakage. The RV1805 uses smaller + * 10pin package and the EXTI input is not present. + * Disable it to avoid leakage. + */ + err = regmap_write_bits(regmap, ABX8XX_REG_OUT_CTRL, + ABX8XX_OUT_CTRL_EXDS, + ABX8XX_OUT_CTRL_EXDS); if (err < 0) { dev_err(&client->dev, "Unable to write output control register\n"); @@ -998,33 +948,18 @@ static int abx80x_probe(struct i2c_client *client) return err; /* Disable unused interrupts */ - err = regmap_read(regmap, ABX8XX_REG_IRQ, &data); - if (err < 0) { - dev_err(&client->dev, "Unable to read irq register\n"); - return -EIO; - } - - err = regmap_write(regmap, ABX8XX_REG_IRQ, - data & ~(ABX8XX_IRQ_EX1E | - ABX8XX_IRQ_EX2E | - ABX8XX_IRQ_TIE | - ABX8XX_IRQ_BLIE)); - if (err < 0) { - dev_err(&client->dev, "Unable to write irq register\n"); - return -EIO; - } - - err = regmap_read(regmap, ABX8XX_REG_OSC, &data); + err = regmap_update_bits(regmap, ABX8XX_REG_IRQ, + ABX8XX_IRQ_EX1E | ABX8XX_IRQ_EX2E | + ABX8XX_IRQ_TIE | ABX8XX_IRQ_BLIE, 0); if (err < 0) { - dev_err(&client->dev, "Unable to read Oscillator Control register\n"); + dev_err(&client->dev, "Unable to update irq register\n"); return -EIO; } - err = regmap_write(regmap, ABX8XX_REG_OSC, - data & ~(ABX8XX_OSC_ACIE | - ABX8XX_OSC_OFIE)); + err = regmap_update_bits(regmap, ABX8XX_REG_OSC, + ABX8XX_OSC_ACIE | ABX8XX_OSC_OFIE, 0); if (err < 0) { - dev_err(&client->dev, "Unable to write Oscillator Control register\n"); + dev_err(&client->dev, "Unable to update Oscillator Control register\n"); return -EIO; }
Before introducing the regmap usage in the driver, updating specific bits of a register was performed using the read-modify-write pattern. Now, the functions regmap_update_bits() and regmap_write_bits() can be used to simplify the code. Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com> --- drivers/rtc/rtc-abx80x.c | 135 ++++++++++----------------------------- 1 file changed, 35 insertions(+), 100 deletions(-)