From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: wsmouse(4) and wstpad: mp-safe filterops
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date: Thu, 23 Jan 2025 18:42:37 +0100

On Thu, 23 Jan 2025 11:14:31 +0100,
Vitaliy Makkoveev <mvs@openbsd.org> wrote:
>
> The last one.
>
> As in wskbd(4), we don't share `sc_refcnt' with interrupts, so we don't
> need to protect it. May be `sc_refcnt' should be replaced with refcnt
> structure, may be completely removed - it's not yet clean to me.
>
> Please note, filt_wseventread() is the only thread that runs
> simultaneously with the rest of the wscons(4). This thread only compare
> `ws_get' with `ws_put'. wscons(4) drivers only increment `ws_put' index
> so at driver side we only need to protect this increment to ensure
> `ws_put' is consistent with WSEVENT_QSIZE. I wrapped `ws_put' and
> `ws_get' access with `ws_mtx' mutex(9) but this is not required.
>
> Index: sys/dev/wscons/wsmouse.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
> diff -u -p -r1.74 wsmouse.c
> --- sys/dev/wscons/wsmouse.c	30 Dec 2024 02:46:00 -0000	1.74
> +++ sys/dev/wscons/wsmouse.c	23 Jan 2025 09:43:25 -0000
> @@ -249,7 +249,6 @@ wsmouse_detach(struct device *self, int
>  	struct wsmouse_softc *sc = (struct wsmouse_softc *)self;
>  	struct wseventvar *evar;
>  	int maj, mn;
> -	int s;
>
>  #if NWSMUX > 0
>  	/* Tell parent mux we're leaving. */
> @@ -262,18 +261,18 @@ wsmouse_detach(struct device *self, int
>  	/* If we're open ... */
>  	evar = sc->sc_base.me_evp;
>  	if (evar != NULL) {
> -		s = spltty();
>  		if (--sc->sc_refcnt >= 0) {
> +			mtx_enter(&evar->ws_mtx);
>  			/* Wake everyone by generating a dummy event. */
>  			if (++evar->ws_put >= WSEVENT_QSIZE)
>  				evar->ws_put = 0;
> -			WSEVENT_WAKEUP(evar);
> +			mtx_leave(&evar->ws_mtx);
> +			wsevent_wakeup(evar);
>  			/* Wait for processes to go away. */
>  			if (tsleep_nsec(sc, PZERO, "wsmdet", SEC_TO_NSEC(60)))
>  				printf("wsmouse_detach: %s didn't detach\n",
>  				       sc->sc_base.me_dv.dv_xname);
>  		}
> -		splx(s);
>  	}
>
>  	/* locate the major number */
> @@ -326,7 +325,7 @@ wsmouseopen(dev_t dev, int flags, int mo
>  		return (EBUSY);
>
>  	evar = &sc->sc_base.me_evar;
> -	if (wsevent_init(evar))
> +	if (wsevent_init_flags(evar, WSEVENT_MPSAFE))
>  		return (EBUSY);
>
>  	error = wsmousedoopen(sc, evar);
> @@ -495,7 +494,9 @@ wsmouse_do_ioctl(struct wsmouse_softc *s
>  	case FIOASYNC:
>  		if (sc->sc_base.me_evp == NULL)
>  			return (EINVAL);
> +		mtx_enter(&sc->sc_base.me_evp->ws_mtx);
>  		sc->sc_base.me_evp->ws_async = *(int *)data != 0;
> +		mtx_leave(&sc->sc_base.me_evp->ws_mtx);
>  		return (0);
>
>  	case FIOGETOWN:
> @@ -955,7 +956,10 @@ wsmouse_evq_put(struct evq_access *evq,
>  	struct wscons_event *ev;
>  	int space;
>
> +	mtx_enter(&evq->evar->ws_mtx);
>  	space = evq->evar->ws_get - evq->put;
> +	mtx_leave(&evq->evar->ws_mtx);
> +
>  	if (space != 1 && space != 1 - WSEVENT_QSIZE) {
>  		ev = &evq->evar->ws_q[evq->put++];
>  		evq->put %= WSEVENT_QSIZE;

Here and in wsmouse_log_events this block does not smell mp-safe.

Am I wrong? Should I call mtx_leave after the if block?

> @@ -1099,7 +1103,11 @@ void
>  wsmouse_log_events(struct wsmouseinput *input, struct evq_access *evq)
>  {
>  	struct wscons_event *ev;
> -	int n = evq->evar->ws_put;
> +	int n;
> +
> +	mtx_enter(&evq->evar->ws_mtx);
> +	n = evq->evar->ws_put;
> +	mtx_leave(&evq->evar->ws_mtx);
>
>  	if (n != evq->put) {
>  		printf("[%s-ev][%04d]", DEVNAME(input), LOGTIME(&evq->ts));
> @@ -1138,7 +1146,9 @@ wsmouse_input_sync(struct device *sc)
>  	evq.evar = *input->evar;
>  	if (evq.evar == NULL)
>  		return;
> +	mtx_enter(&evq.evar->ws_mtx);
>  	evq.put = evq.evar->ws_put;
> +	mtx_leave(&evq.evar->ws_mtx);
>  	evq.result = EVQ_RESULT_NONE;
>  	getnanotime(&evq.ts);
>
> @@ -1181,8 +1191,10 @@ wsmouse_input_sync(struct device *sc)
>  			if (input->flags & LOG_EVENTS) {
>  				wsmouse_log_events(input, &evq);
>  			}
> +			mtx_enter(&evq.evar->ws_mtx);
>  			evq.evar->ws_put = evq.put;
> -			WSEVENT_WAKEUP(evq.evar);
> +			mtx_leave(&evq.evar->ws_mtx);
> +			wsevent_wakeup(evq.evar);
>  		}
>  	}
>
> Index: sys/dev/wscons/wstpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/wscons/wstpad.c,v
> diff -u -p -r1.34 wstpad.c
> --- sys/dev/wscons/wstpad.c	25 Mar 2024 13:01:49 -0000	1.34
> +++ sys/dev/wscons/wstpad.c	23 Jan 2025 09:43:25 -0000
> @@ -931,7 +931,9 @@ wstpad_tap_timeout(void *p)
>  			tp->tap.state = TAP_DETECT;
>  		}
>  		if (ev) {
> +			mtx_enter(&evq.evar->ws_mtx);
>  			evq.put = evq.evar->ws_put;
> +			mtx_leave(&evq.evar->ws_mtx);
>  			evq.result = EVQ_RESULT_NONE;
>  			getnanotime(&evq.ts);
>  			wsmouse_evq_put(&evq, ev, btn);
> @@ -940,8 +942,10 @@ wstpad_tap_timeout(void *p)
>  				if (input->flags & LOG_EVENTS) {
>  					wsmouse_log_events(input, &evq);
>  				}
> +				mtx_enter(&evq.evar->ws_mtx);
>  				evq.evar->ws_put = evq.put;
> -				WSEVENT_WAKEUP(evq.evar);
> +				mtx_leave(&evq.evar->ws_mtx);
> +				wsevent_wakeup(evq.evar);
>  			} else {
>  				input->sbtn.sync |= tp->tap.button;
>  			}

--
wbr, Kirill