From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: Martin Pieuchot , "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Wed, 19 Mar 2025 22:47:21 +0100 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;