On Wed, Mar 19, 2025 at 09:04:34PM GMT, Marcus Glocker wrote:

> On Wed, Mar 19, 2025 at 11:35:09AM GMT, Martin Pieuchot 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 don't get an uvm fault panic anymore, but I still can manage to
> freeze the system when using ffplay and detaching the device during
> that in X.  I also tried to remove both usbd_delay_ms() for a test,
> but makes no difference.  I currently can't see what is happening here.
> 
> We anyway could remove the usbd_delay_ms() in detach, since the only
> purpose was to get around this issue by naively waiting for
> uvideo_vs_cb() to complete, which doesn't work out very well we know
> now ;-)
> 
> Needs some more deep dive it looks like ...

I was playing a little bit puzzle with both of your diffs to understand
which part fixes what.

With the attached diff I can't provoke an uvm fault panic, nor a freeze
anymore.  While mpi's@ diff was taking care about the uvm fault panic,
kirill's@ diff was fixing the freeze.

What did I change:

mpi@ diff [1]:

- I removed the usbd_delay_ms() from uvideo_detach(), since now it's
  simple not required anymore.
- I removed the usbd_is_dying() comment from uvideo_vs_cb(), since it
  seems to be enough to check for the usb xfer status.

kirill@ diff [2]:

- I only took over the videoioctl() peace to understand where ffplay
  went crazy.  I'm not sure if ffplay is just keep looping over
  ioctl().  device_lookup() seems to be useful, since it does additional
  checks to find out if the device is still active, like DVF_ACTIVE, as
  already stated by kirill@ in another mail.  I'm not sure about
  device_unref(), but audio(4) uses it as well.
 
In case testing for others would be successful too, I think we should
commit the fixes in separate parts.  The uvideo(4) diff for fixing
the uvm fault panic, the video(4) diff to fix the freeze.  I'm
potentially fine with kirill's@ full video(4) diff [2], but would like
to have mpi@'s feedback on that as well.

[1] https://marc.info/?l=openbsd-tech&m=174238030006419&w=2
[2] https://marc.info/?l=openbsd-tech&m=174180759929893&w=2


Index: sys/dev/video.c
===================================================================
RCS file: /cvs/src/sys/dev/video.c,v
diff -u -p -u -p -r1.59 video.c
--- sys/dev/video.c	16 Dec 2024 21:22:51 -0000	1.59
+++ sys/dev/video.c	19 Mar 2025 21:19:42 -0000
@@ -256,10 +256,15 @@ videoioctl(dev_t dev, u_long cmd, caddr_
 
 	KERNEL_ASSERT_LOCKED();
 
-	if (unit >= video_cd.cd_ndevs ||
-	    (sc = video_cd.cd_devs[unit]) == NULL || sc->hw_if == NULL)
+	sc = (struct video_softc *)device_lookup(&video_cd, unit);
+	if (sc == NULL)
 		return (ENXIO);
 
+	if (sc->hw_if == NULL) {
+		error = ENXIO;
+		goto done;
+	}
+
 	DPRINTF(3, "video_ioctl(%zu, '%c', %zu)\n",
 	    IOCPARM_LEN(cmd), (int) IOCGROUP(cmd), cmd & 0xff);
 
@@ -279,10 +284,10 @@ videoioctl(dev_t dev, u_long cmd, caddr_
 		error = (ENOTTY);
 	}
 	if (error != ENOTTY)
-		return (error);
+		goto done;
 
 	if ((error = video_claim(sc, p->p_p)))
-		return (error);
+		goto done;
 
 	/*
 	 * The following IOCTLs can only be called by the device owner.
@@ -406,6 +411,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
 		error = (ENOTTY);
 	}
 
+done:
+	device_unref(&sc->dev);
 	return (error);
 }
 
Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -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 21:19:43 -0000
@@ -685,13 +685,10 @@ uvideo_detach(struct device *self, int f
 	struct uvideo_softc *sc = (struct uvideo_softc *)self;
 	int rv = 0;
 
-	/* Wait for outstanding requests to complete */
-	usbd_delay_ms(sc->sc_udev, UVIDEO_NFRAMES_MAX);
-
 	if (sc->sc_videodev != NULL)
 		rv = config_detach(sc->sc_videodev, flags);
 
-	uvideo_vs_free_frame(sc);
+	uvideo_close(sc);
 
 	return (rv);
 }
@@ -2136,9 +2133,6 @@ skip_set_alt:
 void
 uvideo_vs_close(struct uvideo_softc *sc)
 {
-	if (usbd_is_dying(sc->sc_udev))
-		return;
-
 	if (sc->sc_vs_cur->bulk_running == 1) {
 		sc->sc_vs_cur->bulk_running = 0;
 
@@ -2154,6 +2148,10 @@ uvideo_vs_close(struct uvideo_softc *sc)
 		sc->sc_vs_cur->pipeh = NULL;
 	}
 
+	/* No need to mess with HW if the device is gone. */
+	if (usbd_is_dying(sc->sc_udev))
+		return;
+
 	if (sc->sc_vs_cur->bulk_endpoint) {
 		/*
 		 * UVC doesn't specify how to notify a bulk-based device
@@ -2173,8 +2171,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);
 	}
 }
 
@@ -2282,9 +2279,6 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
 	usbd_status error;
 
 	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;