[PATCH for v3.17 0/2] vb2 fixes

[PATCH for v3.17 0/2] vb2 fixes

[Hans Verkuil]

I saw this post http://www.mail-archive.com/linux-media@vger.kernel.org/msg77864.html
and decided to quickly fix the pwc videobuf2-core.c warning. While doing that
I encountered two more vb2 bugs: one very confusing typo in a comment and one
regression from an earlier patch that needs to be applied from 3.15 and up.

Both are corner cases relating to what should be done when start_streaming fails.

Regards,

	Hans

[PATCH for v3.17 1/2] videobuf2-core.h: fix comment

[Hans Verkuil]

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

The comment for start_streaming that tells the developer with which vb2 state
buffers should be returned to vb2 gave the wrong state. Very confusing.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/media/videobuf2-core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index fc910a6..80fa725 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -295,7 +295,7 @@ struct vb2_buffer {
  *			can return an error if hardware fails, in that case all
  *			buffers that have been already given by the @buf_queue
  *			callback are to be returned by the driver by calling
- *			@vb2_buffer_done(VB2_BUF_STATE_DEQUEUED).
+ *			@vb2_buffer_done(VB2_BUF_STATE_QUEUED).
  *			If you need a minimum number of buffers before you can
  *			start streaming, then set @min_buffers_needed in the
  *			vb2_queue structure. If that is non-zero then
-- 
2.0.1

[PATCH for v3.17 2/2] vb2: fix vb2 state check when start_streaming fails

[Hans Verkuil]

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

Commit bd994ddb2a12a3ff48cd549ec82cdceaea9614df (vb2: Fix stream start and
buffer completion race) broke the buffer state check in vb2_buffer_done.

So accept all three possible states there since I can no longer tell the
difference between vb2_buffer_done called from start_streaming or from
elsewhere.

Instead add a WARN_ON at the end of start_streaming that will check whether
any buffers were added to the done list, since that implies that the wrong
state was used as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: stable@vger.kernel.org      # for v3.15 and up
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index d3f2a22..7f70fd52 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1165,13 +1165,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
 		return;
 
-	if (!q->start_streaming_called) {
-		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
-			state = VB2_BUF_STATE_QUEUED;
-	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
-			   state != VB2_BUF_STATE_ERROR)) {
-			state = VB2_BUF_STATE_ERROR;
-	}
+	if (WARN_ON(state != VB2_BUF_STATE_DONE &&
+		    state != VB2_BUF_STATE_ERROR &&
+		    state != VB2_BUF_STATE_QUEUED))
+		state = VB2_BUF_STATE_ERROR;
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
@@ -1783,6 +1780,12 @@ static int vb2_start_streaming(struct vb2_queue *q)
 		/* Must be zero now */
 		WARN_ON(atomic_read(&q->owned_by_drv_count));
 	}
+	/*
+	 * If done_list is not empty, then start_streaming() didn't call
+	 * vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED) but STATE_ERROR or
+	 * STATE_DONE.
+	 */
+	WARN_ON(!list_empty(&q->done_list));
 	return ret;
 }
 
-- 
2.0.1

Re: [PATCH for v3.17 1/2] videobuf2-core.h: fix comment

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Monday 04 August 2014 12:27:11 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The comment for start_streaming that tells the developer with which vb2
> state buffers should be returned to vb2 gave the wrong state. Very
> confusing.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I wonder whether we couldn't simplify drivers by moving this into vb2 though. 
A failed start_streaming requires drivers to dequeue all buffers internally, 
but the call to vb2_buffer_done() could be handled inside vb2. On the other 
hand it would make the vb2 warning go away, and drivers that fail to dequeue 
buffers internally would not be caught as easily, so I won't push for that 
change.

> ---
>  include/media/videobuf2-core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index fc910a6..80fa725 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -295,7 +295,7 @@ struct vb2_buffer {
>   *			can return an error if hardware fails, in that case all
>   *			buffers that have been already given by the @buf_queue
>   *			callback are to be returned by the driver by calling
> - *			@vb2_buffer_done(VB2_BUF_STATE_DEQUEUED).
> + *			@vb2_buffer_done(VB2_BUF_STATE_QUEUED).
>   *			If you need a minimum number of buffers before you can
>   *			start streaming, then set @min_buffers_needed in the
>   *			vb2_queue structure. If that is non-zero then

-- 
Regards,

Laurent Pinchart

Re: [PATCH for v3.17 2/2] vb2: fix vb2 state check when start_streaming fails

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Monday 04 August 2014 12:27:12 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Commit bd994ddb2a12a3ff48cd549ec82cdceaea9614df (vb2: Fix stream start and
> buffer completion race) broke the buffer state check in vb2_buffer_done.
> 
> So accept all three possible states there since I can no longer tell the
> difference between vb2_buffer_done called from start_streaming or from
> elsewhere.
> 
> Instead add a WARN_ON at the end of start_streaming that will check whether
> any buffers were added to the done list, since that implies that the wrong
> state was used as well.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: stable@vger.kernel.org      # for v3.15 and up
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Given that I've introduced a few vb2 bugs I hope my review still has some 
value :-)

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index d3f2a22..7f70fd52 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1165,13 +1165,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state) if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
>  		return;
> 
> -	if (!q->start_streaming_called) {
> -		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
> -			state = VB2_BUF_STATE_QUEUED;
> -	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
> -			   state != VB2_BUF_STATE_ERROR)) {
> -			state = VB2_BUF_STATE_ERROR;
> -	}
> +	if (WARN_ON(state != VB2_BUF_STATE_DONE &&
> +		    state != VB2_BUF_STATE_ERROR &&
> +		    state != VB2_BUF_STATE_QUEUED))
> +		state = VB2_BUF_STATE_ERROR;
> 
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
> @@ -1783,6 +1780,12 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  		/* Must be zero now */
>  		WARN_ON(atomic_read(&q->owned_by_drv_count));
>  	}
> +	/*
> +	 * If done_list is not empty, then start_streaming() didn't call
> +	 * vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED) but STATE_ERROR or
> +	 * STATE_DONE.
> +	 */
> +	WARN_ON(!list_empty(&q->done_list));
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH for v3.17 1/2] videobuf2-core.h: fix comment

[Hans Verkuil]

On 08/04/2014 12:40 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday 04 August 2014 12:27:11 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The comment for start_streaming that tells the developer with which vb2
>> state buffers should be returned to vb2 gave the wrong state. Very
>> confusing.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I wonder whether we couldn't simplify drivers by moving this into vb2 though. 
> A failed start_streaming requires drivers to dequeue all buffers internally, 
> but the call to vb2_buffer_done() could be handled inside vb2. On the other 
> hand it would make the vb2 warning go away, and drivers that fail to dequeue 
> buffers internally would not be caught as easily, so I won't push for that 
> change.

The driver owns the buffers at that point. So if I just dequeue them internally
then I have no idea what sort of driver-internal data structures I am corrupting.

Most drivers use a linked list of some sort, so that is typically the one that
gets messed up (and that actually happens). By far the best approach is to
require that drivers just hand over the buffers themselves, that way everything
happens in a controlled manner.

Regards,

	Hans

> 
>> ---
>>  include/media/videobuf2-core.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index fc910a6..80fa725 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -295,7 +295,7 @@ struct vb2_buffer {
>>   *			can return an error if hardware fails, in that case all
>>   *			buffers that have been already given by the @buf_queue
>>   *			callback are to be returned by the driver by calling
>> - *			@vb2_buffer_done(VB2_BUF_STATE_DEQUEUED).
>> + *			@vb2_buffer_done(VB2_BUF_STATE_QUEUED).
>>   *			If you need a minimum number of buffers before you can
>>   *			start streaming, then set @min_buffers_needed in the
>>   *			vb2_queue structure. If that is non-zero then
> 

Re: [PATCH for v3.17 2/2] vb2: fix vb2 state check when start_streaming fails

[Hans Verkuil]

On 08/04/2014 12:44 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday 04 August 2014 12:27:12 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Commit bd994ddb2a12a3ff48cd549ec82cdceaea9614df (vb2: Fix stream start and
>> buffer completion race) broke the buffer state check in vb2_buffer_done.
>>
>> So accept all three possible states there since I can no longer tell the
>> difference between vb2_buffer_done called from start_streaming or from
>> elsewhere.
>>
>> Instead add a WARN_ON at the end of start_streaming that will check whether
>> any buffers were added to the done list, since that implies that the wrong
>> state was used as well.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: stable@vger.kernel.org      # for v3.15 and up
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Given that I've introduced a few vb2 bugs I hope my review still has some 
> value :-)

Since I reviewed your original patch as well, I'm not going to throw stones at
you :-)

Regards,

	Hans