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

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 conditi=
on that loses interrupts.
- [High] Unconditional failure in `abx80x_read_alarm` due to incorrect hand=
ling 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
   =20
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 =3D 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 =3D 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 =3D to_i2c_client(dev);
> +	struct abx80x_priv *priv =3D i2c_get_clientdata(client);
>  	unsigned char buf[7];
> =20
>  	int irq_mask, err;
> =20
> -	if (client->irq <=3D 0)
> +	if (priv->irq <=3D 0)
>  		return -EINVAL;
> =20
>  	err =3D 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 =3D 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?

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260731184831.4403=
[email protected]?part=3D4