Re: [PATCH v3 2/8] rtc: abx80x: add mutex protection for register writes

On Fri, Jul 31, 2026 at 07:02:55PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Low] Build failure due to jumping over a `__cleanup__` variable declaration in a switch statement.
> 
> Pre-existing issues:
> - [High] Shared IRQ handler unconditionally returns IRQ_HANDLED and clears the status register, risking interrupt storms and breaking battery low detection.
> - [High] Missing mutex protection on I2C reads allows them to interleave with and silently break multi-step configuration key write sequences.
> --
> 
> --- Patch [2]: [PATCH v3 2/8] rtc: abx80x: add mutex protection for register writes ---
> commit d3bc95e6c97e9f4f1dbb461f4986e984a4ccd25a
> Author: Antoni Pokusinski <[email protected]>
> 
> rtc: abx80x: add mutex protection for register writes
> 
> The ABX80X RTC driver performs multi-step register operations such as
> NVMEM transfers or register writes preceded by a configuration key
> write. Add a mutex to serialize all the register writes to protect
> these sequences against race conditions.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index 5486d9d0b1e5d..8d791b9ec7d21 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> [ ... ]
> > @@ -219,6 +221,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> 
> [Severity: High]
> This is a pre-existing issue, but should I2C reads like those performed in
> abx80x_rtc_read_time() also be protected by the new mutex?
> 
> If an unprotected I2C read interleaves with a protected multi-step write
> (such as abx80x_rtc_set_autocalibration()), it can reset the configuration
> key on the hardware. This would cause the target register write to be
> silently ignored.
>
Not true. The reset of the configuration key happens on any register write
but on on read.
> >  static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> [ ... ]
> > @@ -263,6 +268,8 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> >  	struct rtc_device *rtc = priv->rtc;
> >  	int status;
> >  
> > +	guard(mutex)(&priv->lock);
> > +
> >  	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> >  	if (status < 0)
> >  		return IRQ_NONE;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this interrupt handler
> unconditionally clear the status register and return IRQ_HANDLED later in
> the function?
> 
> If a shared device or spurious interrupt triggers abx80x_handle_irq(),
> returning IRQ_HANDLED for unrelated interrupts prevents the core IRQ
> subsystem from disabling a stuck shared line.
> 
> Additionally, writing 0 unconditionally clears all flags, including
> ABX8XX_STATUS_BLF, which prevents userspace from detecting a low battery.
>
> [ ... ]
> > @@ -539,6 +559,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:
> > +		guard(mutex)(&priv->lock);
> > +
> 
> [Severity: Low]
> Does this cause a build failure?
> 
> The guard macro declares a cleanup variable inside the switch statement
> without a surrounding compound block. Any other ioctl command jumping to the
> default label will bypass the variable's initialization, which GCC usually
> flags as a hard error.
> 
Even though I didn't experience any issue, this review seems valid to
me. I'd change the guard() here to mutex_lock/unlock in v4
> >  		status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> >  		if (status < 0)
> >  			return status;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2