Download raw body.
sys/uvideo: avoid using queue.h
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 ;-)
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;
sys/uvideo: avoid using queue.h