Re: CVS commit: src/sys/dev/usb

[Hans Rosenfeld <hans%netbsd.org@localhost>]

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