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;
Date:   Mon, 16 Jul 2018 22:50:38 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Mukesh Ojha <mojha@codeaurora.org>
cc:     john.stultz@linaro.org, linux-kernel@vger.kernel.org,
        gkohli@codeaurora.org, cpandya@codeaurora.org,
        neeraju@codeaurora.org
Subject: Re: [PATCH v4] time: Fix extra sleeptime injection when suspend
 fails
In-Reply-To: <1531773650-30424-1-git-send-email-mojha@codeaurora.org>
Message-ID: <alpine.DEB.2.21.1807162241470.1693@nanos.tec.linutronix.de>
References: <1531773650-30424-1-git-send-email-mojha@codeaurora.org>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 17 Jul 2018, Mukesh Ojha wrote:
> @@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
>  	struct timespec64	sleep_time;
>  	int err;
>  
> -	if (timekeeping_rtc_skipresume())
> +	if (!timekeeping_rtc_skipresume())
>  		return 0;

That does not make any sense at all, really. 

>  /* Flag for if there is a persistent clock on this platform */
>  static bool persistent_clock_exists;
> @@ -1610,7 +1622,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
>   */
>  bool timekeeping_rtc_skipresume(void)
>  {
> -	return sleeptime_injected;
> +	return suspend_timing_needed;

Just make this !suspend_timing_needed and the function name and its return
value still makes sense.

> @@ -1701,13 +1714,13 @@ void timekeeping_resume(void)
>  					      tk->tkr_mono.mask);
>  		nsec = mul_u64_u32_shr(cyc_delta, clock->mult, clock->shift);
>  		ts_delta = ns_to_timespec64(nsec);
> -		sleeptime_injected = true;
> +		suspend_timing_needed = false;
>  	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
>  		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
> -		sleeptime_injected = true;
> +		suspend_timing_needed = false;
>  	}
>  
> -	if (sleeptime_injected)
> +	if (!suspend_timing_needed)
>  		__timekeeping_inject_sleeptime(tk, &ts_delta);

This reads odd as well. I'd rather keep a local variable inject_sleeptime
or such and set that in the code pathes above.

   	if (...) {
	   	...
		inject_sleeptime = true;
   	} else if (...) {
	   	...
		inject_sleeptime = true;
	}

	if (inject_sleeptime) {
		suspend_timing_needed = false;
		__timekeeping_inject_sleeptime();
	}

Hmm? Just blindly converting everything results in functional, but
nonsensical code. Think about what happens when you look at that stuff 6
month from now...

Thanks,

	tglx