Re: [PATCH] time, ntp: Do not update time_state in middle of leap

From: Prarit Bhargava <prarit@redhat.com>
To: Miroslav Lichvar <mlichvar@redhat.com>
Cc: John Stultz <john.stultz@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] time, ntp: Do not update time_state in middle of leap
Date: Thu, 05 Feb 2015 08:20:08 -0500	[thread overview]
Message-ID: <54D36E08.1060400@redhat.com> (raw)
In-Reply-To: <20150204163005.GF26460@localhost>

On 02/04/2015 11:30 AM, Miroslav Lichvar wrote:
> Prarit Bhargava wrote:
>> While this is highly unlikely to ever happen in the real world it is
>> still something we should protect against, as breaking the state machine
>> is obviously bad.
> 
> I'm not sure what exactly breaks here. If the PLL is disabled before
> time_state is set to TIME_OOP, the insertion/deletion will be aborted.

Yes, that is correct.

> If after that, adjtimex() will return with TIME_ERROR as expected, or
> not?

It is possible that an adjtimex() will set the time_state here back to TIME_OK
and return TIME_OK to userspace.  Again, and I want to stress this, this is
extremely unlikely to happen.  I only hit this due to a bug in a test program.
But at the end of the day, it is possible that this happens and we should
protect against it.

[  942.952833] time_state [1] change from TIME_OK to TIME_INS

Fri Feb 13 18:59:51 2015 + 318126 us    TIME_INS
Fri Feb 13 18:59:51 2015 + 818167 us    TIME_INS
Fri Feb 13 18:59:52 2015 + 318208 us    TIME_INS
Fri Feb 13 18:59:52 2015 + 818248 us    TIME_INS
Fri Feb 13 18:59:53 2015 + 318290 us    TIME_INS
Fri Feb 13 18:59:53 2015 + 818331 us    TIME_INS
Fri Feb 13 18:59:54 2015 + 318372 us    TIME_INS
Fri Feb 13 18:59:54 2015 + 818413 us    TIME_INS
Fri Feb 13 18:59:55 2015 + 318454 us    TIME_INS
Fri Feb 13 18:59:55 2015 + 818495 us    TIME_INS
Fri Feb 13 18:59:56 2015 + 318534 us    TIME_INS
Fri Feb 13 18:59:56 2015 + 818575 us    TIME_INS
Fri Feb 13 18:59:57 2015 + 318617 us    TIME_INS
Fri Feb 13 18:59:57 2015 + 818660 us    TIME_INS
Fri Feb 13 18:59:58 2015 + 318702 us    TIME_INS
Fri Feb 13 18:59:58 2015 + 818744 us    TIME_INS
Fri Feb 13 18:59:59 2015 + 318785 us    TIME_INS
Fri Feb 13 18:59:59 2015 + 818837 us    TIME_INS

[  952.953143] time_state [4] change from TIME_INS to TIME_OOP
[  952.953150] Clock: inserting leap second 23:59:60 UTC
[  953.299905] process_adj_status: insert_leap_sec[1223] setting time_state back
to TIME_OK [1, 1]   <<< adjtimex() call
[  953.299913] time_state [9] change from TIME_OOP to TIME_OK

Fri Feb 13 18:59:59 2015 + 318878 us    TIME_OK
Fri Feb 13 18:59:59 2015 + 818931 us    TIME_OK

[  954.064237] time_state [1] change from TIME_OK to TIME_INS

Fri Feb 13 19:00:00 2015 + 318972 us    TIME_INS
Fri Feb 13 19:00:00 2015 + 819012 us    TIME_INS
Fri Feb 13 19:00:01 2015 + 319051 us    TIME_INS
Fri Feb 13 19:00:01 2015 + 819089 us    TIME_INS
Fri Feb 13 19:00:02 2015 + 319128 us    TIME_INS

P.

> 
>>  static inline void process_adj_status(struct timex *txc, struct timespec64 *ts)
>>  {
>> -	if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
>> +	if ((time_status & STA_PLL) && !(txc->status & STA_PLL) &&
>> +	    (time_state != TIME_OOP)) {
>>  		time_state = TIME_OK;
>>  		time_status = STA_UNSYNC;
>>  		/* restart PPS frequency calibration */
> 
> Shouldn't be time_status reset and the PPS calibration restarted even
> when state is TIME_OOP?

No, this should only happen after the leap second is done IMO (which should be
no more than 2 seconds later).

> 