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

On Fri, Jul 31, 2026 at 07:01:37PM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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));
>     ...
> }
Same answer as in patch 2 ("rtc: abx80x: add mutex protection for register writes"):
register reads do not cause a reset of the configuration key, so no
issue here.
> 
> [ ... ]
> > @@ -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/[email protected]?part=6