Re: [PATCH v3 4/8] rtc: abx80x: add irq to struct abx80x_priv

On Fri, Jul 31, 2026 at 07:04:49PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Read-modify-write of the status register introduces a race condition that loses interrupts.
> - [High] Unconditional failure in `abx80x_read_alarm` due to incorrect handling of `i2c_smbus_read_i2c_block_data` return value.
> --
> 
> --- Patch [4]: [PATCH v3 4/8] rtc: abx80x: add irq to struct abx80x_priv ---
> commit be354d8ef94de08e1314876fad55888243ca019f
> Author: Antoni Pokusinski <[email protected]>
> 
> rtc: abx80x: add irq to struct abx80x_priv
>     
> This adds the irq field to struct abx80x_priv so the interrupt number is not
> strictly tied to the i2c_client structure. This allows it to be more easily
> accessed for ABX81X SPI clocks.
> 
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> > @@ -299,11 +300,12 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential race condition with
> the status register.
> 
> In abx80x_handle_irq(), there is a read-modify-write sequence:
> 
> 	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
> 	...
> 	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
> 				  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));
> 
> If a hardware interrupt (such as an RTC alarm or watchdog) sets a new flag
> in ABX8XX_REG_STATUS exactly between the read and write calls, wouldn't the
> write operation write back a 0 for the newly set bit and inadvertently clear
> it without handling it?
> 
> This same pattern appears to be present in abx80x_ioctl() as well:
> 
> 	tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
> 					status);
> 
> Could this cause hardware interrupts or status flags to be silently dropped?
> 
> >  static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > +	struct abx80x_priv *priv = i2c_get_clientdata(client);
> >  	unsigned char buf[7];
> >  
> >  	int irq_mask, err;
> >  
> > -	if (client->irq <= 0)
> > +	if (priv->irq <= 0)
> >  		return -EINVAL;
> >  
> >  	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this error check work
> as intended?
> 
> i2c_smbus_read_i2c_block_data() returns the number of bytes read (which
> would be 7) on success.
> 
> Looking at the code just below this in abx80x_read_alarm():
> 
> 	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
> 					    sizeof(buf), buf);
> 	if (err)
> 		return err;
> 
> Wouldn't this evaluate to true on a successful read, causing the function
> to prematurely abort and pass the byte count back as an error code?
Will add a separate patch to fix that in v4 as it may be useful to
backport it to stable releases 
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4