Re: [PATCH v2] time: Fix sleeptime injection for non-stop clocksource & persistent clock

[Mukesh Ojha]

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 <mo...@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