ping
> On 27 Sep 2023, at 23:19, Vitaliy Makkoveev <m...@openbsd.org> wrote:
>
> Introduce `sc_mtx` mutex(9) and use it for `sc_frames_ready' and
> `sc_rklist' knotes list protection.
>
> Index: sys/dev/video.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/video.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 video.c
> --- sys/dev/video.c 2 Jul 2022 08:50:41 -0000 1.57
> +++ sys/dev/video.c 27 Sep 2023 20:15:34 -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,8 @@ 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 +67,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 +114,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 +216,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 +374,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 +449,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 +464,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 +504,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 +538,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 +571,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 +593,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 +629,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 +640,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);
>
>
