Message-ID: <54D49C83.40706@redhat.com>
Date: Fri, 06 Feb 2015 05:50:43 -0500
From: Prarit Bhargava <prarit@redhat.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131028 Thunderbird/17.0.10
MIME-Version: 1.0
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
References: <20150204163005.GF26460@localhost> <54D36E08.1060400@redhat.com> <20150206103815.GB23998@localhost>
In-Reply-To: <20150206103815.GB23998@localhost>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2503
Lines: 59


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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/