Re: [PATCH v3] time: Fix incorrect sleeptime injection when suspend fails

[John Stultz <john.stultz@linaro.org>]

On Fri, Jul 6, 2018 at 6:17 AM, Mukesh Ojha <mojha@codeaurora.org> wrote:
> Currently, there exists a corner case assuming when there is
> only one clocksource e.g RTC, and system failed to go to
> suspend mode. While resume rtc_resume() injects the sleeptime
> as timekeeping_rtc_skipresume() returned 'false' (default value
> of sleeptime_injected) due to which we can see mismatch in
> timestamps.
>
> This issue can also come in a system where more than one
> clocksource are present and very first suspend fails.
>
> Fix this by handling `sleeptime_injected` flag properly.
>
> Success case:
> ------------
>                                         {sleeptime_injected=false}
> rtc_suspend() => timekeeping_suspend() => timekeeping_resume() =>
>
> (sleeptime injected)
>  rtc_resume()
>
> Failure case:
> ------------
>          {failure in sleep path} {sleeptime_injected=false}
> rtc_suspend()     =>          rtc_resume()
>
> sleeptime injected again which was not required as the suspend failed)
>
> Originally-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
> ---
> Changes in v3:
>  * Updated commit subject and description.
>  * Updated the patch as per the fix given by Thomas Gleixner.
>
> Changes in v2:
>  * Updated the commit text.
>  * Removed extra variable and used the earlier static
>    variable 'sleeptime_injected'.
>
>  kernel/time/timekeeping.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 4786df9..32ae9ae 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;

I worry this upside-down logic is too subtle to be easily reasoned
about, and will just lead to future mistakes.

Can we instead call this "suspend_timing_needed" and only set it to
true when we don't inject any sleep time on resume?

I think that will help make things a bit more clear, no?

thanks
-john