| Message ID | 1425882471-5591-2-git-send-email-xlpang@126.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Mar 8, 2015 at 11:27 PM, Xunlei Pang <xlpang@126.com> wrote: > From: Xunlei Pang <pang.xunlei@linaro.org> > > When there's no persistent clock, normally timekeeping_suspend_time > should always be zero, but this can break in timekeeping_suspend(). > > At T1, there was a system suspend, so old_delta was assigned T1. > After some time, one time adjustment happened, and xtime got the > value of T1-dt(0s<dt<2s). Then, there comes another system suspend > soon after this adjustment, obviously we will get a small negative > delta_delta, resulting in a negative timekeeping_suspend_time. > > This is problematic, when doing timekeeping_resume() if there is > no nonstop clocksource for example, it will hit the else leg and > inject the improper sleeptime which is the wrong logic. > > So, we can solve this problem by only doing delta related code when > the persistent clock is existent. Actually the code only makes sense > for persistent clock cases. > > This patch also improves the name of timekeeping_suspend_time. So re-reviewing this here. I really think the renaming here isn't an improvement. It does clarify that its related only to the persistent clock, but the resulting code is much uglier due to the 80col reformatting you've done. Can we drop the rename, which should make the fix in this code more clear? thanks -john
John Stultz <john.stultz@linaro.org> wrote 2015-03-19 AM 01:17:15: > John Stultz <john.stultz@linaro.org> > 2015-03-19 AM 01:17 > > This patch also improves the name of timekeeping_suspend_time. > > So re-reviewing this here. I really think the renaming here isn't an > improvement. It does clarify that its related only to the persistent > clock, but the resulting code is much uglier due to the 80col > reformatting you've done. > > Can we drop the rename, which should make the fix in this code more clear? Indeed, the code becomes ugly due to my renaming, sometimes the 80col rule is awful :) I'll modify this and send out the rest of this series separately. Thanks! Regards, Xunlei -------------------------------------------------------- ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately. -------------------------------------------------------- ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 49b1643..2aae419 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1100,8 +1100,8 @@ void __init timekeeping_init(void) raw_spin_unlock_irqrestore(&timekeeper_lock, flags); } -/* time in seconds when suspend began */ -static struct timespec64 timekeeping_suspend_time; +/* time in seconds when suspend began for persistent clock */ +static struct timespec64 persistent_clock_suspendtime; /** * __timekeeping_inject_sleeptime - Internal function to add sleep interval @@ -1229,8 +1229,9 @@ static void timekeeping_resume(void) ts_delta = ns_to_timespec64(nsec); suspendtime_found = true; - } else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) { - ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time); + } else if (timespec64_compare(&ts_new, + &persistent_clock_suspendtime) > 0) { + ts_delta = timespec64_sub(ts_new, persistent_clock_suspendtime); suspendtime_found = true; } @@ -1262,14 +1263,15 @@ static int timekeeping_suspend(void) struct timespec tmp; read_persistent_clock(&tmp); - timekeeping_suspend_time = timespec_to_timespec64(tmp); + persistent_clock_suspendtime = timespec_to_timespec64(tmp); /* * On some systems the persistent_clock can not be detected at * timekeeping_init by its return value, so if we see a valid * value returned, update the persistent_clock_exists flag. */ - if (timekeeping_suspend_time.tv_sec || timekeeping_suspend_time.tv_nsec) + if (persistent_clock_suspendtime.tv_sec || + persistent_clock_suspendtime.tv_nsec) persistent_clock_exist = true; raw_spin_lock_irqsave(&timekeeper_lock, flags); @@ -1277,24 +1279,28 @@ static int timekeeping_suspend(void) timekeeping_forward_now(tk); timekeeping_suspended = 1; - /* - * To avoid drift caused by repeated suspend/resumes, - * which each can add ~1 second drift error, - * try to compensate so the difference in system time - * and persistent_clock time stays close to constant. - */ - delta = timespec64_sub(tk_xtime(tk), timekeeping_suspend_time); - delta_delta = timespec64_sub(delta, old_delta); - if (abs(delta_delta.tv_sec) >= 2) { + if (has_persistent_clock()) { /* - * if delta_delta is too large, assume time correction - * has occured and set old_delta to the current delta. + * To avoid drift caused by repeated suspend/resumes, + * which each can add ~1 second drift error, + * try to compensate so the difference in system time + * and persistent_clock time stays close to constant. */ - old_delta = delta; - } else { - /* Otherwise try to adjust old_system to compensate */ - timekeeping_suspend_time = - timespec64_add(timekeeping_suspend_time, delta_delta); + delta = timespec64_sub(tk_xtime(tk), + persistent_clock_suspendtime); + delta_delta = timespec64_sub(delta, old_delta); + if (abs(delta_delta.tv_sec) >= 2) { + /* + * if delta_delta is too large, assume time correction + * has occurred and set old_delta to the current delta. + */ + old_delta = delta; + } else { + /* Otherwise try to adjust old_system to compensate */ + persistent_clock_suspendtime = + timespec64_add(persistent_clock_suspendtime, + delta_delta); + } } timekeeping_update(tk, TK_MIRROR);