[PATCH v5] media: gspca: don't set streaming before the transfers are set up

[YuanZhe Liu]

gspca_init_transfer() sets gspca_dev->streaming before it submits the
URBs, so an URB that completes while the remaining ones are still being
submitted is processed: gspca_frame_add() hands its buffer to videobuf2.

If one of the submissions then 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.

Set gspca_dev->streaming only once the transfers are set up.  Until then
isoc_irq() and bulk_irq() discard the completions, so no frame can be
completed before start_streaming() returns and the done list is empty
when it fails.  The subdrivers that start the transfers themselves
(bulk_nurbs == 0) get the flag right after their start callback, as
before.

Fixes: 1f5965c4dfd7 ("media: gspca: convert to vb2")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Bin Yu <byu@xxxxxxxxxxxxx>
Signed-off-by: Bin Yu <byu@xxxxxxxxxxxxx>
Co-developed-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
The flag is just set too early: isoc_irq() and bulk_irq() already discard
the completions while streaming is clear, so moving the assignment after
the URB submission is enough.  The subdrivers that start the transfers
themselves (bulk_nurbs == 0) get the flag right after their start
callback, as before.

Changes in v5:
- Don't park the frames that complete early; set gspca_dev->streaming once
  the transfers are set up instead.
- Drop the pending list, the extra flag and the retry state reset that v4
  added.

 drivers/media/usb/gspca/gspca.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 594d73e50b9f..69361ad4d8f9 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -840,11 +840,12 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 			goto out;
 		}
 		v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
-		gspca_dev->streaming = true;
 
 		/* some bulk transfers are started by the subdriver */
-		if (gspca_dev->cam.bulk && gspca_dev->cam.bulk_nurbs == 0)
+		if (gspca_dev->cam.bulk && gspca_dev->cam.bulk_nurbs == 0) {
+			gspca_dev->streaming = true;
 			break;
+		}
 
 		/* submit the URBs */
 		for (n = 0; n < MAX_NURBS; n++) {
@@ -855,8 +856,10 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 			if (ret < 0)
 				break;
 		}
-		if (ret >= 0)
+		if (ret >= 0) {
+			gspca_dev->streaming = true;
 			break;			/* transfer is started */
+		}
 
 		/* something when wrong
 		 * stop the webcam and free the transfer resources */
-- 
2.34.1