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: 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 Signed-off-by: Bin Yu Co-developed-by: Cheng Wen Signed-off-by: Cheng Wen 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; } @@ -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 */ -- 2.34.1