Re: [PATCH v5 5/9] rtc: abx80x: add irq to struct abx80x_priv

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Failure to clear client->irq on IRQ request error leaves a dangling positive IRQ value that is erroneously disabled during system shutdown.
--

--- Patch [5]: [PATCH v5 5/9] rtc: abx80x: add irq to struct abx80x_priv ---
commit e5e02251b0d732530fe4bd8df1edc078a44d0fe3
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add irq to struct abx80x_priv
    
This commit adds the irq number to struct abx80x_priv so it is not tied
to the i2c_client. This allows the irq to be easily accessed in the case
of ABX81X SPI clocks.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 485d1ea08030d..12a4dbfec6396 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c

[ ... ]

> @@ -994,19 +997,19 @@ static int abx80x_probe(struct i2c_client *client)
>  		return -EIO;
>  	}
>  
> -	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,
>  						"abx8xx",
>  						client);
>  		if (err) {
>  			dev_err(&client->dev, "unable to request IRQ, alarms disabled\n");
> -			client->irq = 0;
> +			priv->irq = 0;
[Severity: Medium]
By removing the client->irq = 0 assignment here, doesn't this leave a
dangling positive IRQ value in the I2C client if the request fails?

If devm_request_threaded_irq() fails, the driver gracefully falls back to
running with alarms disabled. However, during system shutdown, the I2C core
in i2c_device_shutdown() relies on client->irq to disable interrupts:

drivers/i2c/i2c-core-base.c:i2c_device_shutdown() {
    ...
    if (client->irq > 0)
        disable_irq(client->irq);
    ...
}

Since this driver uses IRQF_SHARED and has no .shutdown callback to
override the default behavior, leaving client->irq > 0 means the I2C core
will unconditionally call disable_irq() on an IRQ line the driver failed
to acquire.

Could this erroneously mask the shared IRQ line for other active devices
during system shutdown?

>  		}
>  	}

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