* [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock @ 2018-05-30 11:44 Mukesh Ojha 2018-06-14 14:07 ` Mukesh Ojha 2018-06-22 21:27 ` Thomas Gleixner 0 siblings, 2 replies; 7+ messages in thread From: Mukesh Ojha @ 2018-05-30 11:44 UTC (permalink / raw) To: john.stultz, tglx, linux-kernel; +Cc: neeraju, gkohli, cpandya, Mukesh Ojha Currently, for both non-stop clocksource and persistent clock there is a corner case, when a driver failed to go suspend mode. rtc_resume() injects the sleeptime as timekeeping_rtc_skipresume() returned 'false'(sleeptime_injected=false) due to which we can see mismatch in timestamps between system clock and other timers. Fix this by updating sleeptime_injected=true for both non-stop clocksource and persistent clock. Success case: ------------ {sleeptime_injected=true} rtc_suspend() => timekeeping_suspend() => timekeeping_resume() => rtc_resume() Failure case: ------------ {failure in sleep path} {sleeptime_injected=false} rtc_suspend() => rtc_resume() Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> --- Changes in v2: * Updated the commit text. * Removed extra variable and used the earlier static variable 'sleeptime_injected'. kernel/time/timekeeping.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 49cbcee..2754c1b 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1610,6 +1610,17 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk, */ bool timekeeping_rtc_skipresume(void) { + struct timekeeper *tk = &tk_core.timekeeper; + /* + * This is to ensure that we don't end up injecting + * the sleeptime via rtc_resume() for non-stop clocksource + * when we fail to sleep. + */ + if (!sleeptime_injected) + sleeptime_injected = ((tk->tkr_mono.clock->flags & + CLOCK_SOURCE_SUSPEND_NONSTOP) || + (persistent_clock_exists)) ? true : false; + return sleeptime_injected; } -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-05-30 11:44 [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock Mukesh Ojha @ 2018-06-14 14:07 ` Mukesh Ojha 2018-06-22 21:27 ` Thomas Gleixner 1 sibling, 0 replies; 7+ messages in thread From: Mukesh Ojha @ 2018-06-14 14:07 UTC (permalink / raw) To: john.stultz, tglx, linux-kernel; +Cc: neeraju, gkohli, cpandya Any input on this ? Thanks, Mukesh On 5/30/2018 5:14 PM, Mukesh Ojha wrote: > Currently, for both non-stop clocksource and persistent clock > there is a corner case, when a driver failed to go suspend mode. > rtc_resume() injects the sleeptime as timekeeping_rtc_skipresume() > returned 'false'(sleeptime_injected=false) due to which we can > see mismatch in timestamps between system clock and other timers. > > Fix this by updating sleeptime_injected=true for both non-stop > clocksource and persistent clock. > > Success case: > ------------ > {sleeptime_injected=true} > rtc_suspend() => timekeeping_suspend() => timekeeping_resume() => > rtc_resume() > > Failure case: > ------------ > {failure in sleep path} {sleeptime_injected=false} > rtc_suspend() => rtc_resume() > > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > --- > Changes in v2: > * Updated the commit text. > * Removed extra variable and used the earlier static > variable 'sleeptime_injected'. > > kernel/time/timekeeping.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 49cbcee..2754c1b 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -1610,6 +1610,17 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk, > */ > bool timekeeping_rtc_skipresume(void) > { > + struct timekeeper *tk = &tk_core.timekeeper; > + /* > + * This is to ensure that we don't end up injecting > + * the sleeptime via rtc_resume() for non-stop clocksource > + * when we fail to sleep. > + */ > + if (!sleeptime_injected) > + sleeptime_injected = ((tk->tkr_mono.clock->flags & > + CLOCK_SOURCE_SUSPEND_NONSTOP) || > + (persistent_clock_exists)) ? true : false; > + > return sleeptime_injected; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-05-30 11:44 [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock Mukesh Ojha 2018-06-14 14:07 ` Mukesh Ojha @ 2018-06-22 21:27 ` Thomas Gleixner 2018-06-25 14:38 ` Mukesh Ojha 1 sibling, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2018-06-22 21:27 UTC (permalink / raw) To: Mukesh Ojha; +Cc: john.stultz, linux-kernel, neeraju, gkohli, cpandya On Wed, 30 May 2018, Mukesh Ojha wrote: > Currently, for both non-stop clocksource and persistent clock > there is a corner case, when a driver failed to go suspend mode. > rtc_resume() injects the sleeptime as timekeeping_rtc_skipresume() > returned 'false'(sleeptime_injected=false) due to which we can > see mismatch in timestamps between system clock and other timers. > > Fix this by updating sleeptime_injected=true for both non-stop > clocksource and persistent clock. > > Success case: > ------------ > {sleeptime_injected=true} > rtc_suspend() => timekeeping_suspend() => timekeeping_resume() => > rtc_resume() > > Failure case: > ------------ > {failure in sleep path} {sleeptime_injected=false} > rtc_suspend() => rtc_resume() I can see the problem. > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > --- > Changes in v2: > * Updated the commit text. > * Removed extra variable and used the earlier static > variable 'sleeptime_injected'. > > kernel/time/timekeeping.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 49cbcee..2754c1b 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -1610,6 +1610,17 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk, > */ > bool timekeeping_rtc_skipresume(void) > { > + struct timekeeper *tk = &tk_core.timekeeper; > + /* > + * This is to ensure that we don't end up injecting > + * the sleeptime via rtc_resume() for non-stop clocksource > + * when we fail to sleep. > + */ > + if (!sleeptime_injected) > + sleeptime_injected = ((tk->tkr_mono.clock->flags & > + CLOCK_SOURCE_SUSPEND_NONSTOP) || > + (persistent_clock_exists)) ? true : false; But this is really a horrible hack. The right thing to do is to keep track whether timekeeping_suspend() has been reached in the first place. There is a very simple way to do that. Uncompiled and completely untested patch below, but you get the idea. Thanks, tglx 8<------------------- diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 4786df904c22..32ae9aea61c3 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1510,8 +1510,20 @@ void __weak read_boot_clock64(struct timespec64 *ts) ts->tv_nsec = 0; } -/* Flag for if timekeeping_resume() has injected sleeptime */ -static bool sleeptime_injected; +/* + * Flag reflecting whether timekeeping_resume() has injected sleeptime. + * + * The flag starts of true and is only cleared when a suspend reaches + * timekeeping_suspend(), timekeeping_resume() sets it when the timekeeper + * clocksource is not stopping across suspend and has been used to update + * sleep time. If the timekeeper clocksource has stopped then the flag + * stays false and is used by the RTC resume code to decide whether sleep + * time must be injected and if so the flag gets set then. + * + * If a suspend fails before reaching timekeeping_resume() then the flag + * stays true and prevents erroneous sleeptime injection. + */ +static bool sleeptime_injected = true; /* Flag for if there is a persistent clock on this platform */ static bool persistent_clock_exists; @@ -1646,6 +1658,8 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta) raw_spin_lock_irqsave(&timekeeper_lock, flags); write_seqcount_begin(&tk_core.seq); + sleeptime_injected = true; + timekeeping_forward_now(tk); __timekeeping_inject_sleeptime(tk, delta); @@ -1671,7 +1685,6 @@ void timekeeping_resume(void) struct timespec64 ts_new, ts_delta; u64 cycle_now; - sleeptime_injected = false; read_persistent_clock64(&ts_new); clockevents_resume(); @@ -1743,6 +1756,8 @@ int timekeeping_suspend(void) if (timekeeping_suspend_time.tv_sec || timekeeping_suspend_time.tv_nsec) persistent_clock_exists = true; + sleeptime_injected = false; + raw_spin_lock_irqsave(&timekeeper_lock, flags); write_seqcount_begin(&tk_core.seq); timekeeping_forward_now(tk); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-06-22 21:27 ` Thomas Gleixner @ 2018-06-25 14:38 ` Mukesh Ojha 2018-06-25 15:04 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: Mukesh Ojha @ 2018-06-25 14:38 UTC (permalink / raw) To: Thomas Gleixner; +Cc: john.stultz, linux-kernel, neeraju, gkohli, cpandya Hi Thomas, Thanks you very much for your time and reply. On 6/23/2018 2:57 AM, Thomas Gleixner wrote: > On Wed, 30 May 2018, Mukesh Ojha wrote: >> Currently, for both non-stop clocksource and persistent clock >> there is a corner case, when a driver failed to go suspend mode. >> rtc_resume() injects the sleeptime as timekeeping_rtc_skipresume() >> returned 'false'(sleeptime_injected=false) due to which we can >> see mismatch in timestamps between system clock and other timers. >> >> Fix this by updating sleeptime_injected=true for both non-stop >> clocksource and persistent clock. >> >> Success case: >> ------------ >> {sleeptime_injected=true} >> rtc_suspend() => timekeeping_suspend() => timekeeping_resume() => >> rtc_resume() >> >> Failure case: >> ------------ >> {failure in sleep path} {sleeptime_injected=false} >> rtc_suspend() => rtc_resume() > I can see the problem. > >> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >> --- >> Changes in v2: >> * Updated the commit text. >> * Removed extra variable and used the earlier static >> variable 'sleeptime_injected'. >> >> kernel/time/timekeeping.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c >> index 49cbcee..2754c1b 100644 >> --- a/kernel/time/timekeeping.c >> +++ b/kernel/time/timekeeping.c >> @@ -1610,6 +1610,17 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk, >> */ >> bool timekeeping_rtc_skipresume(void) >> { >> + struct timekeeper *tk = &tk_core.timekeeper; >> + /* >> + * This is to ensure that we don't end up injecting >> + * the sleeptime via rtc_resume() for non-stop clocksource >> + * when we fail to sleep. >> + */ >> + if (!sleeptime_injected) >> + sleeptime_injected = ((tk->tkr_mono.clock->flags & >> + CLOCK_SOURCE_SUSPEND_NONSTOP) || >> + (persistent_clock_exists)) ? true : false; > But this is really a horrible hack. The right thing to do is to keep track > whether timekeeping_suspend() has been reached in the first place. There is > a very simple way to do that. Uncompiled and completely untested patch > below, but you get the idea. Yeah, missed completely the fact that the issue can also come where only clocksource is RTC. > Thanks, > > tglx > > 8<------------------- > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 4786df904c22..32ae9aea61c3 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -1510,8 +1510,20 @@ void __weak read_boot_clock64(struct timespec64 *ts) > ts->tv_nsec = 0; > } > > -/* Flag for if timekeeping_resume() has injected sleeptime */ > -static bool sleeptime_injected; > +/* > + * Flag reflecting whether timekeeping_resume() has injected sleeptime. > + * > + * The flag starts of true and is only cleared when a suspend reaches > + * timekeeping_suspend(), timekeeping_resume() sets it when the timekeeper > + * clocksource is not stopping across suspend and has been used to update > + * sleep time. If the timekeeper clocksource has stopped then the flag > + * stays false and is used by the RTC resume code to decide whether sleep > + * time must be injected and if so the flag gets set then. > + * > + * If a suspend fails before reaching timekeeping_resume() then the flag > + * stays true and prevents erroneous sleeptime injection. > + */ > +static bool sleeptime_injected = true; This will prevent first sleep failure. > > /* Flag for if there is a persistent clock on this platform */ > static bool persistent_clock_exists; > @@ -1646,6 +1658,8 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta) > raw_spin_lock_irqsave(&timekeeper_lock, flags); > write_seqcount_begin(&tk_core.seq); > > + sleeptime_injected = true; This will prevent further extra sleeptime injection if sleep fails (valid for RTC only). Looks good! > + > timekeeping_forward_now(tk); > > __timekeeping_inject_sleeptime(tk, delta); > @@ -1671,7 +1685,6 @@ void timekeeping_resume(void) > struct timespec64 ts_new, ts_delta; > u64 cycle_now; > > - sleeptime_injected = false; > read_persistent_clock64(&ts_new); > > clockevents_resume(); > @@ -1743,6 +1756,8 @@ int timekeeping_suspend(void) > if (timekeeping_suspend_time.tv_sec || timekeeping_suspend_time.tv_nsec) > persistent_clock_exists = true; > > + sleeptime_injected = false; I did not get the exact valid point of moving it from `timekeeping_suspend` to `timekeeping_resume`. Although it will not have any side effect. > + > raw_spin_lock_irqsave(&timekeeper_lock, flags); > write_seqcount_begin(&tk_core.seq); > timekeeping_forward_now(tk); > Thanks for the change; will check and update. Cheers, Mukesh ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-06-25 14:38 ` Mukesh Ojha @ 2018-06-25 15:04 ` Thomas Gleixner 2018-07-06 8:15 ` Mukesh Ojha 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2018-06-25 15:04 UTC (permalink / raw) To: Mukesh Ojha; +Cc: john.stultz, linux-kernel, neeraju, gkohli, cpandya On Mon, 25 Jun 2018, Mukesh Ojha wrote: > On 6/23/2018 2:57 AM, Thomas Gleixner wrote: > > @@ -1671,7 +1685,6 @@ void timekeeping_resume(void) > > struct timespec64 ts_new, ts_delta; > > u64 cycle_now; > > - sleeptime_injected = false; > > read_persistent_clock64(&ts_new); > > clockevents_resume(); > > @@ -1743,6 +1756,8 @@ int timekeeping_suspend(void) > > if (timekeeping_suspend_time.tv_sec || > > timekeeping_suspend_time.tv_nsec) > > persistent_clock_exists = true; > > + sleeptime_injected = false; > > I did not get the exact valid point of moving it from `timekeeping_suspend` to > `timekeeping_resume`. It's the other way round. I move it from resume to suspend. Simply because it should only be set to 'false' when suspend is reached. It would work the other way round as well, but I felt it's inconsistent. Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-06-25 15:04 ` Thomas Gleixner @ 2018-07-06 8:15 ` Mukesh Ojha 2018-07-06 8:58 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: Mukesh Ojha @ 2018-07-06 8:15 UTC (permalink / raw) To: Thomas Gleixner; +Cc: john.stultz, linux-kernel, neeraju, gkohli, cpandya Hi Thomas, Could you raise a formal patch on this as you are the author now? Thanks, Mukesh On 6/25/2018 8:34 PM, Thomas Gleixner wrote: > On Mon, 25 Jun 2018, Mukesh Ojha wrote: >> On 6/23/2018 2:57 AM, Thomas Gleixner wrote: >>> @@ -1671,7 +1685,6 @@ void timekeeping_resume(void) >>> struct timespec64 ts_new, ts_delta; >>> u64 cycle_now; >>> - sleeptime_injected = false; >>> read_persistent_clock64(&ts_new); >>> clockevents_resume(); >>> @@ -1743,6 +1756,8 @@ int timekeeping_suspend(void) >>> if (timekeeping_suspend_time.tv_sec || >>> timekeeping_suspend_time.tv_nsec) >>> persistent_clock_exists = true; >>> + sleeptime_injected = false; >> I did not get the exact valid point of moving it from `timekeeping_suspend` to >> `timekeeping_resume`. > It's the other way round. I move it from resume to suspend. Simply because > it should only be set to 'false' when suspend is reached. It would work the > other way round as well, but I felt it's inconsistent. > > Thanks, > > tglx > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock 2018-07-06 8:15 ` Mukesh Ojha @ 2018-07-06 8:58 ` Thomas Gleixner 0 siblings, 0 replies; 7+ messages in thread From: Thomas Gleixner @ 2018-07-06 8:58 UTC (permalink / raw) To: Mukesh Ojha; +Cc: john.stultz, linux-kernel, neeraju, gkohli, cpandya On Fri, 6 Jul 2018, Mukesh Ojha wrote: > Hi Thomas, > > Could you raise a formal patch on this as you are the author now? I'm short of cycles ATM. Feel free to pick it up and write a nice changelog. Just add Originally-by: Thomas Gleixner <tglx@linutronix.de> Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-06 8:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-30 11:44 [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock Mukesh Ojha 2018-06-14 14:07 ` Mukesh Ojha 2018-06-22 21:27 ` Thomas Gleixner 2018-06-25 14:38 ` Mukesh Ojha 2018-06-25 15:04 ` Thomas Gleixner 2018-07-06 8:15 ` Mukesh Ojha 2018-07-06 8:58 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).