From: Kirill A. Korinsky Subject: Re: sys/uvideo: avoid using queue.h To: Marcus Glocker Cc: tech@openbsd.org Date: Tue, 18 Mar 2025 14:21:33 +0100 On Tue, 18 Mar 2025 14:11:37 +0100, Marcus Glocker wrote: > > On Tue, Mar 18, 2025 at 12:50:38AM GMT, Kirill A. Korinsky wrote: > > > Regarding freezing on detach. I was able to reproduce it with your latest > > diff where you improved buffer utilization (like on snapshot), and with my > > diff and your mutex diff the system crashes with OCRed stacktrace: > > > > panic: attempt to access user address 0x62d98 from ELI > > Stopped at panic+0x114f cmp w21, #0x0 > > TID PID UID FLAGS PFLAGS CPU COMMAND > > *1076599 3134 35 0x12 0 0 xorg > > 338189 39674 0 0x10000 0 3 hotplugd > > > > db_enter() at panic+0x1c > > panic() at kdata_abort+0x180 > > do_el0_sync() at handle_el1h_sync+0x68 > > handle_el1h_sync() at uvideo_vs_decode_stream_header+0x9c > > uvideo_vs_decode_stream_header() at uvideo_vs_cb+0x94 > > uvideo_vs_cb() at usb_transfer_complete+0x220 > > usb_transfer_complete() at xhci_event_dequeue+0x10c > > > > Ok, next. Adding only my diff with device_lookup changes the crash to: > > > > panic: uvm_fault failed: fffff80002fd48 esr 96000004 far dead07fdeadbf37 > > Stopped at panic+0x140: cmp w21, #0x0 > > TID PID UID FLAGS PFLAGS CPU COMMAND > > 304169 16111 35 0x12 0 7 xorg > > > > db_enter() at panic+0x13c > > panic() at kdata_abort+0x170 > > kdata_abort() at handle_el1h_sync+0x68 > > handle_el1h_sync() at usbd_free_xfer+0x58 > > usbd_free_xfer() at uvideo_vs_free_isoc+0x48 > > uvideo_vs_free_isoc() at uvideo_close+0xe0 > > uvideo_close() at videoclose+0x5c > > > > but it only happens if ffplay doesn't quit on detach and consumes a lot of > > wired errors (really a lot), and I forced to kill it. > > > > Yeah, non of diffs help with that, and I think that all consumer should be > > stop before device is detached. > > In any case, a user-land application still shouldn't crash the kernel. > I'm still puzzled where exactly and why this happens. I think we should > try to understand and tackle this specific crash next. > Sure it shouldn't, but let move one step on the time. And I think that switch to device_lookup is logical the next step, at least it doesn't crash system each detach. > > Anyway, I'd like to point that I like your diff, and I think it should go > > because it improves behaviour and make it compatible with Linux. > > > > I also would like to point that I had discovered the freeze after nuke of > > the second bcopy was commited, and I recall that I can't reproduce it easy > > if I had enabled debug. I think that this issue may turn back when we > > removed bcopy again, but so far I agree that SIMPLEQ is the best option. > > I'm willing to take OKs to get this peace in ;-) Following the same > diff for reference. > Personally, I think that this way it is cleaner: int uvideo_get_buf(struct uvideo_softc *sc) { int i, idx; /* find a buffer which is ready for queueing */ for (i = 0; i < sc->sc_mmap_count; i++) { idx = sc->sc_mmap_buffer_idx; if (sc->sc_mmap[idx].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) { if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) sc->sc_mmap_buffer_idx = 0; return idx; } if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) sc->sc_mmap_buffer_idx = 0; } return -1; } and you have OK kirill@ for your version or for the "cleaner version". > > Index: sys/dev/usb/uvideo.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/uvideo.c,v > diff -u -p -u -p -r1.252 uvideo.c > --- sys/dev/usb/uvideo.c 12 Mar 2025 14:08:51 -0000 1.252 > +++ sys/dev/usb/uvideo.c 17 Mar 2025 20:35:40 -0000 > @@ -68,6 +68,7 @@ struct uvideo_softc { > struct uvideo_mmap sc_mmap[UVIDEO_MAX_BUFFERS]; > uint8_t *sc_mmap_buffer; > size_t sc_mmap_buffer_size; > + int sc_mmap_buffer_idx; > q_mmap sc_mmap_q; > int sc_mmap_count; > int sc_mmap_flag; > @@ -172,6 +173,7 @@ void uvideo_vs_decode_stream_header(str > uint8_t *, int); > void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *, > uint8_t *, int); > +int uvideo_get_buf(struct uvideo_softc *); > void uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int); > void uvideo_read(struct uvideo_softc *, uint8_t *, int); > usbd_status uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t, > @@ -2498,6 +2500,30 @@ uvideo_vs_decode_stream_header_isight(st > } > } > > +int > +uvideo_get_buf(struct uvideo_softc *sc) > +{ > + int i, idx = sc->sc_mmap_buffer_idx; > + > + /* find a buffer which is ready for queueing */ > + for (i = 0; i < sc->sc_mmap_count; i++) { > + if (sc->sc_mmap[sc->sc_mmap_buffer_idx].v4l2_buf.flags & > + V4L2_BUF_FLAG_QUEUED) { > + idx = sc->sc_mmap_buffer_idx; > + if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) > + sc->sc_mmap_buffer_idx = 0; > + break; > + } > + if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) > + sc->sc_mmap_buffer_idx = 0; > + } > + > + if (i == sc->sc_mmap_count) > + return -1; > + > + return idx; > +} > + > void > uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err) > { > @@ -2507,11 +2533,8 @@ uvideo_mmap_queue(struct uvideo_softc *s > panic("%s: mmap buffers not allocated", __func__); > > /* find a buffer which is ready for queueing */ > - for (i = 0; i < sc->sc_mmap_count; i++) { > - if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) > - break; > - } > - if (i == sc->sc_mmap_count) { > + i = uvideo_get_buf(sc); > + if (i == -1) { > DPRINTF(1, "%s: %s: mmap queue is full!\n", > DEVNAME(sc), __func__); > return; > @@ -3552,6 +3575,8 @@ uvideo_reqbufs(void *v, struct v4l2_requ > sc->sc_mmap[i].v4l2_buf.m.offset, > sc->sc_mmap[i].v4l2_buf.length); > } > + > + sc->sc_mmap_buffer_idx = 0; > > /* tell how many buffers we have really allocated */ > rb->count = sc->sc_mmap_count; -- wbr, Kirill