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

[sashiko-bot@kernel.org]

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 <apokusinski01@gmail.com>

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?

[ ... ]

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