From owner-svn-src-head@freebsd.org Sat Jan 13 13:38:17 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5BE6CE65997; Sat, 13 Jan 2018 13:38:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 0561821C0; Sat, 13 Jan 2018 13:38:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 073A61A60D9; Sun, 14 Jan 2018 00:17:51 +1100 (AEDT) Date: Sun, 14 Jan 2018 00:17:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Warner Losh 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 In-Reply-To: <201801101725.w0AHP8Ca020379@repo.freebsd.org> Message-ID: <20180113225638.Q2336@besplex.bde.org> References: <201801101725.w0AHP8Ca020379@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=YbvN30Zf c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=E3LQvFw-N6LwsXHdYfEA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Jan 2018 13:38:17 -0000 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 + 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