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

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   hans
> Date:           Sun Mar 23 12:07:24 UTC 2025
> 
> Modified Files:
>         src/sys/dev/usb: uts.c
> 
> Log Message:
> uts(4): make sure the device is enabled before calling uhidev_close()
> 
> This check was already there, but only enabled for DIAGNOSTIC kernels.
> The check and early return is always needed, but the message should only
> be printed in DIAGNOSTIC kernels.
> 
> Fixes PR kern/59206
> 
> +	if (!sc->sc_enabled) {
>  #ifdef DIAGNOSTIC
> -	if (!sc->sc_enabled) {
>  		printf("uts_disable: not enabled\n");
> +#endif
>  		return;
>  	}
> -#endif

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.

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;

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.)