From: Vitaliy Makkoveev 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 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 #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -34,6 +36,11 @@ #include +/* + * 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);