Re: [PATCH v2 6/8] rtc: abx80x: replace read-modify-write pattern with regmap helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware state machine race in Read-Modify-Write on Write-0-To-Clear status registers causes lost interrupts.
- [Medium] Unprotected non-atomic configuration key write allows concurrent operations to silently lock the hardware.
--

--- Patch [6]: [PATCH v2 6/8] rtc: abx80x: replace read-modify-write pattern with regmap helpers ---
commit f4c200ec9260a52df31083ec22d071486d5b6fcb
Author: Antoni Pokusinski <apokusinski01@gmail.com>

rtc: abx80x: replace read-modify-write pattern with regmap helpers
    
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.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 3c5ab0ffbea84..a7686206eb926 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -246,18 +246,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);

[Severity: High]
This is a pre-existing issue, but does using regmap_update_bits() here
create a race condition that could lose hardware interrupts? If a hardware
event triggers between the read and write operations inside
regmap_update_bits() and sets a flag to 1 in the ABX8XX_REG_OSS register,
this write will write back a 0 for that flag since it was 0 when read.
Because the hardware clears flags when a 0 is written, the new interrupt
flag would be cleared before it can be processed.

[ ... ]

> @@ -390,9 +379,9 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
>  	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: Medium]
This isn't a bug introduced by this patch, but does the configuration key
sequence here need to be atomic? The driver unlocks the ABX8XX_REG_OSC
register by writing to ABX8XX_CFG_KEY_OSC. If another thread performs a
concurrent RTC operation that writes to a different register before the
regmap_write_bits() call completes, the hardware resets the configuration key
to 0x00 and the write to ABX8XX_REG_OSC will silently fail.

[ ... ]

> @@ -466,24 +455,14 @@ static ssize_t oscillator_store(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	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));

[Severity: Medium]
This is also a pre-existing issue, but can concurrent operations interrupt
the unlock-and-write sequence in oscillator_store()? If another RTC update
occurs between abx80x_write_config_key() and regmap_write_bits(), the
configuration key will be reset and the oscillator update will be ignored.

[ ... ]

> @@ -550,17 +529,8 @@ 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:
> -		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;
> -
> -		return 0;
> +		return regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS,
> +					  ABX8XX_STATUS_BLF, 0);

[Severity: High]
This is a pre-existing issue, but does performing a read-modify-write on
the ABX8XX_REG_STATUS register risk losing concurrent hardware updates? If an
alarm triggers between the read and write phases and sets the ABX8XX_STATUS_AF
flag to 1, the read-modify-write will write back 0 for that flag. Because this
is a write-0-to-clear register, this clears the alarm interrupt before the
handler can process it.

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