From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: "Kirill A. Korinsky" Cc: Martin Pieuchot , tech@openbsd.org Date: Thu, 20 Mar 2025 22:16:36 +0100 On Thu, Mar 20, 2025 at 09:18:47PM GMT, Kirill A. Korinsky wrote: > On Thu, 20 Mar 2025 20:56:20 +0100, > Marcus Glocker wrote: > > > > On Thu, Mar 20, 2025 at 03:24:29PM GMT, Kirill A. Korinsky wrote: > > > > > > How is this possible? How can a device be detached before it is > > > > attached? > > > > > > > > Anyway, if we are going to check if `sc_vs_cur' is set, can we do it in > > > > uvideo_open() to ensure it is not possible to open such device? > > > > > > > > Then, I'd suggest you move the check inside uvideo_close() to ease the > > > > understanding and for symmetry ;) > > > > > > > > And should we set this pointer just before /* init map queue */ in > > > > uvideo_attach_hook()? Before that some operations might fail... > > > > > > > > > > > > > > How? I need to attach and detach devices fast enough and enough times to > > > discover that machine in the ddb. > > > > > > I suspect that this can be related to 300 ms delay inside detach somehow, > > > because as soon as I had switched the webcam to bulk mode I can't reproduce > > > it. > > > > > > So, here an updated diff where I've moved test that sc_vs_cur is not NULL to > > > both uvideo_open and uvideo_close, as suggested. > > > > Works fine for me together with the full video.c patch. Couldn't force > > any crashes/freezes on amd64/arm64. But the expert for uvideo(4) > > crashing is kirill@. So if it's stable for you, it's probably stable > > for everyone ;-) > > > > When let commit it! > > I'm waiting for OK to do it. I'm basically OK with both diffs. But lets give mpi@ also a chance to review again. > > 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 20 Mar 2025 19:40:29 -0000 > > @@ -140,14 +140,13 @@ videoopen(dev_t dev, int flags, int fmt, > > > > 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->sc_open) { > > DPRINTF(1, "%s: device already open\n", __func__); > > - return (0); > > + goto done; > > } > > > > sc->sc_vidmode = VIDMODE_NONE; > > @@ -162,12 +161,15 @@ videoopen(dev_t dev, int flags, int fmt, > > DPRINTF(1, "%s: set device to open\n", __func__); > > } > > > > +done: > > + device_unref(&sc->dev); > > return (error); > > } > > > > int > > videoclose(dev_t dev, int flags, int fmt, struct proc *p) > > { > > + int unit = VIDEOUNIT(dev); > > struct video_softc *sc; > > int error = 0; > > > > @@ -175,11 +177,14 @@ videoclose(dev_t dev, int flags, int fmt > > > > DPRINTF(1, "%s: last close\n", __func__); > > > > - sc = video_cd.cd_devs[VIDEOUNIT(dev)]; > > + sc = (struct video_softc *)device_lookup(&video_cd, unit); > > + if (sc == NULL) > > + return (ENXIO); > > > > error = video_stop(sc); > > sc->sc_open = 0; > > > > + device_unref(&sc->dev); > > return (error); > > } > > > > @@ -188,29 +193,33 @@ videoread(dev_t dev, struct uio *uio, in > > { > > int unit = VIDEOUNIT(dev); > > struct video_softc *sc; > > - int error; > > + int error = 0; > > size_t size; > > > > KERNEL_ASSERT_LOCKED(); > > > > - if (unit >= video_cd.cd_ndevs || > > - (sc = video_cd.cd_devs[unit]) == NULL) > > + sc = (struct video_softc *)device_lookup(&video_cd, unit); > > + if (sc == NULL) > > return (ENXIO); > > > > - if (sc->sc_dying) > > - return (EIO); > > + if (sc->sc_dying) { > > + error = EIO; > > + goto done; > > + } > > > > - if (sc->sc_vidmode == VIDMODE_MMAP) > > - return (EBUSY); > > + if (sc->sc_vidmode == VIDMODE_MMAP) { > > + error = EBUSY; > > + goto done; > > + } > > > > if ((error = video_claim(sc, curproc->p_p))) > > - return (error); > > + goto done; > > > > /* start the stream if not already started */ > > if (sc->sc_vidmode == VIDMODE_NONE && sc->hw_if->start_read) { > > error = sc->hw_if->start_read(sc->hw_hdl); > > if (error) > > - return (error); > > + goto done; > > sc->sc_vidmode = VIDMODE_READ; > > } > > > > @@ -226,7 +235,7 @@ videoread(dev_t dev, struct uio *uio, in > > error = EIO; > > if (error) { > > mtx_leave(&sc->sc_mtx); > > - return (error); > > + goto done; > > } > > } > > sc->sc_frames_ready--; > > @@ -239,11 +248,13 @@ videoread(dev_t dev, struct uio *uio, in > > bzero(sc->sc_fbuffer, size); > > error = uiomove(sc->sc_fbuffer, size, uio); > > if (error) > > - return (error); > > + goto done; > > > > DPRINTF(1, "uiomove successfully done (%zu bytes)\n", size); > > > > - return (0); > > +done: > > + device_unref(&sc->dev); > > + return (error); > > } > > > > int > > @@ -256,10 +267,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 +295,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 +422,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_ > > error = (ENOTTY); > > } > > > > +done: > > + device_unref(&sc->dev); > > return (error); > > } > > > > @@ -421,19 +439,19 @@ videommap(dev_t dev, off_t off, int prot > > > > DPRINTF(2, "%s: off=%lld, prot=%d\n", __func__, off, prot); > > > > - if (unit >= video_cd.cd_ndevs || > > - (sc = video_cd.cd_devs[unit]) == NULL) > > + sc = (struct video_softc *)device_lookup(&video_cd, unit); > > + if (sc == NULL) > > return (-1); > > > > if (sc->sc_dying) > > - return (-1); > > + goto err; > > > > if (sc->hw_if->mappage == NULL) > > - return (-1); > > + goto err; > > > > p = sc->hw_if->mappage(sc->hw_hdl, off, prot); > > if (p == NULL) > > - return (-1); > > + goto err; > > if (pmap_extract(pmap_kernel(), (vaddr_t)p, &pa) == FALSE) > > panic("videommap: invalid page"); > > sc->sc_vidmode = VIDMODE_MMAP; > > @@ -442,7 +460,12 @@ videommap(dev_t dev, off_t off, int prot > > if (off == 0) > > sc->sc_fbuffer_mmap = p; > > > > + device_unref(&sc->dev); > > return (pa); > > + > > +err: > > + device_unref(&sc->dev); > > + return (-1); > > } > > > > void > > @@ -504,16 +527,18 @@ videokqfilter(dev_t dev, struct knote *k > > { > > int unit = VIDEOUNIT(dev); > > struct video_softc *sc; > > - int error; > > + int error = 0; > > > > KERNEL_ASSERT_LOCKED(); > > > > - if (unit >= video_cd.cd_ndevs || > > - (sc = video_cd.cd_devs[unit]) == NULL) > > + sc = (struct video_softc *)device_lookup(&video_cd, unit); > > + if (sc == NULL) > > return (ENXIO); > > > > - if (sc->sc_dying) > > - return (ENXIO); > > + if (sc->sc_dying) { > > + error = ENXIO; > > + goto done; > > + } > > > > switch (kn->kn_filter) { > > case EVFILT_READ: > > @@ -521,11 +546,12 @@ videokqfilter(dev_t dev, struct knote *k > > kn->kn_hook = sc; > > break; > > default: > > - return (EINVAL); > > + error = EINVAL; > > + goto done; > > } > > > > if ((error = video_claim(sc, curproc->p_p))) > > - return (error); > > + goto done; > > > > /* > > * Start the stream in read() mode if not already started. If > > @@ -540,7 +566,9 @@ videokqfilter(dev_t dev, struct knote *k > > > > klist_insert(&sc->sc_rklist, kn); > > > > - return (0); > > +done: > > + device_unref(&sc->dev); > > + return (error); > > } > > > > int > > 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 20 Mar 2025 19:40:32 -0000 > > @@ -465,7 +465,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 (usbd_is_dying(sc->sc_udev) || sc->sc_vs_cur == NULL) > > return (EIO); > > > > /* pointers to upper video layer */ > > @@ -487,6 +487,9 @@ uvideo_close(void *addr) > > > > DPRINTF(1, "%s: uvideo_close: sc=%p\n", DEVNAME(sc), sc); > > > > + if (sc->sc_vs_cur == NULL) > > + return (EIO); > > + > > #ifdef UVIDEO_DUMP > > usb_rem_task(sc->sc_udev, &sc->sc_task_write); > > #endif > > @@ -685,13 +688,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 +2136,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 +2151,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 +2174,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 +2282,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; > > -- > wbr, Kirill