Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <1530883047-17452-1-git-send-email-mojha@codeaurora.org>
References: <1530883047-17452-1-git-send-email-mojha@codeaurora.org>
From:   John Stultz <john.stultz@linaro.org>
Date:   Tue, 10 Jul 2018 13:13:35 -0700
Message-ID: <CALAqxLVs=EbbJ83bw=YRb=1B7E2uLcnrUh+qWEjm9U3iYd7FAg@mail.gmail.com>
Subject: Re: [PATCH v3] time: Fix incorrect sleeptime injection when suspend fails
To:     Mukesh Ojha <mojha@codeaurora.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        lkml <linux-kernel@vger.kernel.org>, gkohli@codeaurora.org,
        cpandya@codeaurora.org, neeraju@codeaurora.org,
        Baolin Wang <baolin.wang@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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