[PATCHv2 0/4] gspca: convert to vb2gspca: convert to vb2

[PATCHv2 0/4] gspca: convert to vb2gspca: convert to vb2

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The first patch converts the gspca driver to the vb2 framework.
It was much easier to do than I expected and it saved almost 600
lines of gspca driver code.

The second patch fixes v4l2-compliance warnings for g/s_parm.

The third patch clears relevant fields in v4l2_streamparm in
v4l_s_parm(). This was never done before since v4l2-compliance
didn't check this.

The final patch deletes the now unused v4l2_disable_ioctl_locking()
function.

Tested with three different gspca webcams, and tested suspend/resume
as well.

I'll test with a few more webcams next week and if those tests all
succeed then I'll post a pull request.

Regards,

	Hans

Changes since v1:

- Re-added 'if (gspca_dev->present)' before the dq_callback call.
- Added Reviewed-by tags from Hans de Goede.

Hans Verkuil (4):
  gspca: convert to vb2
  gspca: fix g/s_parm handling
  v4l2-ioctl: clear fields in s_parm
  v4l2-ioctl: delete unused v4l2_disable_ioctl_locking

 drivers/media/usb/gspca/Kconfig            |   1 +
 drivers/media/usb/gspca/gspca.c            | 925 ++++-----------------
 drivers/media/usb/gspca/gspca.h            |  38 +-
 drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
 drivers/media/usb/gspca/ov534.c            |   1 -
 drivers/media/usb/gspca/topro.c            |   1 -
 drivers/media/usb/gspca/vc032x.c           |   2 +-
 drivers/media/v4l2-core/v4l2-ioctl.c       |  19 +-
 include/media/v4l2-dev.h                   |  15 -
 9 files changed, 210 insertions(+), 796 deletions(-)

-- 
2.17.0

[PATCHv2 1/4] gspca: convert to vb2

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/usb/gspca/Kconfig            |   1 +
 drivers/media/usb/gspca/gspca.c            | 900 ++++-----------------
 drivers/media/usb/gspca/gspca.h            |  38 +-
 drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
 drivers/media/usb/gspca/vc032x.c           |   2 +-
 5 files changed, 180 insertions(+), 765 deletions(-)

diff --git a/drivers/media/usb/gspca/Kconfig b/drivers/media/usb/gspca/Kconfig
index bc9a439745aa..d3b6665c342d 100644
--- a/drivers/media/usb/gspca/Kconfig
+++ b/drivers/media/usb/gspca/Kconfig
@@ -2,6 +2,7 @@ menuconfig USB_GSPCA
 	tristate "GSPCA based webcams"
 	depends on VIDEO_V4L2
 	depends on INPUT || INPUT=n
+	select VIDEOBUF2_VMALLOC
 	default m
 	---help---
 	  Say Y here if you want to enable selecting webcams based
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index d29773b8f696..96f409676ba3 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -82,32 +82,6 @@ static void PDEBUG_MODE(struct gspca_dev *gspca_dev, int debug, char *txt,
 #define GSPCA_MEMORY_NO 0	/* V4L2_MEMORY_xxx starts from 1 */
 #define GSPCA_MEMORY_READ 7
 
-#define BUF_ALL_FLAGS (V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE)
-
-/*
- * VMA operations.
- */
-static void gspca_vm_open(struct vm_area_struct *vma)
-{
-	struct gspca_frame *frame = vma->vm_private_data;
-
-	frame->vma_use_count++;
-	frame->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED;
-}
-
-static void gspca_vm_close(struct vm_area_struct *vma)
-{
-	struct gspca_frame *frame = vma->vm_private_data;
-
-	if (--frame->vma_use_count <= 0)
-		frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_MAPPED;
-}
-
-static const struct vm_operations_struct gspca_vm_ops = {
-	.open		= gspca_vm_open,
-	.close		= gspca_vm_close,
-};
-
 /*
  * Input and interrupt endpoint handling functions
  */
@@ -356,7 +330,7 @@ static void isoc_irq(struct urb *urb)
 	struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
 
 	gspca_dbg(gspca_dev, D_PACK, "isoc irq\n");
-	if (!gspca_dev->streaming)
+	if (!vb2_start_streaming_called(&gspca_dev->queue))
 		return;
 	fill_frame(gspca_dev, urb);
 }
@@ -370,7 +344,7 @@ static void bulk_irq(struct urb *urb)
 	int st;
 
 	gspca_dbg(gspca_dev, D_PACK, "bulk irq\n");
-	if (!gspca_dev->streaming)
+	if (!vb2_start_streaming_called(&gspca_dev->queue))
 		return;
 	switch (urb->status) {
 	case 0:
@@ -417,25 +391,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 			const u8 *data,
 			int len)
 {
-	struct gspca_frame *frame;
-	int i, j;
+	struct gspca_buffer *buf;
 
 	gspca_dbg(gspca_dev, D_PACK, "add t:%d l:%d\n",	packet_type, len);
 
-	if (packet_type == FIRST_PACKET) {
-		i = atomic_read(&gspca_dev->fr_i);
+	spin_lock(&gspca_dev->qlock);
+	buf = list_first_entry_or_null(&gspca_dev->buf_list,
+				       typeof(*buf), list);
+	spin_unlock(&gspca_dev->qlock);
 
-		/* if there are no queued buffer, discard the whole frame */
-		if (i == atomic_read(&gspca_dev->fr_q)) {
+	if (packet_type == FIRST_PACKET) {
+		/* if there is no queued buffer, discard the whole frame */
+		if (!buf) {
 			gspca_dev->last_packet_type = DISCARD_PACKET;
 			gspca_dev->sequence++;
 			return;
 		}
-		j = gspca_dev->fr_queue[i];
-		frame = &gspca_dev->frame[j];
-		v4l2_get_timestamp(&frame->v4l2_buf.timestamp);
-		frame->v4l2_buf.sequence = gspca_dev->sequence++;
-		gspca_dev->image = frame->data;
+		gspca_dev->image = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
 		gspca_dev->image_len = 0;
 	} else {
 		switch (gspca_dev->last_packet_type) {
@@ -453,10 +425,10 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 
 	/* append the packet to the frame buffer */
 	if (len > 0) {
-		if (gspca_dev->image_len + len > gspca_dev->frsz) {
+		if (gspca_dev->image_len + len > gspca_dev->pixfmt.sizeimage) {
 			gspca_err(gspca_dev, "frame overflow %d > %d\n",
 				  gspca_dev->image_len + len,
-				  gspca_dev->frsz);
+				  gspca_dev->pixfmt.sizeimage);
 			packet_type = DISCARD_PACKET;
 		} else {
 /* !! image is NULL only when last pkt is LAST or DISCARD
@@ -476,80 +448,23 @@ 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) {
-		i = atomic_read(&gspca_dev->fr_i);
-		j = gspca_dev->fr_queue[i];
-		frame = &gspca_dev->frame[j];
-		frame->v4l2_buf.bytesused = gspca_dev->image_len;
-		frame->v4l2_buf.flags = (frame->v4l2_buf.flags
-					 | V4L2_BUF_FLAG_DONE)
-					& ~V4L2_BUF_FLAG_QUEUED;
-		i = (i + 1) % GSPCA_MAX_FRAMES;
-		atomic_set(&gspca_dev->fr_i, i);
-		wake_up_interruptible(&gspca_dev->wq);	/* event = new frame */
+		spin_lock(&gspca_dev->qlock);
+		list_del(&buf->list);
+		spin_unlock(&gspca_dev->qlock);
+		buf->vb.vb2_buf.timestamp = ktime_get_ns();
+		vb2_set_plane_payload(&buf->vb.vb2_buf, 0,
+				      gspca_dev->image_len);
+		buf->vb.sequence = gspca_dev->sequence++;
+		buf->vb.field = V4L2_FIELD_NONE;
 		gspca_dbg(gspca_dev, D_FRAM, "frame complete len:%d\n",
-			  frame->v4l2_buf.bytesused);
+			  gspca_dev->image_len);
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 		gspca_dev->image = NULL;
 		gspca_dev->image_len = 0;
 	}
 }
 EXPORT_SYMBOL(gspca_frame_add);
 
-static int frame_alloc(struct gspca_dev *gspca_dev, struct file *file,
-			enum v4l2_memory memory, unsigned int count)
-{
-	struct gspca_frame *frame;
-	unsigned int frsz;
-	int i;
-
-	frsz = gspca_dev->pixfmt.sizeimage;
-	gspca_dbg(gspca_dev, D_STREAM, "frame alloc frsz: %d\n", frsz);
-	frsz = PAGE_ALIGN(frsz);
-	if (count >= GSPCA_MAX_FRAMES)
-		count = GSPCA_MAX_FRAMES - 1;
-	gspca_dev->frbuf = vmalloc_32(frsz * count);
-	if (!gspca_dev->frbuf) {
-		pr_err("frame alloc failed\n");
-		return -ENOMEM;
-	}
-	gspca_dev->capt_file = file;
-	gspca_dev->memory = memory;
-	gspca_dev->frsz = frsz;
-	gspca_dev->nframes = count;
-	for (i = 0; i < count; i++) {
-		frame = &gspca_dev->frame[i];
-		frame->v4l2_buf.index = i;
-		frame->v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		frame->v4l2_buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
-		frame->v4l2_buf.field = V4L2_FIELD_NONE;
-		frame->v4l2_buf.length = frsz;
-		frame->v4l2_buf.memory = memory;
-		frame->v4l2_buf.sequence = 0;
-		frame->data = gspca_dev->frbuf + i * frsz;
-		frame->v4l2_buf.m.offset = i * frsz;
-	}
-	atomic_set(&gspca_dev->fr_q, 0);
-	atomic_set(&gspca_dev->fr_i, 0);
-	gspca_dev->fr_o = 0;
-	return 0;
-}
-
-static void frame_free(struct gspca_dev *gspca_dev)
-{
-	int i;
-
-	gspca_dbg(gspca_dev, D_STREAM, "frame free\n");
-	if (gspca_dev->frbuf != NULL) {
-		vfree(gspca_dev->frbuf);
-		gspca_dev->frbuf = NULL;
-		for (i = 0; i < gspca_dev->nframes; i++)
-			gspca_dev->frame[i].data = NULL;
-	}
-	gspca_dev->nframes = 0;
-	gspca_dev->frsz = 0;
-	gspca_dev->capt_file = NULL;
-	gspca_dev->memory = GSPCA_MEMORY_NO;
-}
-
 static void destroy_urbs(struct gspca_dev *gspca_dev)
 {
 	struct urb *urb;
@@ -583,22 +498,6 @@ static int gspca_set_alt0(struct gspca_dev *gspca_dev)
 	return ret;
 }
 
-/* Note: both the queue and the usb locks should be held when calling this */
-static void gspca_stream_off(struct gspca_dev *gspca_dev)
-{
-	gspca_dev->streaming = 0;
-	gspca_dev->usb_err = 0;
-	if (gspca_dev->sd_desc->stopN)
-		gspca_dev->sd_desc->stopN(gspca_dev);
-	destroy_urbs(gspca_dev);
-	gspca_input_destroy_urb(gspca_dev);
-	gspca_set_alt0(gspca_dev);
-	gspca_input_create_urb(gspca_dev);
-	if (gspca_dev->sd_desc->stop0)
-		gspca_dev->sd_desc->stop0(gspca_dev);
-	gspca_dbg(gspca_dev, D_STREAM, "stream off OK\n");
-}
-
 /*
  * look for an input transfer endpoint in an alternate setting.
  *
@@ -829,6 +728,22 @@ static int create_urbs(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
+/* Note: both the queue and the usb locks should be held when calling this */
+static void gspca_stream_off(struct gspca_dev *gspca_dev)
+{
+	gspca_dev->streaming = false;
+	gspca_dev->usb_err = 0;
+	if (gspca_dev->sd_desc->stopN)
+		gspca_dev->sd_desc->stopN(gspca_dev);
+	destroy_urbs(gspca_dev);
+	gspca_input_destroy_urb(gspca_dev);
+	gspca_set_alt0(gspca_dev);
+	gspca_input_create_urb(gspca_dev);
+	if (gspca_dev->sd_desc->stop0)
+		gspca_dev->sd_desc->stop0(gspca_dev);
+	gspca_dbg(gspca_dev, D_STREAM, "stream off OK\n");
+}
+
 /*
  * start the USB transfer
  */
@@ -844,7 +759,6 @@ 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->sequence = 0;
 
 	gspca_dev->usb_err = 0;
 
@@ -924,8 +838,8 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 			destroy_urbs(gspca_dev);
 			goto out;
 		}
-		gspca_dev->streaming = 1;
 		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)
@@ -1165,11 +1079,9 @@ static int vidioc_try_fmt_vid_cap(struct file *file,
 			      struct v4l2_format *fmt)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
-	ret = try_fmt_vid_cap(gspca_dev, fmt);
-	if (ret < 0)
-		return ret;
+	if (try_fmt_vid_cap(gspca_dev, fmt) < 0)
+		return -EINVAL;
 	return 0;
 }
 
@@ -1177,36 +1089,22 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
 			    struct v4l2_format *fmt)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
+	int mode;
 
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
+	if (vb2_is_busy(&gspca_dev->queue))
+		return -EBUSY;
 
-	ret = try_fmt_vid_cap(gspca_dev, fmt);
-	if (ret < 0)
-		goto out;
-
-	if (gspca_dev->nframes != 0
-	    && fmt->fmt.pix.sizeimage > gspca_dev->frsz) {
-		ret = -EINVAL;
-		goto out;
-	}
+	mode = try_fmt_vid_cap(gspca_dev, fmt);
+	if (mode < 0)
+		return -EINVAL;
 
-	if (gspca_dev->streaming) {
-		ret = -EBUSY;
-		goto out;
-	}
-	gspca_dev->curr_mode = ret;
+	gspca_dev->curr_mode = mode;
 	if (gspca_dev->sd_desc->try_fmt)
 		/* subdriver try_fmt can modify format parameters */
 		gspca_dev->pixfmt = fmt->fmt.pix;
 	else
-		gspca_dev->pixfmt = gspca_dev->cam.cam_mode[ret];
-
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
+		gspca_dev->pixfmt = gspca_dev->cam.cam_mode[mode];
+	return 0;
 }
 
 static int vidioc_enum_framesizes(struct file *file, void *priv,
@@ -1281,53 +1179,6 @@ static void gspca_release(struct v4l2_device *v4l2_device)
 	kfree(gspca_dev);
 }
 
-static int dev_open(struct file *file)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
-
-	gspca_dbg(gspca_dev, D_STREAM, "[%s] open\n", current->comm);
-
-	/* protect the subdriver against rmmod */
-	if (!try_module_get(gspca_dev->module))
-		return -ENODEV;
-
-	ret = v4l2_fh_open(file);
-	if (ret)
-		module_put(gspca_dev->module);
-	return ret;
-}
-
-static int dev_close(struct file *file)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-
-	gspca_dbg(gspca_dev, D_STREAM, "[%s] close\n", current->comm);
-
-	/* Needed for gspca_stream_off, always lock before queue_lock! */
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock)) {
-		mutex_unlock(&gspca_dev->usb_lock);
-		return -ERESTARTSYS;
-	}
-
-	/* if the file did the capture, free the streaming resources */
-	if (gspca_dev->capt_file == file) {
-		if (gspca_dev->streaming)
-			gspca_stream_off(gspca_dev);
-		frame_free(gspca_dev);
-	}
-	module_put(gspca_dev->module);
-	mutex_unlock(&gspca_dev->queue_lock);
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	gspca_dbg(gspca_dev, D_STREAM, "close done\n");
-
-	return v4l2_fh_release(file);
-}
-
 static int vidioc_querycap(struct file *file, void  *priv,
 			   struct v4l2_capability *cap)
 {
@@ -1377,167 +1228,9 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 {
 	if (i > 0)
 		return -EINVAL;
-	return (0);
-}
-
-static int vidioc_reqbufs(struct file *file, void *priv,
-			  struct v4l2_requestbuffers *rb)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int i, ret = 0, streaming;
-
-	i = rb->memory;			/* (avoid compilation warning) */
-	switch (i) {
-	case GSPCA_MEMORY_READ:			/* (internal call) */
-	case V4L2_MEMORY_MMAP:
-	case V4L2_MEMORY_USERPTR:
-		break;
-	default:
-		return -EINVAL;
-	}
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-
-	if (gspca_dev->memory != GSPCA_MEMORY_NO
-	    && gspca_dev->memory != GSPCA_MEMORY_READ
-	    && gspca_dev->memory != rb->memory) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/* only one file may do the capture */
-	if (gspca_dev->capt_file != NULL
-	    && gspca_dev->capt_file != file) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/* if allocated, the buffers must not be mapped */
-	for (i = 0; i < gspca_dev->nframes; i++) {
-		if (gspca_dev->frame[i].vma_use_count) {
-			ret = -EBUSY;
-			goto out;
-		}
-	}
-
-	/* stop streaming */
-	streaming = gspca_dev->streaming;
-	if (streaming) {
-		gspca_stream_off(gspca_dev);
-
-		/* Don't restart the stream when switching from read
-		 * to mmap mode */
-		if (gspca_dev->memory == GSPCA_MEMORY_READ)
-			streaming = 0;
-	}
-
-	/* free the previous allocated buffers, if any */
-	if (gspca_dev->nframes != 0)
-		frame_free(gspca_dev);
-	if (rb->count == 0)			/* unrequest */
-		goto out;
-	ret = frame_alloc(gspca_dev, file, rb->memory, rb->count);
-	if (ret == 0) {
-		rb->count = gspca_dev->nframes;
-		if (streaming)
-			ret = gspca_init_transfer(gspca_dev);
-	}
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	gspca_dbg(gspca_dev, D_STREAM, "reqbufs st:%d c:%d\n", ret, rb->count);
-	return ret;
-}
-
-static int vidioc_querybuf(struct file *file, void *priv,
-			   struct v4l2_buffer *v4l2_buf)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	struct gspca_frame *frame;
-
-	if (v4l2_buf->index >= gspca_dev->nframes)
-		return -EINVAL;
-
-	frame = &gspca_dev->frame[v4l2_buf->index];
-	memcpy(v4l2_buf, &frame->v4l2_buf, sizeof *v4l2_buf);
 	return 0;
 }
 
-static int vidioc_streamon(struct file *file, void *priv,
-			   enum v4l2_buf_type buf_type)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
-
-	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-
-	/* check the capture file */
-	if (gspca_dev->capt_file != file) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (gspca_dev->nframes == 0
-	    || !(gspca_dev->frame[0].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED)) {
-		ret = -EINVAL;
-		goto out;
-	}
-	if (!gspca_dev->streaming) {
-		ret = gspca_init_transfer(gspca_dev);
-		if (ret < 0)
-			goto out;
-	}
-	PDEBUG_MODE(gspca_dev, D_STREAM, "stream on OK",
-		    gspca_dev->pixfmt.pixelformat,
-		    gspca_dev->pixfmt.width, gspca_dev->pixfmt.height);
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
-}
-
-static int vidioc_streamoff(struct file *file, void *priv,
-				enum v4l2_buf_type buf_type)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int i, ret;
-
-	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-
-	if (!gspca_dev->streaming) {
-		ret = 0;
-		goto out;
-	}
-
-	/* check the capture file */
-	if (gspca_dev->capt_file != file) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/* stop streaming */
-	gspca_stream_off(gspca_dev);
-	/* In case another thread is waiting in dqbuf */
-	wake_up_interruptible(&gspca_dev->wq);
-
-	/* empty the transfer queues */
-	for (i = 0; i < gspca_dev->nframes; i++)
-		gspca_dev->frame[i].v4l2_buf.flags &= ~BUF_ALL_FLAGS;
-	atomic_set(&gspca_dev->fr_q, 0);
-	atomic_set(&gspca_dev->fr_i, 0);
-	gspca_dev->fr_o = 0;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
-}
-
 static int vidioc_g_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
@@ -1561,7 +1254,7 @@ static int vidioc_g_parm(struct file *filp, void *priv,
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
-	parm->parm.capture.readbuffers = gspca_dev->nbufread;
+	parm->parm.capture.readbuffers = 2;
 
 	if (gspca_dev->sd_desc->get_streamparm) {
 		gspca_dev->usb_err = 0;
@@ -1575,13 +1268,8 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 			struct v4l2_streamparm *parm)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
-	unsigned int n;
 
-	n = parm->parm.capture.readbuffers;
-	if (n == 0 || n >= GSPCA_MAX_FRAMES)
-		parm->parm.capture.readbuffers = gspca_dev->nbufread;
-	else
-		gspca_dev->nbufread = n;
+	parm->parm.capture.readbuffers = 2;
 
 	if (gspca_dev->sd_desc->set_streamparm) {
 		gspca_dev->usb_err = 0;
@@ -1592,418 +1280,138 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 	return 0;
 }
 
-static int dev_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	struct gspca_frame *frame;
-	struct page *page;
-	unsigned long addr, start, size;
-	int i, ret;
-
-	start = vma->vm_start;
-	size = vma->vm_end - vma->vm_start;
-	gspca_dbg(gspca_dev, D_STREAM, "mmap start:%08x size:%d\n",
-		  (int) start, (int)size);
-
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-	if (gspca_dev->capt_file != file) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	frame = NULL;
-	for (i = 0; i < gspca_dev->nframes; ++i) {
-		if (gspca_dev->frame[i].v4l2_buf.memory != V4L2_MEMORY_MMAP) {
-			gspca_dbg(gspca_dev, D_STREAM, "mmap bad memory type\n");
-			break;
-		}
-		if ((gspca_dev->frame[i].v4l2_buf.m.offset >> PAGE_SHIFT)
-						== vma->vm_pgoff) {
-			frame = &gspca_dev->frame[i];
-			break;
-		}
-	}
-	if (frame == NULL) {
-		gspca_dbg(gspca_dev, D_STREAM, "mmap no frame buffer found\n");
-		ret = -EINVAL;
-		goto out;
-	}
-	if (size != frame->v4l2_buf.length) {
-		gspca_dbg(gspca_dev, D_STREAM, "mmap bad size\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/*
-	 * - VM_IO marks the area as being a mmaped region for I/O to a
-	 *   device. It also prevents the region from being core dumped.
-	 */
-	vma->vm_flags |= VM_IO;
-
-	addr = (unsigned long) frame->data;
-	while (size > 0) {
-		page = vmalloc_to_page((void *) addr);
-		ret = vm_insert_page(vma, start, page);
-		if (ret < 0)
-			goto out;
-		start += PAGE_SIZE;
-		addr += PAGE_SIZE;
-		size -= PAGE_SIZE;
-	}
-
-	vma->vm_ops = &gspca_vm_ops;
-	vma->vm_private_data = frame;
-	gspca_vm_open(vma);
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
-}
-
-static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file,
-				enum v4l2_memory memory)
+static int gspca_queue_setup(struct vb2_queue *vq,
+			     unsigned int *nbuffers, unsigned int *nplanes,
+			     unsigned int sizes[], struct device *alloc_devs[])
 {
-	if (!gspca_dev->present)
-		return -ENODEV;
-	if (gspca_dev->capt_file != file || gspca_dev->memory != memory ||
-			!gspca_dev->streaming)
-		return -EINVAL;
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vq);
 
-	/* check if a frame is ready */
-	return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);
+	if (*nplanes)
+		return sizes[0] < gspca_dev->pixfmt.sizeimage ? -EINVAL : 0;
+	*nplanes = 1;
+	sizes[0] = gspca_dev->pixfmt.sizeimage;
+	return 0;
 }
 
-static int frame_ready(struct gspca_dev *gspca_dev, struct file *file,
-			enum v4l2_memory memory)
+static int gspca_buffer_prepare(struct vb2_buffer *vb)
 {
-	int ret;
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vb->vb2_queue);
+	unsigned long size = gspca_dev->pixfmt.sizeimage;
 
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-	ret = frame_ready_nolock(gspca_dev, file, memory);
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
+	if (vb2_plane_size(vb, 0) < size) {
+		gspca_err(gspca_dev, "buffer too small (%lu < %lu)\n",
+			 vb2_plane_size(vb, 0), size);
+		return -EINVAL;
+	}
+	return 0;
 }
 
-/*
- * dequeue a video buffer
- *
- * If nonblock_ing is false, block until a buffer is available.
- */
-static int vidioc_dqbuf(struct file *file, void *priv,
-			struct v4l2_buffer *v4l2_buf)
+static void gspca_buffer_finish(struct vb2_buffer *vb)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	struct gspca_frame *frame;
-	int i, j, ret;
-
-	gspca_dbg(gspca_dev, D_FRAM, "dqbuf\n");
-
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-
-	for (;;) {
-		ret = frame_ready_nolock(gspca_dev, file, v4l2_buf->memory);
-		if (ret < 0)
-			goto out;
-		if (ret > 0)
-			break;
-
-		mutex_unlock(&gspca_dev->queue_lock);
-
-		if (file->f_flags & O_NONBLOCK)
-			return -EAGAIN;
-
-		/* wait till a frame is ready */
-		ret = wait_event_interruptible_timeout(gspca_dev->wq,
-			frame_ready(gspca_dev, file, v4l2_buf->memory),
-			msecs_to_jiffies(3000));
-		if (ret < 0)
-			return ret;
-		if (ret == 0)
-			return -EIO;
-
-		if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-			return -ERESTARTSYS;
-	}
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vb->vb2_queue);
 
-	i = gspca_dev->fr_o;
-	j = gspca_dev->fr_queue[i];
-	frame = &gspca_dev->frame[j];
-
-	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
-
-	frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
-	memcpy(v4l2_buf, &frame->v4l2_buf, sizeof *v4l2_buf);
-	gspca_dbg(gspca_dev, D_FRAM, "dqbuf %d\n", j);
-	ret = 0;
-
-	if (gspca_dev->memory == V4L2_MEMORY_USERPTR) {
-		if (copy_to_user((__u8 __user *) frame->v4l2_buf.m.userptr,
-				 frame->data,
-				 frame->v4l2_buf.bytesused)) {
-			gspca_err(gspca_dev, "dqbuf cp to user failed\n");
-			ret = -EFAULT;
-		}
-	}
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-
-	if (ret == 0 && gspca_dev->sd_desc->dq_callback) {
-		mutex_lock(&gspca_dev->usb_lock);
-		gspca_dev->usb_err = 0;
-		if (gspca_dev->present)
-			gspca_dev->sd_desc->dq_callback(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
-	}
+	if (!gspca_dev->sd_desc->dq_callback)
+		return;
 
-	return ret;
+	gspca_dev->usb_err = 0;
+	if (gspca_dev->present)
+		gspca_dev->sd_desc->dq_callback(gspca_dev);
 }
 
-/*
- * queue a video buffer
- *
- * Attempting to queue a buffer that has already been
- * queued will return -EINVAL.
- */
-static int vidioc_qbuf(struct file *file, void *priv,
-			struct v4l2_buffer *v4l2_buf)
+static void gspca_buffer_queue(struct vb2_buffer *vb)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	struct gspca_frame *frame;
-	int i, index, ret;
-
-	gspca_dbg(gspca_dev, D_FRAM, "qbuf %d\n", v4l2_buf->index);
-
-	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
-		return -ERESTARTSYS;
-
-	index = v4l2_buf->index;
-	if ((unsigned) index >= gspca_dev->nframes) {
-		gspca_dbg(gspca_dev, D_FRAM,
-			  "qbuf idx %d >= %d\n", index, gspca_dev->nframes);
-		ret = -EINVAL;
-		goto out;
-	}
-	if (v4l2_buf->memory != gspca_dev->memory) {
-		gspca_dbg(gspca_dev, D_FRAM, "qbuf bad memory type\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	frame = &gspca_dev->frame[index];
-	if (frame->v4l2_buf.flags & BUF_ALL_FLAGS) {
-		gspca_dbg(gspca_dev, D_FRAM, "qbuf bad state\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	frame->v4l2_buf.flags |= V4L2_BUF_FLAG_QUEUED;
-
-	if (frame->v4l2_buf.memory == V4L2_MEMORY_USERPTR) {
-		frame->v4l2_buf.m.userptr = v4l2_buf->m.userptr;
-		frame->v4l2_buf.length = v4l2_buf->length;
-	}
-
-	/* put the buffer in the 'queued' queue */
-	i = atomic_read(&gspca_dev->fr_q);
-	gspca_dev->fr_queue[i] = index;
-	atomic_set(&gspca_dev->fr_q, (i + 1) % GSPCA_MAX_FRAMES);
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vb->vb2_queue);
+	struct gspca_buffer *buf = to_gspca_buffer(vb);
+	unsigned long flags;
 
-	v4l2_buf->flags |= V4L2_BUF_FLAG_QUEUED;
-	v4l2_buf->flags &= ~V4L2_BUF_FLAG_DONE;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->queue_lock);
-	return ret;
+	spin_lock_irqsave(&gspca_dev->qlock, flags);
+	list_add_tail(&buf->list, &gspca_dev->buf_list);
+	spin_unlock_irqrestore(&gspca_dev->qlock, flags);
 }
 
-/*
- * allocate the resources for read()
- */
-static int read_alloc(struct gspca_dev *gspca_dev,
-			struct file *file)
+static void gspca_return_all_buffers(struct gspca_dev *gspca_dev,
+				     enum vb2_buffer_state state)
 {
-	struct v4l2_buffer v4l2_buf;
-	int i, ret;
-
-	gspca_dbg(gspca_dev, D_STREAM, "read alloc\n");
+	struct gspca_buffer *buf, *node;
+	unsigned long flags;
 
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-
-	if (gspca_dev->nframes == 0) {
-		struct v4l2_requestbuffers rb;
-
-		memset(&rb, 0, sizeof rb);
-		rb.count = gspca_dev->nbufread;
-		rb.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		rb.memory = GSPCA_MEMORY_READ;
-		ret = vidioc_reqbufs(file, gspca_dev, &rb);
-		if (ret != 0) {
-			gspca_dbg(gspca_dev, D_STREAM, "read reqbuf err %d\n",
-				  ret);
-			goto out;
-		}
-		memset(&v4l2_buf, 0, sizeof v4l2_buf);
-		v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		v4l2_buf.memory = GSPCA_MEMORY_READ;
-		for (i = 0; i < gspca_dev->nbufread; i++) {
-			v4l2_buf.index = i;
-			ret = vidioc_qbuf(file, gspca_dev, &v4l2_buf);
-			if (ret != 0) {
-				gspca_dbg(gspca_dev, D_STREAM, "read qbuf err: %d\n",
-					  ret);
-				goto out;
-			}
-		}
+	spin_lock_irqsave(&gspca_dev->qlock, flags);
+	list_for_each_entry_safe(buf, node, &gspca_dev->buf_list, list) {
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
+		list_del(&buf->list);
 	}
-
-	/* start streaming */
-	ret = vidioc_streamon(file, gspca_dev, V4L2_BUF_TYPE_VIDEO_CAPTURE);
-	if (ret != 0)
-		gspca_dbg(gspca_dev, D_STREAM, "read streamon err %d\n", ret);
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	spin_unlock_irqrestore(&gspca_dev->qlock, flags);
 }
 
-static __poll_t dev_poll(struct file *file, poll_table *wait)
+static int gspca_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	__poll_t req_events = poll_requested_events(wait);
-	__poll_t ret = 0;
-
-	gspca_dbg(gspca_dev, D_FRAM, "poll\n");
-
-	if (req_events & EPOLLPRI)
-		ret |= v4l2_ctrl_poll(file, wait);
-
-	if (req_events & (EPOLLIN | EPOLLRDNORM)) {
-		/* if reqbufs is not done, the user would use read() */
-		if (gspca_dev->memory == GSPCA_MEMORY_NO) {
-			if (read_alloc(gspca_dev, file) != 0) {
-				ret |= EPOLLERR;
-				goto out;
-			}
-		}
-
-		poll_wait(file, &gspca_dev->wq, wait);
-
-		/* check if an image has been received */
-		if (mutex_lock_interruptible(&gspca_dev->queue_lock) != 0) {
-			ret |= EPOLLERR;
-			goto out;
-		}
-		if (gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i))
-			ret |= EPOLLIN | EPOLLRDNORM;
-		mutex_unlock(&gspca_dev->queue_lock);
-	}
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vq);
+	int ret;
 
-out:
-	if (!gspca_dev->present)
-		ret |= EPOLLHUP;
+	gspca_dev->sequence = 0;
 
+	ret = gspca_init_transfer(gspca_dev);
+	if (ret)
+		gspca_return_all_buffers(gspca_dev, VB2_BUF_STATE_QUEUED);
 	return ret;
 }
 
-static ssize_t dev_read(struct file *file, char __user *data,
-		    size_t count, loff_t *ppos)
+static void gspca_stop_streaming(struct vb2_queue *vq)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
-	struct gspca_frame *frame;
-	struct v4l2_buffer v4l2_buf;
-	struct timeval timestamp;
-	int n, ret, ret2;
-
-	gspca_dbg(gspca_dev, D_FRAM, "read (%zd)\n", count);
-	if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */
-		ret = read_alloc(gspca_dev, file);
-		if (ret != 0)
-			return ret;
-	}
+	struct gspca_dev *gspca_dev = vb2_get_drv_priv(vq);
 
-	/* get a frame */
-	v4l2_get_timestamp(&timestamp);
-	timestamp.tv_sec--;
-	n = 2;
-	for (;;) {
-		memset(&v4l2_buf, 0, sizeof v4l2_buf);
-		v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		v4l2_buf.memory = GSPCA_MEMORY_READ;
-		ret = vidioc_dqbuf(file, gspca_dev, &v4l2_buf);
-		if (ret != 0) {
-			gspca_dbg(gspca_dev, D_STREAM, "read dqbuf err %d\n",
-				  ret);
-			return ret;
-		}
-
-		/* if the process slept for more than 1 second,
-		 * get a newer frame */
-		frame = &gspca_dev->frame[v4l2_buf.index];
-		if (--n < 0)
-			break;			/* avoid infinite loop */
-		if (frame->v4l2_buf.timestamp.tv_sec >= timestamp.tv_sec)
-			break;
-		ret = vidioc_qbuf(file, gspca_dev, &v4l2_buf);
-		if (ret != 0) {
-			gspca_dbg(gspca_dev, D_STREAM, "read qbuf err %d\n",
-				  ret);
-			return ret;
-		}
-	}
+	gspca_stream_off(gspca_dev);
 
-	/* copy the frame */
-	if (count > frame->v4l2_buf.bytesused)
-		count = frame->v4l2_buf.bytesused;
-	ret = copy_to_user(data, frame->data, count);
-	if (ret != 0) {
-		gspca_err(gspca_dev, "read cp to user lack %d / %zd\n",
-			  ret, count);
-		ret = -EFAULT;
-		goto out;
-	}
-	ret = count;
-out:
-	/* in each case, requeue the buffer */
-	ret2 = vidioc_qbuf(file, gspca_dev, &v4l2_buf);
-	if (ret2 != 0)
-		return ret2;
-	return ret;
+	/* Release all active buffers */
+	gspca_return_all_buffers(gspca_dev, VB2_BUF_STATE_ERROR);
 }
 
+static const struct vb2_ops gspca_qops = {
+	.queue_setup		= gspca_queue_setup,
+	.buf_prepare		= gspca_buffer_prepare,
+	.buf_finish		= gspca_buffer_finish,
+	.buf_queue		= gspca_buffer_queue,
+	.start_streaming	= gspca_start_streaming,
+	.stop_streaming		= gspca_stop_streaming,
+	.wait_prepare		= vb2_ops_wait_prepare,
+	.wait_finish		= vb2_ops_wait_finish,
+};
+
 static const struct v4l2_file_operations dev_fops = {
 	.owner = THIS_MODULE,
-	.open = dev_open,
-	.release = dev_close,
-	.read = dev_read,
-	.mmap = dev_mmap,
+	.open = v4l2_fh_open,
+	.release = vb2_fop_release,
 	.unlocked_ioctl = video_ioctl2,
-	.poll	= dev_poll,
+	.read = vb2_fop_read,
+	.mmap = vb2_fop_mmap,
+	.poll = vb2_fop_poll,
 };
 
 static const struct v4l2_ioctl_ops dev_ioctl_ops = {
 	.vidioc_querycap	= vidioc_querycap,
-	.vidioc_dqbuf		= vidioc_dqbuf,
-	.vidioc_qbuf		= vidioc_qbuf,
 	.vidioc_enum_fmt_vid_cap = vidioc_enum_fmt_vid_cap,
 	.vidioc_try_fmt_vid_cap	= vidioc_try_fmt_vid_cap,
 	.vidioc_g_fmt_vid_cap	= vidioc_g_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap	= vidioc_s_fmt_vid_cap,
-	.vidioc_streamon	= vidioc_streamon,
 	.vidioc_enum_input	= vidioc_enum_input,
 	.vidioc_g_input		= vidioc_g_input,
 	.vidioc_s_input		= vidioc_s_input,
-	.vidioc_reqbufs		= vidioc_reqbufs,
-	.vidioc_querybuf	= vidioc_querybuf,
-	.vidioc_streamoff	= vidioc_streamoff,
 	.vidioc_g_jpegcomp	= vidioc_g_jpegcomp,
 	.vidioc_s_jpegcomp	= vidioc_s_jpegcomp,
 	.vidioc_g_parm		= vidioc_g_parm,
 	.vidioc_s_parm		= vidioc_s_parm,
 	.vidioc_enum_framesizes = vidioc_enum_framesizes,
 	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+
+	.vidioc_reqbufs		= vb2_ioctl_reqbufs,
+	.vidioc_create_bufs	= vb2_ioctl_create_bufs,
+	.vidioc_querybuf	= vb2_ioctl_querybuf,
+	.vidioc_qbuf		= vb2_ioctl_qbuf,
+	.vidioc_dqbuf		= vb2_ioctl_dqbuf,
+	.vidioc_expbuf		= vb2_ioctl_expbuf,
+	.vidioc_streamon	= vb2_ioctl_streamon,
+	.vidioc_streamoff	= vb2_ioctl_streamoff,
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.vidioc_g_chip_info	= vidioc_g_chip_info,
 	.vidioc_g_register	= vidioc_g_register,
@@ -2034,6 +1442,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 {
 	struct gspca_dev *gspca_dev;
 	struct usb_device *dev = interface_to_usbdev(intf);
+	struct vb2_queue *q;
 	int ret;
 
 	pr_info("%s-" GSPCA_VERSION " probing %04x:%04x\n",
@@ -2078,20 +1487,37 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	ret = v4l2_device_register(&intf->dev, &gspca_dev->v4l2_dev);
 	if (ret)
 		goto out;
+	gspca_dev->present = true;
 	gspca_dev->sd_desc = sd_desc;
-	gspca_dev->nbufread = 2;
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
 	gspca_dev->vdev = gspca_template;
 	gspca_dev->vdev.v4l2_dev = &gspca_dev->v4l2_dev;
 	video_set_drvdata(&gspca_dev->vdev, gspca_dev);
 	gspca_dev->module = module;
-	gspca_dev->present = 1;
 
 	mutex_init(&gspca_dev->usb_lock);
 	gspca_dev->vdev.lock = &gspca_dev->usb_lock;
-	mutex_init(&gspca_dev->queue_lock);
 	init_waitqueue_head(&gspca_dev->wq);
 
+	/* Initialize the vb2 queue */
+	q = &gspca_dev->queue;
+	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+	q->drv_priv = gspca_dev;
+	q->buf_struct_size = sizeof(struct gspca_buffer);
+	q->ops = &gspca_qops;
+	q->mem_ops = &vb2_vmalloc_memops;
+	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->min_buffers_needed = 2;
+	q->lock = &gspca_dev->usb_lock;
+	ret = vb2_queue_init(q);
+	if (ret)
+		goto out;
+	gspca_dev->vdev.queue = q;
+
+	INIT_LIST_HEAD(&gspca_dev->buf_list);
+	spin_lock_init(&gspca_dev->qlock);
+
 	/* configure the subdriver and initialize the USB device */
 	ret = sd_desc->config(gspca_dev, id);
 	if (ret < 0)
@@ -2109,14 +1535,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	if (ret)
 		goto out;
 
-	/*
-	 * Don't take usb_lock for these ioctls. This improves latency if
-	 * usb_lock is taken for a long time, e.g. when changing a control
-	 * value, and a new frame is ready to be dequeued.
-	 */
-	v4l2_disable_ioctl_locking(&gspca_dev->vdev, VIDIOC_DQBUF);
-	v4l2_disable_ioctl_locking(&gspca_dev->vdev, VIDIOC_QBUF);
-	v4l2_disable_ioctl_locking(&gspca_dev->vdev, VIDIOC_QUERYBUF);
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	if (!gspca_dev->sd_desc->get_register)
 		v4l2_disable_ioctl(&gspca_dev->vdev, VIDIOC_DBG_G_REGISTER);
@@ -2198,8 +1616,8 @@ void gspca_disconnect(struct usb_interface *intf)
 		  video_device_node_name(&gspca_dev->vdev));
 
 	mutex_lock(&gspca_dev->usb_lock);
+	gspca_dev->present = false;
 
-	gspca_dev->present = 0;
 	destroy_urbs(gspca_dev);
 
 #if IS_ENABLED(CONFIG_INPUT)
@@ -2211,11 +1629,8 @@ void gspca_disconnect(struct usb_interface *intf)
 	}
 #endif
 	/* Free subdriver's streaming resources / stop sd workqueue(s) */
-	if (gspca_dev->sd_desc->stop0 && gspca_dev->streaming)
-		gspca_dev->sd_desc->stop0(gspca_dev);
-	gspca_dev->streaming = 0;
+	vb2_queue_release(&gspca_dev->queue);
 	gspca_dev->dev = NULL;
-	wake_up_interruptible(&gspca_dev->wq);
 
 	v4l2_device_disconnect(&gspca_dev->v4l2_dev);
 	video_unregister_device(&gspca_dev->vdev);
@@ -2234,7 +1649,7 @@ int gspca_suspend(struct usb_interface *intf, pm_message_t message)
 
 	gspca_input_destroy_urb(gspca_dev);
 
-	if (!gspca_dev->streaming)
+	if (!vb2_start_streaming_called(&gspca_dev->queue))
 		return 0;
 
 	mutex_lock(&gspca_dev->usb_lock);
@@ -2266,8 +1681,7 @@ int gspca_resume(struct usb_interface *intf)
 	 * only write to the device registers on s_ctrl when streaming ->
 	 * Clear streaming to avoid setting all ctrls twice.
 	 */
-	streaming = gspca_dev->streaming;
-	gspca_dev->streaming = 0;
+	streaming = vb2_start_streaming_called(&gspca_dev->queue);
 	if (streaming)
 		ret = gspca_init_transfer(gspca_dev);
 	else
diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h
index 249cb38a542f..b0ced2e14006 100644
--- a/drivers/media/usb/gspca/gspca.h
+++ b/drivers/media/usb/gspca/gspca.h
@@ -9,6 +9,8 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/videobuf2-v4l2.h>
+#include <media/videobuf2-vmalloc.h>
 #include <linux/mutex.h>
 
 
@@ -138,19 +140,22 @@ enum gspca_packet_type {
 	LAST_PACKET
 };
 
-struct gspca_frame {
-	__u8 *data;			/* frame buffer */
-	int vma_use_count;
-	struct v4l2_buffer v4l2_buf;
+struct gspca_buffer {
+	struct vb2_v4l2_buffer vb;
+	struct list_head list;
 };
 
+static inline struct gspca_buffer *to_gspca_buffer(struct vb2_buffer *vb2)
+{
+	return container_of(vb2, struct gspca_buffer, vb.vb2_buf);
+}
+
 struct gspca_dev {
 	struct video_device vdev;	/* !! must be the first item */
 	struct module *module;		/* subdriver handling the device */
 	struct v4l2_device v4l2_dev;
 	struct usb_device *dev;
-	struct file *capt_file;		/* file doing video capture */
-					/* protected by queue_lock */
+
 #if IS_ENABLED(CONFIG_INPUT)
 	struct input_dev *input_dev;
 	char phys[64];			/* physical device path */
@@ -176,34 +181,29 @@ struct gspca_dev {
 	struct urb *int_urb;
 #endif
 
-	__u8 *frbuf;				/* buffer for nframes */
-	struct gspca_frame frame[GSPCA_MAX_FRAMES];
-	u8 *image;				/* image beeing filled */
-	__u32 frsz;				/* frame size */
+	u8 *image;				/* image being filled */
 	u32 image_len;				/* current length of image */
-	atomic_t fr_q;				/* next frame to queue */
-	atomic_t fr_i;				/* frame being filled */
-	signed char fr_queue[GSPCA_MAX_FRAMES];	/* frame queue */
-	char nframes;				/* number of frames */
-	u8 fr_o;				/* next frame to dequeue */
 	__u8 last_packet_type;
 	__s8 empty_packet;		/* if (-1) don't check empty packets */
-	__u8 streaming;			/* protected by both mutexes (*) */
+	bool streaming;
 
 	__u8 curr_mode;			/* current camera mode */
 	struct v4l2_pix_format pixfmt;	/* current mode parameters */
 	__u32 sequence;			/* frame sequence number */
 
+	struct vb2_queue queue;
+
+	spinlock_t qlock;
+	struct list_head buf_list;
+
 	wait_queue_head_t wq;		/* wait queue */
 	struct mutex usb_lock;		/* usb exchange protection */
-	struct mutex queue_lock;	/* ISOC queue protection */
 	int usb_err;			/* USB error - protected by usb_lock */
 	u16 pkt_size;			/* ISOC packet size */
 #ifdef CONFIG_PM
 	char frozen;			/* suspend - resume */
 #endif
-	char present;			/* device connected */
-	char nbufread;			/* number of buffers for read() */
+	bool present;
 	char memory;			/* memory type (V4L2_MEMORY_xxx) */
 	__u8 iface;			/* USB interface number */
 	__u8 alt;			/* USB alternate setting */
diff --git a/drivers/media/usb/gspca/m5602/m5602_core.c b/drivers/media/usb/gspca/m5602/m5602_core.c
index b83ec4285a0b..30b7cf1feedd 100644
--- a/drivers/media/usb/gspca/m5602/m5602_core.c
+++ b/drivers/media/usb/gspca/m5602/m5602_core.c
@@ -342,7 +342,7 @@ static void m5602_urb_complete(struct gspca_dev *gspca_dev,
 		data += 4;
 		len -= 4;
 
-		if (cur_frame_len + len <= gspca_dev->frsz) {
+		if (cur_frame_len + len <= gspca_dev->pixfmt.sizeimage) {
 			gspca_dbg(gspca_dev, D_FRAM, "Continuing frame %d copying %d bytes\n",
 				  sd->frame_count, len);
 
@@ -351,7 +351,7 @@ static void m5602_urb_complete(struct gspca_dev *gspca_dev,
 		} else {
 			/* Add the remaining data up to frame size */
 			gspca_frame_add(gspca_dev, INTER_PACKET, data,
-				    gspca_dev->frsz - cur_frame_len);
+				gspca_dev->pixfmt.sizeimage - cur_frame_len);
 		}
 	}
 }
diff --git a/drivers/media/usb/gspca/vc032x.c b/drivers/media/usb/gspca/vc032x.c
index 6b11597977c9..52d071659634 100644
--- a/drivers/media/usb/gspca/vc032x.c
+++ b/drivers/media/usb/gspca/vc032x.c
@@ -3642,7 +3642,7 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 		int size, l;
 
 		l = gspca_dev->image_len;
-		size = gspca_dev->frsz;
+		size = gspca_dev->pixfmt.sizeimage;
 		if (len > size - l)
 			len = size - l;
 	}
-- 
2.17.0

[PATCHv2 2/4] gspca: fix g/s_parm handling

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Fix v4l2-compliance error: s_parm never set V4L2_CAP_TIMEPERFRAME.
Also various g/s_parm-related cleanups.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/usb/gspca/gspca.c | 29 ++++++++++++++++-------------
 drivers/media/usb/gspca/ov534.c |  1 -
 drivers/media/usb/gspca/topro.c |  1 -
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 96f409676ba3..a383058b0cb3 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -1254,14 +1254,15 @@ static int vidioc_g_parm(struct file *filp, void *priv,
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
-	parm->parm.capture.readbuffers = 2;
+	parm->parm.capture.readbuffers = gspca_dev->queue.min_buffers_needed;
 
-	if (gspca_dev->sd_desc->get_streamparm) {
-		gspca_dev->usb_err = 0;
-		gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
-		return gspca_dev->usb_err;
-	}
-	return 0;
+	if (!gspca_dev->sd_desc->get_streamparm)
+		return 0;
+
+	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+	gspca_dev->usb_err = 0;
+	gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
+	return gspca_dev->usb_err;
 }
 
 static int vidioc_s_parm(struct file *filp, void *priv,
@@ -1269,15 +1270,17 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
-	parm->parm.capture.readbuffers = 2;
+	parm->parm.capture.readbuffers = gspca_dev->queue.min_buffers_needed;
 
-	if (gspca_dev->sd_desc->set_streamparm) {
-		gspca_dev->usb_err = 0;
-		gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
-		return gspca_dev->usb_err;
+	if (!gspca_dev->sd_desc->set_streamparm) {
+		parm->parm.capture.capability = 0;
+		return 0;
 	}
 
-	return 0;
+	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+	gspca_dev->usb_err = 0;
+	gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
+	return gspca_dev->usb_err;
 }
 
 static int gspca_queue_setup(struct vb2_queue *vq,
diff --git a/drivers/media/usb/gspca/ov534.c b/drivers/media/usb/gspca/ov534.c
index f293921a1f2b..d06dc0755b9a 100644
--- a/drivers/media/usb/gspca/ov534.c
+++ b/drivers/media/usb/gspca/ov534.c
@@ -1476,7 +1476,6 @@ static void sd_get_streamparm(struct gspca_dev *gspca_dev,
 	struct v4l2_fract *tpf = &cp->timeperframe;
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	cp->capability |= V4L2_CAP_TIMEPERFRAME;
 	tpf->numerator = 1;
 	tpf->denominator = sd->frame_rate;
 }
diff --git a/drivers/media/usb/gspca/topro.c b/drivers/media/usb/gspca/topro.c
index 82e2be14cad8..6f3ec0366a2f 100644
--- a/drivers/media/usb/gspca/topro.c
+++ b/drivers/media/usb/gspca/topro.c
@@ -4780,7 +4780,6 @@ static void sd_get_streamparm(struct gspca_dev *gspca_dev,
 	struct v4l2_fract *tpf = &cp->timeperframe;
 	int fr, i;
 
-	cp->capability |= V4L2_CAP_TIMEPERFRAME;
 	tpf->numerator = 1;
 	i = get_fr_idx(gspca_dev);
 	if (i & 0x80) {
-- 
2.17.0

[PATCHv2 3/4] v4l2-ioctl: clear fields in s_parm

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Zero the reserved capture/output array.

Zero the extendedmode (it is never used in drivers).

Clear all flags in capture/outputmode except for V4L2_MODE_HIGHQUALITY,
as that is the only valid flag.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a40dbec271f1..212aac1d22c1 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1952,7 +1952,22 @@ static int v4l_s_parm(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_streamparm *p = arg;
 	int ret = check_fmt(file, p->type);
 
-	return ret ? ret : ops->vidioc_s_parm(file, fh, p);
+	if (ret)
+		return ret;
+
+	/* Note: extendedmode is never used in drivers */
+	if (V4L2_TYPE_IS_OUTPUT(p->type)) {
+		memset(p->parm.output.reserved, 0,
+		       sizeof(p->parm.output.reserved));
+		p->parm.output.extendedmode = 0;
+		p->parm.output.outputmode &= V4L2_MODE_HIGHQUALITY;
+	} else {
+		memset(p->parm.capture.reserved, 0,
+		       sizeof(p->parm.capture.reserved));
+		p->parm.capture.extendedmode = 0;
+		p->parm.capture.capturemode &= V4L2_MODE_HIGHQUALITY;
+	}
+	return ops->vidioc_s_parm(file, fh, p);
 }
 
 static int v4l_queryctrl(const struct v4l2_ioctl_ops *ops,
-- 
2.17.0

[PATCHv2 4/4] v4l2-ioctl: delete unused v4l2_disable_ioctl_locking

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The last user of this 'feature' was the gspca driver. Now that
that driver has been converted to vb2 we can delete this code.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c |  2 --
 include/media/v4l2-dev.h             | 15 ---------------
 2 files changed, 17 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 212aac1d22c1..c23fbfe90a9e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2666,8 +2666,6 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
 {
 	if (_IOC_NR(cmd) >= V4L2_IOCTLS)
 		return vdev->lock;
-	if (test_bit(_IOC_NR(cmd), vdev->disable_locking))
-		return NULL;
 	if (vdev->queue && vdev->queue->lock &&
 			(v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE))
 		return vdev->queue->lock;
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 73073f6ee48c..30423aefe7c5 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -238,7 +238,6 @@ struct v4l2_file_operations {
  * @ioctl_ops: pointer to &struct v4l2_ioctl_ops with ioctl callbacks
  *
  * @valid_ioctls: bitmap with the valid ioctls for this device
- * @disable_locking: bitmap with the ioctls that don't require locking
  * @lock: pointer to &struct mutex serialization lock
  *
  * .. note::
@@ -291,7 +290,6 @@ struct video_device
 	const struct v4l2_ioctl_ops *ioctl_ops;
 	DECLARE_BITMAP(valid_ioctls, BASE_VIDIOC_PRIVATE);
 
-	DECLARE_BITMAP(disable_locking, BASE_VIDIOC_PRIVATE);
 	struct mutex *lock;
 };
 
@@ -446,19 +444,6 @@ void video_device_release_empty(struct video_device *vdev);
  */
 bool v4l2_is_known_ioctl(unsigned int cmd);
 
-/** v4l2_disable_ioctl_locking - mark that a given command
- *	shouldn't use core locking
- *
- * @vdev: pointer to &struct video_device
- * @cmd: ioctl command
- */
-static inline void v4l2_disable_ioctl_locking(struct video_device *vdev,
-					      unsigned int cmd)
-{
-	if (_IOC_NR(cmd) < BASE_VIDIOC_PRIVATE)
-		set_bit(_IOC_NR(cmd), vdev->disable_locking);
-}
-
 /**
  * v4l2_disable_ioctl- mark that a given command isn't implemented.
  *	shouldn't use core locking
-- 
2.17.0

Re: [PATCHv2 0/4] gspca: convert to vb2gspca: convert to vb2

[Ezequiel Garcia]

On 13 May 2018 at 06:47, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The first patch converts the gspca driver to the vb2 framework.
> It was much easier to do than I expected and it saved almost 600
> lines of gspca driver code.
>
> The second patch fixes v4l2-compliance warnings for g/s_parm.
>
> The third patch clears relevant fields in v4l2_streamparm in
> v4l_s_parm(). This was never done before since v4l2-compliance
> didn't check this.
>
> The final patch deletes the now unused v4l2_disable_ioctl_locking()
> function.
>
> Tested with three different gspca webcams, and tested suspend/resume
> as well.
>
> I'll test with a few more webcams next week and if those tests all
> succeed then I'll post a pull request.
>
> Regards,
>
>         Hans
>
> Changes since v1:
>
> - Re-added 'if (gspca_dev->present)' before the dq_callback call.
> - Added Reviewed-by tags from Hans de Goede.
>
> Hans Verkuil (4):
>   gspca: convert to vb2
>   gspca: fix g/s_parm handling
>   v4l2-ioctl: clear fields in s_parm
>   v4l2-ioctl: delete unused v4l2_disable_ioctl_locking
>
>  drivers/media/usb/gspca/Kconfig            |   1 +
>  drivers/media/usb/gspca/gspca.c            | 925 ++++-----------------
>  drivers/media/usb/gspca/gspca.h            |  38 +-
>  drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
>  drivers/media/usb/gspca/ov534.c            |   1 -
>  drivers/media/usb/gspca/topro.c            |   1 -
>  drivers/media/usb/gspca/vc032x.c           |   2 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c       |  19 +-
>  include/media/v4l2-dev.h                   |  15 -
>  9 files changed, 210 insertions(+), 796 deletions(-)
>

Got a NULL pointer testing this series. However, I don't think
the problem is with this series per-se, but more of a long-standing
race.

[ 1133.771530] BUG: unable to handle kernel NULL pointer dereference
at 00000000000000c4
[ 1133.779354] PGD 0 P4D 0
[ 1133.781885] Oops: 0000 [#1] PREEMPT SMP PTI
[ 1133.786065] CPU: 1 PID: 0 Comm: swapper/1 Not tainted
4.17.0-rc3-next-20180503-ARCH-00029-gb14b92b054cc-dirty #11
[ 1133.796306] Hardware name: ASUS All Series/H81M-D R2.0, BIOS 0504 05/14/2015
[ 1133.803346] RIP: 0010:sd_pkt_scan+0x246/0x350 [gspca_sonixj]
[ 1133.808994] Code: 00 89 d9 4c 89 e2 be 03 00 00 00 4c 89 ef e8 f1
06 c9 ff 49 8b 95 30 05 00 00 41 6b 85 d0 08 00 00 64 41 0f b7 8d d4
08 00 00 <0f> af 8a c4 00 00 00 31 d2 f7 f1 83 f8 54 0f 8f a6 00 00 00
83 f8
[ 1133.827845] RSP: 0018:ffff88011fa83c78 EFLAGS: 00010002
[ 1133.833061] RAX: 0000000000282d8c RBX: 00000000000002ee RCX: 0000000000000027
[ 1133.840204] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffff8800c10b97e0
[ 1133.847343] RBP: ffff88011fa83ca0 R08: 00000000000241a0 R09: ffffffffa021d45e
[ 1133.854469] R10: 000000000000032c R11: ffff880115938700 R12: ffff8800c765b840
[ 1133.861591] R13: ffff8800c10b9000 R14: 000000000000032c R15: 000000000000032c
[ 1133.868726] FS:  0000000000000000(0000) GS:ffff88011fa80000(0000)
knlGS:0000000000000000
[ 1133.876802] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1133.882540] CR2: 00000000000000c4 CR3: 000000000200a004 CR4: 00000000001606e0
[ 1133.889663] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1133.896788] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1133.903920] Call Trace:
[ 1133.906365]  <IRQ>
[ 1133.908376]  ? sd_stop0+0x40/0x40 [gspca_sonixj]
[ 1133.912988]  isoc_irq+0xb8/0x130 [gspca_main]
[ 1133.917340]  __usb_hcd_giveback_urb+0x64/0xe0 [usbcore]
[ 1133.922565]  usb_hcd_giveback_urb+0x11f/0x130 [usbcore]
[ 1133.927782]  xhci_giveback_urb_in_irq.isra.20+0x84/0x100 [xhci_hcd]
[ 1133.934038]  ? handle_cmd_completion+0x2cd/0x1100 [xhci_hcd]
[ 1133.939689]  xhci_td_cleanup+0xfb/0x170 [xhci_hcd]
[ 1133.944490]  finish_td+0xb3/0xf0 [xhci_hcd]
[ 1133.948669]  xhci_irq+0x1532/0x2130 [xhci_hcd]
[ 1133.953106]  ? handle_irq_event+0x47/0x5b
[ 1133.957110]  xhci_msi_irq+0x11/0x20 [xhci_hcd]
[ 1133.961546]  __handle_irq_event_percpu+0x42/0x1b0
[ 1133.966243]  handle_irq_event_percpu+0x32/0x80
[ 1133.970681]  handle_irq_event+0x3c/0x5b
[ 1133.974511]  handle_edge_irq+0x7f/0x1b0
[ 1133.978342]  handle_irq+0x1a/0x30
[ 1133.981654]  do_IRQ+0x46/0xd0
[ 1133.984617]  common_interrupt+0xf/0xf
[ 1133.988274]  </IRQ>

Common sense tells me that the gspca_dev->urb[0] is
set to NULL in destroy_urbs() and then accessed:

static void sd_pkt_scan(struct gspca_dev *gspca_dev,
                        u8 *data,                       /* isoc packet */
                        int len)                        /* iso packet length */
{
[..]
                r = (sd->pktsz * 100) /
                        (sd->npkt *
                                gspca_dev->urb[0]->iso_frame_desc[0].length);

As nothing protects the gspca_dev->urb array.

This is confirmed by disassembly, if I did the math right.
sd_pkt_scan+0x246 is 0xC56:

     c3a:       e8 00 00 00 00          callq  c3f <sd_pkt_scan+0x22f>
                                gspca_dev->urb[0]->iso_frame_desc[0].length);
     c3f:       49 8b 95 30 05 00 00    mov    0x530(%r13),%rdx
                r = (sd->pktsz * 100) /
     c46:       41 6b 85 d0 08 00 00    imul   $0x64,0x8d0(%r13),%eax
     c4d:       64
                        (sd->npkt *
     c4e:       41 0f b7 8d d4 08 00    movzwl 0x8d4(%r13),%ecx
     c55:       00
     c56:       0f af 8a c4 00 00 00    imul   0xc4(%rdx),%ecx
                r = (sd->pktsz * 100) /

Where %rdx seems to be gspca_dev->urb[0].

I think we should fix these gspca-state-urbs in all the gspca sub-drivers:

$ git grep "urb\[.*->" -- drivers/media/usb/gspca/
drivers/media/usb/gspca/benq.c: if (urb == gspca_dev->urb[0] || urb ==
gspca_dev->urb[2])
drivers/media/usb/gspca/finepix.c:                      gspca_dev->urb[0]->pipe,
drivers/media/usb/gspca/finepix.c:
gspca_dev->urb[0]->transfer_buffer,
drivers/media/usb/gspca/finepix.c:      usb_clear_halt(gspca_dev->dev,
gspca_dev->urb[0]->pipe);
drivers/media/usb/gspca/gspca.c:
 gspca_dev->urb[0]->pipe);
drivers/media/usb/gspca/jeilinj.c:
gspca_dev->urb[0]->pipe,
drivers/media/usb/gspca/jeilinj.c:
gspca_dev->urb[0]->transfer_buffer,
drivers/media/usb/gspca/jeilinj.c:              buf =
gspca_dev->urb[0]->transfer_buffer;
drivers/media/usb/gspca/sn9c20x.c:
gspca_dev->urb[0]->iso_frame_desc[0].length);
drivers/media/usb/gspca/sonixj.c:
gspca_dev->urb[0]->iso_frame_desc[0].length);
drivers/media/usb/gspca/xirlink_cit.c:
usb_clear_halt(gspca_dev->dev, gspca_dev->urb[0]->pipe);
drivers/media/usb/gspca/xirlink_cit.c:
usb_clear_halt(gspca_dev->dev, gspca_dev->urb[0]->pipe);

Thoughts?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCHv2 0/4] gspca: convert to vb2gspca: convert to vb2

[Hans Verkuil]

On 18/05/18 19:51, Ezequiel Garcia wrote:
> On 13 May 2018 at 06:47, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The first patch converts the gspca driver to the vb2 framework.
>> It was much easier to do than I expected and it saved almost 600
>> lines of gspca driver code.
>>
>> The second patch fixes v4l2-compliance warnings for g/s_parm.
>>
>> The third patch clears relevant fields in v4l2_streamparm in
>> v4l_s_parm(). This was never done before since v4l2-compliance
>> didn't check this.
>>
>> The final patch deletes the now unused v4l2_disable_ioctl_locking()
>> function.
>>
>> Tested with three different gspca webcams, and tested suspend/resume
>> as well.
>>
>> I'll test with a few more webcams next week and if those tests all
>> succeed then I'll post a pull request.
>>
>> Regards,
>>
>>         Hans
>>
>> Changes since v1:
>>
>> - Re-added 'if (gspca_dev->present)' before the dq_callback call.
>> - Added Reviewed-by tags from Hans de Goede.
>>
>> Hans Verkuil (4):
>>   gspca: convert to vb2
>>   gspca: fix g/s_parm handling
>>   v4l2-ioctl: clear fields in s_parm
>>   v4l2-ioctl: delete unused v4l2_disable_ioctl_locking
>>
>>  drivers/media/usb/gspca/Kconfig            |   1 +
>>  drivers/media/usb/gspca/gspca.c            | 925 ++++-----------------
>>  drivers/media/usb/gspca/gspca.h            |  38 +-
>>  drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
>>  drivers/media/usb/gspca/ov534.c            |   1 -
>>  drivers/media/usb/gspca/topro.c            |   1 -
>>  drivers/media/usb/gspca/vc032x.c           |   2 +-
>>  drivers/media/v4l2-core/v4l2-ioctl.c       |  19 +-
>>  include/media/v4l2-dev.h                   |  15 -
>>  9 files changed, 210 insertions(+), 796 deletions(-)
>>
> 
> Got a NULL pointer testing this series. However, I don't think
> the problem is with this series per-se, but more of a long-standing
> race.
> 
> [ 1133.771530] BUG: unable to handle kernel NULL pointer dereference
> at 00000000000000c4
> [ 1133.779354] PGD 0 P4D 0
> [ 1133.781885] Oops: 0000 [#1] PREEMPT SMP PTI
> [ 1133.786065] CPU: 1 PID: 0 Comm: swapper/1 Not tainted
> 4.17.0-rc3-next-20180503-ARCH-00029-gb14b92b054cc-dirty #11
> [ 1133.796306] Hardware name: ASUS All Series/H81M-D R2.0, BIOS 0504 05/14/2015
> [ 1133.803346] RIP: 0010:sd_pkt_scan+0x246/0x350 [gspca_sonixj]
> [ 1133.808994] Code: 00 89 d9 4c 89 e2 be 03 00 00 00 4c 89 ef e8 f1
> 06 c9 ff 49 8b 95 30 05 00 00 41 6b 85 d0 08 00 00 64 41 0f b7 8d d4
> 08 00 00 <0f> af 8a c4 00 00 00 31 d2 f7 f1 83 f8 54 0f 8f a6 00 00 00
> 83 f8
> [ 1133.827845] RSP: 0018:ffff88011fa83c78 EFLAGS: 00010002
> [ 1133.833061] RAX: 0000000000282d8c RBX: 00000000000002ee RCX: 0000000000000027
> [ 1133.840204] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffff8800c10b97e0
> [ 1133.847343] RBP: ffff88011fa83ca0 R08: 00000000000241a0 R09: ffffffffa021d45e
> [ 1133.854469] R10: 000000000000032c R11: ffff880115938700 R12: ffff8800c765b840
> [ 1133.861591] R13: ffff8800c10b9000 R14: 000000000000032c R15: 000000000000032c
> [ 1133.868726] FS:  0000000000000000(0000) GS:ffff88011fa80000(0000)
> knlGS:0000000000000000
> [ 1133.876802] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1133.882540] CR2: 00000000000000c4 CR3: 000000000200a004 CR4: 00000000001606e0
> [ 1133.889663] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1133.896788] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 1133.903920] Call Trace:
> [ 1133.906365]  <IRQ>
> [ 1133.908376]  ? sd_stop0+0x40/0x40 [gspca_sonixj]
> [ 1133.912988]  isoc_irq+0xb8/0x130 [gspca_main]
> [ 1133.917340]  __usb_hcd_giveback_urb+0x64/0xe0 [usbcore]
> [ 1133.922565]  usb_hcd_giveback_urb+0x11f/0x130 [usbcore]
> [ 1133.927782]  xhci_giveback_urb_in_irq.isra.20+0x84/0x100 [xhci_hcd]
> [ 1133.934038]  ? handle_cmd_completion+0x2cd/0x1100 [xhci_hcd]
> [ 1133.939689]  xhci_td_cleanup+0xfb/0x170 [xhci_hcd]
> [ 1133.944490]  finish_td+0xb3/0xf0 [xhci_hcd]
> [ 1133.948669]  xhci_irq+0x1532/0x2130 [xhci_hcd]
> [ 1133.953106]  ? handle_irq_event+0x47/0x5b
> [ 1133.957110]  xhci_msi_irq+0x11/0x20 [xhci_hcd]
> [ 1133.961546]  __handle_irq_event_percpu+0x42/0x1b0
> [ 1133.966243]  handle_irq_event_percpu+0x32/0x80
> [ 1133.970681]  handle_irq_event+0x3c/0x5b
> [ 1133.974511]  handle_edge_irq+0x7f/0x1b0
> [ 1133.978342]  handle_irq+0x1a/0x30
> [ 1133.981654]  do_IRQ+0x46/0xd0
> [ 1133.984617]  common_interrupt+0xf/0xf
> [ 1133.988274]  </IRQ>
> 
> Common sense tells me that the gspca_dev->urb[0] is
> set to NULL in destroy_urbs() and then accessed:
> 
> static void sd_pkt_scan(struct gspca_dev *gspca_dev,
>                         u8 *data,                       /* isoc packet */
>                         int len)                        /* iso packet length */
> {
> [..]
>                 r = (sd->pktsz * 100) /
>                         (sd->npkt *
>                                 gspca_dev->urb[0]->iso_frame_desc[0].length);
> 
> As nothing protects the gspca_dev->urb array.
> 
> This is confirmed by disassembly, if I did the math right.
> sd_pkt_scan+0x246 is 0xC56:
> 
>      c3a:       e8 00 00 00 00          callq  c3f <sd_pkt_scan+0x22f>
>                                 gspca_dev->urb[0]->iso_frame_desc[0].length);
>      c3f:       49 8b 95 30 05 00 00    mov    0x530(%r13),%rdx
>                 r = (sd->pktsz * 100) /
>      c46:       41 6b 85 d0 08 00 00    imul   $0x64,0x8d0(%r13),%eax
>      c4d:       64
>                         (sd->npkt *
>      c4e:       41 0f b7 8d d4 08 00    movzwl 0x8d4(%r13),%ecx
>      c55:       00
>      c56:       0f af 8a c4 00 00 00    imul   0xc4(%rdx),%ecx
>                 r = (sd->pktsz * 100) /
> 
> Where %rdx seems to be gspca_dev->urb[0].
> 
> I think we should fix these gspca-state-urbs in all the gspca sub-drivers:
> 
> $ git grep "urb\[.*->" -- drivers/media/usb/gspca/
> drivers/media/usb/gspca/benq.c: if (urb == gspca_dev->urb[0] || urb ==
> gspca_dev->urb[2])
> drivers/media/usb/gspca/finepix.c:                      gspca_dev->urb[0]->pipe,
> drivers/media/usb/gspca/finepix.c:
> gspca_dev->urb[0]->transfer_buffer,
> drivers/media/usb/gspca/finepix.c:      usb_clear_halt(gspca_dev->dev,
> gspca_dev->urb[0]->pipe);
> drivers/media/usb/gspca/gspca.c:
>  gspca_dev->urb[0]->pipe);
> drivers/media/usb/gspca/jeilinj.c:
> gspca_dev->urb[0]->pipe,
> drivers/media/usb/gspca/jeilinj.c:
> gspca_dev->urb[0]->transfer_buffer,
> drivers/media/usb/gspca/jeilinj.c:              buf =
> gspca_dev->urb[0]->transfer_buffer;
> drivers/media/usb/gspca/sn9c20x.c:
> gspca_dev->urb[0]->iso_frame_desc[0].length);
> drivers/media/usb/gspca/sonixj.c:
> gspca_dev->urb[0]->iso_frame_desc[0].length);
> drivers/media/usb/gspca/xirlink_cit.c:
> usb_clear_halt(gspca_dev->dev, gspca_dev->urb[0]->pipe);
> drivers/media/usb/gspca/xirlink_cit.c:
> usb_clear_halt(gspca_dev->dev, gspca_dev->urb[0]->pipe);
> 
> Thoughts?
> 

Should be fixed in v3. The main problem was that the URBs were destroyed
too soon. By moving that to gspca_release this should no longer happen.

Regards,

	Hans