timekeeping: Ignore the bogus sleep time if pm_trace is enabled

Previously we encountered some memory overflow issues due to the bogus sleep time brought by inconsistent rtc, which is triggered when pm_trace is enabled, and we have fixed it in recent kernel. However it's improper in the first place to call __timekeeping_inject_sleeptime() in case that pm_trace is enabled simply because that "hash" time value will wreckage the timekeeping subsystem.

So this patch ignores the sleep time if pm_trace is enabled in the following situation:
1. rtc is used as persist clock to compensate for sleep time, or
2. rtc is used to calculate the sleep time in rtc_resume.

Cc [email protected] (3.17+)

Cc Rafael J. Wysocki <[email protected]>

Cc John Stultz <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Xunlei Pang <[email protected]>

Cc Zhang Rui <[email protected]>

Cc [email protected]

Cc [email protected]

Suggested-by Xunlei Pang <[email protected]>

Suggested-by Rafael J. Wysocki <[email protected]>

Suggested-by Thomas Gleixner <[email protected]>

Reported-by Janek Kozicki <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 arch/x86/kernel/rtc.c     | 12 ++++++++++++
 kernel/time/timekeeping.c |  3 ++-
 2 files changed, 14 insertions(+), 1 deletion(-)

arch/x86/kernel/rtc.c +2 -0

index 79c6311c..5c28197 100644
@@ -8,6 +8,7 @@
 #include <linux/export.h>
 #include <linux/pnp.h>
 #include <linux/of.h>
+#include <linux/pm-trace.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>

@@ -144,6 +145,17 @@ int update_persistent_clock(struct timespec now)
 void read_persistent_clock(struct timespec *ts)
 {
 	x86_platform.get_wallclock(ts);
+

arch/x86/kernel/rtc.c +0 -0

 }
 
 

kernel/time/timekeeping.c +1 -0

index 3b65746..9af885d 100644
@@ -23,6 +23,7 @@
 #include <linux/stop_machine.h>
 #include <linux/pvclock_gtod.h>
 #include <linux/compiler.h>
+#include <linux/pm-trace.h>
 
 #include "tick-internal.h"
 #include "ntp_internal.h"

arch/x86/kernel/rtc.c int update_persistent_clock(struct timespec now) · line 158

Quoted 11 lines · Chen Yu

arch/x86/kernel/rtc.c +11 -0

+
+	/*
+	 * Make rtc-based persistent clock unusable
+	 * if pm_trace is enabled, only take effect
+	 * for timekeeping_suspend/resume.
+	 */
+	if (pm_trace_is_enabled() &&
+	    x86_platform.get_wallclock == mach_get_cmos_time) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}

I'm not sure about this. Looks hackish.

Quoted 14 lines · Rafael J. Wysocki

arch/x86/kernel/rtc.c int update_persistent_clock(struct timespec now) · line 158

Quoted 11 lines · Chen Yu

arch/x86/kernel/rtc.c +11 -0

+
+	/*
+	 * Make rtc-based persistent clock unusable
+	 * if pm_trace is enabled, only take effect
+	 * for timekeeping_suspend/resume.
+	 */
+	if (pm_trace_is_enabled() &&
+	    x86_platform.get_wallclock == mach_get_cmos_time) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}

I'm not sure about this. Looks hackish.

Indeed. Can't you just keep track that pm_trace fiddled with the cmos clock and then discard the value either in the core or in mach_get_cmos_time()

Hi Thomas, Rafael,

Quoted 16 lines · Thomas Gleixner

Quoted 14 lines · Rafael J. Wysocki

arch/x86/kernel/rtc.c int update_persistent_clock(struct timespec now) · line 158

Quoted 11 lines · Chen Yu

arch/x86/kernel/rtc.c +11 -0

+
+	/*
+	 * Make rtc-based persistent clock unusable
+	 * if pm_trace is enabled, only take effect
+	 * for timekeeping_suspend/resume.
+	 */
+	if (pm_trace_is_enabled() &&
+	    x86_platform.get_wallclock == mach_get_cmos_time) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}

I'm not sure about this. Looks hackish.

Quoted 2 lines

Indeed. Can't you just keep track that pm_trace fiddled with the cmos clock and then discard the value either in the core or in mach_get_cmos_time()

The previous version is more straightforward, since it ignored the bogus rtc in core. Would you please take a glance at it too, thanks:
https://patchwork.kernel.org/patch/9287347/

Quoted 18 lines · Chen Yu

Hi Thomas, Rafael,

Quoted 16 lines · Thomas Gleixner

Quoted 14 lines · Rafael J. Wysocki

Review

Quoted 11 lines · Chen Yu

(quoted diff) +11 -0

+
+	/*
+	 * Make rtc-based persistent clock unusable
+	 * if pm_trace is enabled, only take effect
+	 * for timekeeping_suspend/resume.
+	 */
+	if (pm_trace_is_enabled() &&
+	    x86_platform.get_wallclock == mach_get_cmos_time) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}

I'm not sure about this. Looks hackish.

Quoted 6 lines

Quoted 2 lines

Indeed. Can't you just keep track that pm_trace fiddled with the cmos clock and then discard the value either in the core or in mach_get_cmos_time()

The previous version is more straightforward, since it ignored the bogus rtc in core. Would you please take a glance at it too, thanks:
https://patchwork.kernel.org/patch/9287347/

This is the same hackery just different:

Review

Quoted 6 lines

(quoted diff) +6 -0

+bool persistent_clock_is_usable(void)
+{
+	/* Unusable if pm_trace is enabled. */
+	return !((x86_platform.get_wallclock == mach_get_cmos_time) &&
+	        pm_trace_is_enabled());
+}

I really have no idea why this is burried in x86 land. The pm_trace hackery issues mc146818_set_time() to fiddle with the RTC. So any implementation of this is affected.

So that very piece of pmtrace code should keep track of the wreckage it did to the RTC and provide the fact to the core timekeeping code which can then skip the update.

Quoted 3 lines

I really have no idea why this is burried in x86 land. The pm_trace hackery issues mc146818_set_time() to fiddle with the RTC. So any implementation of this is affected.

OK, I've changed this patch according to this suggestion.

OK. I've moved most of the logic into the pm_trace component, Once the mc146818_set_time has modified the RTC by pm_trace, related flags will be set which indicates the unusable of RTC. And timekeeping system is able to query these flags to decide whether it should inject the sleep time. (We tried to make this patch as simple as possible, but it looks like we have to deal with persistent clock for x86, which makes this patch a little more complicated). Here's the trial version of it, any suggestion would be appreciated:

+unsigned int timekeeping_tainted;

Quoted 1 line · Chen Yu

+extern void pm_trace_untaint_timekeeping(void);

And how exactly do you untaint it? Just by clearing the flags. That makes the RTC time magically correct again?

include/linux/pm-trace.h line 14

Quoted 4 lines

include/linux/pm-trace.h +4 -0

+int arch_pm_trace_taint_pclock(void)
+{
+	return (x86_platform.get_wallclock == mach_get_cmos_time);
+}

Groan. I told you to do it in the mc14xxx related places. There are not that many in the kernel

Here is a completely uncompiled/untested patch which should address the issue in a halfways clean way.

Thanks,

tglx