Index | Thread | Search

From:
Vitaliy Makkoveev <mvs@openbsd.org>
Subject:
video(4): replace selinfo with klist and make `video_filtops' mp-safe
To:
kirill@openbsd.org, mglocker@openbsd.org, tech@openbsd.org
Date:
Sat, 14 Dec 2024 07:07:09 +0300

Download raw body.

Thread
Since {,u}video(4) are no more orphaned, I like to remind about my old
diff which replaces legacy selinfo data with klist. This time selinfo is
just a wrapper around klist, but protected with kernel lock. This diff
introduces `sc_mtx' mutex(9) to protect `sc_frames_ready' and klist
data.

I usa it with:
uvideo0 at uhub0 port 7 configuration 1 interface 0 "SunplusIT Inc HD Webcam" rev 2.01/3.01 addr 3

Index: sys/dev/video.c
===================================================================
RCS file: /cvs/src/sys/dev/video.c,v
diff -u -p -r1.57 video.c
--- sys/dev/video.c	2 Jul 2022 08:50:41 -0000	1.57
+++ sys/dev/video.c	14 Dec 2024 03:51:56 -0000
@@ -20,12 +20,14 @@
 #include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/errno.h>
+#include <sys/event.h>
 #include <sys/ioctl.h>
 #include <sys/fcntl.h>
 #include <sys/device.h>
 #include <sys/vnode.h>
 #include <sys/kernel.h>
 #include <sys/malloc.h>
+#include <sys/mutex.h>
 #include <sys/conf.h>
 #include <sys/proc.h>
 #include <sys/videoio.h>
@@ -34,6 +36,11 @@
 
 #include <uvm/uvm_extern.h>
 
+/*
+ * Locks used to protect struct members and global data
+ *	m	sc_mtx
+ */
+
 #ifdef VIDEO_DEBUG
 int video_debug = 1;
 #define DPRINTF(l, x...) do { if ((l) <= video_debug) printf(x); } while (0)
@@ -50,6 +57,7 @@ struct video_softc {
 	struct process		*sc_owner;	/* owner process */
 	uint8_t			 sc_open;	/* device opened */
 
+	struct mutex		 sc_mtx;
 	int			 sc_fsize;
 	uint8_t			*sc_fbuffer;
 	caddr_t			 sc_fbuffer_mmap;
@@ -58,9 +66,9 @@ struct video_softc {
 #define		VIDMODE_NONE	0
 #define		VIDMODE_MMAP	1
 #define		VIDMODE_READ	2
-	int			 sc_frames_ready;
+	int			 sc_frames_ready;	/* [m] */
 
-	struct selinfo		 sc_rsel;	/* read selector */
+	struct klist		 sc_rklist;		/* [m] read selector */
 };
 
 int	videoprobe(struct device *, void *, void *);
@@ -105,6 +113,8 @@ videoattach(struct device *parent, struc
 	sc->sc_dev = parent;
 	sc->sc_fbufferlen = 0;
 	sc->sc_owner = NULL;
+	mtx_init(&sc->sc_mtx, IPL_MPFLOOR);
+	klist_init_mutex(&sc->sc_rklist, &sc->sc_mtx);
 
 	if (sc->hw_if->get_bufsize)
 		sc->sc_fbufferlen = (sc->hw_if->get_bufsize)(sc->hw_hdl);
@@ -205,21 +215,28 @@ videoread(dev_t dev, struct uio *uio, in
 
 	DPRINTF(1, "resid=%zu\n", uio->uio_resid);
 
+	mtx_enter(&sc->sc_mtx);
+
 	if (sc->sc_frames_ready < 1) {
 		/* block userland read until a frame is ready */
-		error = tsleep_nsec(sc, PWAIT | PCATCH, "vid_rd", INFSLP);
+		error = msleep_nsec(sc, &sc->sc_mtx, PWAIT | PCATCH,
+		    "vid_rd", INFSLP);
 		if (sc->sc_dying)
 			error = EIO;
-		if (error)
+		if (error) {
+			mtx_leave(&sc->sc_mtx);
 			return (error);
+		}
 	}
+	sc->sc_frames_ready--;
+
+	mtx_leave(&sc->sc_mtx);
 
 	/* move no more than 1 frame to userland, as per specification */
 	size = ulmin(uio->uio_resid, sc->sc_fsize);
 	if (!video_record_enable)
 		bzero(sc->sc_fbuffer, size);
 	error = uiomove(sc->sc_fbuffer, size, uio);
-	sc->sc_frames_ready--;
 	if (error)
 		return (error);
 
@@ -356,7 +373,9 @@ videoioctl(dev_t dev, u_long cmd, caddr_
 		    (struct v4l2_buffer *)data);
 		if (!video_record_enable)
 			bzero(sc->sc_fbuffer_mmap + vb->m.offset, vb->length);
+		mtx_enter(&sc->sc_mtx);
 		sc->sc_frames_ready--;
+		mtx_leave(&sc->sc_mtx);
 		break;
 	case VIDIOC_STREAMON:
 		if (sc->hw_if->streamon)
@@ -429,11 +448,8 @@ void
 filt_videodetach(struct knote *kn)
 {
 	struct video_softc *sc = kn->kn_hook;
-	int s;
 
-	s = splhigh();
-	klist_remove_locked(&sc->sc_rsel.si_note, kn);
-	splx(s);
+	klist_remove(&sc->sc_rklist, kn);
 }
 
 int
@@ -447,11 +463,39 @@ filt_videoread(struct knote *kn, long hi
 	return (0);
 }
 
+int
+filt_videomodify(struct kevent *kev, struct knote *kn)
+{
+	struct video_softc *sc = kn->kn_hook;
+	int active;
+
+	mtx_enter(&sc->sc_mtx);
+	active = knote_modify(kev, kn); 
+	mtx_leave(&sc->sc_mtx);
+
+	return (active);
+}
+
+int
+filt_videoprocess(struct knote *kn, struct kevent *kev)
+{
+	struct video_softc *sc = kn->kn_hook;
+	int active;
+
+	mtx_enter(&sc->sc_mtx);
+	active = knote_process(kn, kev);
+	mtx_leave(&sc->sc_mtx);
+
+	return (active);
+}
+
 const struct filterops video_filtops = {
-	.f_flags	= FILTEROP_ISFD,
+	.f_flags	= FILTEROP_ISFD | FILTEROP_MPSAFE,
 	.f_attach	= NULL,
 	.f_detach	= filt_videodetach,
 	.f_event	= filt_videoread,
+	.f_modify	= filt_videomodify,
+	.f_process	= filt_videoprocess,
 };
 
 int
@@ -459,7 +503,7 @@ videokqfilter(dev_t dev, struct knote *k
 {
 	int unit = VIDEOUNIT(dev);
 	struct video_softc *sc;
-	int s, error;
+	int error;
 
 	KERNEL_ASSERT_LOCKED();
 
@@ -493,9 +537,7 @@ videokqfilter(dev_t dev, struct knote *k
 		sc->sc_vidmode = VIDMODE_READ;
 	}
 
-	s = splhigh();
-	klist_insert_locked(&sc->sc_rsel.si_note, kn);
-	splx(s);
+	klist_insert(&sc->sc_rklist, kn);
 
 	return (0);
 }
@@ -528,13 +570,15 @@ video_intr(void *addr)
 	struct video_softc *sc = (struct video_softc *)addr;
 
 	DPRINTF(3, "video_intr sc=%p\n", sc);
+	mtx_enter(&sc->sc_mtx);
 	if (sc->sc_vidmode != VIDMODE_NONE)
 		sc->sc_frames_ready++;
 	else
 		printf("%s: interrupt but no streams!\n", __func__);
 	if (sc->sc_vidmode == VIDMODE_READ)
 		wakeup(sc);
-	selwakeup(&sc->sc_rsel);
+	knote_locked(&sc->sc_rklist, 0);
+	mtx_leave(&sc->sc_mtx);
 }
 
 int
@@ -548,7 +592,9 @@ video_stop(struct video_softc *sc)
 		error = sc->hw_if->close(sc->hw_hdl);
 
 	sc->sc_vidmode = VIDMODE_NONE;
+	mtx_enter(&sc->sc_mtx);
 	sc->sc_frames_ready = 0;
+	mtx_leave(&sc->sc_mtx);
 	sc->sc_owner = NULL;
 
 	return (error);
@@ -582,7 +628,7 @@ int
 videodetach(struct device *self, int flags)
 {
 	struct video_softc *sc = (struct video_softc *)self;
-	int s, maj, mn;
+	int maj, mn;
 
 	/* locate the major number */
 	for (maj = 0; maj < nchrdev; maj++)
@@ -593,9 +639,8 @@ videodetach(struct device *self, int fla
 	mn = self->dv_unit;
 	vdevgone(maj, mn, mn, VCHR);
 
-	s = splhigh();
-	klist_invalidate(&sc->sc_rsel.si_note);
-	splx(s);
+	klist_invalidate(&sc->sc_rklist);
+	klist_free(&sc->sc_rklist);
 
 	free(sc->sc_fbuffer, M_DEVBUF, sc->sc_fbufferlen);