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

[Prarit Bhargava <prarit@redhat.com>]

On 02/06/2015 05:38 AM, Miroslav Lichvar wrote:
> On Thu, Feb 05, 2015 at 08:20:08AM -0500, Prarit Bhargava wrote:
>> On 02/04/2015 11:30 AM, Miroslav Lichvar wrote:
>>> 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.
> 
> Could it break any applications? I guess PLL is normally disabled only
> when a time synchronization process ends. FWIW, the reference
> nanokernel implementation has this too.

Not that I saw.  I did take a look with top, etc., to see if anything in
userspace went bad, and I ran programs that were calling gettimeofday() and
clock_gettime() to see if there were any problems.  I didn't see anything.  I
also played around with a program to see if the timer expiry failed but again,
didn't see anything.

The outcome of TIME_INS->TIME_OOP->TIME_OK->TIME_INS, AFAICT was only that
TIME_INS was left issued.  Which could lead to another leap second insertion
down the road unless ntp (or some other program) was left to reset the state.

> 
>>>> -	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).
> 
> But that will not happen automatically, the application would have to
> enable and disable the PLL again. Interestingly, the "time_status =
> STA_UNSYNC" assignment doesn't seem to do anything here, as the

Hmmm ... good point.  I didn't think of that.  Let me go back and change the
code to do the reset.

> variable is always reset couple lines after that, STA_UNSYNC is not a
> readonly flag.
> 

P.