| Message ID | 1472402439-1146-1-git-send-email-yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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: stable@vger.kernel.org (3.17+) > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Xunlei Pang <xlpang@redhat.com> > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: linux-kernel@vger.kernel.org > Cc: linux-pm@vger.kernel.org > Suggested-by: Xunlei Pang <xlpang@redhat.com> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Reported-by: Janek Kozicki <cosurgi@gmail.com> > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > 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 <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); > + > + /* > + * 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 <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" > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <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); + + /* + * 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 <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" @@ -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(); } /**
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: stable@vger.kernel.org (3.17+) Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: John Stultz <john.stultz@linaro.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Xunlei Pang <xlpang@redhat.com> Cc: Zhang Rui <rui.zhang@intel.com> Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org Suggested-by: Xunlei Pang <xlpang@redhat.com> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Suggested-by: Thomas Gleixner <tglx@linutronix.de> Reported-by: Janek Kozicki <cosurgi@gmail.com> Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- arch/x86/kernel/rtc.c | 12 ++++++++++++ kernel/time/timekeeping.c | 3 ++- 2 files changed, 14 insertions(+), 1 deletion(-)