svn commit: r327767 - in head/sys: conf i386/bios i386/conf i386/isa
Bruce Evans
brde at optusnet.com.au
Sun Jan 14 06:44:05 UTC 2018
On Sat, 13 Jan 2018, Warner Losh wrote:
> [trimmed, sorry ]
Your mail client also corrupted the formatting, especially for tabs. It
sends plain text with tabs corrupted to spaces and an html attachment with
full-width tabs corrupted to "=C2=A0 =C2=A0 =C2=A0 =C2=A0 ", both with
charset UTF-8. The plain text is readable, but some (non-UTF-8-aware)
mail clients apparently display the html and further corrupt the tab to
"\xc2\xa0 \xc2\xa0 \xc2\xa0 \xc2\xa0". With my default font, 0xc2 is
rendered as a line-drawing graphics character, \xa0 is rendered as a
space, and " " is rendered as a space. The html is further mis-displayed
by adding a large left margin. The same mail client doesn't do this for
html attachments from other senders. The same mail client quotes only
the plain text for replies. Other senders tend to put raw '\xa0' in the
plain text after corrupting tabs to that, and this is preserved in replies.
Non-UTF-8-aware vi renders '\xa0' as "\xa0" so that the errors are more
obvious.
>> 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.
>>> ...
At least the quoting seems to be correct (in the plain text version).
>> @@ -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.
Actually, the quoting is broken by splitting long lines in a simple way --
the method seems to be to reformat to width 80 without reformatting whole
paragraphs. This is only bad for source code. Reformatting "paragraphs"
in source code would be worse.
>> 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.
Attach also uses Giant locking, and it is not clear that this is enough.
It is certainly not enough for detach.
Locking hints should be kept until Giant goes away. Perhaps they could
be assertions that Giant is held (with this implicitly implying that
Giant is enough), but it is easier to keep using the spls.
>
> 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?
I thought that it was for something like detach.
apm_del_hook() seems to be just unused garbage. It is only called by
apm_hook_disestablish(), but that is never called according to grep.
Grep would have missed any magic to generate a call from a .m file.
This API also has style bugs: the verbs 'del' and 'disestablish' are
in different places; 'disestablish' is too verbose 'del' relatively
to concise; both seem to be for destructors of the same thing so why
are there 2 of them? Well, the 'del' one is just to do most of the
work for the 'disestablish' one. It is only called once so not doing
it directly is just an obfuscation.
Further grepping shows that apm_hook_disestablish() was last used in
FreeBSD-3. It was used by pccard/pccard.c then. In FreeBSD-4 and
FreeBSD-8, its non-use looks just like in -current, except its
implementation was duplicated for pc98. FreeBSD-3 doesn't support
pc98, at least for apm hooks.
Similarly for apm_del_hook():
- only for i386 in FreeBSD-3
- its implementation is duplicated for pc98 in FreeBSD-4 and FreeBSD-8,
- its use in versions that have it looks just like now.
Here is the use in FreeBSD-3:
X /*
X * pccard_remove_controller - Called when the slot
X * driver is unloaded. The plan is to unload
X * drivers from the slots, and then remove the
X * slots from the slot list, and then finally
X * remove the controller structure. Messy...
X */
X void
X pccard_remove_controller(struct slot_ctrl *ctrl)
X {
X struct slot *slt, *next, *last = 0;
X struct slot_ctrl *cl;
X struct pccard_devinfo *devi;
X
X for (slt = slot_list; slt; slt = next) {
X next = slt->next;
X /*
X * If this slot belongs to this controller,
X * remove this slot.
X */
X if (slt->ctrl == ctrl) {
X pccard_slots[slt->slotnum] = 0;
X if (slt->insert_seq)
X untimeout(inserted, (void *)slt, slt->insert_ch);
X /*
X * Unload the drivers attached to this slot.
X */
X while (devi = slt->devices)
X remove_device(devi);
X /*
X * Disable the slot and unlink the slot from the
X * slot list.
X */
X disable_slot(slt);
X if (last)
X last->next = next;
X else
X slot_list = next;
X #if NAPM > 0
X apm_hook_disestablish(APM_HOOK_SUSPEND,
X &s_hook[slt->slotnum]);
X apm_hook_disestablish(APM_HOOK_RESUME,
X &r_hook[slt->slotnum]);
X #endif
This is quite complicated. Apparently, any pccard can have an apm hook.
pccard removal has all of the usual complications of forced device detach.
and apm hook removal^Wdisestabishment too.
Further grepping shows that APM_HOOK_RESUME and APM_HOOK_SUSPEND are
still used. In FreeBSD-4, they are only used for apm_execute_hook()
calls and it is unclear where the hooks are attached^Westablished.
In -current, they are also used for apm_hook_establish() calls. These
establishment calls are only done in apm_attach(). The functions are
public so the might be called from out-of-tree modules, but that would
be even more fragile. Detachment doesn't seem to be supported for apm
(there is no apm_detach()), so disestablishment is apparently not needed
except for the out-of-tree modules.
apm_execute_hook() has been static in bios/apm.c since at least FreeBSD-3
so it doesn't have the problems from exporting apm_hook_establish().
apm_hook_estabish() is used by a lot of device drivers in FreeBSD-3:
syscons, xe, sound, ze, zp, psm and aic6360 as well as by apm_attach()
and pccard_alloc_slot(). It was just as unused as apm_hook_disestabish()
in FreeBSD-4 through FreeBSD-11. Someone named imp restored its use
in apm_attach() in precisely the commit in this thread (r327767). The
combined commit obfuscates this especially well. apm_hook_estabish() is
now used again to hook the apm_rtc_suspend() and apm_rtc_resume functions.
I can't find any other use of the hooks between FreeBSD-4 and -current
(assuming that they are not established by static initialization).
The last previous use of them seems to have been removed in r65865.
This added pmtimer as a normal device driver so its suspend and resume
methods are called in a normal way and the ugly hooks were not needed.
pmtimer didn't have an attach method even as a device, so it had no
complications for detachment. Similarly, before r65865 and now, it
needs no complications for disestablishment and the disestablishment
code remains unused.
The current commit doesn't undo the removals in r65865 and seems to
be missing some details:
apm.c r65864:
X 6512 phk /* default suspend hook */
X 6512 phk sc->sc_suspend.ah_fun = apm_default_suspend;
X 6512 phk sc->sc_suspend.ah_arg = sc;
X 6512 phk sc->sc_suspend.ah_name = "default suspend";
X 6512 phk sc->sc_suspend.ah_order = APM_MAX_ORDER;
X 3258 dg
X 6512 phk /* default resume hook */
X 6512 phk sc->sc_resume.ah_fun = apm_default_resume;
X 6512 phk sc->sc_resume.ah_arg = sc;
X 6512 phk sc->sc_resume.ah_name = "default resume";
X 6512 phk sc->sc_resume.ah_order = APM_MIN_ORDER;
X 3309 phk
X 6512 phk apm_hook_establish(APM_HOOK_SUSPEND, &sc->sc_suspend);
X 6512 phk apm_hook_establish(APM_HOOK_RESUME , &sc->sc_resume);
X 6512 phk
X 40751 msmith /* Power the system off using APM */
apm.c r327774 (current):
X 40751 msmith /* Power the system off using APM */
X 327767 imp EVENTHANDLER_REGISTER(shutdown_final, apm_power_off, NULL,
X 50107 msmith SHUTDOWN_PRI_LAST);
X 40751 msmith
X 85835 iwasaki /* Register APM again to pass the correct argument of pm_func. */
X 85835 iwasaki power_pm_register(POWER_PM_TYPE_APM, apm_pm_func, sc);
X 85835 iwasaki
X 6512 phk sc->initialized = 1;
X 65865 iwasaki sc->suspending = 0;
X 158922 imp sc->running = 0;
X 6512 phk
X 198707 ed make_dev(&apm_cdevsw, APMDEV_NORMAL,
X 198707 ed UID_ROOT, GID_OPERATOR, 0664, "apm");
X 198707 ed make_dev(&apm_cdevsw, APMDEV_CTL,
X 198707 ed UID_ROOT, GID_OPERATOR, 0660, "apmctl");
X 327767 imp
X 327767 imp sc->sc_suspend.ah_fun = apm_rtc_suspend;
X 327767 imp sc->sc_suspend.ah_arg = sc;
X 327767 imp apm_hook_establish(APM_HOOK_SUSPEND, &sc->sc_suspend);
X 327767 imp
X 327767 imp sc->sc_resume.ah_fun = apm_rtc_resume;
X 327767 imp sc->sc_resume.ah_arg = sc;
X 327767 imp apm_hook_establish(APM_HOOK_RESUME, &sc->sc_resume);
This doesn't initialize ah_name or ah_order.
ah_name is only used in debugging and error code. Presumably sc_resume is
initialized to all-zeros, so ah_name is NULL and the function name is
printed as something like "null".
ah_order defaulting to 0 presumably gives these hook precedence, but since
hooks were unused before there are no other hooks and the fancy precedence
code is has no effect.
X 327767 imp
X 3258 dg return 0;
X 3258 dg }
Summary: pmtimer should be turned back into a device driver to take
advantage of device infrastructure and apm hooks removed.
Bruce
More information about the svn-src-all
mailing list