Date: Sun, 14 Jan 2018 00:17:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327774 - in head: . sys/i386/bios Message-ID: <20180113225638.Q2336@besplex.bde.org> In-Reply-To: <201801101725.w0AHP8Ca020379@repo.freebsd.org> References: <201801101725.w0AHP8Ca020379@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180113225638.Q2336>