[PATCH 1/9] s5p: Don't check vb2_queue_init() return value

[PATCH 1/9] s5p: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/s5p-fimc/fimc-m2m.c  |    7 +++----
 drivers/media/platform/s5p-g2d/g2d.c        |    7 +++----
 drivers/media/platform/s5p-jpeg/jpeg-core.c |    7 +++----
 drivers/media/platform/s5p-mfc/s5p_mfc.c    |   14 ++++----------
 drivers/media/platform/s5p-tv/mixer_video.c |    9 +--------
 5 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-m2m.c b/drivers/media/platform/s5p-fimc/fimc-m2m.c
index ab4c15a..52a2ae6 100644
--- a/drivers/media/platform/s5p-fimc/fimc-m2m.c
+++ b/drivers/media/platform/s5p-fimc/fimc-m2m.c
@@ -627,9 +627,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
@@ -638,7 +636,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 static int fimc_m2m_open(struct file *file)
diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 30195ef..a77bfae 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -158,9 +158,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
@@ -169,7 +167,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 90459cef..fe8cd53 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1230,9 +1230,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->ops = &s5p_jpeg_qops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
@@ -1241,7 +1239,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->ops = &s5p_jpeg_qops;
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 /*
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index e3e616d..6f785bc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -740,11 +740,8 @@ static int s5p_mfc_open(struct file *file)
 		goto err_queue_init;
 	}
 	q->mem_ops = (struct vb2_mem_ops *)&vb2_dma_contig_memops;
-	ret = vb2_queue_init(q);
-	if (ret) {
-		mfc_err("Failed to initialize videobuf2 queue(capture)\n");
-		goto err_queue_init;
-	}
+	vb2_queue_init(q);
+
 	/* Init videobuf2 queue for OUTPUT */
 	q = &ctx->vq_src;
 	q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
@@ -761,11 +758,8 @@ static int s5p_mfc_open(struct file *file)
 		goto err_queue_init;
 	}
 	q->mem_ops = (struct vb2_mem_ops *)&vb2_dma_contig_memops;
-	ret = vb2_queue_init(q);
-	if (ret) {
-		mfc_err("Failed to initialize videobuf2 queue(output)\n");
-		goto err_queue_init;
-	}
+	vb2_queue_init(q);
+
 	init_waitqueue_head(&ctx->queue);
 	mutex_unlock(&dev->mfc_mutex);
 	mfc_debug_leave();
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index a9c6be3..c77b73f 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -774,11 +774,7 @@ static int mxr_video_open(struct file *file)
 		goto fail_fh_open;
 	}
 
-	ret = vb2_queue_init(&layer->vb_queue);
-	if (ret != 0) {
-		mxr_err(mdev, "failed to initialize vb2 queue\n");
-		goto fail_power;
-	}
+	vb2_queue_init(&layer->vb_queue);
 	/* set default format, first on the list */
 	layer->fmt = layer->fmt_array[0];
 	/* setup default geometry */
@@ -787,9 +783,6 @@ static int mxr_video_open(struct file *file)
 
 	return 0;
 
-fail_power:
-	mxr_power_put(mdev);
-
 fail_fh_open:
 	v4l2_fh_release(file);
 
-- 
1.7.8.6

[PATCH 2/9] coda: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Javier Martin <javier.martin@vista-silicon.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/coda.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
index 3ea822f..fc246ab 100644
--- a/drivers/media/platform/coda.c
+++ b/drivers/media/platform/coda.c
@@ -1256,9 +1256,7 @@ static int coda_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->ops = &coda_qops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP;
@@ -1267,7 +1265,8 @@ static int coda_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->ops = &coda_qops;
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 static int coda_open(struct file *file)
-- 
1.7.8.6

[PATCH 3/9] mem2mem-emmaprp: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Javier Martin <javier.martin@vista-silicon.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/mx2_emmaprp.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/mx2_emmaprp.c b/drivers/media/platform/mx2_emmaprp.c
index 59aaca4..17e5c7e 100644
--- a/drivers/media/platform/mx2_emmaprp.c
+++ b/drivers/media/platform/mx2_emmaprp.c
@@ -764,9 +764,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->ops = &emmaprp_qops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
@@ -775,7 +773,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->ops = &emmaprp_qops;
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 /*
-- 
1.7.8.6

[PATCH 4/9] mem2mem-deinterlace: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Javier Martin <javier.martin@vista-silicon.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/m2m-deinterlace.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/m2m-deinterlace.c b/drivers/media/platform/m2m-deinterlace.c
index 9afd930..591e1b8 100644
--- a/drivers/media/platform/m2m-deinterlace.c
+++ b/drivers/media/platform/m2m-deinterlace.c
@@ -873,9 +873,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	q_data[V4L2_M2M_SRC].sizeimage = (640 * 480 * 3) / 2;
 	q_data[V4L2_M2M_SRC].field = V4L2_FIELD_SEQ_TB;
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
@@ -889,7 +887,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	q_data[V4L2_M2M_DST].sizeimage = (640 * 480 * 3) / 2;
 	q_data[V4L2_M2M_SRC].field = V4L2_FIELD_INTERLACED_TB;
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 /*
-- 
1.7.8.6

[PATCH 5/9] marvel-cam: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index ce2b7b4..e117adb 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1098,7 +1098,7 @@ static const struct vb2_ops mcam_vb2_sg_ops = {
 
 #endif /* MCAM_MODE_DMA_SG */
 
-static int mcam_setup_vb2(struct mcam_camera *cam)
+static void mcam_setup_vb2(struct mcam_camera *cam)
 {
 	struct vb2_queue *vq = &cam->vb_queue;
 
@@ -1139,7 +1139,7 @@ static int mcam_setup_vb2(struct mcam_camera *cam)
 #endif
 		break;
 	}
-	return vb2_queue_init(vq);
+	vb2_queue_init(vq);
 }
 
 static void mcam_cleanup_vb2(struct mcam_camera *cam)
@@ -1548,15 +1548,12 @@ static int mcam_v4l_open(struct file *filp)
 	frames = singles = delivered = 0;
 	mutex_lock(&cam->s_mutex);
 	if (cam->users == 0) {
-		ret = mcam_setup_vb2(cam);
-		if (ret)
-			goto out;
+		mcam_setup_vb2(cam);
 		mcam_ctlr_power_up(cam);
 		__mcam_cam_reset(cam);
 		mcam_set_config_needed(cam, 1);
 	}
 	(cam->users)++;
-out:
 	mutex_unlock(&cam->s_mutex);
 	return ret;
 }
-- 
1.7.8.6

[PATCH 6/9] soc-camera: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Cc: Josh Wu <josh.wu@atmel.com>
Cc: Javier Martin <javier.martin@vista-silicon.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/soc_camera/atmel-isi.c      |    3 ++-
 drivers/media/platform/soc_camera/mx2_camera.c     |    3 ++-
 drivers/media/platform/soc_camera/mx3_camera.c     |    3 ++-
 .../platform/soc_camera/sh_mobile_ceu_camera.c     |    3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 6274a91..0fe61d6 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -515,7 +515,8 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 	q->ops = &isi_video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 
-	return vb2_queue_init(q);
+	vb2_queue_init(q);
+	return 0;
 }
 
 static int isi_camera_set_fmt(struct soc_camera_device *icd,
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 256187f..0fc7714 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -981,7 +981,8 @@ static int mx2_camera_init_videobuf(struct vb2_queue *q,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct mx2_buffer);
 
-	return vb2_queue_init(q);
+	vb2_queue_init(q);
+	return 0;
 }
 
 #define MX2_BUS_FLAGS	(V4L2_MBUS_MASTER | \
diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 16975c6..7e9ea36 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -456,7 +456,8 @@ static int mx3_camera_init_videobuf(struct vb2_queue *q,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct mx3_camera_buffer);
 
-	return vb2_queue_init(q);
+	vb2_queue_init(q);
+	return 0;
 }
 
 /* First part of ipu_csi_init_interface() */
diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index 0baaf94..ff32659 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -2026,7 +2026,8 @@ static int sh_mobile_ceu_init_videobuf(struct vb2_queue *q,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct sh_mobile_ceu_buffer);
 
-	return vb2_queue_init(q);
+	vb2_queue_init(q);
+	return 0;
 }
 
 static struct soc_camera_host_ops sh_mobile_ceu_host_ops = {
-- 
1.7.8.6

[PATCH 7/9] stk1160: Don't check vb2_queue_init() return

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-core.c |    4 +---
 drivers/media/usb/stk1160/stk1160-v4l.c  |   12 +++---------
 drivers/media/usb/stk1160/stk1160.h      |    2 +-
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
index 74236fd..0af08e7 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -306,9 +306,7 @@ static int stk1160_probe(struct usb_interface *interface,
 	usb_set_intfdata(interface, dev);
 
 	/* initialize videobuf2 stuff */
-	rc = stk1160_vb2_setup(dev);
-	if (rc < 0)
-		goto free_err;
+	stk1160_vb2_setup(dev);
 
 	/*
 	 * There is no need to take any locks here in probe
diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
index fe6e857..abb933d 100644
--- a/drivers/media/usb/stk1160/stk1160-v4l.c
+++ b/drivers/media/usb/stk1160/stk1160-v4l.c
@@ -670,12 +670,10 @@ void stk1160_clear_queue(struct stk1160 *dev)
 	spin_unlock_irqrestore(&dev->buf_lock, flags);
 }
 
-int stk1160_vb2_setup(struct stk1160 *dev)
+void stk1160_vb2_setup(struct stk1160 *dev)
 {
-	int rc;
-	struct vb2_queue *q;
+	struct vb2_queue *q = &dev->vb_vidq;
 
-	q = &dev->vb_vidq;
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
 	q->drv_priv = dev;
@@ -683,14 +681,10 @@ int stk1160_vb2_setup(struct stk1160 *dev)
 	q->ops = &stk1160_video_qops;
 	q->mem_ops = &vb2_vmalloc_memops;
 
-	rc = vb2_queue_init(q);
-	if (rc < 0)
-		return rc;
+	vb2_queue_init(q);
 
 	/* initialize video dma queue */
 	INIT_LIST_HEAD(&dev->avail_bufs);
-
-	return 0;
 }
 
 int stk1160_video_register(struct stk1160 *dev)
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index 3feba00..3618481 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -173,7 +173,7 @@ struct regval {
 };
 
 /* Provided by stk1160-v4l.c */
-int stk1160_vb2_setup(struct stk1160 *dev);
+void stk1160_vb2_setup(struct stk1160 *dev);
 int stk1160_video_register(struct stk1160 *dev);
 void stk1160_video_unregister(struct stk1160 *dev);
 void stk1160_clear_queue(struct stk1160 *dev);
-- 
1.7.8.6

[PATCH 8/9] mem2mem_testdev: Don't check vb2_queue_init() return value

[Ezequiel Garcia]

Right now vb2_queue_init() returns always 0
and it will be changed to return void.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/platform/mem2mem_testdev.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/mem2mem_testdev.c b/drivers/media/platform/mem2mem_testdev.c
index 9a8b14f..a2bd0b8 100644
--- a/drivers/media/platform/mem2mem_testdev.c
+++ b/drivers/media/platform/mem2mem_testdev.c
@@ -845,9 +845,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	src_vq->ops = &m2mtest_qops;
 	src_vq->mem_ops = &vb2_vmalloc_memops;
 
-	ret = vb2_queue_init(src_vq);
-	if (ret)
-		return ret;
+	vb2_queue_init(src_vq);
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	dst_vq->io_modes = VB2_MMAP;
@@ -856,7 +854,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	dst_vq->ops = &m2mtest_qops;
 	dst_vq->mem_ops = &vb2_vmalloc_memops;
 
-	return vb2_queue_init(dst_vq);
+	vb2_queue_init(dst_vq);
+	return 0;
 }
 
 static const struct v4l2_ctrl_config m2mtest_ctrl_trans_time_msec = {
-- 
1.7.8.6

[PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |    3 +--
 include/media/videobuf2-core.h           |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4da3df6..ea45842 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1736,7 +1736,7 @@ EXPORT_SYMBOL_GPL(vb2_poll);
  * to the struct vb2_queue description in include/media/videobuf2-core.h
  * for more information.
  */
-int vb2_queue_init(struct vb2_queue *q)
+void vb2_queue_init(struct vb2_queue *q)
 {
 	BUG_ON(!q);
 	BUG_ON(!q->ops);
@@ -1755,7 +1755,6 @@ int vb2_queue_init(struct vb2_queue *q)
 	if (q->buf_struct_size == 0)
 		q->buf_struct_size = sizeof(struct vb2_buffer);
 
-	return 0;
 }
 EXPORT_SYMBOL_GPL(vb2_queue_init);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8dd9b6c..ed6854a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -324,7 +324,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req);
 int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
 int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b);
 
-int vb2_queue_init(struct vb2_queue *q);
+void vb2_queue_init(struct vb2_queue *q);
 
 void vb2_queue_release(struct vb2_queue *q);
 
-- 
1.7.8.6

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Jonathan Corbet]

On Sat, 25 Aug 2012 00:09:06 -0300
Ezequiel Garcia <elezegarcia@gmail.com> wrote:

> -int vb2_queue_init(struct vb2_queue *q)
> +void vb2_queue_init(struct vb2_queue *q)
>  {
>  	BUG_ON(!q);
>  	BUG_ON(!q->ops);

If this change goes through in this form, you can add my ack for the
Marvell piece.  But I have to wonder...might it not be better to retain
the return value and use it to return -EINVAL instead of the seven BUG_ON()
calls found in that function?  It shouldn't be necessary to bring things
down in this situation, and, who knows, one of those might just be turned
into a DOS vector with some driver someday.

jon

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

(Ccing videobuf2 authors)

On Sat, Aug 25, 2012 at 12:28 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Sat, 25 Aug 2012 00:09:06 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>
>> -int vb2_queue_init(struct vb2_queue *q)
>> +void vb2_queue_init(struct vb2_queue *q)
>>  {
>>       BUG_ON(!q);
>>       BUG_ON(!q->ops);
>
> If this change goes through in this form, you can add my ack for the
> Marvell piece.  But I have to wonder...might it not be better to retain
> the return value and use it to return -EINVAL instead of the seven BUG_ON()
> calls found in that function?  It shouldn't be necessary to bring things
> down in this situation, and, who knows, one of those might just be turned
> into a DOS vector with some driver someday.
>

The mentioned BUG_ON() are these:

void vb2_queue_init(struct vb2_queue *q)
{
        BUG_ON(!q);
        BUG_ON(!q->ops);
        BUG_ON(!q->mem_ops);
        BUG_ON(!q->type);
        BUG_ON(!q->io_modes);
[...]

Unless I'm overlooking something they look fine to me,
since vb2_queue should always be prepared  by the driver.

On the other hand, it seems these BUG_ON are inherited from videobuf1
(see videobuf_queue_core_init).

Marek, Pawel: What do you think?

Thanks,
Ezequiel.

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Jonathan Corbet]

On Sat, 25 Aug 2012 13:12:01 -0300
Ezequiel Garcia <elezegarcia@gmail.com> wrote:

> The mentioned BUG_ON() are these:
> 
> void vb2_queue_init(struct vb2_queue *q)
> {
>         BUG_ON(!q);
>         BUG_ON(!q->ops);
>         BUG_ON(!q->mem_ops);
>         BUG_ON(!q->type);
>         BUG_ON(!q->io_modes);
> [...]
> 
> Unless I'm overlooking something they look fine to me,
> since vb2_queue should always be prepared  by the driver.

http://permalink.gmane.org/gmane.linux.kernel/1347333 is, I believe, the
definitive word on this kind of use of BUG_ON()...

jon

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

Hi Jon,

On Sat, Aug 25, 2012 at 2:30 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Sat, 25 Aug 2012 13:12:01 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>
>> The mentioned BUG_ON() are these:
>>
>> void vb2_queue_init(struct vb2_queue *q)
>> {
>>         BUG_ON(!q);
>>         BUG_ON(!q->ops);
>>         BUG_ON(!q->mem_ops);
>>         BUG_ON(!q->type);
>>         BUG_ON(!q->io_modes);
>> [...]
>>
>> Unless I'm overlooking something they look fine to me,
>> since vb2_queue should always be prepared  by the driver.
>
> http://permalink.gmane.org/gmane.linux.kernel/1347333 is, I believe, the
> definitive word on this kind of use of BUG_ON()...
>

As usual Linus's words are truly enlightening.

Perhaps you could help me understand this better:

1.
Why do we need to check for all these conditions in the first place?
There are many other functions relying on "struct vb2_queue *q"
not being null (almost all of them) and we don't check for it.
What makes vb2_queue_init() so special that we need to check for it?

2.
If a DoS attack is the concern here, I wonder how this would be achieved?
vb2_queue_init() is an "internal" (so to speak) function, that will only
be called by videobuf2 drivers.

I'm not arguing, truly. I wan't to understand what's the rationale behind
putting BUG_ON, or WARN_ON, or return -EINVAL in a case like this.

Thanks,
Ezequiel.

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Jonathan Corbet]

On Sun, 26 Aug 2012 19:59:40 -0300
Ezequiel Garcia <elezegarcia@gmail.com> wrote:

> 1.
> Why do we need to check for all these conditions in the first place?
> There are many other functions relying on "struct vb2_queue *q"
> not being null (almost all of them) and we don't check for it.
> What makes vb2_queue_init() so special that we need to check for it?

There are plenty of developers who would argue for the removal of the
BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
thereafter.  I'm a bit less convinced; there are attackers who are very
good at exploiting null pointer dereferences, and some systems still allow
the low part of the address space to be mapped.

In general, IMO, checks for consistency make sense; it's nice if the
kernel can *tell* you that something is wrong.

What's a mistake is the BUG_ON; that should really only be used in places
where things simply cannot continue.  In this case, the initialization can
be failed, the V4L2 device will likely be unavailable, but everything else
can continue as normal.  -EINVAL is the right response here.

> 2.
> If a DoS attack is the concern here, I wonder how this would be achieved?
> vb2_queue_init() is an "internal" (so to speak) function, that will only
> be called by videobuf2 drivers.

It would depend on a driver bug, but the sad fact is that driver bugs do
exist.  Perhaps it's as simple as getting the driver module to load when
the hardware is absent, for example.

> I'm not arguing, truly. I wan't to understand what's the rationale behind
> putting BUG_ON, or WARN_ON, or return -EINVAL in a case like this.

In short: we want the kernel to be as robust as it can be.  Detecting
problems before they can snowball helps in that regard.  Hitting the big
red BUG_ON() button in situations where things can continue does not.  At
least, that's how I see it.

jon

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

Hi Jon,

Thanks for your answers, I really appreciate it.

On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Sun, 26 Aug 2012 19:59:40 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>
>> 1.
>> Why do we need to check for all these conditions in the first place?
>> There are many other functions relying on "struct vb2_queue *q"
>> not being null (almost all of them) and we don't check for it.
>> What makes vb2_queue_init() so special that we need to check for it?
>
> There are plenty of developers who would argue for the removal of the
> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
> thereafter.  I'm a bit less convinced; there are attackers who are very
> good at exploiting null pointer dereferences, and some systems still allow
> the low part of the address space to be mapped.
>
> In general, IMO, checks for consistency make sense; it's nice if the
> kernel can *tell* you that something is wrong.
>
> What's a mistake is the BUG_ON; that should really only be used in places
> where things simply cannot continue.  In this case, the initialization can
> be failed, the V4L2 device will likely be unavailable, but everything else
> can continue as normal.  -EINVAL is the right response here.
>

I see your point.

What I really can't seem to understand is why we should have a check
at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.

Thanks a lot!
Ezequiel.

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Mauro Carvalho Chehab]

Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
> Hi Jon,
> 
> Thanks for your answers, I really appreciate it.
> 
> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
>> On Sun, 26 Aug 2012 19:59:40 -0300
>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>>
>>> 1.
>>> Why do we need to check for all these conditions in the first place?
>>> There are many other functions relying on "struct vb2_queue *q"
>>> not being null (almost all of them) and we don't check for it.
>>> What makes vb2_queue_init() so special that we need to check for it?
>>
>> There are plenty of developers who would argue for the removal of the
>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
>> thereafter.  I'm a bit less convinced; there are attackers who are very
>> good at exploiting null pointer dereferences, and some systems still allow
>> the low part of the address space to be mapped.
>>
>> In general, IMO, checks for consistency make sense; it's nice if the
>> kernel can *tell* you that something is wrong.
>>
>> What's a mistake is the BUG_ON; that should really only be used in places
>> where things simply cannot continue.  In this case, the initialization can
>> be failed, the V4L2 device will likely be unavailable, but everything else
>> can continue as normal.  -EINVAL is the right response here.
>>
> 
> I see your point.
> 
> What I really can't seem to understand is why we should have a check
> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.

Those BUG_ON() checks are there since likely the first version of VB1.
VB2 just inherited it.

The think is that letting the VB code to run without checking for some
conditions is evil, as it could cause mass memory corruption, as the
videobuf code writes on a large amount of memory (typically, something
like 512MB written on every 1/30s). So, the code has protections, in order
to try avoiding it. Even so, with VB1, when the output buffer is at the
video adapter memory region (what is called PCI2PCI memory transfers),
there are known bugs with some chipsets that will cause mass data corruption
at the hard disks (as the PCI2PCI transfers interfere at the data transfers
from/to the disk, due to hardware bugs).

Calling WARN_ON_ONCE() and returning some error code works, provided that
we enforce that the error code will be handled at the drivers that call
vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
and double_checking the code at VB2 callers.

Regards,
Mauro

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
>> Hi Jon,
>>
>> Thanks for your answers, I really appreciate it.
>>
>> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
>>> On Sun, 26 Aug 2012 19:59:40 -0300
>>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>>>
>>>> 1.
>>>> Why do we need to check for all these conditions in the first place?
>>>> There are many other functions relying on "struct vb2_queue *q"
>>>> not being null (almost all of them) and we don't check for it.
>>>> What makes vb2_queue_init() so special that we need to check for it?
>>>
>>> There are plenty of developers who would argue for the removal of the
>>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
>>> thereafter.  I'm a bit less convinced; there are attackers who are very
>>> good at exploiting null pointer dereferences, and some systems still allow
>>> the low part of the address space to be mapped.
>>>
>>> In general, IMO, checks for consistency make sense; it's nice if the
>>> kernel can *tell* you that something is wrong.
>>>
>>> What's a mistake is the BUG_ON; that should really only be used in places
>>> where things simply cannot continue.  In this case, the initialization can
>>> be failed, the V4L2 device will likely be unavailable, but everything else
>>> can continue as normal.  -EINVAL is the right response here.
>>>
>>
>> I see your point.
>>
>> What I really can't seem to understand is why we should have a check
>> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.
>
> Those BUG_ON() checks are there since likely the first version of VB1.
> VB2 just inherited it.
>
> The think is that letting the VB code to run without checking for some
> conditions is evil, as it could cause mass memory corruption, as the
> videobuf code writes on a large amount of memory (typically, something
> like 512MB written on every 1/30s). So, the code has protections, in order
> to try avoiding it. Even so, with VB1, when the output buffer is at the
> video adapter memory region (what is called PCI2PCI memory transfers),
> there are known bugs with some chipsets that will cause mass data corruption
> at the hard disks (as the PCI2PCI transfers interfere at the data transfers
> from/to the disk, due to hardware bugs).
>
> Calling WARN_ON_ONCE() and returning some error code works, provided that
> we enforce that the error code will be handled at the drivers that call
> vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
> and double_checking the code at VB2 callers.
>

So, you want me to resend?

And this whole patchset patchset should
be dropped because we'll return something from vb2_queue_init.

Thanks,
Ezequiel.

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Ezequiel Garcia]

Mauro,

(Cc got messed up, so I'm sending this to you and media list)

On Sat, Sep 15, 2012 at 10:37 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Sat, 15 Sep 2012 10:05:40 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
>
>> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>> > Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
>> >> Hi Jon,
>> >>
>> >> Thanks for your answers, I really appreciate it.
>> >>
>> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
>> >>> On Sun, 26 Aug 2012 19:59:40 -0300
>> >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> >>>
>> >>>> 1.
>> >>>> Why do we need to check for all these conditions in the first place?
>> >>>> There are many other functions relying on "struct vb2_queue *q"
>> >>>> not being null (almost all of them) and we don't check for it.
>> >>>> What makes vb2_queue_init() so special that we need to check for it?
>> >>>
>> >>> There are plenty of developers who would argue for the removal of the
>> >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
>> >>> thereafter.  I'm a bit less convinced; there are attackers who are very
>> >>> good at exploiting null pointer dereferences, and some systems still allow
>> >>> the low part of the address space to be mapped.
>> >>>
>> >>> In general, IMO, checks for consistency make sense; it's nice if the
>> >>> kernel can *tell* you that something is wrong.
>> >>>
>> >>> What's a mistake is the BUG_ON; that should really only be used in places
>> >>> where things simply cannot continue.  In this case, the initialization can
>> >>> be failed, the V4L2 device will likely be unavailable, but everything else
>> >>> can continue as normal.  -EINVAL is the right response here.
>> >>>
>> >>
>> >> I see your point.
>> >>
>> >> What I really can't seem to understand is why we should have a check
>> >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.
>> >
>> > Those BUG_ON() checks are there since likely the first version of VB1.
>> > VB2 just inherited it.
>> >
>> > The think is that letting the VB code to run without checking for some
>> > conditions is evil, as it could cause mass memory corruption, as the
>> > videobuf code writes on a large amount of memory (typically, something
>> > like 512MB written on every 1/30s). So, the code has protections, in order
>> > to try avoiding it. Even so, with VB1, when the output buffer is at the
>> > video adapter memory region (what is called PCI2PCI memory transfers),
>> > there are known bugs with some chipsets that will cause mass data corruption
>> > at the hard disks (as the PCI2PCI transfers interfere at the data transfers
>> > from/to the disk, due to hardware bugs).
>> >
>> > Calling WARN_ON_ONCE() and returning some error code works, provided that
>> > we enforce that the error code will be handled at the drivers that call
>> > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
>> > and double_checking the code at VB2 callers.
>> >
>>
>> So, you want me to resend?
>
> Yes, please.
>

I can't decide on coding style. So, how about this?:

(copy pasted on gmail, sorry if spaces are mangled):

  */
 int vb2_queue_init(struct vb2_queue *q)
 {
-       BUG_ON(!q);
-       BUG_ON(!q->ops);
-       BUG_ON(!q->mem_ops);
-       BUG_ON(!q->type);
-       BUG_ON(!q->io_modes);
-
-       BUG_ON(!q->ops->queue_setup);
-       BUG_ON(!q->ops->buf_queue);
+       /*
+        * Sanity check
+        */
+       if (WARN_ON_ONCE(!q)            ||
+           WARN_ON_ONCE(!q->ops)       ||
+           WARN_ON_ONCE(!q->mem_ops)   ||
+           WARN_ON_ONCE(!q->type)      ||
+           WARN_ON_ONCE(!q->io_modes)  ||
+           WARN_ON_ONCE(!q->ops->queue_setup) ||
+           WARN_ON_ONCE(!q->ops->buf_queue))
+               return -EINVAL;

        INIT_LIST_HEAD(&q->queued_list);
        INIT_LIST_HEAD(&q->done_list);

Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

[Mauro Carvalho Chehab]

Em Sat, 15 Sep 2012 12:22:48 -0300
Ezequiel Garcia <elezegarcia@gmail.com> escreveu:

> Mauro,
> 
> (Cc got messed up, so I'm sending this to you and media list)
> 
> On Sat, Sep 15, 2012 at 10:37 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em Sat, 15 Sep 2012 10:05:40 -0300
> > Ezequiel Garcia <elezegarcia@gmail.com> escreveu:
> >
> >> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab
> >> <mchehab@redhat.com> wrote:
> >> > Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
> >> >> Hi Jon,
> >> >>
> >> >> Thanks for your answers, I really appreciate it.
> >> >>
> >> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> >> >>> On Sun, 26 Aug 2012 19:59:40 -0300
> >> >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> >> >>>
> >> >>>> 1.
> >> >>>> Why do we need to check for all these conditions in the first place?
> >> >>>> There are many other functions relying on "struct vb2_queue *q"
> >> >>>> not being null (almost all of them) and we don't check for it.
> >> >>>> What makes vb2_queue_init() so special that we need to check for it?
> >> >>>
> >> >>> There are plenty of developers who would argue for the removal of the
> >> >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
> >> >>> thereafter.  I'm a bit less convinced; there are attackers who are very
> >> >>> good at exploiting null pointer dereferences, and some systems still allow
> >> >>> the low part of the address space to be mapped.
> >> >>>
> >> >>> In general, IMO, checks for consistency make sense; it's nice if the
> >> >>> kernel can *tell* you that something is wrong.
> >> >>>
> >> >>> What's a mistake is the BUG_ON; that should really only be used in places
> >> >>> where things simply cannot continue.  In this case, the initialization can
> >> >>> be failed, the V4L2 device will likely be unavailable, but everything else
> >> >>> can continue as normal.  -EINVAL is the right response here.
> >> >>>
> >> >>
> >> >> I see your point.
> >> >>
> >> >> What I really can't seem to understand is why we should have a check
> >> >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.
> >> >
> >> > Those BUG_ON() checks are there since likely the first version of VB1.
> >> > VB2 just inherited it.
> >> >
> >> > The think is that letting the VB code to run without checking for some
> >> > conditions is evil, as it could cause mass memory corruption, as the
> >> > videobuf code writes on a large amount of memory (typically, something
> >> > like 512MB written on every 1/30s). So, the code has protections, in order
> >> > to try avoiding it. Even so, with VB1, when the output buffer is at the
> >> > video adapter memory region (what is called PCI2PCI memory transfers),
> >> > there are known bugs with some chipsets that will cause mass data corruption
> >> > at the hard disks (as the PCI2PCI transfers interfere at the data transfers
> >> > from/to the disk, due to hardware bugs).
> >> >
> >> > Calling WARN_ON_ONCE() and returning some error code works, provided that
> >> > we enforce that the error code will be handled at the drivers that call
> >> > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
> >> > and double_checking the code at VB2 callers.
> >> >
> >>
> >> So, you want me to resend?
> >
> > Yes, please.
> >
> 
> I can't decide on coding style. So, how about this?:
> 
> (copy pasted on gmail, sorry if spaces are mangled):
> 
>   */
>  int vb2_queue_init(struct vb2_queue *q)
>  {
> -       BUG_ON(!q);
> -       BUG_ON(!q->ops);
> -       BUG_ON(!q->mem_ops);
> -       BUG_ON(!q->type);
> -       BUG_ON(!q->io_modes);
> -
> -       BUG_ON(!q->ops->queue_setup);
> -       BUG_ON(!q->ops->buf_queue);
> +       /*
> +        * Sanity check
> +        */
> +       if (WARN_ON_ONCE(!q)            ||
> +           WARN_ON_ONCE(!q->ops)       ||
> +           WARN_ON_ONCE(!q->mem_ops)   ||
> +           WARN_ON_ONCE(!q->type)      ||
> +           WARN_ON_ONCE(!q->io_modes)  ||
> +           WARN_ON_ONCE(!q->ops->queue_setup) ||
> +           WARN_ON_ONCE(!q->ops->buf_queue))
> +               return -EINVAL;
> 
>         INIT_LIST_HEAD(&q->queued_list);
>         INIT_LIST_HEAD(&q->done_list);

It seems OK on my eyes.

-- 
Regards,
Mauro