Date: Sat, 13 Jan 2018 20:53:10 -0700 From: Warner Losh <imp@bsdimp.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Warner Losh <imp@freebsd.org>, src-committers <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: <CANCZdfo_SiF5FMpcbRjX14FMXERPa0EbWex9z4SqrRny8b_2dQ@mail.gmail.com> In-Reply-To: <20180113225638.Q2336@besplex.bde.org> References: <201801101725.w0AHP8Ca020379@repo.freebsd.org> <20180113225638.Q2336@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Jan 13, 2018 at 6:17 AM, Bruce Evans <brde@optusnet.com.au> wrote:
> 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.
>
I suspect this code will no longer be running in 2028, let alone 2038.
> 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
>
tc_setclock() sets the absolute time. It's not a phase shift.
> 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.
>
OK.
> 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;
>
How is this then, it fixes the delta vs time setting in your version and
tweaks variable names to remove banal comments....
static struct timespec suspend_time; /* Saved RTC time at
suspend */
static struct timespec diff_interval; /* Delta between systime
and RTC */
static int
apm_rtc_suspend(void *arg __unused)
{
nanotime(&diff_interval);
inittodr(0); /* systime = RTC */
nanotime(&suspend_time);
timespecsub(&diff_interval, &suspend_time);
return (0);
}
static int
apm_rtc_resume(void *arg __unused)
{
struct timespec resume_time, new_time, suspend_interval;
u_int second, minute, hour;
timer_restore();
inittodr(0); /* Set to updated RTC time */
nanotime(&resume_time);
new_time = resume_time;
timespecadd(&new_time, &diff_interval);
tc_setclock(&new_time); /* Pre-suspend system time + sleep
time */
suspend_interval = resume_time;
timespecsub(&suspend_interval, &suspend_time);
second = suspend_interval.tv_sec;
#ifdef FIXUP_CALLTODO
/* Fixup the calltodo list with the delta time. */
adjust_timeout_calltodo(&suspend_interval);
#endif /* PMTIMER_FIXUP_CALLTODO */
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfo_SiF5FMpcbRjX14FMXERPa0EbWex9z4SqrRny8b_2dQ>