svn commit: r327774 - in head: . sys/i386/bios

[Bruce Evans]

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