svn commit: r247086 - head/sys/x86/isa

[Bruce Evans]

On Tue, 21 May 2013, Alexey Dokuchaev wrote:

> On Mon, May 20, 2013 at 07:20:48AM -0600, Warner Losh wrote:
>> On May 19, 2013, at 8:21 PM, Alexey Dokuchaev wrote:
>>> Apparently not: I've been running FreeBSD 8.4-PRERELEASE without pmtimer
>>> for a while, and noticed that my laptop stops keeping time during suspend.
>>> I've never noticed that behavior before (presumably, with pmtimer).
>>>
>>> I will soon rebuild the kernel and put pmtimer back to see it fixes time
>>> keeping for me.  If it will, apparently it is still useful for i386, and
>>> not just for APM, but ACPI as well...
>>
>> It fights ACPI in what it does... It was needed for APM... Let me know how
>> it turns out...
>
> Just rebuilt vanilla r250824; timekeeping works fine (with pmtimer now).  My
> kernel config and loader.conf for reference:
>
>    http://193.124.210.26/{DARIA,loader.conf}

In theory, acpi should handle this, but I've never been able to find where
acpi either reads an acpi clock that gives the time or sets the time to
the current time on resume after reading it from some other clock.

Now I just found acpi_resync_clock() in dev/acpica/acpi.c.  This if under
an amd64 ifdef, so it can't work for i386, and its quality is much lower
than that of pmtimer's already low quality (PS: actually it is equally low
in a different way):

@ static void
@ acpi_resync_clock(struct acpi_softc *sc)
@ {
@ #ifdef __amd64__
@     if (!acpi_reset_clock)
@ 	return;

Since the code has not been tested (according to the comment on the sysctl),
there is a sysctl to clear this variable to turn it off.  The variable
defaults to on.

@ 
@     /*
@      * Warm up timecounter again and reset system clock.
@      */
@     (void)timecounter->tc_get_timecount(timecounter);
@     (void)timecounter->tc_get_timecount(timecounter);
@     inittodr(time_second + sc->acpi_sleep_delay);
@ #endif
@ }

The parameter to the inittodr() is confusing and not much better than
garbage.  I think time_second is normally the time at which the system
was suspended.  sc->acpi_sleep_delay is just a short delay (default 1
second) and we add it to compensate for resume sleeping for that long.
So the arg is usually much earlier than the current time.  inittodr()
only using it when reading the time from a hardware clock (always the
RTC on x86?) fails; then inittodr() sets the time to that value.  Here
it might as well do nothing, since the time is already set to the old
value (not quite, since setting the time also syncs with the hardware
timecounter).

Locking for inittodr() is broken (almost nonexistent), but that is not
this code's problem.

So the result of the above should be that the time is set to that of the
RTC on x86.

pmtimer is (PS: was) much more careful than this.  On resume, it adjusts
the time by the amount that the RTC time differed from the system time
at the time of the suspend.  This handles all drift between the clocks
that occured before the suspend.  There can be a further significantly
large drift during long sleeps.  Also, inittodr() is buggy, at least
for the x86 RTC version -- it adds an error of up to 1 second by not
syncing with the RTC seconds rollover.  The last bug is fixed in my
version (my inittodr() busy-waits for the rollover, but I have a better
method in rtcintr() that reduces the error to < 10 usec plus the drift
less the average drift.  I only use the rtcintr() fixup after pseudo-
suspensions by ddb)).

@ static struct timeval suspend_time;
@ static struct timeval diff_time;
@ 
@ static int
@ pmtimer_suspend(device_t dev)
@ {
@ 
@ 	microtime(&diff_time);
@ 	inittodr(0);

This inittodr(0) is a hack to read the RTC.  Originally there was no API
for this, but now CLOCK_GETTIME() is correct, at least if you can find
the relevant clock.  pmtimer can probablu assume the RTC, but acpi can't.
This clobbers the current time, but we don't care since we are going to
sleep.

@ 	microtime(&suspend_time);

This reads the current (clobbered) current time.  This completes the
hack for reading the RTC.

@ 	timevalsub(&diff_time, &suspend_time);

The fixup to add later.  The RTC only has seconds granularity, but other
clocks moght have more.  microtime() gives us microseconds granularity
but only seconds accuracy for the RTC due to the granularity in inittodr().

Locking is missing for all of this, and now it is not just inittodr()'s
fault.  The error of up to ~1 second may be increased by races, but not
by another 1 second (unless the races in inittodr() gave a corrupt value),
so we don't care.

This last pretended to do locking in FreeBSD-4.  splsoftclock() was used.
This didn't actually work, since splhigh() was needed.  When spl became
null, the splsoftclock() was left as a reminder to fix the locking, but
someone removed it without fixing the locking :-(.  Intermediate versions
with everything Giant locked had a better chance of working than even the
ones mis-locked by splsoftclock().

@ 	return (0);
@ }
@ 
@ static int
@ pmtimer_resume(device_t dev)
@ {
@ 	u_int second, minute, hour;
@ 	struct timeval resume_time, tmp_time;
@ 
@ 	/* modified for adjkerntz */

The comment is cryptic.  I think it means that the fixup is much more
necessary than might first appear, since adjkerntz can adjust the RTC
by an hour or so for DST changes.  The current time must not be changed
in this case, but without a fixup that takes into account the RTC
adjustment before the suspend, the error might be a full hour or so.
This might depend on how smart inittodr() is.  It will need to add an
an even larger adjustment if the RTC is not on UTC.  The DST adjustment
should be folded into this, so perhaps the comment no longer applies.

@ 	timer_restore();		/* restore the all timers */
@ 	inittodr(0);			/* adjust time to RTC */

This gives the current RTC time, hopefully adjusted to UTC including DST.

@ 	microtime(&resume_time);
@ 	getmicrotime(&tmp_time);
@ 	timevaladd(&tmp_time, &diff_time);

Adjust, with the usual races.

@ 
@ #ifdef FIXME
@ 	/* XXX THIS DOESN'T WORK!!! */
@ 	time = tmp_time;
@ #endif

Bah, the adjustment isn't even applied.  This rotted with timecounters
even before FreeBSD-4.  The current time used to be the simple variable
'time', but timecounters added lots of state to set.  Timecounters also
added the tc_setclock() API to set it.  This should be used here.  It
is racy, but inittodr() has already just used it.

@ 
@ #ifdef PMTIMER_FIXUP_CALLTODO
@ 	/* Calculate the delta time suspended */
@ 	timevalsub(&resume_time, &suspend_time);
@ 	/* Fixup the calltodo list with the delta time. */
@ 	adjust_timeout_calltodo(&resume_time);
@ 	/* 
@ 	 * We've already calculated resume_time to be the delta between 
@ 	 * the suspend and the resume. 
@ 	 */
@ 	second = resume_time.tv_sec; 
@ #else /* !PMTIMER_FIXUP_CALLTODO */
@ 	second = resume_time.tv_sec - suspend_time.tv_sec; 
@ #endif /* PMTIMER_FIXUP_CALLTODO */

The PMTIMER_FIXUP_CALLTODO garbage is purer.  It stopped working before it
was committed.

@ 	hour = second / 3600;
@ 	second %= 3600;
@ 	minute = second / 60;
@ 	second %= 60;
@ 	log(LOG_NOTICE, "wakeup from sleeping state (slept %02d:%02d:%02d)\n",
@ 		hour, minute, second);
@ 	return (0);
@ }

So it seems that pmtimer has a lot of dead code and reduces to not
much more than an inittodr(0) and usually acts like the inittodr(!0)
in the acpi amd64 resume code.  acpi amd64 resume also seems to be
missing the verbose logging of the suspension time, but maybe acpi
utilities give that.

Bruce