Re: [PATCH 2/6] 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] The shared IRQ handler unconditionally returns IRQ_HANDLED and inadvertently clears all status bits.
- [High] The `abx80x_read_alarm` function fails unconditionally on success due to incorrect error checking.
--

--- Patch [2]: [PATCH 2/6] rtc: abx80x: add irq to struct abx80x_priv ---
commit ccef71986338cc749e8440b88d073968d465ea5c
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add irq to struct abx80x_priv
    
This patch moves the irq number into struct abx80x_priv so that it
is no longer tied to the i2c_client. This allows the irq number to
be accessed in the case of ABX81X SPI clocks.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5486d9d0b1e5d..9b0e1d48d9ba3 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -285,11 +286,12 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
>  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 is a pre-existing issue, but doesn't i2c_smbus_read_i2c_block_data() 
return the number of bytes read on success?

Looking at drivers/rtc/rtc-abx80x.c:abx80x_read_alarm():

	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
					    sizeof(buf), buf);
	if (err)
		return err;

Since it returns 7 on success, will this unconditionally fail and return
early with an error code, leaving the alarm structure uninitialized?

[ ... ]
> @@ -921,19 +925,19 @@ static int abx80x_probe(struct i2c_client *client)
>  	if (err)
>  		return err;
>  
> -	if (client->irq > 0) {
> -		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
> -		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +	if (priv->irq > 0) {
> +		dev_info(&client->dev, "IRQ %d supplied\n", priv->irq);
> +		err = devm_request_threaded_irq(&client->dev, priv->irq, NULL,
>  						abx80x_handle_irq,
>  						IRQF_SHARED | IRQF_ONESHOT,

[Severity: High]
This isn't a bug introduced by this patch, but since the IRQ is registered
here with IRQF_SHARED, does abx80x_handle_irq() correctly handle shared
interrupts?

Looking at drivers/rtc/rtc-abx80x.c:abx80x_handle_irq():

static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
{
	[ ... ]
	status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
	if (status < 0)
		return IRQ_NONE;
	[ ... ]
	i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0);

	return IRQ_HANDLED;
}

If no relevant interrupt flags are set for this device, it still returns
IRQ_HANDLED and writes 0 to ABX8XX_REG_STATUS, clearing all status bits
including non-IRQ bits like BLF. Can this break other devices sharing the
interrupt line and silently drop hardware status flags?

>  						"abx8xx",
>  						client);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2