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

[sashiko-bot@kernel.org]

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 <apokusinski01@gmail.com>

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?

[ ... ]
> @@ -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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725145718.14733-1-apokusinski01@gmail.com?part=5