On Wed, 19 Mar 2025 11:35:09 +0100,
Martin Pieuchot <mpi@grenadille.net> wrote:
> 
> There are two issues here.  The first one is that uvideo_detach()
> doesn't "close" the USB endpoints which should abort the transfers.  I'd
> suggest uvideo_detach() calls uvideo_close().
> 
> The second issue is that uvideo_close() doesn't close the pipe due to
> the wrong isdying() check.  This is what aborts transfers and stop the
> callbacks.
> 
> Here's an untested diff that illustrates all of this.  Note that the
> kernel might now call uvideo_close() twice and some pointers might have
> to be cleaned.
> 
> The "dying" check should be avoided as much as possible.  I don't think
> it is necessary in the callback if the pipe is aborted during the close
> operation.  Btw usbd_close_pipe() implies usbd_abort_pipe().
> 
>

Thanks for the diff, I just tested it.

> @@ -2171,8 +2172,7 @@ uvideo_vs_close(struct uvideo_softc *sc)
>  		usbd_delay_ms(sc->sc_udev, 100);
>  
>  		/* switch back to default interface (turns off cam LED) */
> -		if (!usbd_is_dying(sc->sc_udev))
> -			(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> +		(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
>  	}
>  }
>  

this hunk is required, because kernel may free memora which is pointed by
sc_vs_cur for 100 ms wait, and system crashed.

Yes, I had tested it, and crash inside usbd_set_interface is back.

Anyway, I had reused your diff in that I'm working on right now and would
like to point that I can't crash the system anymore with inlined diff and
https://marc.info/?l=openbsd-tech&m=174180759929893&w=2

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.253 uvideo.c
--- sys/dev/usb/uvideo.c	18 Mar 2025 13:38:15 -0000	1.253
+++ sys/dev/usb/uvideo.c	19 Mar 2025 13:14:46 -0000
@@ -24,6 +24,7 @@
 #include <sys/stat.h>
 #include <sys/kthread.h>
 #include <sys/stdint.h>
+#include <sys/atomic.h>
 
 #include <uvm/uvm_extern.h>
 
@@ -50,12 +51,18 @@ int uvideo_debug = 1;
 #define byteof(x) ((x) >> 3)
 #define bitof(x)  (1L << ((x) & 0x7))
 
+#define UVIDEO_FLAG_ATTACHED		0x00000001
+#define UVIDEO_FLAG_CLOSED		0x00000002
+#define UVIDEO_FLAG_PARSING_FRAME	0x00000004
+
 struct uvideo_softc {
 	struct device				 sc_dev;
 	struct usbd_device			*sc_udev;
 	int					 sc_iface;
 	int					 sc_nifaces;
 
+	int					 sc_flags;
+
 	struct device				*sc_videodev;
 
 	int					 sc_max_ctrl_size;
@@ -465,7 +472,7 @@ uvideo_open(void *addr, int flags, int *
 
 	DPRINTF(1, "%s: uvideo_open: sc=%p\n", DEVNAME(sc), sc);
 
-	if (usbd_is_dying(sc->sc_udev))
+	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
 		return (EIO);
 
 	/* pointers to upper video layer */
@@ -487,6 +494,9 @@ uvideo_close(void *addr)
 
 	DPRINTF(1, "%s: uvideo_close: sc=%p\n", DEVNAME(sc), sc);
 
+	if (sc->sc_flags & UVIDEO_FLAG_CLOSED)
+		return (EIO);
+
 #ifdef UVIDEO_DUMP
 	usb_rem_task(sc->sc_udev, &sc->sc_task_write);
 #endif
@@ -501,6 +511,9 @@ uvideo_close(void *addr)
 
 	/* free video stream frame buffer */
 	uvideo_vs_free_frame(sc);
+
+	atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_CLOSED);
+
 	return (0);
 }
 
@@ -550,6 +563,8 @@ uvideo_attach(struct device *parent, str
 
 	sc->sc_udev = uaa->device;
 
+	sc->sc_flags = UVIDEO_FLAG_ATTACHED;
+
 	/* Find the first unclaimed video interface. */
 	for (i = 0; i < uaa->nifaces; i++) {
 		if (usbd_iface_claimed(sc->sc_udev, i))
@@ -685,13 +700,16 @@ uvideo_detach(struct device *self, int f
 	struct uvideo_softc *sc = (struct uvideo_softc *)self;
 	int rv = 0;
 
+	atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_ATTACHED);
+
 	/* Wait for outstanding requests to complete */
-	usbd_delay_ms(sc->sc_udev, UVIDEO_NFRAMES_MAX);
+	if (sc->sc_flags & UVIDEO_FLAG_PARSING_FRAME)
+		usbd_delay_ms(sc->sc_udev, 1);
 
 	if (sc->sc_videodev != NULL)
 		rv = config_detach(sc->sc_videodev, flags);
 
-	uvideo_vs_free_frame(sc);
+	uvideo_close(sc);
 
 	return (rv);
 }
@@ -2136,7 +2154,7 @@ skip_set_alt:
 void
 uvideo_vs_close(struct uvideo_softc *sc)
 {
-	if (usbd_is_dying(sc->sc_udev))
+	if (sc->sc_flags & UVIDEO_FLAG_CLOSED)
 		return;
 
 	if (sc->sc_vs_cur->bulk_running == 1) {
@@ -2258,7 +2276,12 @@ uvideo_vs_start_bulk_thread(void *arg)
 
 		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
 
+		if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
+			break;
+
+		atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
 		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
+		atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
 	}
 	usbd_ref_decr(sc->sc_udev);
 
@@ -2283,9 +2306,6 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
 
 	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
 
-	if (usbd_is_dying(sc->sc_udev))
-		return;
-
 	for (i = 0; i < sc->sc_nframes; i++)
 		ixfer->size[i] = sc->sc_vs_cur->psize;
 
@@ -2335,7 +2355,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 			/* frame is empty */
 			continue;
 
+		if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
+			return;
+
+		atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
 		sc->sc_decode_stream_header(sc, frame, frame_size);
+		atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
 	}
 
 skip:	/* setup new transfer */
@@ -3596,6 +3621,9 @@ uvideo_querybuf(void *v, struct v4l2_buf
 	    qb->index >= sc->sc_mmap_count)
 		return (EINVAL);
 
+	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
+		return (EINVAL);
+
 	bcopy(&sc->sc_mmap[qb->index].v4l2_buf, qb,
 	    sizeof(struct v4l2_buffer));
 
@@ -3644,6 +3672,9 @@ uvideo_dqbuf(void *v, struct v4l2_buffer
 		if (error)
 			return (EINVAL);
 	}
+
+	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
+		return (EINVAL);
 
 	mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q);
 	if (mmap == NULL)