Re: [PATCH v4] media: gspca: don't complete buffers before streaming has started

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 03/11/2026 17:15, YuanZhe Liu wrote:
> gspca enables the data transfers before it knows whether start_streaming()
> will succeed: gspca_init_transfer() sets gspca_dev->streaming before the
> URBs are submitted, so an URB that has already been submitted can complete
> and gspca_frame_add() can hand its buffer to videobuf2 while the remaining
> URBs are still being submitted.
> 
> When one of those submissions fails, gspca_start_streaming() returns the
> buffers that are still queued to vb2 in QUEUED state, but the buffer that
> has already been completed is no longer on the driver's queue, and
> vb2_buffer_done() cannot take it back (it only accepts active buffers).
> vb2_start_streaming() then warns because the driver left a buffer on the
> done list although it failed to start:
> 
>  ------------[ cut here ]------------
>  WARNING: drivers/media/common/videobuf2/videobuf2-core.c:1821 at vb2_start_streaming+0x312/0x680
>  ...
>  Call Trace:
>   <TASK>
>   vb2_core_streamon+0x285/0x470
>   __vb2_init_fileio+0xdc1/0x1120
>   __vb2_perform_fileio+0xa7a/0x15e0
>   vb2_fop_read+0x219/0x3e0
>   v4l2_read+0x1fc/0x320
>   vfs_read+0x1d9/0x980
>   ksys_read+0x111/0x210
>  ...
>  gspca_main: set alt 0 err -19
>  gspca_main: usb_submit_urb alt 1 err -2
> 
> This was observed on a camera whose USB configuration was changed from
> userspace while a read() was starting the stream: the endpoint is removed,
> so usb_submit_urb() fails with -ENOENT, and the driver is unbound.
> 
> Do not hand a buffer to videobuf2 before the transfers are set up.  A frame
> that completes earlier is moved from the queue of free buffers to a pending
> list, and is given to videobuf2 only once gspca_init_transfer() reports
> success.  A failing start_streaming() therefore always leaves the done list
> empty, while the frame is not lost either: the subdrivers that restart
> their transfer from the frame completion callback (sq930x, se401) keep
> working when the very first frame completes inside that window.  When the
> start fails, the pending frames are put back on the driver's queue and
> gspca_start_streaming() returns them in QUEUED state with the others.
> 
> The frame has to be taken out of the queue of free buffers: it is that
> queue's first entry, so the next frame would take the same buffer and
> overwrite the completed one, which would then never reach videobuf2.
> 
> The state of the frame being received is now also reset when a lower
> alternate setting is retried, so that the packets of the retried transfer
> cannot be appended to a frame left over from the previous one.
> 
> A driver side flag is needed because videobuf2 sets q->streaming before
> calling start_streaming() when VIDIOC_STREAMON comes in with fewer buffers
> queued than min_queued_buffers, and start_streaming() is then called later
> from vb2_core_qbuf(), with the first URBs possibly already completed.  The
> flag is cleared when gspca_init_transfer() starts rather than when the
> stream is stopped: gspca_suspend() does not go through gspca_stream_off(),
> and gspca_resume() calls gspca_init_transfer() directly.
> 
> Fixes: 1f5965c4dfd7 ("media: gspca: convert to vb2")
> Cc: stable@vger.kernel.org
> Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
> Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
> ---
> Changes in v4:
> - Move the frames that complete before the transfers are set up to a separate
>   pending list.  Leaving them in the queue of free buffers, as v3 did, did
>   not protect them: the completed frame was still that queue's first entry,
>   so the next FIRST_PACKET took the same buffer and overwrote the frame,
>   which was then never handed to videobuf2.  The subdrivers that send
>   FIRST_PACKET right after LAST_PACKET (etoms, ov519, konica, nw80x) hit
>   that on every start.
> - Put the pending frames back on the queue in gspca_return_all_buffers(), so
>   a failed start still returns every buffer to videobuf2.
>  drivers/media/usb/gspca/gspca.c | 58 ++++++++++++++++++++++++++++++---
>  drivers/media/usb/gspca/gspca.h |  2 ++
>  2 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> index 594d73e50b9f..1232f5d10bae 100644
> --- a/drivers/media/usb/gspca/gspca.c
> +++ b/drivers/media/usb/gspca/gspca.c
> @@ -392,7 +392,7 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
>  	spin_unlock_irqrestore(&gspca_dev->qlock, flags);
>  
>  	if (packet_type == FIRST_PACKET) {
> -		/* if there is no queued buffer, discard the whole frame */
> +		/* If there is no queued buffer, discard the whole frame */
>  		if (!buf) {
>  			gspca_dev->last_packet_type = DISCARD_PACKET;
>  			gspca_dev->sequence++;
> @@ -439,11 +439,10 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
>  	 * next first packet, wake up the application and advance
>  	 * in the queue */
>  	if (packet_type == LAST_PACKET) {
> +		bool ready;
> +
>  		if (gspca_dev->image_len > gspca_dev->pixfmt.sizeimage)
>  			gspca_dev->image_len = gspca_dev->pixfmt.sizeimage;
> -		spin_lock_irqsave(&gspca_dev->qlock, flags);
> -		list_del(&buf->list);
> -		spin_unlock_irqrestore(&gspca_dev->qlock, flags);
>  		buf->vb.vb2_buf.timestamp = ktime_get_ns();
>  		vb2_set_plane_payload(&buf->vb.vb2_buf, 0,
>  				      gspca_dev->image_len);
> @@ -451,7 +450,25 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
>  		buf->vb.field = V4L2_FIELD_NONE;
>  		gspca_dbg(gspca_dev, D_FRAM, "frame complete len:%d\n",
>  			  gspca_dev->image_len);
> -		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +		/*
> +		 * The frame can complete before gspca_init_transfer() has set
> +		 * up all the transfers: the first URB can complete while the
> +		 * others are still being submitted, and a submission can then
> +		 * fail.  Videobuf2 requires that start_streaming() leaves its
> +		 * done list empty in that case, and a completed buffer cannot
> +		 * be taken back, so the frame is only released to videobuf2
> +		 * once the transfers are known to be set up.  Until then it is
> +		 * parked on the pending list, out of reach of the next frame.
> +		 */
> +		spin_lock_irqsave(&gspca_dev->qlock, flags);
> +		ready = gspca_dev->streaming_ready;
> +		if (ready)
> +			list_del(&buf->list);
> +		else
> +			list_move_tail(&buf->list, &gspca_dev->pending);
> +		spin_unlock_irqrestore(&gspca_dev->qlock, flags);
> +		if (ready)
> +			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  		gspca_dev->image = NULL;
>  		gspca_dev->image_len = 0;
>  	}
> @@ -732,6 +749,7 @@ static int create_urbs(struct gspca_dev *gspca_dev,
>  static void gspca_stream_off(struct gspca_dev *gspca_dev)
>  {
>  	gspca_dev->streaming = false;
> +	gspca_dev->streaming_ready = false;
>  	gspca_dev->usb_err = 0;
>  	if (gspca_dev->sd_desc->stopN)
>  		gspca_dev->sd_desc->stopN(gspca_dev);
> @@ -745,6 +763,26 @@ static void gspca_stream_off(struct gspca_dev *gspca_dev)
>  	gspca_dbg(gspca_dev, D_STREAM, "stream off OK\n");
>  }
>  
> +/*
> + * The transfers are set up: from now on the frames are handed to videobuf2
> + * as soon as they are complete.  Release the ones that completed while the
> + * URBs were being submitted, they could not be completed earlier because
> + * start_streaming() could still have failed.
> + */
> +static void gspca_flush_pending_frames(struct gspca_dev *gspca_dev)
> +{
> +	struct gspca_buffer *buf, *node;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&gspca_dev->qlock, flags);
> +	gspca_dev->streaming_ready = true;
> +	list_for_each_entry_safe(buf, node, &gspca_dev->pending, list) {
> +		list_del(&buf->list);
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	}
> +	spin_unlock_irqrestore(&gspca_dev->qlock, flags);
> +}
> +
>  /*
>   * start the USB transfer
>   */
> @@ -760,6 +798,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
>  	gspca_dev->image = NULL;
>  	gspca_dev->image_len = 0;
>  	gspca_dev->last_packet_type = DISCARD_PACKET;
> +	gspca_dev->streaming_ready = false;
>  
>  	gspca_dev->usb_err = 0;
>  
> @@ -870,6 +909,10 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
>  		/* the bandwidth is not wide enough
>  		 * negotiate or try a lower alternate setting */
>  retry:
> +		/* the frame of the previous transfer is lost */
> +		gspca_dev->image = NULL;
> +		gspca_dev->image_len = 0;
> +		gspca_dev->last_packet_type = DISCARD_PACKET;
>  		gspca_err(gspca_dev, "alt %d - bandwidth not wide enough, trying again\n",
>  			  alt);
>  		msleep(20);	/* wait for kill complete */
> @@ -887,6 +930,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
>  		}
>  	}
>  out:
> +	if (ret >= 0)
> +		gspca_flush_pending_frames(gspca_dev);
>  	gspca_input_create_urb(gspca_dev);
>  	return ret;
>  }

This patch feels far too complicated. Isn't the problem simply that gspca_init_transfer
sets gspca_dev->streaming too early?

I don't really want to make such a large change for this very old and rarely used
driver.

Regards,

	Hans

> @@ -1338,6 +1383,8 @@ static void gspca_return_all_buffers(struct gspca_dev *gspca_dev,
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&gspca_dev->qlock, flags);
> +	/* the frames that never reached videobuf2 are still queued */
> +	list_splice_init(&gspca_dev->pending, &gspca_dev->buf_list);
>  	list_for_each_entry_safe(buf, node, &gspca_dev->buf_list, list) {
>  		vb2_buffer_done(&buf->vb.vb2_buf, state);
>  		list_del(&buf->list);
> @@ -1518,6 +1565,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
>  	gspca_dev->vdev.queue = q;
>  
>  	INIT_LIST_HEAD(&gspca_dev->buf_list);
> +	INIT_LIST_HEAD(&gspca_dev->pending);
>  	spin_lock_init(&gspca_dev->qlock);
>  
>  	/* configure the subdriver and initialize the USB device */
> diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h
> index a6554d5e9e1a..93f304787cd4 100644
> --- a/drivers/media/usb/gspca/gspca.h
> +++ b/drivers/media/usb/gspca/gspca.h
> @@ -187,6 +187,7 @@ struct gspca_dev {
>  	__u8 last_packet_type;
>  	__s8 empty_packet;		/* if (-1) don't check empty packets */
>  	bool streaming;
> +	bool streaming_ready;		/* buffers can be completed */
>  
>  	__u8 curr_mode;			/* current camera mode */
>  	struct v4l2_pix_format pixfmt;	/* current mode parameters */
> @@ -196,6 +197,7 @@ struct gspca_dev {
>  
>  	spinlock_t qlock;
>  	struct list_head buf_list;
> +	struct list_head pending;	/* completed, not given to videobuf2 yet */
>  
>  	wait_queue_head_t wq;		/* wait queue */
>  	struct mutex usb_lock;		/* usb exchange protection */