On Thu, Jan 23, 2025 at 06:42:37PM +0100, Kirill A. Korinsky wrote:
> 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.
> >



> > @@ -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?
> 

See my diff description. When some variable modification happens with
two or more locks held, you need only one of them to read-only access.

This time all except filt_wseventread() serialized with kernel lock.
filt_wseventread() does read-only access, all `ws_get' and `ws_put'
modifications happens with both kernel lock and `ws_mtx' mutex(9) held,
so actually we don't need to take `ws_mtx' in this hunk, kernel lock is
pretty enough.

Note, this diff doesn't make ws* devices fully mp-safe, it only makes
mp-safe polling. I see no reason to introduce useless locks and
serialization for now. May be I will start doing this in next -current
cycle.