Download raw body.
sys/uvideo: avoid using queue.h
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 <marcus@nazgul.ch> 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
sys/uvideo: avoid using queue.h