>
> Ulrich> There's a very old bug in the kernel that rarely occurs (who's
> Ulrich> using NTP?): The code to update the CMOS clock does not consider
> Ulrich> the hour field. I had no clever idea to fix it, so I simply added
> Ulrich> a message to log the problem (well, a part of it).
>
> Oh, no, please... If you have no clever idea to fix it -- a log message every
> 11 minutes ? This behaviour is _intentional_ -- I have no idea about the time
Hmm (Not to say I'm getting tired): The message WILL NOT occur every
11 minutes, it will only occur if the clock could not be updated. In
fact the message could even reappear after one minute (that's the way
the code works), but it should go away after 30 times.
> zone the RTC is running on, so hours are untouched. No one from a half-hour
> or fifteen-minute time zone has complained yet :-|. A semi-clever approach
> would be to watch warp_time() from outside and add the kernel time zone
> conditionally.
If your CMOS clock is (let's say) at 15:55 and the new time is 16:05,
the code will currently update your clock to 15:05, because it
ignores the hour (and the dirfference is only 10 minutes).
The correct fix to deal with the time zones is to store it in the
kernel (at least if the CMOS runs localtime) and use it whenever
setting or reading the clock. I think we should have a working
routine in the kernel to set the CMOS clock; after all we have the
RTC driver.
>
> Ulrich> In adjtimex() several parameters were not checked, making it
> Ulrich> possibly to pass invalid values to the kernel. Also the plain old
> Ulrich> adjtim() was dependent on the time_status & STA_PLL, which should
> Ulrich> be completely independent of adjtimex() (ntp_adjtime).
>
> Ulrich> Especially it was allowed to change the read-only bits of
> Ulrich> time_status...
>
> This however is a bug. Classic adjtime() shouldn't mess with the KPLL bits or
> at least set them to a defined state. Please note that use of adjtime() and
> ntp_adjtime() _*IS*_ exclusive for practical reasons. Note that adjtime() is
> _not_ limited to half a second !
I agree on the last one. It was caused by careless reading of the
source.
>
> Ulrich> The function now even returns TIME_ERROR if the flags in
> Ulrich> time_state indicate an error condition.
>
> You have read RFC-1589 before doing modifications, haven't You ?
More than once (therefore the page and section numbers in the
comment).
>
> Ulrich> It should go into 2.1 as well I guess...
>
> Please, work these suggestions into your patch, and maybe You can provide a
> diff -Bub next time if You re-indent things ? TIA
I tried not to re-indent things, but when rewriting code I dropped
the 2-space indent. I hope you don't feel personally insulted. The
style I used in the kernel isn't the style I usually write code, but
IHMO 4 spaces is really the minimum for indentation. (No intention to
start another flame-war). If you want to see real differences, save
the original, apply the patch, and do a space-insensitice diff after
that.
>
> Thanks a lot for your attention and work !
Same for you.
Ulrich