From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: treat frame as done when mmap queue is overflowed
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Tue, 25 Feb 2025 22:07:30 +0100

On Mon, Feb 24, 2025 at 02:09:01PM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> when a completed frame cannot be added to the mmap queue due to
> overflow, we do not reset the sample, FID, or error flags. Instead, we
> discard subsequent frames and samples until the stream flips the FID
> bit.
> 
> Here is a diff that prevents this behavior.
> 
> Ok?

ok mglocker@

PS:
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.
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.240 uvideo.c
> --- sys/dev/usb/uvideo.c	23 Feb 2025 14:12:15 -0000	1.240
> +++ sys/dev/usb/uvideo.c	24 Feb 2025 00:09:13 -0000
> @@ -2244,6 +2244,7 @@ uvideo_vs_decode_stream_header(struct uv
>  	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 */
> @@ -2327,7 +2328,7 @@ uvideo_vs_decode_stream_header(struct uv
>  			/* mmap */
>  			if (uvideo_mmap_queue(sc, fb->buf, fb->offset,
>  				    fb->error))
> -				return (USBD_NOMEM);
> +				ret = USBD_NOMEM;
>  		} else if (fb->error) {
>  			DPRINTF(1, "%s: %s: error frame, skipped!\n",
>  			    DEVNAME(sc), __func__);
> @@ -2341,7 +2342,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->error = 0;
>  	}
>  
> -	return (USBD_NORMAL_COMPLETION);
> +	return (ret);
>  }
>  
>  /*
> @@ -2366,6 +2367,7 @@ uvideo_vs_decode_stream_header_isight(st
>  {
>  	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 };
> @@ -2384,7 +2386,7 @@ uvideo_vs_decode_stream_header_isight(st
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
>  			if (uvideo_mmap_queue(sc, fb->buf, fb->offset, 0))
> -				return (USBD_NOMEM);
> +				ret = USBD_NOMEM;
>  		} else {
>  			/* read */
>  			uvideo_read(sc, fb->buf, fb->offset);
> @@ -2399,7 +2401,7 @@ uvideo_vs_decode_stream_header_isight(st
>  		}
>  	}
>  
> -	return (USBD_NORMAL_COMPLETION);
> +	return (ret);
>  }
>  
>  int
> @@ -2416,7 +2418,7 @@ uvideo_mmap_queue(struct uvideo_softc *s
>  			break;
>  	}
>  	if (i == sc->sc_mmap_count) {
> -		DPRINTF(1, "%s: %s: mmap queue is full!",
> +		DPRINTF(1, "%s: %s: mmap queue is full!\n",
>  		    DEVNAME(sc), __func__);
>  		return ENOMEM;
>  	}
>