[PATCH v2 0/5] rtc: Fix problems with missing UIE irqs

[PATCH v2 0/5] rtc: Fix problems with missing UIE irqs

[Esben Haabendal]

This fixes a couple of different problems, that can cause RTC (alarm)
irqs to be missing when generating UIE interrupts.

The first commit fixes a long-standing problem, which has been
documented in a comment since 2010. This fixes a race that could cause
UIE irqs to stop being generated, which was easily reproduced by
timing the use of RTC_UIE_ON ioctl with the seconds tick in the RTC.

The last commit ensures that RTC (alarm) irqs are enabled whenever
RTC_UIE_ON ioctl is used.

The driver specific commits avoids kernel warnings about unbalanced
enable_irq/disable_irq, which gets triggered on first RTC_UIE_ON with
the last commit. Before this series, the same warning should be seen
on initial RTC_AIE_ON with those drivers.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v2:
- Dropped patch for rtc-st-lpc driver.
- Link to v1: https://lore.kernel.org/r/20241203-rtc-uie-irq-fixes-v1-0-01286ecd9f3f@geanix.com

---
Esben Haabendal (5):
      rtc: interface: Fix long-standing race when setting alarm
      rtc: isl12022: Fix initial enable_irq/disable_irq balance
      rtc: cpcap: Fix initial enable_irq/disable_irq balance
      rtc: tps6586x: Fix initial enable_irq/disable_irq balance
      rtc: interface: Ensure alarm irq is enabled when UIE is enabled

 drivers/rtc/interface.c    | 27 +++++++++++++++++++++++++++
 drivers/rtc/rtc-cpcap.c    |  1 +
 drivers/rtc/rtc-isl12022.c |  1 +
 drivers/rtc/rtc-tps6586x.c |  1 +
 4 files changed, 30 insertions(+)
---
base-commit: 82f2b0b97b36ee3fcddf0f0780a9a0825d52fec3
change-id: 20241203-rtc-uie-irq-fixes-f2838782d0f8

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

[PATCH v2 1/5] rtc: interface: Fix long-standing race when setting alarm

[Esben Haabendal]

As described in the old comment dating back to
commit 6610e0893b8b ("RTC: Rework RTC code to use timerqueue for events")
from 2010, we have been living with a race window when setting alarm
with an expiry in the near future (i.e. next second).
With 1 second resolution, it can happen that the second ticks after the
check for the timer having expired, but before the alarm is actually set.
When this happen, no alarm IRQ is generated, at least not with some RTC
chips (isl12022 is an example of this).

With UIE RTC timer being implemented on top of alarm irq, being re-armed
every second, UIE will occasionally fail to work, as an alarm irq lost
due to this race will stop the re-arming loop.

For now, I have limited the additional expiry check to only be done for
alarms set to next seconds. I expect it should be good enough, although I
don't know if we can now for sure that systems with loads could end up
causing the same problems for alarms set 2 seconds or even longer in the
future.

I haven't been able to reproduce the problem with this check in place.

Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/interface.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index aaf76406cd7d7d2cfd5479fc1fc892fcb5efbb6b..e365e8fd166db31f8b44fac9fb923d36881b1394 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -443,6 +443,29 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
 	else
 		err = rtc->ops->set_alarm(rtc->dev.parent, alarm);
 
+	/*
+	 * Check for potential race described above. If the waiting for next
+	 * second, and the second just ticked since the check above, either
+	 *
+	 * 1) It ticked after the alarm was set, and an alarm irq should be
+	 *    generated.
+	 *
+	 * 2) It ticked before the alarm was set, and alarm irq most likely will
+	 * not be generated.
+	 *
+	 * While we cannot easily check for which of these two scenarios we
+	 * are in, we can return -ETIME to signal that the timer has already
+	 * expired, which is true in both cases.
+	 */
+	if ((scheduled - now) <= 1) {
+		err = __rtc_read_time(rtc, &tm);
+		if (err)
+			return err;
+		now = rtc_tm_to_time64(&tm);
+		if (scheduled <= now)
+			return -ETIME;
+	}
+
 	trace_rtc_set_alarm(rtc_tm_to_time64(&alarm->time), err);
 	return err;
 }

-- 
2.49.0

[PATCH v2 2/5] rtc: isl12022: Fix initial enable_irq/disable_irq balance

[Esben Haabendal]

Interrupts are automatically enabled when requested, so we need to
initialize irq_enabled accordingly to avoid causing an unbalanced enable
warning.

Fixes: c62d658e5253 ("rtc: isl12022: Add alarm support")
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/rtc-isl12022.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index 9b44839a7402c9ab7da634d95152c2520eb8552e..5fc52dc6421305e957a0c3a169009c3f6a6c7320 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -413,6 +413,7 @@ static int isl12022_setup_irq(struct device *dev, int irq)
 	if (ret)
 		return ret;
 
+	isl12022->irq_enabled = true;
 	ret = devm_request_threaded_irq(dev, irq, NULL,
 					isl12022_rtc_interrupt,
 					IRQF_SHARED | IRQF_ONESHOT,

-- 
2.49.0

[PATCH v2 3/5] rtc: cpcap: Fix initial enable_irq/disable_irq balance

[Esben Haabendal]

Interrupts are automatically enabled when requested, so we need to
initialize alarm_enabled accordingly to avoid causing an unbalanced enable
warning.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/rtc-cpcap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-cpcap.c b/drivers/rtc/rtc-cpcap.c
index 568a89e79c11424967d4af239d252a80a073a341..682f85abfa0e9627c8410220c9cebad7371d360b 100644
--- a/drivers/rtc/rtc-cpcap.c
+++ b/drivers/rtc/rtc-cpcap.c
@@ -268,6 +268,7 @@ static int cpcap_rtc_probe(struct platform_device *pdev)
 		return err;
 
 	rtc->alarm_irq = platform_get_irq(pdev, 0);
+	rtc->alarm_enabled = true;
 	err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL,
 					cpcap_rtc_alarm_irq,
 					IRQF_TRIGGER_NONE | IRQF_ONESHOT,

-- 
2.49.0

[PATCH v2 4/5] rtc: tps6586x: Fix initial enable_irq/disable_irq balance

[Esben Haabendal]

Interrupts are automatically enabled when requested, so we need to
initialize irq_en accordingly to avoid causing an unbalanced enable
warning.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/rtc-tps6586x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
index 54c8429b16bfcc692b1f4d5404f0c42f720e93b4..76ecf7b798f0de22aa89a552a263b473ab3065ef 100644
--- a/drivers/rtc/rtc-tps6586x.c
+++ b/drivers/rtc/rtc-tps6586x.c
@@ -258,6 +258,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
 
 	irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN);
 
+	rtc->irq_en = true;
 	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
 				tps6586x_rtc_irq,
 				IRQF_ONESHOT,

-- 
2.49.0

[PATCH v2 5/5] rtc: interface: Ensure alarm irq is enabled when UIE is enabled

[Esben Haabendal]

When setting a normal alarm, user-space is responsible for using
RTC_AIE_ON/RTC_AIE_OFF to control if alarm irq should be enabled.

But when RTC_UIE_ON is used, interrupts must be enabled so that the
requested irq events are generated.
When RTC_UIE_OFF is used, alarm irq is disabled if there are no other
alarms queued, so this commit brings symmetry to that.

Signed-off-by: Esben Haabendal <esben@geanix.com>
Cc: stable@vger.kernel.org
---
 drivers/rtc/interface.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index e365e8fd166db31f8b44fac9fb923d36881b1394..39db12f267cc627febb78e67400aaf8fc3301b0c 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -617,6 +617,10 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
 		rtc->uie_rtctimer.node.expires = ktime_add(now, onesec);
 		rtc->uie_rtctimer.period = ktime_set(1, 0);
 		err = rtc_timer_enqueue(rtc, &rtc->uie_rtctimer);
+		if (!err && rtc->ops && rtc->ops->alarm_irq_enable)
+			err = rtc->ops->alarm_irq_enable(rtc->dev.parent, 1);
+		if (err)
+			goto out;
 	} else {
 		rtc_timer_remove(rtc, &rtc->uie_rtctimer);
 	}

-- 
2.49.0

Re: [PATCH v2 0/5] rtc: Fix problems with missing UIE irqs

[Alexandre Belloni]

On Fri, 16 May 2025 09:23:34 +0200, Esben Haabendal wrote:
> This fixes a couple of different problems, that can cause RTC (alarm)
> irqs to be missing when generating UIE interrupts.
> 
> The first commit fixes a long-standing problem, which has been
> documented in a comment since 2010. This fixes a race that could cause
> UIE irqs to stop being generated, which was easily reproduced by
> timing the use of RTC_UIE_ON ioctl with the seconds tick in the RTC.
> 
> [...]

Applied, thanks!

[1/5] rtc: interface: Fix long-standing race when setting alarm
      https://git.kernel.org/abelloni/c/795cda8338ea
[2/5] rtc: isl12022: Fix initial enable_irq/disable_irq balance
      https://git.kernel.org/abelloni/c/9ffe06b6ccd7
[3/5] rtc: cpcap: Fix initial enable_irq/disable_irq balance
      https://git.kernel.org/abelloni/c/e0762fd26ad6
[4/5] rtc: tps6586x: Fix initial enable_irq/disable_irq balance
      https://git.kernel.org/abelloni/c/1502fe0e97be
[5/5] rtc: interface: Ensure alarm irq is enabled when UIE is enabled
      https://git.kernel.org/abelloni/c/9db26d5855d0

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 4/5] rtc: tps6586x: Fix initial enable_irq/disable_irq balance

[Jon Hunter]

Hi Esben,

On 16/05/2025 08:23, Esben Haabendal wrote:
> Interrupts are automatically enabled when requested, so we need to
> initialize irq_en accordingly to avoid causing an unbalanced enable
> warning.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>   drivers/rtc/rtc-tps6586x.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
> index 54c8429b16bfcc692b1f4d5404f0c42f720e93b4..76ecf7b798f0de22aa89a552a263b473ab3065ef 100644
> --- a/drivers/rtc/rtc-tps6586x.c
> +++ b/drivers/rtc/rtc-tps6586x.c
> @@ -258,6 +258,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
>   
>   	irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN);
>   
> +	rtc->irq_en = true;
>   	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
>   				tps6586x_rtc_irq,
>   				IRQF_ONESHOT,
> 

I have bisected a suspend regression on one of our Tegra20 boards (that 
uses this driver) and bisect is pointing to this commit. Reverting this 
commit fixes the problem.

Looking at the above I see that the flag IRQ_NOAUTOEN is being set and 
so now with your change we never enable the interrupt. Hence, the 
wake-ups are now broken and suspend testing fails. So it would seem best 
to revert this.

BTW, I looked at the change to the CPCAP driver and that driver actively 
disables the IRQ after requesting it and so I am wondering if that will 
also have alarm issues? I don't have a board with CPCAP to test.

Thanks
Jon

-- 
nvpublic

Re: [PATCH v2 4/5] rtc: tps6586x: Fix initial enable_irq/disable_irq balance

[Alexandre Belloni]

On 29/10/2025 09:20:17+0000, Jon Hunter wrote:
> Hi Esben,
> 
> On 16/05/2025 08:23, Esben Haabendal wrote:
> > Interrupts are automatically enabled when requested, so we need to
> > initialize irq_en accordingly to avoid causing an unbalanced enable
> > warning.
> > 
> > Signed-off-by: Esben Haabendal <esben@geanix.com>
> > ---
> >   drivers/rtc/rtc-tps6586x.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
> > index 54c8429b16bfcc692b1f4d5404f0c42f720e93b4..76ecf7b798f0de22aa89a552a263b473ab3065ef 100644
> > --- a/drivers/rtc/rtc-tps6586x.c
> > +++ b/drivers/rtc/rtc-tps6586x.c
> > @@ -258,6 +258,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
> >   	irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN);
> > +	rtc->irq_en = true;
> >   	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> >   				tps6586x_rtc_irq,
> >   				IRQF_ONESHOT,
> > 
> 
> I have bisected a suspend regression on one of our Tegra20 boards (that uses
> this driver) and bisect is pointing to this commit. Reverting this commit
> fixes the problem.
> 
> Looking at the above I see that the flag IRQ_NOAUTOEN is being set and so
> now with your change we never enable the interrupt. Hence, the wake-ups are
> now broken and suspend testing fails. So it would seem best to revert this.
> 
> BTW, I looked at the change to the CPCAP driver and that driver actively
> disables the IRQ after requesting it and so I am wondering if that will also
> have alarm issues? I don't have a board with CPCAP to test.
> 

That's right, I guess you can send reverts.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 4/5] rtc: tps6586x: Fix initial enable_irq/disable_irq balance

[Jon Hunter]

On 29/10/2025 10:39, Alexandre Belloni wrote:
> On 29/10/2025 09:20:17+0000, Jon Hunter wrote:
>> Hi Esben,
>>
>> On 16/05/2025 08:23, Esben Haabendal wrote:
>>> Interrupts are automatically enabled when requested, so we need to
>>> initialize irq_en accordingly to avoid causing an unbalanced enable
>>> warning.
>>>
>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>> ---
>>>    drivers/rtc/rtc-tps6586x.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
>>> index 54c8429b16bfcc692b1f4d5404f0c42f720e93b4..76ecf7b798f0de22aa89a552a263b473ab3065ef 100644
>>> --- a/drivers/rtc/rtc-tps6586x.c
>>> +++ b/drivers/rtc/rtc-tps6586x.c
>>> @@ -258,6 +258,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
>>>    	irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN);
>>> +	rtc->irq_en = true;
>>>    	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
>>>    				tps6586x_rtc_irq,
>>>    				IRQF_ONESHOT,
>>>
>>
>> I have bisected a suspend regression on one of our Tegra20 boards (that uses
>> this driver) and bisect is pointing to this commit. Reverting this commit
>> fixes the problem.
>>
>> Looking at the above I see that the flag IRQ_NOAUTOEN is being set and so
>> now with your change we never enable the interrupt. Hence, the wake-ups are
>> now broken and suspend testing fails. So it would seem best to revert this.
>>
>> BTW, I looked at the change to the CPCAP driver and that driver actively
>> disables the IRQ after requesting it and so I am wondering if that will also
>> have alarm issues? I don't have a board with CPCAP to test.
>>
> 
> That's right, I guess you can send reverts.

You guessed right! I certainly can. I am happy to send a revert for this 
because I can clearly test and verify this. I am not sure if you and 
Esben want to review the rest of the series.

Jon

-- 
nvpublic