Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932715AbbLBQC5 (ORCPT ); Wed, 2 Dec 2015 11:02:57 -0500 Received: from mail-wm0-f44.google.com ([74.125.82.44]:34152 "EHLO mail-wm0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750901AbbLBQCy (ORCPT ); Wed, 2 Dec 2015 11:02:54 -0500 Subject: Re: [RESEND RFC/PATCH 6/8] media: platform: mtk-vcodec: Add Mediatek V4L2 Video Encoder Driver To: tiffany lin References: <1447764885-23100-1-git-send-email-tiffany.lin@mediatek.com> <1447764885-23100-7-git-send-email-tiffany.lin@mediatek.com> <56588622.8060600@linaro.org> <1448883594.25093.45.camel@mtksdaap41> <1448966550.7534.95.camel@mtksdaap41> <565DBFF3.1000409@linaro.org> <1449061708.8326.5.camel@mtksdaap41> Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Catalin Marinas , Will Deacon , Mauro Carvalho Chehab , Matthias Brugger , Daniel Kurtz , Sascha Hauer , Hongzhou Yang , Hans Verkuil , Laurent Pinchart , Sakari Ailus , Mikhail Ulyanov , Fabien Dessenne , Arnd Bergmann , Darren Etheridge , Peter Griffin , Benoit Parrot , =?UTF-8?B?QW5kcmV3LUNUIENoZW4gKOmZs+aZuui/qik=?= , =?UTF-8?B?RWRkaWUgSHVhbmcgKOm7g+aZuuWCkSk=?= , =?UTF-8?B?WWluZ2pvZSBDaGVuICjpmbPoi7HmtLIp?= , =?UTF-8?B?SmFtZXNKSiBMaWFvICjlu5blu7rmmbop?= , =?UTF-8?B?RGFuaWVsIEhzaWFvICjola3kvK/liZsp?= , "devicetree@vger.kernel.org" , lkml , "linux-arm-kernel@lists.infradead.org" , "linux-media@vger.kernel.org" , "linux-mediatek@lists.infradead.org" , =?UTF-8?B?UG9DaHVuIExpbiAo5p6X5p+P5ZCbKQ==?= From: Daniel Thompson Message-ID: <565F162A.4070106@linaro.org> Date: Wed, 2 Dec 2015 16:02:50 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1449061708.8326.5.camel@mtksdaap41> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2085 Lines: 55 On 02/12/15 13:08, tiffany lin wrote: >>> We need MTK_STATE_ABORT to inform encoder thread (mtk_venc_worker) that >>> stop encodeing job from stopped ctx instance. >>> When user space qbuf, we need to make sure everything is ready to sent >>> buf to encode. >> >> Agree that you need a flag here. In fact currently you have two, >> MTK_STATE_ABORT and an unused one called aborting. >> >> You need to be very careful with these flags though. They are a magnet >> for data race bugs (especially combined with SMP). >> >> For example at present I can't see any locking in the worker code. This >> means there is nothing to make all those read-modify-write sequences >> that manage the state atomic (thus risking state corruption). >> > We prevent that one function set the flag and others clear the flag. > So there is no special lock to protect state. What prevents concurrent access from different calling contexts? It looks to me like the work on the work queue may run concurrently with the ioctl calls. > +static void vb2ops_venc_stop_streaming(struct vb2_queue *q) > +{ > + struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(q); > + struct v4l2_device *v4l2_dev = &ctx->dev->v4l2_dev; > + struct vb2_buffer *src_buf, *dst_buf; > + int retry; > + int ret; > + > + mtk_v4l2_debug(2, "[%d]-> type=%d", ctx->idx, q->type); > + > + retry = 0; > + while ((ctx->state & MTK_STATE_RUNNING) && (retry < 10)) { > + mtk_vcodec_clean_ctx_int_flags(ctx); > + ctx->state |= MTK_STATE_ABORT; As a simple example I think the above line can run concurrently with the following code near the end of the worker code. > + ctx->state &= ~MTK_STATE_RUNNING; > + v4l2_m2m_job_finish(ctx->dev->m2m_dev_enc, ctx->m2m_ctx); If I'm right then the state of the flags can definitely get clobbered due to the read-modify-write actions on the state. Daniel. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/