Hi Hans,
Thank you for the patch.
On Monday 04 August 2014 12:27:12 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # for v3.15 and up
> Cc: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html