From: Kirill A. Korinsky Subject: Re: sys/uvideo: avoid using queue.h To: Marcus Glocker , "Kirill A. Korinsky" , tech@openbsd.org Date: Wed, 19 Mar 2025 14:18:18 +0100 On Wed, 19 Mar 2025 11:35:09 +0100, 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 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 #include #include +#include #include @@ -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)