From owner-svn-src-head@freebsd.org Sun Jan 14 03:53:12 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 2879EE76B76 for ; Sun, 14 Jan 2018 03:53:12 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io0-x22d.google.com (mail-io0-x22d.google.com [IPv6:2607:f8b0:4001:c06::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CD97C689CB for ; Sun, 14 Jan 2018 03:53:11 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io0-x22d.google.com with SMTP id c17so9889322iod.1 for ; Sat, 13 Jan 2018 19:53:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=PZeVDYEyz/mORzGLIGXCNbrbxBqYmg7bYaMyKueCa0I=; b=Vzz35PJOYovOdLl6rmkwRp4Q0iDy9bWuur5CpCrvxKTaFNg6jxdTR01NKEEtSF8xpM sZ7Z3ZvYdEdPj91wUKltkBuW+AR0duw1grKh7lLS8Cj1E+wFsKAC5LazwSQP+4J2UpXX e1dikHQr01Eo3HfROe0rzVn9sn5MgFc0Pd6rURqZhwEq7ullIp6g56KM2faYo+6Wn4R5 HMcvv4JXX4Ul+eWYJPk3kIspO+WcNkWF42Ddn7GwwZfZ9wzosY+UF6YDpHB/DiP+JVvL iVwqSp4yhCNToS8qvLIG6zn3ULZ3yamWKF0+2fQiM4i6l5IM+s0GobXulxSre350c5Qc ZTqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=PZeVDYEyz/mORzGLIGXCNbrbxBqYmg7bYaMyKueCa0I=; b=aECyF//esAEYdGYEO4Zdd9Cdg6ndaxyrBn9qHTB1hqrvm91w4CQgtdd1Gsnj2UwqEU G4ChvZzH+PAtFbS4lCSCNwr9bIcTp8NIrlZp6PrZT1edgIkD1eQPvXo07mVNEDaRx8mA z4QZuNhGSM8UyxTkvbAxQyazaogAhf7jYETBwIySXMN2wnV1mgTgposBdQtwwCvXxi5n I0lHBH991XWuPBHlLjGjohNSd3u2qpa4C+UW7OEZ6Xj/NK2HIpySPUL//xdOootcV6dH 9Ksja+N2FKQfk0g9mNP2P6fTTYnhRxXUgi/QyuadX67n3Va6jUnJPC/vr5nAniqgovCv IySg== X-Gm-Message-State: AKwxytdz+l5nn247hi5HUMQB9k+fim0qqLwWn62aojmdrNjFRmw93nHA 3h2C+Lpy8NN1khBvAziuZUVV0kL9hoas/Aa+P7Fq1Q== X-Google-Smtp-Source: ACJfBovGYCksThWgIJg8b/5MlhJV2o4KdchbxB9tYfy9eXJ9MVIRRb8JLaxG2beTQg7G/wbeotu8cK41aC/AwZqlv3g= X-Received: by 10.107.200.15 with SMTP id y15mr6719379iof.130.1515901990815; Sat, 13 Jan 2018 19:53:10 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.79.199.131 with HTTP; Sat, 13 Jan 2018 19:53:10 -0800 (PST) X-Originating-IP: [2603:300b:6:5100:18a2:a4f7:170:8dd9] In-Reply-To: <20180113225638.Q2336@besplex.bde.org> References: <201801101725.w0AHP8Ca020379@repo.freebsd.org> <20180113225638.Q2336@besplex.bde.org> From: Warner Losh Date: Sat, 13 Jan 2018 20:53:10 -0700 X-Google-Sender-Auth: wxBQVkpe7FgW8yX39ribnA5mVe8 Message-ID: Subject: Re: svn commit: r327774 - in head: . sys/i386/bios To: Bruce Evans Cc: Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 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: Sun, 14 Jan 2018 03:53:12 -0000 On Sat, Jan 13, 2018 at 6:17 AM, Bruce Evans 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 + 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); }