On Tue, Feb 25, 2025 at 11:19:58PM GMT, Kirill A. Korinsky wrote:

> On Tue, 25 Feb 2025 22:07:30 +0100,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> > 
> > I've noticed that the for loop in uvideo_vs_cb() is checking
> > sc->sc_decode_stream_header() for an USBD_CANCELLED return code, but
> > sc->sc_decode_stream_header() is never returning that.  Probably we
> > should better check for 'error != USBD_NORMAL_COMPLETION' to break
> > the for loop, and then always reset the frame buffers sample, fid, and
> > error flags to zero in sc->sc_decode_stream_header() return error case.
> >
> > I can come up with a diff for that later.
> >
> 
> Not sure that it is a good idea.
> 
> A for loop exists only for isochronous endpoint, and if one of parallel
> readed frame is contained something very wired, other one may contain
> usefull data.

Probably I was too long away from this topic, and for a minute I was
thinking that a broken frame would lead to a broken image, but probably
brain fart.

Anyway, at the moment nothing is checking the values which
sc_decode_stream_header returns, and so we could convert it to a void
function to remove some confusion.  Same for uvideo_mmap_queue.


Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -u -p -r1.244 uvideo.c
--- sys/dev/usb/uvideo.c	25 Feb 2025 22:13:44 -0000	1.244
+++ sys/dev/usb/uvideo.c	26 Feb 2025 20:27:59 -0000
@@ -101,7 +101,7 @@ struct uvideo_softc {
 	void					 (*sc_uplayer_intr)(void *);
 
 	const struct uvideo_devs		*sc_quirk;
-	usbd_status				(*sc_decode_stream_header)
+	void					(*sc_decode_stream_header)
 						    (struct uvideo_softc *,
 						    uint8_t *, int);
 };
@@ -165,11 +165,11 @@ void		uvideo_vs_start_isoc_ixfer(struct 
 		    struct uvideo_isoc_xfer *);
 void		uvideo_vs_cb(struct usbd_xfer *, void *,
 		    usbd_status);
-usbd_status	uvideo_vs_decode_stream_header(struct uvideo_softc *,
+void		uvideo_vs_decode_stream_header(struct uvideo_softc *,
 		    uint8_t *, int); 
-usbd_status	uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
+void		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
 		    uint8_t *, int);
-int		uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int);
+void		uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int);
 void		uvideo_read(struct uvideo_softc *, uint8_t *, int);
 usbd_status	uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t,
 		    uint16_t, uint8_t *, size_t);
@@ -2149,8 +2149,7 @@ uvideo_vs_start_bulk_thread(void *arg)
 
 		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
 
-		(void)sc->sc_decode_stream_header(sc,
-		    sc->sc_vs_cur->bxfer.buf, size);
+		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
 	}
 	usbd_ref_decr(sc->sc_udev);
 
@@ -2205,7 +2204,6 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 	struct uvideo_softc *sc = ixfer->sc;
 	int len, i, frame_size;
 	uint8_t *frame;
-	usbd_status error;
 
 	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
 
@@ -2228,27 +2226,24 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 			/* frame is empty */
 			continue;
 
-		error = sc->sc_decode_stream_header(sc, frame, frame_size);
-		if (error == USBD_CANCELLED)
-			break;
+		sc->sc_decode_stream_header(sc, frame, frame_size);
 	}
 
 skip:	/* setup new transfer */
 	uvideo_vs_start_isoc_ixfer(sc, ixfer);
 }
 
-usbd_status
+void
 uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
     int frame_size)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	struct usb_video_stream_header *sh;
 	int sample_len;
-	usbd_status ret = USBD_NORMAL_COMPLETION;
 
 	if (frame_size < UVIDEO_SH_MIN_LEN)
 		/* frame too small to contain a valid stream header */
-		return (USBD_INVAL);
+		return;
 
 	sh = (struct usb_video_stream_header *)frame;
 
@@ -2256,7 +2251,7 @@ uvideo_vs_decode_stream_header(struct uv
 
 	if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
 		/* invalid header size */
-		return (USBD_INVAL);
+		return;
 
 	DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
 
@@ -2326,9 +2321,7 @@ uvideo_vs_decode_stream_header(struct uv
 #endif
 		if (sc->sc_mmap_flag) {
 			/* mmap */
-			if (uvideo_mmap_queue(sc, fb->buf, fb->offset,
-				    fb->error))
-				ret = USBD_NOMEM;
+			uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error);
 		} else if (fb->error) {
 			DPRINTF(1, "%s: %s: error frame, skipped!\n",
 			    DEVNAME(sc), __func__);
@@ -2341,8 +2334,6 @@ uvideo_vs_decode_stream_header(struct uv
 		fb->fid = 0;
 		fb->error = 0;
 	}
-
-	return (ret);
 }
 
 /*
@@ -2361,13 +2352,12 @@ uvideo_vs_decode_stream_header(struct uv
  * Sometimes the stream header is prefixed by a unknown byte.  Therefore
  * we check for the magic value on two offsets.
  */
-usbd_status
+void
 uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
     int frame_size)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	int sample_len, header = 0;
-	usbd_status ret = USBD_NORMAL_COMPLETION;
 	uint8_t magic[] = {
 	    0x11, 0x22, 0x33, 0x44,
 	    0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce };
@@ -2379,14 +2369,13 @@ uvideo_vs_decode_stream_header_isight(st
 
 	if (header && fb->fid == 0) {
 		fb->fid = 1;
-		return (USBD_NORMAL_COMPLETION);
+		return;
 	}
 
 	if (header) {
 		if (sc->sc_mmap_flag) {
 			/* mmap */
-			if (uvideo_mmap_queue(sc, fb->buf, fb->offset, 0))
-				ret = USBD_NOMEM;
+			uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
 		} else {
 			/* read */
 			uvideo_read(sc, fb->buf, fb->offset);
@@ -2400,11 +2389,9 @@ uvideo_vs_decode_stream_header_isight(st
 			fb->offset += sample_len;
 		}
 	}
-
-	return (ret);
 }
 
-int
+void
 uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
 {
 	int i;
@@ -2420,7 +2407,7 @@ uvideo_mmap_queue(struct uvideo_softc *s
 	if (i == sc->sc_mmap_count) {
 		DPRINTF(1, "%s: %s: mmap queue is full!\n",
 		    DEVNAME(sc), __func__);
-		return ENOMEM;
+		return;
 	}
 
 	/* copy frame to mmap buffer and report length */
@@ -2454,8 +2441,6 @@ uvideo_mmap_queue(struct uvideo_softc *s
 	 * ready to dequeue.
 	 */
 	sc->sc_uplayer_intr(sc->sc_uplayer_arg);
-
-	return 0;
 }
 
 void