From patchwork Fri Jan 6 12:15:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kieran Bingham X-Patchwork-Id: 9500855 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3CC15606E0 for ; Fri, 6 Jan 2017 12:18:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2E8E020246 for ; Fri, 6 Jan 2017 12:18:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 234CF28478; Fri, 6 Jan 2017 12:18:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A69A528496 for ; Fri, 6 Jan 2017 12:18:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161372AbdAFMRm (ORCPT ); Fri, 6 Jan 2017 07:17:42 -0500 Received: from mail.kernel.org ([198.145.29.136]:43224 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753277AbdAFMQY (ORCPT ); Fri, 6 Jan 2017 07:16:24 -0500 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 30C57203DC; Fri, 6 Jan 2017 12:15:39 +0000 (UTC) Received: from CookieMonster.cookiemonster.local (cpc89244-aztw30-2-0-cust5770.18-1.cable.virginm.net [86.31.182.139]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 300BD203EC; Fri, 6 Jan 2017 12:15:37 +0000 (UTC) From: Kieran Bingham To: laurent.pinchart@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, Kieran Bingham Subject: [PATCH v4 1/4] v4l: vsp1: Prevent multiple streamon race commencing pipeline early Date: Fri, 6 Jan 2017 12:15:28 +0000 Message-Id: X-Mailer: git-send-email 2.7.4 In-Reply-To: References: In-Reply-To: References: X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP With multiple inputs through the BRU it is feasible for the streams to race each other at stream-on. Multiple VIDIOC_STREAMON calls racing each other could have process N-1 skipping over the pipeline setup section and then start the pipeline early, if videobuf2 has already enqueued buffers to the driver for process N but not called the .start_streaming() operation yet In the case of the video pipelines, this can present two serious issues. 1) A null-dereference if the pipe->dl is committed at the same time as the vsp1_video_setup_pipeline() is processing 2) A hardware hang, where a display list is committed without having called vsp1_video_setup_pipeline() first Repair this issue, by ensuring that only the stream which configures the pipeline is able to start it. Signed-off-by: Kieran Bingham Reviewed-by: Laurent Pinchart --- v4: - Revert and rework back to v1 implementation style - Provide detailed comments on the race v3: - Move 'flag reset' to be inside the vsp1_reset_wpf() function call - Tidy up the wpf->pipe reference for the configured flag To test this race, I have used the vsp-unit-test-0007.sh from Laurent's VSP-Tests [0] in iteration. Without this patch, failures can be seen be seen anywhere up to the 150 iterations mark. With this patch in place, tests have successfully iterated over 1500 loops. The function affected by this change appears to have been around since v4.6-rc2-105-g351bbf99f245 and thus could be included in stable trees from that point forward. The issue may have been prevalent before that but the solution would need reworking for earlier version. [0] http://git.ideasonboard.com/renesas/vsp-tests.git --- drivers/media/platform/vsp1/vsp1_video.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c index e6592b576ca3..f7dc249eb398 100644 --- a/drivers/media/platform/vsp1/vsp1_video.c +++ b/drivers/media/platform/vsp1/vsp1_video.c @@ -797,6 +797,7 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count) { struct vsp1_video *video = vb2_get_drv_priv(vq); struct vsp1_pipeline *pipe = video->rwpf->pipe; + bool start_pipeline = false; unsigned long flags; int ret; @@ -807,11 +808,23 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count) mutex_unlock(&pipe->lock); return ret; } + + start_pipeline = true; } pipe->stream_count++; mutex_unlock(&pipe->lock); + /* + * vsp1_pipeline_ready() is not sufficient to establish that all streams + * are prepared and the pipeline is configured, as multiple streams + * can race through streamon with buffers already queued; Therefore we + * don't even attempt to start the pipeline until the last stream has + * called through here. + */ + if (!start_pipeline) + return 0; + spin_lock_irqsave(&pipe->irqlock, flags); if (vsp1_pipeline_ready(pipe)) vsp1_video_pipeline_run(pipe);