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