Hi, On Tue, Mar 25, 2025 at 08:28:10PM +0000, Taylor R Campbell wrote: > Thanks for taking a look at this! This change is not quite enough, > though. There are two issues: > > 1. `#ifdef DIAGNOSTIC printf(...)' is almost always wrong. Generally, > either: > > (a) the condition should be KASSERTed, or > (b) the condition should be quietly handled both ways. > > It's not a priori clear in all places where we currently have buggy > #ifdef DIAGNOSTIC printfs branches which case applies.
I'm looking at that again to figure out whether it's (a) or (b). I'm almost done with wsmux, where I think it's (a) in nearly all cases. I'll likely look at the stuff I changed in wsmouse and wskbd again, too. > 2. uts_enable has this questionable logic: > > sc->sc_enabled = 1; > ... > return uhidev_open(sc->sc_hdev, &uts_intr, sc); > > Note that this _always_ sets sc_enabled, even if uhidev_open fails! > This logic should almost certainly instead be: > > error = uhidev_open(...); > if (error) > return error; > sc->sc_enabled = 1; > return 0; Thanks for pointing that out. The uts_enable() logic is definitely broken. Interestingly, ums_enable() similarly just sets sc_enabled and then reverts it back to 0 if uhidev_open() failed, which while it works seems backwards to me. Not sure it's worth changing this, though. > Now what I'm not sure about is whether a _failed_ uts_enable call will > be followed by a uts_disable call. This is a (possibly unwritten) > part of the struct wsmouse_accessops contract; the wsmouse(9) man page > doesn't say one way or another, though. > > If wsmouse(9) guarantees that after a _failed_ enable call, it will > never call disable, then we can (and should) dispense with the > sc_enabled bit altogether and prune all the dead branches. (This is > preferable: less code in multiple drivers, fewer states to consider, > fewer conditionals to exercise.) > > If wsmouse(9) doesn't guarantee this, then we need to keep the extra > state in all the drivers, but there's no reason to have each one print > noise to the console about it. > > (Either way, we should update the wsmouse(9) man page to spell out > what is or isn't guaranteed.) The problem seems to be mostly in wsmux, not wsmouse. In particular, wsmux_do_open() ignores failures opening child devices. Those failures can happen because a child device is already open, which on the system I'm testing on happens to be the case because the X server opened it before opening the mux it is part of. Looking at xsrc/external/mit/xorg-server/dist/config/wscons.c, in wscons_add_pointers() it checks /dev/wsmouse[0-3] in succession, and if it finds a touchscreen type device, it adds a config section for it. Finally, it'll create a config section for /dev/wsmouse, which I presume by convention is actually /dev/wsmux0. So on the system I'm debugging kern/59206 on, X will open /dev/wsmouse1 and /dev/wsmux0 as separate pointer devices. I'm not sure how wsmux currently handles input from wsmouse1 in this case, whether it's all ignored or delivered to X twice, but whichever way it is it doesn't seem to cause problems. The problem that was kern/59206 was triggered later when X stopped, because X of course closes /dev/wsmouse1 and /dev/wsmux0 (in that order). As wsmux doesn't keep track of which of its child devices it successfully opened, wsmux_do_close() will just close each child device regardless of whether a previous open succeeded. So apart from there being a chance that it closes a device that is still open elsewhere, here it actually closed a device again that was closed already, causing kern/59206. Currently the underlying drivers are checking for these situations and handle them gracefully, except for uts(4). I actually think that wsmux really should keep track of which of its child devices it successfully opened, and only close those later. And also, since the X server already opens /dev/wsmouse (aka /dev/wsmux0) using the "ws" driver, perhaps it shouldn't even bother looking at /dev/wsmouse[0-3]? Hans -- %SYSTEM-F-ANARCHISM, The operating system has been overthrown
