From: Marcus Glocker Subject: Re: sys/uvideo: treat frame as done when mmap queue is overflowed To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Wed, 26 Feb 2025 21:34:25 +0100 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 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