Re: [PATCH v2 5/8] rtc: abx80x: use regmap instead of I2C specific API

On Sat, Jul 25, 2026 at 03:13:13PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The true error code from devm_regmap_init_i2c is discarded and masked as a hardcoded -EIO.
> 
> Pre-existing issues:
> - [Critical] Missing synchronization in NVMem transfer function leads to SRAM address page corruption.
> - [High] Unprotected read-modify-write sequences on the RTC status register (ABX8XX_REG_STATUS) between process context and the threaded IRQ handler.
> --
> 
> --- Patch [5]: [PATCH v2 5/8] rtc: abx80x: use regmap instead of I2C specific API ---
> commit 5491aafa92a95d35d02d312fb0fd904216ed5a97
> Author: Antoni Pokusinski <[email protected]>
> 
> rtc: abx80x: use regmap instead of I2C specific API
>     
> Rework the driver so that it uses regmap_* functions for communication
> with the device instead of the i2c_smbus_* API. This way the support for
> ABX81X RTCs which use SPI can be added later.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index b83c789e213d6..3c5ab0ffbea84 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> [ ... ]
> > @@ -279,36 +283,35 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> >  	 * reset kicks in.
> >  	 */
> >  	if (status & ABX8XX_STATUS_WDT) {
> > -		dev_alert(&client->dev, "watchdog timeout interrupt.\n");
> > +		dev_alert(dev, "watchdog timeout interrupt.\n");
> >  		handled = IRQ_HANDLED;
> >  	}
> >  
> >  	if (handled == IRQ_HANDLED)
> > -		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
> > -					  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
> > +		regmap_write(priv->regmap, ABX8XX_REG_STATUS,
> > +			     status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
> 
> [Severity: High]
> This is a pre-existing issue, but can concurrent read-modify-write sequences
> on the ABX8XX_REG_STATUS register overwrite each other's changes?
> 
> The threaded IRQ handler abx80x_handle_irq() acknowledges interrupts with an
> unlocked read-modify-write. At the same time, abx80x_ioctl() does an unlocked
> read-modify-write to clear the BLF bit:
> 
> abx80x_ioctl()
>     case RTC_VL_CLR:
>         err = regmap_read(priv->regmap, ABX8XX_REG_STATUS, &status);
>         status &= ~ABX8XX_STATUS_BLF;
>         err = regmap_write(priv->regmap, ABX8XX_REG_STATUS, status);
> 
> If the ioctl races with the threaded IRQ handler, could this lead to lost
> interrupt acknowledgments or lost battery low flags?
> 
> [ ... ]
> > @@ -702,31 +704,27 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
> >  	int ret;
> >  
> >  	while (bytes) {
> > -		u8 extram, reg, len, lower, upper;
> > +		u8 reg, len, lower, upper;
> >  
> >  		lower = FIELD_GET(NVMEM_ADDR_LOWER, offset);
> >  		upper = FIELD_GET(NVMEM_ADDR_UPPER, offset);
> > -		extram = FIELD_PREP(ABX8XX_EXTRAM_XADS, upper);
> >  		reg = ABX8XX_SRAM_BASE + lower;
> >  		len = min(lower + bytes, (size_t)ABX8XX_SRAM_WIN_SIZE) - lower;
> >  		len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
> >  
> > -		ret = i2c_smbus_write_byte_data(priv->client, ABX8XX_REG_EXTRAM,
> > -						extram);
> > +		ret = regmap_update_bits(priv->regmap, ABX8XX_REG_EXTRAM,
> > +					 ABX8XX_EXTRAM_XADS, upper);
> >  		if (ret)
> >  			return ret;
> >  
> >  		if (write) {
> > -			ret = i2c_smbus_write_i2c_block_data(priv->client, reg,
> > -							     len, val);
> > +			ret = regmap_bulk_write(priv->regmap, reg, val, len);
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this missing synchronization lead to
> SRAM address page corruption?
> 
> The NVMem core does not serialize device callbacks, which means two threads
> could access NVMem concurrently. If Thread A updates ABX8XX_REG_EXTRAM to
> select the address page, could Thread B preempt and update the same register
> to a different page before Thread A performs its bulk write?
> 
> Would Thread A then write data to the wrong SRAM page?

Ok so according to the review this driver is missing locking altogether. Most of
the comments from sashiko in this v2 regard some race conditions that
exist here. To sum up, the problems are:
* concurrent accesses to the NVMEM xfer function
* lack of synchronization for any "write_config_key + reg_write" operation 
* concurrent accesses to REG_STATUS

I think that adding a single mutex and proper lock guards all
over the driver code would be sufficient, will do that in v3
in a separate patch.
Perhaps some finer-grained locking would be possible but this 
would complicate things too much in my opinion. 

> 
> [ ... ]
> > @@ -779,6 +809,7 @@ MODULE_DEVICE_TABLE(i2c, abx80x_id);
> >  
> >  static int abx80x_probe(struct i2c_client *client)
> >  {
> > +	struct regmap *regmap;
> >  	struct device_node *np = client->dev.of_node;
> >  	struct abx80x_priv *priv;
> >  	int i, data, err, trickle_cfg = -EINVAL;
> > @@ -793,8 +824,27 @@ static int abx80x_probe(struct i2c_client *client)
> >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> >  		return -ENODEV;
> >  
> > -	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ID0,
> > -					    sizeof(buf), buf);
> > +	regmap = devm_regmap_init_i2c(client, &abx80x_regmap_config_i2c);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&client->dev, "Unable to allocate regmap\n");
> > +		return -EIO;
> > +	}
> 
> [Severity: Medium]
> Should this code return the actual error code from devm_regmap_init_i2c() 
> (such as -ENOMEM or -ENODEV) via PTR_ERR(regmap) instead of discarding it 
> for a hardcoded -EIO?
> 
> Masking the original error code could make debugging initialization failures 
> more difficult.
Yes, there should be PTR_ERR() in here, same situation as with the devm_regmap_init_spi()
in PATCH 8.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5