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

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

[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' due to which we can see mismatch in time between
system clock and other timers.

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 from Resend:
 * updated commit text.

 kernel/time/timekeeping.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 427e33d..c022d82 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1511,9 +1511,6 @@ 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 for if there is a persistent clock on this platform */
 static bool persistent_clock_exists;
 
@@ -1611,7 +1608,14 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
  */
 bool timekeeping_rtc_skipresume(void)
 {
-	return sleeptime_injected;
+	struct timekeeper *tk = &tk_core.timekeeper;
+	bool skip_rtc_resume = false;
+
+	skip_rtc_resume = ((tk->tkr_mono.clock->flags &
+			CLOCK_SOURCE_SUSPEND_NONSTOP) ||
+			(persistent_clock_exists)) ? true : false;
+
+	return skip_rtc_resume;
 }
 
 /**
@@ -1671,8 +1675,8 @@ void timekeeping_resume(void)
 	unsigned long flags;
 	struct timespec64 ts_new, ts_delta;
 	cycle_t cycle_now, cycle_delta;
+	bool sleeptime_injected = false;
 
-	sleeptime_injected = false;
 	read_persistent_clock64(&ts_new);
 
 	clockevents_resume();
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

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

[John Stultz]

On Tue, May 29, 2018 at 2:49 AM, Mukesh Ojha <mojha@codeaurora.org> 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' due to which we can see mismatch in time between
> system clock and other timers.
>
> 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>

I'm not sure this patch makes sense yet (since I don't really see how
its used - mind cc'ing me on the patch that makes use of this?)

And more problematic, the patch doesn't seem to apply to mainline.
Could you respin and resend?

thanks
-john

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

[Mukesh Ojha]

Hi John,


On 5/30/2018 7:50 AM, John Stultz wrote:
> On Tue, May 29, 2018 at 2:49 AM, Mukesh Ojha <mojha@codeaurora.org> 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' due to which we can see mismatch in time between
>> system clock and other timers.
>>
>> 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>
> I'm not sure this patch makes sense yet (since I don't really see how
> its used

In a system where both non-stop clocksource and rtc exist

Timestamp mismatch seen between a (Gyro)timer based on non-stop clocksource
and system clock after attempting a sleep.
Here, System clock timestamps increase by some millisecond as compare to
other timer.

And we see this is possible only when timekeeping_resume() has not been 
called
which is keeping sleeptime_injected to its default value(false). So, the 
call to
timekeeping_rtc_skipresume() in rtc_resume() is false and thats ends up in
sleeptime injection via rtc_resume().


When timekeeping_resume() has not been called means one of the driver failed
during suspend in syscore_suspend() in kernel/power/suspend.c .

So if we let rtc inject the sleeptime here, we would see the timestamp 
increase in case of
system clock.

>   - mind cc'ing me on the patch that makes use of this?)
>
> And more problematic, the patch doesn't seem to apply to mainline.
> Could you respin and resend?

Will spin the patch again with some minor code change.

Thanks,
Mukesh

>
> thanks
> -john