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;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 4A8B060116
Subject: Re: [PATCH v4] time: Fix extra sleeptime injection when suspend fails
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     john.stultz@linaro.org, linux-kernel@vger.kernel.org,
        gkohli@codeaurora.org, cpandya@codeaurora.org,
        neeraju@codeaurora.org
References: <1531773650-30424-1-git-send-email-mojha@codeaurora.org>
 <alpine.DEB.2.21.1807162241470.1693@nanos.tec.linutronix.de>
From:   Mukesh Ojha <mojha@codeaurora.org>
Message-ID: <6d6dbd5e-0fad-736e-197d-2c4546dda232@codeaurora.org>
Date:   Tue, 17 Jul 2018 11:49:46 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.21.1807162241470.1693@nanos.tec.linutronix.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 7/17/2018 2:20 AM, Thomas Gleixner wrote:
> 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();
> 	}

Will do suggested change and send in v5.

Thanks.
>
> 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
>