[PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[zhuo-hao.lee]

From: zhuo-hao <zhuo-hao.lee@intel.com>

Before the system go to suspend (S3), if user create a timer with clockid
CLOCK_REALTIME_ALARM/CLOCK_BOOTTIME_ALARM and set a "large" timeout value
to this timer. The function alarmtimer_suspend will be called to setup
a timeout value to RTC timer to avoid the system sleep over time. However,
if the system wakeup early than RTC timeout, the RTC timer will not be cleared.
And this will cause the hpet_rtc_interrupt come unexpectedly until the RTC
timeout. To fix this problem, just adding alarmtimer_resume to cancel the
RTC timer.

Signed-off-by: Zhuo-hao Lee <zhuo-hao.lee@intel.com>
---
 kernel/time/alarmtimer.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 7fbba63..e840ed8 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -271,11 +271,27 @@ static int alarmtimer_suspend(struct device *dev)
 		__pm_wakeup_event(ws, MSEC_PER_SEC);
 	return ret;
 }
+
+static int alarmtimer_resume(struct device *dev)
+{
+	struct rtc_device *rtc;
+
+	rtc = alarmtimer_get_rtcdev();
+	if (rtc)
+		rtc_timer_cancel(rtc, &rtctimer);
+	return 0;
+}
+
 #else
 static int alarmtimer_suspend(struct device *dev)
 {
 	return 0;
 }
+
+static int alarmtimer_resume(struct device *dev)
+{
+	return 0;
+}
 #endif
 
 static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
@@ -800,6 +816,7 @@ out:
 /* Suspend hook structures */
 static const struct dev_pm_ops alarmtimer_pm_ops = {
 	.suspend = alarmtimer_suspend,
+	.resume = alarmtimer_resume,
 };
 
 static struct platform_driver alarmtimer_driver = {
-- 
1.9.1

RE: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[Lee, Zhuo-hao]

Hi tglx & all,

I wrote a simple program which can always hit this problem.
Please use gcc to compile the following code and then "./a.out 10000 1" and watch the /proc/interrupts, 
and you will see that the RTC interrupt will come unexpectedly after system resumed.
This abnormal interrupt should be removed. I tried on the latest linux kernel, 
the bug still exists. So, could you please help to review this patch? Or give me some feedback?


#include <stdio.h>
#include <sys/time.h>
#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <sys/timerfd.h>
#include <time.h>
#include <stdlib.h>
#include <stdint.h>

#define handle_error(msg) \
            do { perror(msg); exit(EXIT_FAILURE); } while (0)

int main(int argc, char *argv[])
{
    struct itimerspec new_value;
    int fd, sec, enable_suspend;
    struct timespec now;
    uint64_t exp;
    ssize_t s;
    pid_t pid;

    if (argc != 3) {
        fprintf(stderr, "%s secs enable_suspend\n",argv[0]);
        exit(EXIT_FAILURE);
    }

    sec = atoi(argv[1]);
    enable_suspend = atoi(argv[2]);

    if (clock_gettime(CLOCK_REALTIME, &now) == -1)
        handle_error("clock_gettime");

    /* Create a CLOCK_REALTIME absolute timer with initial
       expiration and interval as specified in command line */

    new_value.it_value.tv_sec = now.tv_sec + sec;
    new_value.it_value.tv_nsec = now.tv_nsec;
    new_value.it_interval.tv_sec = 0;
    new_value.it_interval.tv_nsec = 0;

    fd = timerfd_create(CLOCK_REALTIME_ALARM, 0);
    if (fd == -1)
        handle_error("timerfd_create");

    if (timerfd_settime(fd, TFD_TIMER_ABSTIME, &new_value, NULL) == -1)
        handle_error("timerfd_settime");

    system("cat /proc/driver/rtc | grep rtc_time");
    system("cat /proc/interrupts | grep -E 'CPU0|rtc0'");
    printf("\n\ntimer started: wait %d seconds\n\n", sec);
    pid = fork();
    if (pid == -1) {
        fprintf(stderr, "fork failed\n");
        exit(EXIT_FAILURE);
    } else if (pid == 0) {
        if(enable_suspend){
            sleep(5);
/* for chromeOS */
 //           system("echo 0 > /sys/class/rtc/rtc0/wakealarm");
//            system("echo +10 > /sys/class/rtc/rtc0/wakealarm");
//            system("powerd_dbus_suspend"); 

/* for Ubuntu */
            system("sudo pm-suspend"); 
        }
        exit(EXIT_SUCCESS);
    } else {
        s = read(fd, &exp, sizeof(uint64_t));
        if (s != sizeof(uint64_t))
            handle_error("read");

        system("cat /proc/driver/rtc |grep rtc_time");
        system("cat /proc/interrupts | grep -E 'CPU0|rtc0'");
    
        exit(EXIT_SUCCESS);
    }
}


Thanks
Lee, Zhuo-hao

-----Original Message-----
From: Lee, Zhuo-hao 
Sent: Thursday, November 5, 2015 1:50 PM
To: linux-kernel@vger.kernel.org
Cc: Thomas Gleixner; zhuohao.lee82@gmail.com; Lee, Zhuo-hao
Subject: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

From: zhuo-hao <zhuo-hao.lee@intel.com>

Before the system go to suspend (S3), if user create a timer with clockid CLOCK_REALTIME_ALARM/CLOCK_BOOTTIME_ALARM and set a "large" timeout value to this timer. The function alarmtimer_suspend will be called to setup a timeout value to RTC timer to avoid the system sleep over time. However, if the system wakeup early than RTC timeout, the RTC timer will not be cleared.
And this will cause the hpet_rtc_interrupt come unexpectedly until the RTC timeout. To fix this problem, just adding alarmtimer_resume to cancel the RTC timer.

Signed-off-by: Zhuo-hao Lee <zhuo-hao.lee@intel.com>
---
 kernel/time/alarmtimer.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 7fbba63..e840ed8 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -271,11 +271,27 @@ static int alarmtimer_suspend(struct device *dev)
 		__pm_wakeup_event(ws, MSEC_PER_SEC);
 	return ret;
 }
+
+static int alarmtimer_resume(struct device *dev) {
+	struct rtc_device *rtc;
+
+	rtc = alarmtimer_get_rtcdev();
+	if (rtc)
+		rtc_timer_cancel(rtc, &rtctimer);
+	return 0;
+}
+
 #else
 static int alarmtimer_suspend(struct device *dev)  {
 	return 0;
 }
+
+static int alarmtimer_resume(struct device *dev) {
+	return 0;
+}
 #endif
 
 static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type) @@ -800,6 +816,7 @@ out:
 /* Suspend hook structures */
 static const struct dev_pm_ops alarmtimer_pm_ops = {
 	.suspend = alarmtimer_suspend,
+	.resume = alarmtimer_resume,
 };
 
 static struct platform_driver alarmtimer_driver = {
--
1.9.1

Re: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[John Stultz]

On Wed, Nov 4, 2015 at 9:50 PM,  <zhuo-hao.lee@intel.com> wrote:
> From: zhuo-hao <zhuo-hao.lee@intel.com>
>

If you could, please CC me on future submissions? I need to add an
entry to MAINTAINERS for alarmtimers. :)

> Before the system go to suspend (S3), if user create a timer with clockid
> CLOCK_REALTIME_ALARM/CLOCK_BOOTTIME_ALARM and set a "large" timeout value
> to this timer. The function alarmtimer_suspend will be called to setup
> a timeout value to RTC timer to avoid the system sleep over time. However,
> if the system wakeup early than RTC timeout, the RTC timer will not be cleared.
> And this will cause the hpet_rtc_interrupt come unexpectedly until the RTC
> timeout. To fix this problem, just adding alarmtimer_resume to cancel the
> RTC timer.

So conceptually the patch makes sense, though I'm not totally sure I
understand the failure you describe.

We have some alarmtimer set for 2 hours from now.
We suspend, and the alarmtimer code sets an rtctimer to wake us up in
2 hours week.
We resume a few minutes later due to user interaction or other
wakeups, but the rtctimer is still set.
A little less then two hours later (while the system has been awake
the whole time), the RTC hardware fires and we run the rtctimer.
???? Something problematic here w/ the hpet_rtc_interrupt?
The alarmtimer's hrtimer fires as normal.


Again, your fix seems reasonable, but I also feel like when the RTC
hardware spuriously fires and we trigger the rtctimer logic, there
shouldn't be any real problems there. So I want to make sure we're not
covering up some underlying issue w/ this fix.

thanks
-john

RE: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[Lee, Zhuo-hao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2877 bytes --]

>If you could, please CC me on future submissions? I need to add an entry to MAINTAINERS >for alarmtimers. :)

Ok, I will do that on the future submissions :)

>> Before the system go to suspend (S3), if user create a timer with 
>> clockid CLOCK_REALTIME_ALARM/CLOCK_BOOTTIME_ALARM and set a "large" 
>> timeout value to this timer. The function alarmtimer_suspend will be 
>> called to setup a timeout value to RTC timer to avoid the system sleep 
>> over time. However, if the system wakeup early than RTC timeout, the RTC timer will not be cleared.
>> And this will cause the hpet_rtc_interrupt come unexpectedly until the 
>> RTC timeout. To fix this problem, just adding alarmtimer_resume to 
>> cancel the RTC timer.

>So conceptually the patch makes sense, though I'm not totally sure I understand the failure you describe.

I had posted a small program which always hit this bug if system wake up earlier than setting time,
Did you receive it?
https://lkml.org/lkml/2015/11/8/326

>We have some alarmtimer set for 2 hours from now.
>We suspend, and the alarmtimer code sets an rtctimer to wake us up in
>2 hours week.
>We resume a few minutes later due to user interaction or other wakeups, but the rtctimer is still set.
>A little less then two hours later (while the system has been awake the whole time), the RTC hardware fires and we run the rtctimer.
>???? Something problematic here w/ the hpet_rtc_interrupt?

Yes. 
If application use timerfd_create(CLOCK_REALTIME_ALARM, 0) to create a timer, and use timerfd_settime() to set 2 hours timer. And then, the user just trigger the system go to suspend, this bug will be hit. 
Before the system go to suspend, alarmtimer will set "current time + 2hr" to rtc timer to avoid the system sleep over time. If the system wake up earlier than 2 hours (for example, user press the wake up key), the hpet_rtc_interrupt will be fired continuously until "current time + 2hr" reached. This abnormal interrupt will cost some system performance and should be avoided.

>The alarmtimer's hrtimer fires as normal.

>Again, your fix seems reasonable, but I also feel like when the RTC hardware spuriously fires and we trigger the rtctimer >logic, there shouldn't be any real problems there. So I want to make sure we're not covering up some underlying issue w/ >this fix.

Agree, there have two problems on this bug:
(1). alarmtimer create a rtc wake up timer however alarmtimer won't remove that timer if the system wake up earlier
(2). rtc wake up timer will trigger hpet_rtc_interrupt continuously until timer timeout.
This patch only fixed (1). Fixing (1) can avoid (2).
However, The (2) is another story which it is not covered by this patch.


Thanks
Lee, Zhuo-hao

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[Thomas Gleixner]

On Sat, 14 Nov 2015, Lee, Zhuo-hao wrote:

> (1). alarmtimer create a rtc wake up timer however alarmtimer won't
>      remove that timer if the system wake up earlier

That's hardly a bug. That's a slight incorrectness which needs to be
fixed.

> (2). rtc wake up timer will trigger hpet_rtc_interrupt continuously
> until timer timeout.

> This patch only fixed (1). Fixing (1) can avoid (2).
> However, The (2) is another story which it is not covered by this patch.

And that's the real interesting question. Why is hpet_rtc_interrupt
continously triggered?

Thanks,

	tglx

RE: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[Lee, Zhuo-hao]

>> (1). alarmtimer create a rtc wake up timer however alarmtimer won't
>>      remove that timer if the system wake up earlier

>That's hardly a bug. That's a slight incorrectness which needs to be fixed.
I think this timer is useless after system resume. For the correctness, I think it should be fixed.
Do you agree? Or do you have any other suggestions? :)

>> (2). rtc wake up timer will trigger hpet_rtc_interrupt continuously 
>> until timer timeout.

>> This patch only fixed (1). Fixing (1) can avoid (2).
>> However, The (2) is another story which it is not covered by this patch.

>And that's the real interesting question. Why is hpet_rtc_interrupt continously triggered?

I think this is caused by driver implementation. 
My explanation for the continuous  hpet_rtc_interrupt():
1. In hpet_rtc_timer_init(), it will set "delta" to 1/64 seconds in AIE mode (about 16ms, because of the value DEFAULT_RTC_SHIFT), 
     that will cause hpet_rtc_interrupt() be triggered in every 16ms if the wakeup time (global variable hpet_alarm_time) is not reached.
2. When someone calls rtc_timer_start (ex: alarmtimer_suspend ), hpet driver will set timeout value (current time + delta) 
     to hpet_alarm_time, and then enable hpet timer.
3. In hpet_rtc_interrupt(), it will call hpet_rtc_timer_reinit() to reinit timer and add "delta" to 
     the hpet_t1_cmp for the next timeout value ( so we will receive an interrupt after 16ms).
     If hpet_alarm_time is reached, the irq_handler() will be called, the hpet_rtc_flags will be set to 0 and the hpet timer will be disable.
     Otherwise, we will receive an interrupt in each 16ms.
If the logic of 1,2,3 are correct, no bugs inside, maybe we can redesign the driver to reduce this periodic interrupt.

The following call path enable hpet timer when system go to suspend, just for you reference :)
alarmtimer_suspend
--rtc_timer_start
----rtc_timer_enqueue
------__rtc_set_alarm
--------cmos_set_alarm
----------cmos_irq_enable
------------hpet_set_rtc_irq_bit

Thanks,
Lee, Zhuo-hao

RE: [PATCH v2] alarmtimer: fix unexpected rtc interrupt when system resume from S3

[Thomas Gleixner]

On Tue, 17 Nov 2015, Lee, Zhuo-hao wrote:

Can you please fix your e-mail client to do proper line breaks around
80 char?

> >> (1). alarmtimer create a rtc wake up timer however alarmtimer won't
> >>      remove that timer if the system wake up earlier
> 
> > That's hardly a bug. That's a slight incorrectness which needs to be fixed.
> 
> I think this timer is useless after system resume. For the
> correctness, I think it should be fixed.  Do you agree? Or do you
> have any other suggestions? :)

Care to read what I wrote?

> >   ... That's a slight incorrectness which needs to be fixed.

> >> (2). rtc wake up timer will trigger hpet_rtc_interrupt continuously 
> >> until timer timeout.
> 
> >> This patch only fixed (1). Fixing (1) can avoid (2).
> >> However, The (2) is another story which it is not covered by this patch.
> 
> > And that's the real interesting question. Why is hpet_rtc_interrupt
> > continously triggered?

> I think this is caused by driver implementation. 
> My explanation for the continuous  hpet_rtc_interrupt():
> 1. In hpet_rtc_timer_init(), it will set "delta" to 1/64 seconds in
>    AIE mode (about 16ms, because of the value DEFAULT_RTC_SHIFT),
>    that will cause hpet_rtc_interrupt() be triggered in every 16ms
>    if the wakeup time (global variable hpet_alarm_time) is not
>    reached.
> 
> 2. When someone calls rtc_timer_start (ex: alarmtimer_suspend ),
>    hpet driver will set timeout value (current time + delta) to
>    hpet_alarm_time, and then enable hpet timer.
>
> 3. In hpet_rtc_interrupt(), it will call hpet_rtc_timer_reinit() to
>    reinit timer and add "delta" to the hpet_t1_cmp for the next
>    timeout value ( so we will receive an interrupt after 16ms).
>
>    If hpet_alarm_time is reached, the irq_handler() will be called, the
>    hpet_rtc_flags will be set to 0 and the hpet timer will be
>    disable.
>
>    Otherwise, we will receive an interrupt in each 16ms.

Right. We cannot do much about it. So the changelog of this patch
wants some explanation. Something like this:

  This was noticed because the HPET RTC emulation fires an interrupt
  every 16ms up to the point where the alarm time is reached.
  
Simply because you wouldn't have noticed if the interrupt happened
after 2 hours.

Thanks,

	tglx