From: Kirill A. Korinsky <kirill@korins.ky>
Subject: sys/uvideo: avoid using queue.h
To: OpenBSD tech <tech@openbsd.org>
Cc: Marcus Glocker <mglocker@openbsd.org>
Date: Thu, 06 Mar 2025 09:36:46 +0100

tech@,

macroses from queue.h doesn't thread safe and wraping it by mutex on
uvideo use case seems isn't so good idea.

It has a hardcoded number of used buffers as 8 and already has a code
which iterates against them to find the buffer which is ready for
queueing in uvideo_mmap_getbuf.

Here I'd like to replace SIMPLEQ_* by similar logic.

It fixes two bugs:

1. If isochronous webcam sends two frames in one transfer,
SIMPLEQ_INSERT_TAIL adds the same element twice and creates infinity
loop inside sc_mmap_q which will be iterating in uvideo_vs_free_frame.

2. Prevents from calling SIMPLEQ_INSERT_TAIL and SIMPLEQ_REMOVE_HEAD for
the same element in the queue which may lead to unexpected state.

This diff was suceffuly tested on:
 - Elgato Facecam Pro (isochronous)
 - Jabra PanaCast (bulk)

Ok?

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.249 uvideo.c
--- sys/dev/usb/uvideo.c	4 Mar 2025 22:59:01 -0000	1.249
+++ sys/dev/usb/uvideo.c	5 Mar 2025 22:40:46 -0000
@@ -69,7 +69,6 @@ struct uvideo_softc {
 	struct uvideo_mmap			*sc_mmap_cur;
 	uint8_t					*sc_mmap_buffer;
 	size_t					 sc_mmap_buffer_size;
-	q_mmap					 sc_mmap_q;
 	int					 sc_mmap_count;
 	int					 sc_mmap_flag;
 
@@ -671,8 +670,6 @@ uvideo_attach_hook(struct device *self)
 	if (error != USBD_NORMAL_COMPLETION)
 		return;
 
-	/* init mmap queue */
-	SIMPLEQ_INIT(&sc->sc_mmap_q);
 	sc->sc_mmap_count = 0;
 
 	DPRINTF(1, "uvideo_attach: doing video_attach_mi\n");
@@ -1962,9 +1959,6 @@ uvideo_vs_free_frame(struct uvideo_softc
 		sc->sc_mmap_buffer_size = 0;
 	}
 
-	while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q))
-		SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
-
 	sc->sc_mmap_count = 0;
 }
 
@@ -2556,7 +2551,6 @@ uvideo_mmap_queue(struct uvideo_softc *s
 	/* queue it */
 	sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
 	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
-	SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, sc->sc_mmap_cur, q_frames);
 	DPRINTF(2, "%s: %s: frame queued\n", DEVNAME(sc), __func__);
 
 	wakeup(sc);
@@ -3624,32 +3618,34 @@ int
 uvideo_dqbuf(void *v, struct v4l2_buffer *dqb)
 {
 	struct uvideo_softc *sc = v;
-	struct uvideo_mmap *mmap;
-	int error;
+	int error, i;
 
 	if (dqb->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
 	    dqb->memory != V4L2_MEMORY_MMAP)
 		return (EINVAL);
 
-	if (SIMPLEQ_EMPTY(&sc->sc_mmap_q)) {
+again:
+	/* find a buffer which is done */
+	for (i = 0; i < sc->sc_mmap_count; i++) {
+		if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE)
+			break;
+	}
+
+	if (i == sc->sc_mmap_count) {
 		/* mmap queue is empty, block until first frame is queued */
 		error = tsleep_nsec(sc, 0, "vid_mmap", SEC_TO_NSEC(10));
 		if (error)
 			return (EINVAL);
+		goto again;
 	}
 
-	mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q);
-	if (mmap == NULL)
-		panic("uvideo_dqbuf: NULL pointer!");
-
-	bcopy(&mmap->v4l2_buf, dqb, sizeof(struct v4l2_buffer));
+	bcopy(&sc->sc_mmap[i].v4l2_buf, dqb, sizeof(struct v4l2_buffer));
 
-	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
-	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
+	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
+	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
 
 	DPRINTF(2, "%s: %s: frame dequeued from index %d\n",
-	    DEVNAME(sc), __func__, mmap->v4l2_buf.index);
-	SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
+	    DEVNAME(sc), __func__, sc->sc_mmap[i].v4l2_buf.index);
 
 	return (0);
 }