Re: [PATCH v5 4/9] rtc: abx80x: properly handle shared IRQs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The write to the Oscillator Control Register (ABX8XX_REG_OSC) in `abx80x_probe` is silently ignored by the hardware because the required configuration key (`ABX8XX_CFG_KEY_OSC`) is not written to the Configuration Key Register beforehand. If the RTC has previously enabled interrupts like Auto Calibration or Oscillator Fail (e.g., from the bootloader or prior system state), they will remain enabled. When these interrupts trigger, the newly modified `abx80x_handle_irq` returns `IRQ_NONE` without clearing the interrupt status bits, leading to an interrupt storm that will eventually cause the kernel to disable the shared IRQ line.
--

--- Patch [4]: [PATCH v5 4/9] rtc: abx80x: properly handle shared IRQs ---
commit 7460b93d807b8ac0b8ec90945bfa5691c0f9cd26
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: properly handle shared IRQs

This patch modifies the abx80x interrupt handler to support shared IRQs by
only returning IRQ_HANDLED when alarm or watchdog events occur. It also
updates the probe function to disable unused interrupts.

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

[ ... ]

> @@ -950,6 +963,37 @@ static int abx80x_probe(struct i2c_client *client)
>  	if (err)
>  		return err;
>  
> +	/* Disable unused interrupts */

[ ... ]

> +	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
> +	if (data < 0) {
> +		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC,
> +					data & ~(ABX8XX_OSC_ACIE |
> +						 ABX8XX_OSC_OFIE));

[Severity: High]
Will this write to ABX8XX_REG_OSC be silently ignored by the hardware?

It appears that writing to the Oscillator Control Register requires an unlock
key to be written to the Configuration Key Register first.

If this write fails and the RTC was previously left with ACIE or OFIE
interrupts enabled (e.g., from the bootloader), could it lead to an interrupt
storm?

When those interrupts fire, the updated abx80x_handle_irq() will now skip
them and return IRQ_NONE without clearing the status bits:

	if (status & ABX8XX_STATUS_AF) {
		rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF);
		handled = IRQ_HANDLED;
	}
        ...
	if (handled == IRQ_HANDLED)
		i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
					  status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT));

	return handled;

Since the status bits are not cleared for unhandled events, the IRQ controller
would continually retrigger and eventually disable the shared IRQ line.

> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
> +		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,

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