On Wed, 10 Jan 2018, Warner Losh wrote:
Log:
inittodr(0) actually sets the time, so there's no need to call
tc_setclock(). It's redundant. Tweak UPDATING based on code review of
past releases.
No, tc_setclock() is not redundant (except for bugs). initodr(0) sets
a raw RTC time (plus a racy timezone offset). tc_setclock() is supposed to
fix this up to a less raw time using calculations done at suspend time.
The calculations are still done, so are even more bogus then when the
fixup was null under FIXME (then the FIXME a least indicated what
needed to be done).
Modified: head/UPDATING
==============================================================================
--- head/UPDATING Wed Jan 10 16:56:02 2018 (r327773)
+++ head/UPDATING Wed Jan 10 17:25:08 2018 (r327774)
@@ -53,8 +53,9 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 12.x IS SLOW:
20180110:
On i386, pmtimer has been removed. It's functionality has been folded
- into apm. It was a nop on ACPI. Users may need to remove it from kernel
- config files.
+ into apm. It was a nop on ACPI in current for a while now (but was still
+ needed on i386 in FreeBSD 11 and earlier). Users may need to remove it
+ from kernel config files.
It is indeed still needed in FreeBSD-11. acpci resume is still broken there
on all arches except amd64, by bogusly ifdefing acpi_resync_clock() to do
nothing except on amd64 (and even on amd64, there is a sysctl to give the
same brokenness).
pmtimer was made a no-op if acpi is configured in the same commit that fixed
acpi resume.
Modified: head/sys/i386/bios/apm.c
==============================================================================
--- head/sys/i386/bios/apm.c Wed Jan 10 16:56:02 2018 (r327773)
+++ head/sys/i386/bios/apm.c Wed Jan 10 17:25:08 2018 (r327774)
@@ -1086,7 +1086,6 @@ apm_rtc_resume(void *arg __unused)
{
u_int second, minute, hour;
struct timeval resume_time, tmp_time;
- struct timespec ts;
/* modified for adjkerntz */
timer_restore(); /* restore the all timers */
@@ -1097,14 +1096,11 @@ apm_rtc_resume(void *arg __unused)
/* Calculate the delta time suspended */
timevalsub(&resume_time, &suspend_time);
- second = ts.tv_sec = resume_time.tv_sec;
- ts.tv_nsec = 0;
- tc_setclock(&ts);
-
The carefully calculated tmp_time is no longer used. It wasn't used before
either, since ts was initialized to a wrong value. tmp_time was last used
under FIXME, but that shouldn't have compiled either since FIXME was
undefineable -- tmp_time as a write-only auto variable in all cases.
The non-use of some of the other timestamp variables is harder to detect
since they are static.
tc_setclock() takes a delta-time as an arg. resume_time is already a
delta-time, but I think it is complete garbage. resume_time was initially
actually the resume time, but is abused to hole the suspension interval
(delta-time). inittodr(0) already advanced the timecounter by approximately
the suspension interval, and inittodr(&ts) gives a garbage time by advancing
by this again.
I think the correct second advance is simply diff_time which was calculated
earlier. The dead tmp_time is <initial resume time> + diff_time. This was
only used in the unreachable FIXME code because the "API" for that needed an
absolute time.
Untested fixes:
X Index: apm.c
X ===================================================================
X --- apm.c (revision 327911)
X +++ apm.c (working copy)
X @@ -1067,17 +1067,17 @@
X } while (apm_event != PMEV_NOEVENT);
X }
X
X -static struct timeval suspend_time;
X -static struct timeval diff_time;
X +static struct timespec diff_time;
X +static struct timespec suspend_time;
X
X static int
X apm_rtc_suspend(void *arg __unused)
X {
X
X - microtime(&diff_time);
X - inittodr(0);
X - microtime(&suspend_time);
X - timevalsub(&diff_time, &suspend_time);
X + nanotime(&diff_time); /* not a difference yet */
X + inittodr(0); /* set tc to raw time seen by RTC */
X + nanotime(&suspend_time); /* record this raw time */
X + timespecsub(&diff_time, &suspend_time); /* diff between tc and RTC */
X return (0);
X }
X
X @@ -1084,23 +1084,22 @@
X static int
X apm_rtc_resume(void *arg __unused)
X {
X + struct timespec resume_time, suspend_interval;
X u_int second, minute, hour;
This already changes too much, so I didn't fix blind truncation of tv_sec
starting in 2038 or other type botches for second/minute/hour.
X - struct timeval resume_time, tmp_time;
X
X - /* modified for adjkerntz */
X - timer_restore(); /* restore the all timers */
X - inittodr(0); /* adjust time to RTC */
X - microtime(&resume_time);
X - getmicrotime(&tmp_time);
X - timevaladd(&tmp_time, &diff_time);
X + timer_restore();
X + inittodr(0); /* set tc to new raw RTC time */
X + nanotime(&resume_time); /* record this raw time */
X + tc_setclock(&diff_time); /* restore old diff to tc */
Fixing tc_setclock() is the easiest part.
X +
X /* Calculate the delta time suspended */
X - timevalsub(&resume_time, &suspend_time);
X + suspend_interval = resume_time;
X + timevalsub(&suspend_interval, &suspend_time);
There are to many comments. I forgot to remove the one above after
unobfuscating resume_time.
X
X -#ifdef PMTIMER_FIXUP_CALLTODO
X - /* Fixup the calltodo list with the delta time. */
X - adjust_timeout_calltodo(&resume_time);
X -#endif /* PMTIMER_FIXUP_CALLTODO */
X - second = resume_time.tv_sec;
X +#ifdef PMTIMER_FIXUP_CALLTODO /* XXX needed unconditionally, but never
worked */
X + adjust_timeout_calltodo(&suspend_interval);
X +#endif
X + second = suspend_interval.tv_sec;
X hour = second / 3600;
X second %= 3600;
X minute = second / 60;
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
