On Tue, 18 Mar 2025 14:11:37 +0100,
Marcus Glocker <marcus@nazgul.ch> 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