| Message ID | 20260811194034.28937-5-apokusinski01@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: abx80x: add support for abx81x | expand |
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 <apokusinski01@gmail.com> 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,
diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c index 0c8d3416fd5a..485d1ea08030 100644 --- a/drivers/rtc/rtc-abx80x.c +++ b/drivers/rtc/rtc-abx80x.c @@ -51,12 +51,18 @@ #define ABX8XX_CTRL2_RSVD BIT(5) #define ABX8XX_REG_IRQ 0x12 +#define ABX8XX_IRQ_EX1E BIT(0) +#define ABX8XX_IRQ_EX2E BIT(1) #define ABX8XX_IRQ_AIE BIT(2) +#define ABX8XX_IRQ_TIE BIT(3) +#define ABX8XX_IRQ_BLIE BIT(4) #define ABX8XX_IRQ_IM_1_4 (0x3 << 5) #define ABX8XX_REG_CD_TIMER_CTL 0x18 #define ABX8XX_REG_OSC 0x1c +#define ABX8XX_OSC_ACIE BIT(0) +#define ABX8XX_OSC_OFIE BIT(1) #define ABX8XX_OSC_FOS BIT(3) #define ABX8XX_OSC_BOS BIT(4) #define ABX8XX_OSC_ACAL_512 BIT(5) @@ -266,27 +272,34 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id) struct i2c_client *client = dev_id; struct abx80x_priv *priv = i2c_get_clientdata(client); struct rtc_device *rtc = priv->rtc; + irqreturn_t handled = IRQ_NONE; int status; guard(mutex)(&priv->lock); status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS); if (status < 0) - return IRQ_NONE; + return handled; - if (status & ABX8XX_STATUS_AF) + if (status & ABX8XX_STATUS_AF) { rtc_update_irq(rtc, 1, RTC_AF | RTC_IRQF); + handled = IRQ_HANDLED; + } /* * It is unclear if we'll get an interrupt before the external * reset kicks in. */ - if (status & ABX8XX_STATUS_WDT) + if (status & ABX8XX_STATUS_WDT) { dev_alert(&client->dev, "watchdog timeout interrupt.\n"); + handled = IRQ_HANDLED; + } - i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, 0); + if (handled == IRQ_HANDLED) + i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS, + status & ~(ABX8XX_STATUS_AF | ABX8XX_STATUS_WDT)); - return IRQ_HANDLED; + return handled; } static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t) @@ -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_IRQ); + if (data < 0) { + dev_err(&client->dev, "Unable to read irq register\n"); + return -EIO; + } + + err = i2c_smbus_write_byte_data(client, ABX8XX_REG_IRQ, + data & ~(ABX8XX_IRQ_EX1E | + ABX8XX_IRQ_EX2E | + ABX8XX_IRQ_TIE | + ABX8XX_IRQ_BLIE)); + if (err < 0) { + dev_err(&client->dev, "Unable to write irq register\n"); + return -EIO; + } + + 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)); + 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,
The abx80x RTC driver registers its interrupt handler with IRQF_SHARED, but the handler always returned IRQ_HANDLED and cleared the status register even when no interrupt source from the RTC was active. This could cause the driver to claim interrupts belonging to other devices sharing the same IRQ line and could also clear unrelated status bits in the RTC status register. Only handle the interrupt when the alarm or watchdog flags are set, and clear only those interrupt status bits that were handled. Return IRQ_NONE when the interrupt was not generated by the RTC. Fixes: 718a820a303c ("rtc: abx80x: add alarm support") Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com> --- drivers/rtc/rtc-abx80x.c | 54 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 5 deletions(-)