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 stays in the driver's queue, marked as complete, 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 marked buffers are still on the driver's queue and gspca_start_streaming() returns them in QUEUED state with the others. 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 v3: - Do not drop the frames that complete before the transfers are set up: keep them on the driver's queue and hand them to videobuf2 once gspca_init_transfer() succeeds. Dropping them would stall the subdrivers that restart their transfer from the frame completion callback (sq930x, se401) when the first frame completes inside that window. - Clear the streaming_ready flag when gspca_init_transfer() starts instead of when the stream is stopped: gspca_suspend() does not call gspca_stream_off(), and gspca_resume() re-enters gspca_init_transfer() with the flag still set. - Reset the state of the frame being received when a lower alternate setting is retried. drivers/media/usb/gspca/gspca.c | 59 ++++++++++++++++++++++++++++++--- drivers/media/usb/gspca/gspca.h | 2 ++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index 594d73e50b9f..ad8a0d686da8 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -389,10 +389,12 @@ void gspca_frame_add(struct gspca_dev *gspca_dev, spin_lock_irqsave(&gspca_dev->qlock, flags); buf = list_first_entry_or_null(&gspca_dev->buf_list, typeof(*buf), list); + if (buf && packet_type == FIRST_PACKET) + buf->frame_complete = false; 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 +441,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 +452,24 @@ 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. + */ + spin_lock_irqsave(&gspca_dev->qlock, flags); + ready = gspca_dev->streaming_ready; + if (ready) + list_del(&buf->list); + else + buf->frame_complete = true; + 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 +750,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 +764,28 @@ 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->buf_list, list) { + if (!buf->frame_complete) + continue; + 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 +801,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 +912,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 +933,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; } @@ -1327,6 +1375,7 @@ static void gspca_buffer_queue(struct vb2_buffer *vb) unsigned long flags; spin_lock_irqsave(&gspca_dev->qlock, flags); + buf->frame_complete = false; list_add_tail(&buf->list, &gspca_dev->buf_list); spin_unlock_irqrestore(&gspca_dev->qlock, flags); } diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h index a6554d5e9e1a..d0475db62da5 100644 --- a/drivers/media/usb/gspca/gspca.h +++ b/drivers/media/usb/gspca/gspca.h @@ -144,6 +144,7 @@ enum gspca_packet_type { struct gspca_buffer { struct vb2_v4l2_buffer vb; struct list_head list; + bool frame_complete; /* frame filled in, not given to videobuf2 yet */ }; static inline struct gspca_buffer *to_gspca_buffer(struct vb2_buffer *vb2) @@ -187,6 +188,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 */ -- 2.34.1