[PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[Ezequiel Garcia]

This replaces BUG_ON() calls with WARN_ON_ONCE(), and returns
EINVAL if some parameter is NULL, as suggested by Jonathan and Mauro.

The BUG_ON() call is too drastic to be used in this case.
See the full discussion here:
http://www.spinics.net/lists/linux-media/msg52462.html

Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
This patchset replaces:
[PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

 drivers/media/v4l2-core/videobuf2-core.c |   19 +++++++++++--------
 include/media/videobuf2-core.h           |    2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4da3df6..e394191 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1738,14 +1738,17 @@ EXPORT_SYMBOL_GPL(vb2_poll);
  */
 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);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8dd9b6c..e04252a 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);
+int __must_check vb2_queue_init(struct vb2_queue *q);
 
 void vb2_queue_release(struct vb2_queue *q);
 
-- 
1.7.8.6

Re: [PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[Hans Verkuil]

On Mon September 17 2012 15:43:54 Ezequiel Garcia wrote:
> This replaces BUG_ON() calls with WARN_ON_ONCE(), and returns
> EINVAL if some parameter is NULL, as suggested by Jonathan and Mauro.
> 
> The BUG_ON() call is too drastic to be used in this case.
> See the full discussion here:
> http://www.spinics.net/lists/linux-media/msg52462.html
> 
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> This patchset replaces:
> [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void
> 
>  drivers/media/v4l2-core/videobuf2-core.c |   19 +++++++++++--------
>  include/media/videobuf2-core.h           |    2 +-
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 4da3df6..e394191 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1738,14 +1738,17 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>   */
>  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))

Why WARN_ON_ONCE? I'd want to see this all the time, not just once.

It's certainly better than BUG_ON, but I'd go for WARN_ON.

Regards,

	Hans

> +		return -EINVAL;
>  
>  	INIT_LIST_HEAD(&q->queued_list);
>  	INIT_LIST_HEAD(&q->done_list);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 8dd9b6c..e04252a 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);
> +int __must_check vb2_queue_init(struct vb2_queue *q);
>  
>  void vb2_queue_release(struct vb2_queue *q);
>  
> 

Re: [PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[Jonathan Corbet]

On Mon, 17 Sep 2012 16:10:43 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Why WARN_ON_ONCE? I'd want to see this all the time, not just once.
> 
> It's certainly better than BUG_ON, but I'd go for WARN_ON.

I like WARN_ON_ONCE better, myself.  Avoids the risk of spamming the logs,
and once is enough to answer that "why doesn't my camera work?" question.
Don't feel all that strongly about it, though...

jon

Re: [PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[Hans Verkuil]

On Mon September 17 2012 17:36:36 Jonathan Corbet wrote:
> On Mon, 17 Sep 2012 16:10:43 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > Why WARN_ON_ONCE? I'd want to see this all the time, not just once.
> > 
> > It's certainly better than BUG_ON, but I'd go for WARN_ON.
> 
> I like WARN_ON_ONCE better, myself.  Avoids the risk of spamming the logs,
> and once is enough to answer that "why doesn't my camera work?" question.
> Don't feel all that strongly about it, though...

However, videobuf2-core.c is a core function of a core module. So it will
give this warning once for one driver, then another is loaded with the same
problem and you'll get no warnings anymore. It makes sense in a driver, but
not here IMHO. Unless I'm missing something.

Neither is this likely to ever spam the logs. It's called only when the device
is initialized.

Regards,

	Hans

Re: [PATCH 1/4] videobuf2-core: Replace BUG_ON and return an error at vb2_queue_init()

[Jonathan Corbet]

On Mon, 17 Sep 2012 17:41:24 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> However, videobuf2-core.c is a core function of a core module. So it will
> give this warning once for one driver, then another is loaded with the same
> problem and you'll get no warnings anymore.

Unlikely scenario, but good point regardless, I hadn't thought about that
aspect of the problem. 

jon