* [PATCH 2/9] coda: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
@ 2012-08-25 3:08 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 3/9] mem2mem-emmaprp: " Ezequiel Garcia
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:08 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: Ezequiel Garcia, Javier Martin
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 3/9] mem2mem-emmaprp: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
2012-08-25 3:08 ` [PATCH 2/9] coda: " Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 4/9] mem2mem-deinterlace: " Ezequiel Garcia
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: Ezequiel Garcia, Javier Martin
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 4/9] mem2mem-deinterlace: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
2012-08-25 3:08 ` [PATCH 2/9] coda: " Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 3/9] mem2mem-emmaprp: " Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 5/9] marvel-cam: " Ezequiel Garcia
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: Ezequiel Garcia, Javier Martin
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 5/9] marvel-cam: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
` (2 preceding siblings ...)
2012-08-25 3:09 ` [PATCH 4/9] mem2mem-deinterlace: " Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 6/9] soc-camera: " Ezequiel Garcia
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: Ezequiel Garcia, Jonathan Corbet
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 6/9] soc-camera: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
` (3 preceding siblings ...)
2012-08-25 3:09 ` [PATCH 5/9] marvel-cam: " Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 7/9] stk1160: Don't check vb2_queue_init() return Ezequiel Garcia
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Josh Wu, Javier Martin, Guennadi Liakhovetski,
Magnus Damm
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 7/9] stk1160: Don't check vb2_queue_init() return
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
` (4 preceding siblings ...)
2012-08-25 3:09 ` [PATCH 6/9] soc-camera: " Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 8/9] mem2mem_testdev: Don't check vb2_queue_init() return value Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void Ezequiel Garcia
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 8/9] mem2mem_testdev: Don't check vb2_queue_init() return value
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
` (5 preceding siblings ...)
2012-08-25 3:09 ` [PATCH 7/9] stk1160: Don't check vb2_queue_init() return Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 3:09 ` [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void Ezequiel Garcia
7 siblings, 0 replies; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-25 3:08 [PATCH 1/9] s5p: Don't check vb2_queue_init() return value Ezequiel Garcia
` (6 preceding siblings ...)
2012-08-25 3:09 ` [PATCH 8/9] mem2mem_testdev: Don't check vb2_queue_init() return value Ezequiel Garcia
@ 2012-08-25 3:09 ` Ezequiel Garcia
2012-08-25 15:28 ` Jonathan Corbet
7 siblings, 1 reply; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 3:09 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-25 3:09 ` [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void Ezequiel Garcia
@ 2012-08-25 15:28 ` Jonathan Corbet
2012-08-25 16:12 ` Ezequiel Garcia
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Corbet @ 2012-08-25 15:28 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: linux-media, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-25 15:28 ` Jonathan Corbet
@ 2012-08-25 16:12 ` Ezequiel Garcia
2012-08-25 17:30 ` Jonathan Corbet
0 siblings, 1 reply; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-25 16:12 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-media, Mauro Carvalho Chehab, Pawel Osciak,
Marek Szyprowski
(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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-25 16:12 ` Ezequiel Garcia
@ 2012-08-25 17:30 ` Jonathan Corbet
2012-08-26 22:59 ` Ezequiel Garcia
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Corbet @ 2012-08-25 17:30 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: linux-media, Mauro Carvalho Chehab, Pawel Osciak,
Marek Szyprowski
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-25 17:30 ` Jonathan Corbet
@ 2012-08-26 22:59 ` Ezequiel Garcia
2012-08-28 16:55 ` Jonathan Corbet
0 siblings, 1 reply; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-26 22:59 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-media, Mauro Carvalho Chehab, Pawel Osciak,
Marek Szyprowski
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-26 22:59 ` Ezequiel Garcia
@ 2012-08-28 16:55 ` Jonathan Corbet
2012-08-28 17:23 ` Ezequiel Garcia
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Corbet @ 2012-08-28 16:55 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: linux-media, Mauro Carvalho Chehab, Pawel Osciak,
Marek Szyprowski
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-28 16:55 ` Jonathan Corbet
@ 2012-08-28 17:23 ` Ezequiel Garcia
2012-09-15 12:48 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 19+ messages in thread
From: Ezequiel Garcia @ 2012-08-28 17:23 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-media, Mauro Carvalho Chehab, Pawel Osciak,
Marek Szyprowski
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-08-28 17:23 ` Ezequiel Garcia
@ 2012-09-15 12:48 ` Mauro Carvalho Chehab
2012-09-15 13:05 ` Ezequiel Garcia
0 siblings, 1 reply; 19+ messages in thread
From: Mauro Carvalho Chehab @ 2012-09-15 12:48 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Jonathan Corbet, linux-media, Pawel Osciak, Marek Szyprowski
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
2012-09-15 12:48 ` Mauro Carvalho Chehab
@ 2012-09-15 13:05 ` Ezequiel Garcia
[not found] ` <20120915103719.4f038ee0@redhat.com>
0 siblings, 1 reply; 19+ messages in thread
From: Ezequiel Garcia @ 2012-09-15 13:05 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jonathan Corbet, linux-media, Pawel Osciak, Marek Szyprowski
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.
^ permalink raw reply [flat|nested] 19+ messages in thread