From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/uvideo: avoid using queue.h
To: marcus@nazgul.ch, tech@openbsd.org
Date: Thu, 20 Mar 2025 15:24:29 +0100

On Thu, 20 Mar 2025 12:59:43 +0100,
Martin Pieuchot <mpi@grenadille.net> wrote:
> 
> > 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	20 Mar 2025 09:43:10 -0000
> > @@ -685,13 +685,12 @@ 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);
> > +	/* avoid free when device detached before descriptor was parsed */
> > +	if (sc->sc_vs_cur)
> > +		uvideo_close(sc);
> >  
> >  	return (rv);
> >  }
> 
> 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.

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	20 Mar 2025 13:39:18 -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;