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

[PATCH 0/6] 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>
---
Esben Haabendal (6):
      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: st-lpc: 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-st-lpc.c   |  1 +
 drivers/rtc/rtc-tps6586x.c |  1 +
 5 files changed, 31 insertions(+)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241203-rtc-uie-irq-fixes-f2838782d0f8

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

[PATCH 1/6] 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.47.1

[PATCH 2/6] 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.47.1

[PATCH 3/6] 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 afc8fcba8f8885c15730e47fe0a9d3f681e3cba4..b74148935d21ff69095e703e99bf024714e41ac1 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.47.1

[PATCH 4/6] rtc: st-lpc: 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.

To: Patrice Chotard <patrice.chotard@foss.st.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/rtc/rtc-st-lpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-st-lpc.c b/drivers/rtc/rtc-st-lpc.c
index c6d4522411b312f94de2701ff4ff491e5323aa96..dbc2c23bca23f5de6de3fd4b39c9c67290bfd78d 100644
--- a/drivers/rtc/rtc-st-lpc.c
+++ b/drivers/rtc/rtc-st-lpc.c
@@ -218,6 +218,7 @@ static int st_rtc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	rtc->irq_enabled = true;
 	ret = devm_request_irq(&pdev->dev, rtc->irq, st_rtc_handler,
 			       IRQF_NO_AUTOEN, pdev->name, rtc);
 	if (ret) {

-- 
2.47.1

[PATCH 5/6] 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 e796729fc817c1bd99f4c70b714a346fc1a795d2..c8ccb4779b50b2c5909e6a64748f52b61e78dac5 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.47.1

[PATCH 6/6] 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 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.47.1

Re: [PATCH 4/6] rtc: st-lpc: Fix initial enable_irq/disable_irq balance

[Alexandre Belloni]

On 03/12/2024 11:45:34+0100, Esben Haabendal wrote:
> Interrupts are automatically enabled when requested, so we need to
> initialize irq_enabled accordingly to avoid causing an unbalanced enable
> warning.
> 
> To: Patrice Chotard <patrice.chotard@foss.st.com>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/rtc/rtc-st-lpc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-st-lpc.c b/drivers/rtc/rtc-st-lpc.c
> index c6d4522411b312f94de2701ff4ff491e5323aa96..dbc2c23bca23f5de6de3fd4b39c9c67290bfd78d 100644
> --- a/drivers/rtc/rtc-st-lpc.c
> +++ b/drivers/rtc/rtc-st-lpc.c
> @@ -218,6 +218,7 @@ static int st_rtc_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	rtc->irq_enabled = true;
>  	ret = devm_request_irq(&pdev->dev, rtc->irq, st_rtc_handler,
>  			       IRQF_NO_AUTOEN, pdev->name, rtc);

Seeing the IRQF_NO_AUTOEN here, I guess the patch is not correct.

>  	if (ret) {
> 
> -- 
> 2.47.1
> 

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

Re: [PATCH 4/6] rtc: st-lpc: Fix initial enable_irq/disable_irq balance

[Esben Haabendal]

Alexandre Belloni <alexandre.belloni@bootlin.com> writes:

> On 03/12/2024 11:45:34+0100, Esben Haabendal wrote:
>> Interrupts are automatically enabled when requested, so we need to
>> initialize irq_enabled accordingly to avoid causing an unbalanced enable
>> warning.
>> 
>> To: Patrice Chotard <patrice.chotard@foss.st.com>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/rtc/rtc-st-lpc.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/rtc/rtc-st-lpc.c b/drivers/rtc/rtc-st-lpc.c
>> index c6d4522411b312f94de2701ff4ff491e5323aa96..dbc2c23bca23f5de6de3fd4b39c9c67290bfd78d 100644
>> --- a/drivers/rtc/rtc-st-lpc.c
>> +++ b/drivers/rtc/rtc-st-lpc.c
>> @@ -218,6 +218,7 @@ static int st_rtc_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  
>> +	rtc->irq_enabled = true;
>>  	ret = devm_request_irq(&pdev->dev, rtc->irq, st_rtc_handler,
>>  			       IRQF_NO_AUTOEN, pdev->name, rtc);
>
> Seeing the IRQF_NO_AUTOEN here, I guess the patch is not correct.

I guess you are right :)
Sorry about that. I only have HW available for testing the rtc-isl12022
driver here.

This patch should be dropped.

>
>>  	if (ret) {
>> 
>> -- 
>> 2.47.1
>> 

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

[Esben Haabendal]

Esben Haabendal <esben@geanix.com> writes:

> 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.

I don't have access to hardware using cpcap, st-lpc or tps6586x rtc
drivers, so I have not been able to test those 3 patches.

/Esben

> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
> Esben Haabendal (6):
>       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: st-lpc: 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-st-lpc.c   |  1 +
>  drivers/rtc/rtc-tps6586x.c |  1 +
>  5 files changed, 31 insertions(+)
> ---
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> change-id: 20241203-rtc-uie-irq-fixes-f2838782d0f8
>
> Best regards,