From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:33466 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752093AbaHDKoX (ORCPT ); Mon, 4 Aug 2014 06:44:23 -0400 From: Laurent Pinchart To: Hans Verkuil Cc: linux-media@vger.kernel.org, Hans Verkuil , stable@vger.kernel.org, #@tschai.lan, for@tschai.lan, v3.15@tschai.lan, and@tschai.lan, up@tschai.lan Subject: Re: [PATCH for v3.17 2/2] vb2: fix vb2 state check when start_streaming fails Date: Mon, 04 Aug 2014 12:44:52 +0200 Message-ID: <4228716.VErhQhH3PE@avalon> In-Reply-To: <1407148032-41607-3-git-send-email-hverkuil@xs4all.nl> References: <1407148032-41607-1-git-send-email-hverkuil@xs4all.nl> <1407148032-41607-3-git-send-email-hverkuil@xs4all.nl> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Hans, Thank you for the patch. On Monday 04 August 2014 12:27:12 Hans Verkuil wrote: > From: Hans Verkuil > > 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 > Cc: stable@vger.kernel.org # for v3.15 and up > Cc: Laurent Pinchart Acked-by: Laurent Pinchart 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