Re: [PATCH v3 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:
- [High] Using `guard(mutex)` inside a `switch` statement case without an enclosing block causes a compilation failure (or uninitialized cleanup execution).
- [High] Configuration Key unlock sequence is broken by unprotected concurrent reads and newly introduced read-modify-write regmap helpers.
--

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

rtc: abx80x: replace read-modify-write pattern with regmap helpers

This commit replaces manual read-modify-write register operations in the 
rtc-abx80x driver with regmap_update_bits() and regmap_write_bits() 
functions to simplify the code.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 23823fc2f5458..52f958e2aecd2 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -371,7 +364,7 @@ static int abx80x_rtc_set_autocalibration(struct device *dev,
>  					  int autocalibration)
>  {
[ ... ]
>  	/* 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 switching to regmap_write_bits() break the configuration key unlock
sequence here?

The hardware requires writing the configuration key immediately before 
writing a protected register. If any other register is accessed in between, 
the unlock state is cleared. 

Because regmap_write_bits() performs a read operation internally before 
writing, does that read happen after the configuration key is written,
causing the hardware to relock itself before the write can execute?

Additionally, can an unlocked concurrent read sequence interleave here and
clear the unlock state? Looking at abx80x_rtc_read_time(), it performs I2C 
reads without taking priv->lock:

abx80x_rtc_read_time() {
    ...
    err = regmap_bulk_read(priv->regmap, ABX8XX_REG_HTH, buf, sizeof(buf));
    ...
}

[ ... ]
> @@ -566,17 +545,8 @@ static int abx80x_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
>  	case RTC_VL_CLR:
>  		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;
> -
> -		return 0;
> +		return regmap_update_bits(priv->regmap, ABX8XX_REG_STATUS,
> +					  ABX8XX_STATUS_BLF, 0);
>  
>  	default:
>  		return -ENOIOCTLCMD;
>  	}

[Severity: High]
Will this cause a compiler error or undefined behavior?

The guard(mutex) is declared inside the case block, but without an 
enclosing scope. If cmd is an unhandled ioctl, the switch statement jumps 
to the default label, bypassing the initialization of the guard(mutex) 
variable. 

Does this require adding braces around the RTC_VL_CLR case to restrict the 
lifetime of the guard?

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