Download raw body.
sys/uvideo: avoid using queue.h
On Wed, 19 Mar 2025 22:47:21 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
>
> On Wed, Mar 19, 2025 at 09:04:34PM GMT, Marcus Glocker wrote:
>
> > 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 ...
>
> I was playing a little bit puzzle with both of your diffs to understand
> which part fixes what.
>
> With the attached diff I can't provoke an uvm fault panic, nor a freeze
> anymore. While mpi's@ diff was taking care about the uvm fault panic,
> kirill's@ diff was fixing the freeze.
>
> What did I change:
>
> mpi@ diff [1]:
>
> - I removed the usbd_delay_ms() from uvideo_detach(), since now it's
> simple not required anymore.
> - I removed the usbd_is_dying() comment from uvideo_vs_cb(), since it
> seems to be enough to check for the usb xfer status.
>
> kirill@ diff [2]:
>
> - I only took over the videoioctl() peace to understand where ffplay
> went crazy. I'm not sure if ffplay is just keep looping over
> ioctl(). device_lookup() seems to be useful, since it does additional
> checks to find out if the device is still active, like DVF_ACTIVE, as
> already stated by kirill@ in another mail. I'm not sure about
> device_unref(), but audio(4) uses it as well.
>
> In case testing for others would be successful too, I think we should
> commit the fixes in separate parts. The uvideo(4) diff for fixing
> the uvm fault panic, the video(4) diff to fix the freeze. I'm
> potentially fine with kirill's@ full video(4) diff [2], but would like
> to have mpi@'s feedback on that as well.
>
> [1] https://marc.info/?l=openbsd-tech&m=174238030006419&w=2
> [2] https://marc.info/?l=openbsd-tech&m=174180759929893&w=2
>
I had tried your short diff and it works, but after multiple attach and
detach of webcam (I haven't count, but it was something like few dozen
times) the system simple crashed with stacktrace:
panic: attempt to access user address 0x28 from EL1
Stopped at panic+0x140: cmp w21, #0x0
TID PID UID PRFLAGS PFLAGS CPU COMMAND
*123765 9168 0 0x14000 0x200 1% usbtask
db_enter() at panic+0x13c
panic() at kdata_abort+0x180
do_el0_sync() at handle_el1h_sync+0x68
handle_el1h_sync() at uvideo_vs_close+0x20
uvideo_vs_close() at uvideo_close+0x20
uvideo_close() at uvideo_detach+0x40
uvideo_detach() at config_detach+0x160
https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.
ddb{1}>
But I can't reproduce it.
>
> 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 19 Mar 2025 21:19:42 -0000
> @@ -256,10 +256,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 +284,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 +411,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
> error = (ENOTTY);
> }
>
> +done:
> + device_unref(&sc->dev);
> return (error);
> }
>
> 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 19 Mar 2025 21:19:43 -0000
> @@ -685,13 +685,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 +2133,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 +2148,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 +2171,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 +2279,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