[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    avoid uvideo lockup when nothing is transferred
From:       Vadim Zhukov <persgray () gmail ! com>
Date:       2020-01-15 22:07:11
Message-ID: 275bf3b0583b7e8e () persx240 ! my ! domain
[Download RAW message or body]

Hi all.

This fixes an issue I'm seeing with a uvideo(4), that doesn't like
commands we're sending to it. The camera simply sends nothing,
and since we're sleeping forever (xfer timeout is 0, which is
USBD_NO_TIMEOUT), we never get out from 'while (bulk_running)' loop,
visible in the scond chunk of the diff. This not only makes video(1)
and other V4L2 users lockup, but also results in process freeze upon
detach: it calls uvideo_vs_close(), which in turn calls
usbd_ref_wait(), which doesn't return because we still have something
in queue.

Also, in case of error we keep bulk_running set. This seems just
a leftover, as well as the first chunk: if we failed to create kthread,
there won't be anything running under bulk_running==1 for sure.

I must thank kettenis@ for help during diagnosis and mpi@ for a patch
for related issue.

OK?

--
WBR,
  Vadim Zhukov


Index: uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.205
diff -u -p -r1.205 uvideo.c
--- uvideo.c	14 Oct 2019 09:20:48 -0000	1.205
+++ uvideo.c	15 Jan 2020 21:54:43 -0000
@@ -2026,6 +2027,7 @@ uvideo_vs_start_bulk(struct uvideo_softc
 	error = kthread_create(uvideo_vs_start_bulk_thread, sc, NULL,
 	    DEVNAME(sc));
 	if (error) {
+		sc->sc_vs_cur->bulk_running = 0;
 		printf("%s: can't create kernel thread!", DEVNAME(sc));
 		return (error);
 	}
@@ -2044,6 +2046,12 @@ uvideo_vs_start_bulk_thread(void *arg)
 	while (sc->sc_vs_cur->bulk_running) {
 		size = UGETDW(sc->sc_desc_probe.dwMaxPayloadTransferSize);
 
+		/*
+		 * We can't wait infinitely, since otherwise we'll
+		 * block forever if camera stops (or don't even starts)
+		 * sending frames. Use '2*' multiplier to compensate
+		 * possible lags.
+		 */
 		usbd_setup_xfer(
 		    sc->sc_vs_cur->bxfer.xfer,
 		    sc->sc_vs_cur->pipeh,
@@ -2051,10 +2059,11 @@ uvideo_vs_start_bulk_thread(void *arg)
 		    sc->sc_vs_cur->bxfer.buf,
 		    size,
 		    USBD_NO_COPY | USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS,
-		    0,
+		    2 * 1000 / (sc->sc_frame_rate || 1),
 		    NULL);
 		error = usbd_transfer(sc->sc_vs_cur->bxfer.xfer);
 		if (error != USBD_NORMAL_COMPLETION) {
+			sc->sc_vs_cur->bulk_running = 0;
 			DPRINTF(1, "%s: error in bulk xfer: %s!\n",
 			    DEVNAME(sc), usbd_errstr(error));
 			break;

[prev in list] [next in list] [prev in thread] [next in thread]