Date: Sat, 13 Jan 2018 20:59:06 -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: r327767 - in head/sys: conf i386/bios i386/conf i386/isa Message-ID: <CANCZdfqA8MRuezrnjwpiYcQdUxZXvY_QdFNcwNLV_u9-xF9Y=w@mail.gmail.com> In-Reply-To: <20180113203848.L2336@besplex.bde.org> References: <201801101459.w0AExJWM055025@repo.freebsd.org> <20180113203848.L2336@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[trimmed, sorry ]
On Sat, Jan 13, 2018 at 4:56 AM, Bruce Evans <brde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqA8MRuezrnjwpiYcQdUxZXvY_QdFNcwNLV_u9-xF9Y=w>