svn commit: r327767 - in head/sys: conf i386/bios i386/conf i386/isa

[Warner Losh]

[trimmed, sorry ]

On Sat, Jan 13, 2018 at 4:56 AM, Bruce Evans <brde at optusnet.com.au> wrote:

> On Wed, 10 Jan 2018, Warner Losh wrote:
>
> Log:
>>  Retire pmtimer driver. Move time fixing into apm driver. Move
>>  Iwasaki-san's copyright over. Remove FIXME code that couldn't possibly
>>  work. Call tc_settime() with our estimate of the delta we've been
>>  alseep (the one we print) to adjust the time. Not sure what to do
>>  about callouts, so keep the small #ifdef in place there.
>>
>
> The FIXME code might have worked before timecounters, but never worked in
> any committed version.  You seem to have updated it correctly for
> timecounters in this commit, but then broke it by removing it in a later
> commit (see another reply).
>
> This commit obfuscates the fix by combining moving of the file with
> changing
> it.  svn is also very broken -- it doesn't show history of the file under
> its old name.  This is more broken than usual -- since the file was merged
> into a different file, svn doesn't show history of the file under its new
> name either.
>
> The callout fixup code is more broken.  It never compiled in any committed
> version, since it uses data structures and logic that went away before it
> was committed.  The option for it (APM_FIXUP_CALLTODO) is so broken that it
> is not a supported option or in LINT, so it is not normally noticed that
> the code under it never compiled.
>
> Long callouts are especially broken.  E.g., nanosleep(1 hour) at least
> used to sleep for 2 hours if the system is suspended for 1 hour during
> the sleep.  I haven't noticed any fixes for this.  Callouts mostly use
> relative timeouts, and relative timeouts just don't work across
> suspensions.
>
> Actually, I have noticed some fixes for nanosleep().  Another bug in it
> was that it is specified to sleep in the time specified by the clock id
> CLOCK_REALTIME, but used to sleep in the time specified by the clock id
> CLOCK_MONOTONIC (the latter should be as close as possible to the former
> except for leap seconds adjustments, but is quite broken -- it doesn't
> count suspended time, and non-leap seconds settings of CLOCK_REALTIME
> also make CLOCK_MONOTONIC useless for determining physical time
> differences.
> nanosleep() now sleeps on the correct clock id, and clock_nanosleep() is
> now implemented.  This is complicated, and I haven't checked if the fix
> works.  It looks to simple to work.  To work, it needs to do things like
> adjust timers whenever the timecounter jumps (mainly for resume and leap
> seconds adjustments).  APM_FIXUP_CALLTODO only tries to handle the easier
> case of resume.


Right, none of that is fixed.

Modified: head/sys/conf/files.i386
>> ============================================================
>> ==================
>> --- head/sys/conf/files.i386    Wed Jan 10 14:58:58 2018        (r327766)
>> +++ head/sys/conf/files.i386    Wed Jan 10 14:59:19 2018        (r327767)
>> @@ -520,7 +520,6 @@ i386/ibcs2/ibcs2_util.c             optional ibcs2
>> i386/ibcs2/ibcs2_xenix.c        optional ibcs2
>> i386/ibcs2/ibcs2_xenix_sysent.c optional ibcs2
>> i386/ibcs2/imgact_coff.c        optional ibcs2
>> -i386/isa/pmtimer.c             optional pmtimer
>> i386/isa/prof_machdep.c         optional profiling-routine
>> i386/linux/imgact_linux.c       optional compat_linux
>> i386/linux/linux_dummy.c        optional compat_linux
>>
>
> pmtimer used to be needed all systems with acpi except amd64, because the
> acpi fixup of timecounters was bogusly ifdefed for amd64 only (or was not
> done by acpi in very old versions?).
>
> The pmtimer code for this fixup is still better tha the acpi code, except
> the FIXME made it unused and the next commit dumbs it down to worse than
> the acpi code (the acpi code is at least simpler).
>




> Modified: head/sys/i386/bios/apm.c
>> ============================================================
>> ==================
>> --- head/sys/i386/bios/apm.c    Wed Jan 10 14:58:58 2018        (r327766)
>> +++ head/sys/i386/bios/apm.c    Wed Jan 10 14:59:19 2018        (r327767)
>> Modified: head/sys/i386/bios/apm.c
>> ============================================================
>> ==================
>> --- head/sys/i386/bios/apm.c    Wed Jan 10 14:58:58 2018        (r327766)
>> +++ head/sys/i386/bios/apm.c    Wed Jan 10 14:59:19 2018        (r327767)
>> @@ -205,7 +232,7 @@ static int
>> apm_driver_version(int version)
>> {
>>         struct apm_softc *sc = &apm_softc;
>> -
>> +
>>         sc->bios.r.eax = (APM_BIOS << 8) | APM_DRVVERSION;
>>         sc->bios.r.ebx  = 0x0;
>>         sc->bios.r.ecx  = version;
>>
>
> Unrelated style fixes make the important changes harder to see.  This
> patch was to remove trailing whitespace, but some mail program broke it
> by removing the whitespace in the source lines.
>
> [... further mangled patches for whitespace not shown]


Yea, I thought about doing two commits, but they got mixed up...


>
> @@ -331,12 +358,10 @@ apm_battery_low(void)
>> static struct apmhook *
>> apm_add_hook(struct apmhook **list, struct apmhook *ah)
>> {
>> -       int s;
>>         struct apmhook *p, *prev;
>>
>>         APM_DPRINT("Add hook \"%s\"\n", ah->ah_name);
>>
>> -       s = splhigh();
>>         if (ah == NULL)
>>                 panic("illegal apm_hook!");
>>         prev = NULL;
>> @@ -351,30 +376,25 @@ apm_add_hook(struct apmhook **list, struct apmhook
>> *ah
>>                 ah->ah_next = prev->ah_next;
>>                 prev->ah_next = ah;
>>         }
>> -       splx(s);
>>         return ah;
>> }
>>
>
> This commit also removes spl*() with no mention of this in the log message.
> In FreeBSD-4 where spl's were not null, splhigh() was probably wrong, but
> it
> was much better than the splsoftclock() used in pmtimer.c.  Hard clock
> interrupts should be disabled, and my fixes for this change softclock to
> statclock + XXX.  It happens that splstatclock() == splclock() ==
> splhigh(),
> so these splhigh()s were good enough.
>
> Now the locking is obscure.  I think it is just Giant, and that is not
> enough.  The spl's should not be removed until the locking is done
> properly.
> The splsoftclock()'s were removed even earlier in pmtimer.c, without doing
> any proper locking of course.


Right, nothing calls these, except in attach, so I removed the broken
locking rather than fixing it.

static void
>> apm_del_hook(struct apmhook **list, struct apmhook *ah)
>> {
>> -       int s;
>>         struct apmhook *p, *prev;
>>
>> -       s = splhigh();
>>         prev = NULL;
>>         for (p = *list; p != NULL; prev = p, p = p->ah_next)
>>                 if (p == ah)
>>                         goto deleteit;
>>         panic("Tried to delete unregistered apm_hook.");
>> -       goto nosuchnode;
>> +       return;
>> deleteit:
>>         if (prev != NULL)
>>                 prev->ah_next = p->ah_next;
>>         else
>>                 *list = p->ah_next;
>> -nosuchnode:
>> -       splx(s);
>> }
>>
>
> Locking for device removal is especially necessary and badly done.  New-bus
> forces lots of Giant locking which is probably not enough.


what calls that?

Warner