Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi,
On Fri, Sep 09, 2016 at 04:33:59PM +0200, Thomas Gleixner wrote:
> On Fri, 9 Sep 2016, Chen Yu wrote:
>
> > +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?
> 
> > +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
> 
OK, I made some small adjustment to make it compiled without pm_trace
configured, and this version is absolutely more professional than
my previous one. I'll do some testing based on it.
Besides I have another question related to the untain
of the pm_trace_rtc_abused flag: after resume back, if the user disabled
the pm_trace, then the following suspend/resume sleep time should become
valid again IMO, because we use the delta rather than the RTC itself for
suspend/resume. So in this version it disable the injection of sleep time
once pm_trace has been used? 

Thanks,
Yu

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi,
On Fri, Sep 09, 2016 at 04:33:59PM +0200, Thomas Gleixner wrote:
> On Fri, 9 Sep 2016, Chen Yu wrote:
>
> > +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?
> 
> > +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
> 
OK, I made some small adjustment to make it compiled without pm_trace
configured, and this version is absolutely more professional than
my previous one. I'll do some testing based on it.
Besides I have another question related to the untain
of the pm_trace_rtc_abused flag: after resume back, if the user disabled
the pm_trace, then the following suspend/resume sleep time should become
valid again IMO, because we use the delta rather than the RTC itself for
suspend/resume. So in this version it disable the injection of sleep time
once pm_trace has been used? 

Thanks,
Yu

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Fri, 9 Sep 2016, Chen Yu wrote:
> > 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.

No you did not! You still have that silly x86 hackery in place.
 
> 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:

It's overengineered and horrible.
 
> +unsigned int timekeeping_tainted;

It does not taint timekeeping. It just wreckages the RTC.

> +void pm_trace_taint_timekeeping(void)
> +{
> + if (pm_trace_is_enabled()) {
> + timekeeping_tainted |= TIMEKEEPING_RTC_TAINTED;
> + if (arch_pm_trace_taint_pclock())
> + timekeeping_tainted |= 
> TIMEKEEPING_PERSISTENT_CLOCK_TAINTED;
> + }

Why would you need all these flags?

> +static inline int pm_trace_rtc_is_tainted(void)
> +{
> + return (timekeeping_tainted & TIMEKEEPING_RTC_TAINTED) ?
> + 1 : 0;

ever heard about bool?

> +}
> +

> +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?

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

--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -64,6 +64,15 @@ void mach_get_cmos_time(struct timespec
unsigned int status, year, mon, day, hour, min, sec, century = 0;
unsigned long flags;
 
+   /*
+* If pm trace abused the RTC as storage set the timespec to 0
+* which tells the caller that this RTC value is bogus.
+*/
+   if (!pm_trace_rtc_valid()) {
+   now->tv_sec = now->tv_nsec = 0;
+   return;
+   }
+
spin_lock_irqsave(_lock, flags);
 
/*
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -74,6 +74,7 @@
 
 #define DEVSEED (7919)
 
+bool pm_trace_rtc_abused __read_mostly;
 static unsigned int dev_hash_value;
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int 
device)
@@ -104,6 +105,7 @@ static int set_magic_time(unsigned int u
time.tm_min = (n % 20) * 3;
n /= 20;
mc146818_set_time();
+   pm_trace_rtc_abused = true;
return n ? -1 : 0;
 }
 
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -189,6 +189,13 @@ static inline void cmos_write_bank2(unsi
 
 static int cmos_read_time(struct device *dev, struct rtc_time *t)
 {
+   /*
+* If pmtrace abused the RTC for storage tell the caller that it is
+* unusable.
+*/
+   if (!pm_trace_rtc_valid())
+   return -EIO;
+
/* REVISIT:  if the clock has a "century" register, use
 * that instead of the heuristic in mc146818_get_time().
 * That'll make Y3K compatility (year > 2070) easy!
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -16,6 +16,7 @@
 #include/* register access macros */
 #include 
 #include 
+#include 
 
 #ifdef __KERNEL__
 #include /* spinlock_t */
--- a/include/linux/pm-trace.h
+++ b/include/linux/pm-trace.h
@@ -6,6 +6,12 @@
 #include 
 
 extern int pm_trace_enabled;
+extern bool pm_trace_rtc_abused;
+
+static inline bool pm_trace_rtc_valid(void)
+{
+   return !pm_trace_rtc_abused;
+}
 
 static inline int pm_trace_is_enabled(void)
 {
@@ -24,6 +30,7 @@ extern int show_trace_dev_match(char *bu
 
 #else
 
+static inline bool pm_trace_rtc_valid(void) { return true; }
 static inline int pm_trace_is_enabled(void) { return 0; }
 
 #define TRACE_DEVICE(dev) do { } while (0)

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Fri, 9 Sep 2016, Chen Yu wrote:
> > 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.

No you did not! You still have that silly x86 hackery in place.
 
> 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:

It's overengineered and horrible.
 
> +unsigned int timekeeping_tainted;

It does not taint timekeeping. It just wreckages the RTC.

> +void pm_trace_taint_timekeeping(void)
> +{
> + if (pm_trace_is_enabled()) {
> + timekeeping_tainted |= TIMEKEEPING_RTC_TAINTED;
> + if (arch_pm_trace_taint_pclock())
> + timekeeping_tainted |= 
> TIMEKEEPING_PERSISTENT_CLOCK_TAINTED;
> + }

Why would you need all these flags?

> +static inline int pm_trace_rtc_is_tainted(void)
> +{
> + return (timekeeping_tainted & TIMEKEEPING_RTC_TAINTED) ?
> + 1 : 0;

ever heard about bool?

> +}
> +

> +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?

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

--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -64,6 +64,15 @@ void mach_get_cmos_time(struct timespec
unsigned int status, year, mon, day, hour, min, sec, century = 0;
unsigned long flags;
 
+   /*
+* If pm trace abused the RTC as storage set the timespec to 0
+* which tells the caller that this RTC value is bogus.
+*/
+   if (!pm_trace_rtc_valid()) {
+   now->tv_sec = now->tv_nsec = 0;
+   return;
+   }
+
spin_lock_irqsave(_lock, flags);
 
/*
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -74,6 +74,7 @@
 
 #define DEVSEED (7919)
 
+bool pm_trace_rtc_abused __read_mostly;
 static unsigned int dev_hash_value;
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int 
device)
@@ -104,6 +105,7 @@ static int set_magic_time(unsigned int u
time.tm_min = (n % 20) * 3;
n /= 20;
mc146818_set_time();
+   pm_trace_rtc_abused = true;
return n ? -1 : 0;
 }
 
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -189,6 +189,13 @@ static inline void cmos_write_bank2(unsi
 
 static int cmos_read_time(struct device *dev, struct rtc_time *t)
 {
+   /*
+* If pmtrace abused the RTC for storage tell the caller that it is
+* unusable.
+*/
+   if (!pm_trace_rtc_valid())
+   return -EIO;
+
/* REVISIT:  if the clock has a "century" register, use
 * that instead of the heuristic in mc146818_get_time().
 * That'll make Y3K compatility (year > 2070) easy!
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -16,6 +16,7 @@
 #include/* register access macros */
 #include 
 #include 
+#include 
 
 #ifdef __KERNEL__
 #include /* spinlock_t */
--- a/include/linux/pm-trace.h
+++ b/include/linux/pm-trace.h
@@ -6,6 +6,12 @@
 #include 
 
 extern int pm_trace_enabled;
+extern bool pm_trace_rtc_abused;
+
+static inline bool pm_trace_rtc_valid(void)
+{
+   return !pm_trace_rtc_abused;
+}
 
 static inline int pm_trace_is_enabled(void)
 {
@@ -24,6 +30,7 @@ extern int show_trace_dev_match(char *bu
 
 #else
 
+static inline bool pm_trace_rtc_valid(void) { return true; }
 static inline int pm_trace_is_enabled(void) { return 0; }
 
 #define TRACE_DEVICE(dev) do { } while (0)

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi,
On Mon, Sep 05, 2016 at 01:54:20AM -0600, Thomas Gleixner wrote:
> On Sun, 4 Sep 2016, Chen Yu wrote:
> > Hi Thomas, Rafael,
> > On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> > > On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > > > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > > > +
> > > > > + /*
> > > > > +  * 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()
> > 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:
> 
> > +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.
OK, I've changed this patch according to this suggestion.

Previously I tried to deal with the case that x86 uses
RTC-CMOS as its presistent clock, not kvm_clock, nor
other get_wallclock, and this seems only be related to x86.
So this piece of code looks hackish.
> 
> 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.
> 
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:



Index: linux/drivers/base/power/trace.c
===
--- linux.orig/drivers/base/power/trace.c
+++ linux/drivers/base/power/trace.c
@@ -75,6 +75,27 @@
 #define DEVSEED (7919)
 
 static unsigned int dev_hash_value;
+unsigned int timekeeping_tainted;
+
+/* Is the persistent clock effected by pm_trace? */
+int __weak arch_pm_trace_taint_pclock(void)
+{
+   return 0;
+}
+
+void pm_trace_untaint_timekeeping(void)
+{
+   timekeeping_tainted = 0;
+}
+
+void pm_trace_taint_timekeeping(void)
+{
+   if (pm_trace_is_enabled()) {
+   timekeeping_tainted |= TIMEKEEPING_RTC_TAINTED;
+   if (arch_pm_trace_taint_pclock())
+   timekeeping_tainted |= 
TIMEKEEPING_PERSISTENT_CLOCK_TAINTED;
+   }
+}
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int 
device)
 {
@@ -104,6 +125,7 @@ static int set_magic_time(unsigned int u
time.tm_min = (n % 20) * 3;
n /= 20;
mc146818_set_time();
+   pm_trace_taint_timekeeping();
return n ? -1 : 0;
 }
 
Index: linux/kernel/power/main.c
===
--- linux.orig/kernel/power/main.c
+++ linux/kernel/power/main.c
@@ -548,6 +548,7 @@ pm_trace_store(struct kobject *kobj, str
if (sscanf(buf, "%d", ) == 1) {
pm_trace_enabled = !!val;
if (pm_trace_enabled) {
+   pm_trace_untaint_timekeeping();
pr_warn("PM: Enabling pm_trace changes system date and 
time during resume.\n"
"PM: Correct system time has to be restored 
manually after resume.\n");
}
Index: linux/include/linux/pm-trace.h
===
--- linux.orig/include/linux/pm-trace.h
+++ linux/include/linux/pm-trace.h
@@ -1,10 +1,14 @@
 #ifndef PM_TRACE_H
 #define PM_TRACE_H
 
+#define TIMEKEEPING_RTC_TAINTED 0x1
+#define TIMEKEEPING_PERSISTENT_CLOCK_TAINTED 0x2
+
 #ifdef CONFIG_PM_TRACE
 #include 
 #include 
 
+extern unsigned int timekeeping_tainted;
 extern int pm_trace_enabled;
 
 static inline int pm_trace_is_enabled(void)
@@ -12,10 +16,23 @@ static inline int pm_trace_is_enabled(vo
return pm_trace_enabled;
 }
 
+static inline int pm_trace_rtc_is_tainted(void)
+{
+   return 

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi,
On Mon, Sep 05, 2016 at 01:54:20AM -0600, Thomas Gleixner wrote:
> On Sun, 4 Sep 2016, Chen Yu wrote:
> > Hi Thomas, Rafael,
> > On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> > > On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > > > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > > > +
> > > > > + /*
> > > > > +  * 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()
> > 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:
> 
> > +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.
OK, I've changed this patch according to this suggestion.

Previously I tried to deal with the case that x86 uses
RTC-CMOS as its presistent clock, not kvm_clock, nor
other get_wallclock, and this seems only be related to x86.
So this piece of code looks hackish.
> 
> 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.
> 
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:



Index: linux/drivers/base/power/trace.c
===
--- linux.orig/drivers/base/power/trace.c
+++ linux/drivers/base/power/trace.c
@@ -75,6 +75,27 @@
 #define DEVSEED (7919)
 
 static unsigned int dev_hash_value;
+unsigned int timekeeping_tainted;
+
+/* Is the persistent clock effected by pm_trace? */
+int __weak arch_pm_trace_taint_pclock(void)
+{
+   return 0;
+}
+
+void pm_trace_untaint_timekeeping(void)
+{
+   timekeeping_tainted = 0;
+}
+
+void pm_trace_taint_timekeeping(void)
+{
+   if (pm_trace_is_enabled()) {
+   timekeeping_tainted |= TIMEKEEPING_RTC_TAINTED;
+   if (arch_pm_trace_taint_pclock())
+   timekeeping_tainted |= 
TIMEKEEPING_PERSISTENT_CLOCK_TAINTED;
+   }
+}
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int 
device)
 {
@@ -104,6 +125,7 @@ static int set_magic_time(unsigned int u
time.tm_min = (n % 20) * 3;
n /= 20;
mc146818_set_time();
+   pm_trace_taint_timekeeping();
return n ? -1 : 0;
 }
 
Index: linux/kernel/power/main.c
===
--- linux.orig/kernel/power/main.c
+++ linux/kernel/power/main.c
@@ -548,6 +548,7 @@ pm_trace_store(struct kobject *kobj, str
if (sscanf(buf, "%d", ) == 1) {
pm_trace_enabled = !!val;
if (pm_trace_enabled) {
+   pm_trace_untaint_timekeeping();
pr_warn("PM: Enabling pm_trace changes system date and 
time during resume.\n"
"PM: Correct system time has to be restored 
manually after resume.\n");
}
Index: linux/include/linux/pm-trace.h
===
--- linux.orig/include/linux/pm-trace.h
+++ linux/include/linux/pm-trace.h
@@ -1,10 +1,14 @@
 #ifndef PM_TRACE_H
 #define PM_TRACE_H
 
+#define TIMEKEEPING_RTC_TAINTED 0x1
+#define TIMEKEEPING_PERSISTENT_CLOCK_TAINTED 0x2
+
 #ifdef CONFIG_PM_TRACE
 #include 
 #include 
 
+extern unsigned int timekeeping_tainted;
 extern int pm_trace_enabled;
 
 static inline int pm_trace_is_enabled(void)
@@ -12,10 +16,23 @@ static inline int pm_trace_is_enabled(vo
return pm_trace_enabled;
 }
 
+static inline int pm_trace_rtc_is_tainted(void)
+{
+   return 

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Sun, 4 Sep 2016, Chen Yu wrote:
> Hi Thomas, Rafael,
> On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> > On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > > +
> > > > +   /*
> > > > +* 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()
> 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:

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

Thanks,

tglx

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Sun, 4 Sep 2016, Chen Yu wrote:
> Hi Thomas, Rafael,
> On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> > On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > > +
> > > > +   /*
> > > > +* 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()
> 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:

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

Thanks,

tglx

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi Thomas, Rafael,
On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > +
> > > + /*
> > > +  * 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()
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/

Thanks,
Yu

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

Hi Thomas, Rafael,
On Fri, Sep 02, 2016 at 09:26:51PM +0200, Thomas Gleixner wrote:
> On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> > On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > > +
> > > + /*
> > > +  * 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()
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/

Thanks,
Yu

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > +
> > +   /*
> > +* 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()

Thanks,

tglx

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Wed, 31 Aug 2016, Rafael J. Wysocki wrote:
> On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> > +
> > +   /*
> > +* 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()

Thanks,

tglx

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Rafael J. Wysocki]

On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> 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: sta...@vger.kernel.org  (3.17+)
> Cc: Rafael J. Wysocki 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Xunlei Pang 
> Cc: Zhang Rui 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Suggested-by: Xunlei Pang 
> Suggested-by: Rafael J. Wysocki 
> Suggested-by: Thomas Gleixner 
> Reported-by: Janek Kozicki 
> Signed-off-by: Chen Yu 
> ---
>  arch/x86/kernel/rtc.c | 12 
>  kernel/time/timekeeping.c |  3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 79c6311c..5c28197 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -144,6 +145,17 @@ int update_persistent_clock(struct timespec now)
>  void read_persistent_clock(struct timespec *ts)
>  {
>   x86_platform.get_wallclock(ts);
> +
> + /*
> +  * 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.

>  }
>  
>  
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 3b65746..9af885d 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1551,7 +1552,7 @@ static void __timekeeping_inject_sleeptime(struct 
> timekeeper *tk,
>   */
>  bool timekeeping_rtc_skipresume(void)
>  {
> - return sleeptime_injected;
> + return sleeptime_injected || pm_trace_is_enabled();
>  }
>  
>  /**

Thanks,
Rafael

Re: [PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Rafael J. Wysocki]

On Monday, August 29, 2016 12:40:39 AM Chen Yu wrote:
> 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: sta...@vger.kernel.org  (3.17+)
> Cc: Rafael J. Wysocki 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Xunlei Pang 
> Cc: Zhang Rui 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Suggested-by: Xunlei Pang 
> Suggested-by: Rafael J. Wysocki 
> Suggested-by: Thomas Gleixner 
> Reported-by: Janek Kozicki 
> Signed-off-by: Chen Yu 
> ---
>  arch/x86/kernel/rtc.c | 12 
>  kernel/time/timekeeping.c |  3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 79c6311c..5c28197 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -144,6 +145,17 @@ int update_persistent_clock(struct timespec now)
>  void read_persistent_clock(struct timespec *ts)
>  {
>   x86_platform.get_wallclock(ts);
> +
> + /*
> +  * 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.

>  }
>  
>  
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 3b65746..9af885d 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1551,7 +1552,7 @@ static void __timekeeping_inject_sleeptime(struct 
> timekeeper *tk,
>   */
>  bool timekeeping_rtc_skipresume(void)
>  {
> - return sleeptime_injected;
> + return sleeptime_injected || pm_trace_is_enabled();
>  }
>  
>  /**

Thanks,
Rafael

[PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

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: sta...@vger.kernel.org  (3.17+)
Cc: Rafael J. Wysocki 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Xunlei Pang 
Cc: Zhang Rui 
Cc: linux-kernel@vger.kernel.org
Cc: linux...@vger.kernel.org
Suggested-by: Xunlei Pang 
Suggested-by: Rafael J. Wysocki 
Suggested-by: Thomas Gleixner 
Reported-by: Janek Kozicki 
Signed-off-by: Chen Yu 
---
 arch/x86/kernel/rtc.c | 12 
 kernel/time/timekeeping.c |  3 ++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 79c6311c..5c28197 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -144,6 +145,17 @@ int update_persistent_clock(struct timespec now)
 void read_persistent_clock(struct timespec *ts)
 {
x86_platform.get_wallclock(ts);
+
+   /*
+* 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;
+   }
 }
 
 
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3b65746..9af885d 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -1551,7 +1552,7 @@ static void __timekeeping_inject_sleeptime(struct 
timekeeper *tk,
  */
 bool timekeeping_rtc_skipresume(void)
 {
-   return sleeptime_injected;
+   return sleeptime_injected || pm_trace_is_enabled();
 }
 
 /**
-- 
2.7.4

[PATCH][RFC v5] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

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: sta...@vger.kernel.org  (3.17+)
Cc: Rafael J. Wysocki 
Cc: John Stultz 
Cc: Thomas Gleixner 
Cc: Xunlei Pang 
Cc: Zhang Rui 
Cc: linux-kernel@vger.kernel.org
Cc: linux...@vger.kernel.org
Suggested-by: Xunlei Pang 
Suggested-by: Rafael J. Wysocki 
Suggested-by: Thomas Gleixner 
Reported-by: Janek Kozicki 
Signed-off-by: Chen Yu 
---
 arch/x86/kernel/rtc.c | 12 
 kernel/time/timekeeping.c |  3 ++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 79c6311c..5c28197 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -144,6 +145,17 @@ int update_persistent_clock(struct timespec now)
 void read_persistent_clock(struct timespec *ts)
 {
x86_platform.get_wallclock(ts);
+
+   /*
+* 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;
+   }
 }
 
 
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3b65746..9af885d 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -1551,7 +1552,7 @@ static void __timekeeping_inject_sleeptime(struct 
timekeeper *tk,
  */
 bool timekeeping_rtc_skipresume(void)
 {
-   return sleeptime_injected;
+   return sleeptime_injected || pm_trace_is_enabled();
 }
 
 /**
-- 
2.7.4