From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: avoid using queue.h
To: Martin Pieuchot <mpi@grenadille.net>
Cc: "Kirill A. Korinsky" <kirill@korins.ky>, tech@openbsd.org
Date: Wed, 19 Mar 2025 21:04:34 +0100

On Wed, Mar 19, 2025 at 11:35:09AM GMT, 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 don't get an uvm fault panic anymore, but I still can manage to
freeze the system when using ffplay and detaching the device during
that in X.  I also tried to remove both usbd_delay_ms() for a test,
but makes no difference.  I currently can't see what is happening here.

We anyway could remove the usbd_delay_ms() in detach, since the only
purpose was to get around this issue by naively waiting for
uvideo_vs_cb() to complete, which doesn't work out very well we know
now ;-)

Needs some more deep dive it looks like ...
 
> Index: dev/usb/uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.252 uvideo.c
> --- dev/usb/uvideo.c	12 Mar 2025 14:08:51 -0000	1.252
> +++ dev/usb/uvideo.c	19 Mar 2025 10:31:28 -0000
> @@ -689,7 +689,7 @@ uvideo_detach(struct device *self, int f
>  	if (sc->sc_videodev != NULL)
>  		rv = config_detach(sc->sc_videodev, flags);
>  
> -	uvideo_vs_free_frame(sc);
> +	uvideo_close(sc);
>  
>  	return (rv);
>  }
> @@ -2134,9 +2134,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;
>  
> @@ -2152,6 +2149,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
> @@ -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);
>  	}
>  }
>  
> @@ -2281,9 +2281,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;
>  
> @@ -2314,7 +2311,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  
>  	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
>  
> -	if (status != USBD_NORMAL_COMPLETION) {
> +	if (status != USBD_NORMAL_COMPLETION/*|| usbd_is_dying(sc->sc_udev)*/){
>  		DPRINTF(1, "%s: %s: %s\n", DEVNAME(sc), __func__,
>  		    usbd_errstr(status));
>  		return;
> 
>