[{"id":24806,"web_url":"https://patchwork.libcamera.org/comment/24806/","msgid":"<Ywu0MJShW/QYxcZb@pendragon.ideasonboard.com>","date":"2022-08-28T18:30:08","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang and Hiro,\n\nThank you for the patch.\n\nOn Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:\n> From: Hirokazu Honda <hiroh@chromium.org>\n> \n> This annotates member variables of ThreadData by clang thread\n> safety annotations.\n> \n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  src/libcamera/base/thread.cpp | 22 +++++++++++++++-------\n>  1 file changed, 15 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 6bda9d14..2e26b83c 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -151,7 +151,7 @@ private:\n>  \tfriend class ThreadMain;\n>  \n>  \tThread *thread_;\n> -\tbool running_;\n> +\tbool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n>  \tpid_t tid_;\n>  \n>  \tMutex mutex_;\n> @@ -160,7 +160,7 @@ private:\n>  \n>  \tConditionVariable cv_;\n>  \tstd::atomic<bool> exit_;\n> -\tint exitCode_;\n> +\tint exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n>  \n>  \tMessageQueue messages_;\n>  };\n> @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)\n>  \t{\n>  \t\tMutexLocker locker(data_->mutex_);\n>  \n> -\t\tif (duration == utils::duration::max())\n> -\t\t\tdata_->cv_.wait(locker, [&]() { return !data_->running_; });\n> -\t\telse\n> -\t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n> -\t\t\t\t\t\t\t  [&]() { return !data_->running_; });\n> +\t\tif (duration == utils::duration::max()) {\n> +\t\t\tdata_->cv_.wait(\n> +\t\t\t\tlocker,\n> +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n> +\t\t\t\t\treturn !data_->running_;\n> +\t\t\t\t});\n> +\t\t} else {\n> +\t\t\thasFinished = data_->cv_.wait_for(\n> +\t\t\t\tlocker, duration,\n> +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n> +\t\t\t\t\treturn !data_->running_;\n> +\t\t\t\t});\n> +\t\t}\n\nLet's improve readability a bit:\n\n\t\tauto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n\t\t\treturn !data_->running_;\n\t\t});\n\n\t\tif (duration == utils::duration::max())\n\t\t\tdata_->cv_.wait(locker, isRunning);\n\t\telse\n\t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n\t\t\t\t\t\t\t  isRunning);\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \t}\n>  \n>  \tif (thread_.joinable())","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B885C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 28 Aug 2022 18:30:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D058861FC0;\n\tSun, 28 Aug 2022 20:30:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46FFE61FBD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Aug 2022 20:30:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC18FE5;\n\tSun, 28 Aug 2022 20:30:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661711417;\n\tbh=miNKRNAAtxnZPFrD4ck3nZsmYBDV6NZ1U8CHxZaIQe0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=a+l5bjucPPxfwEVXL6mSqLoamES6jBlCzWeausH6USPVXsS6fiR+2SDzcXtScj9y/\n\tzn0kcmdcRrWjeuXJPPHH7zharNudNvFAudpEmuUBgCG1b6P0Agew0BhhFupIiOwkcu\n\tLthqdfqx55H+qup3qsJoe3z8H0TVUw3C4RLoXSmLyWWQPGbN3Kmegevrc4WqCOiLly\n\tJLWcnina1HjfuI/gGgM0PAFnqJmNiVq6AxWirK1q9ZTOHyA9eChVoSAwtb5VpcrDq6\n\tzRSikF8mS/Du5Bd4vvGbmQdycUwSXYvYRvHjucxhDDGHGfbDlOH2Y3ZIW2muSdihfy\n\t3oNMc3BcWJHFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661711416;\n\tbh=miNKRNAAtxnZPFrD4ck3nZsmYBDV6NZ1U8CHxZaIQe0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lA0bAbMpQgeZ+YOKQMOWL4QC6kcGXo8ibY8WYPyhdr6ZdWjyvpE5RzIpHCelIstL5\n\t2FH/JohcnWBGW+P27yKk4QG50so2TemPVU5NzfHBOS6L+3Y0CgcQCUuui4YDhqt7l3\n\tH11qbe+GlMZm/fy2bEOCac7n0eC97vXpkH5DkiFw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lA0bAbMp\"; dkim-atps=neutral","Date":"Sun, 28 Aug 2022 21:30:08 +0300","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<Ywu0MJShW/QYxcZb@pendragon.ideasonboard.com>","References":"<20220620165027.549085-1-umang.jain@ideasonboard.com>\n\t<20220620165027.549085-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220620165027.549085-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24810,"web_url":"https://patchwork.libcamera.org/comment/24810/","msgid":"<Ywu/hwwJmv4EEhfE@pendragon.ideasonboard.com>","date":"2022-08-28T19:18:31","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Aug 28, 2022 at 09:30:08PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Umang and Hiro,\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:\n> > From: Hirokazu Honda <hiroh@chromium.org>\n> > \n> > This annotates member variables of ThreadData by clang thread\n> > safety annotations.\n> > \n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > ---\n> >  src/libcamera/base/thread.cpp | 22 +++++++++++++++-------\n> >  1 file changed, 15 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index 6bda9d14..2e26b83c 100644\n> > --- a/src/libcamera/base/thread.cpp\n> > +++ b/src/libcamera/base/thread.cpp\n> > @@ -151,7 +151,7 @@ private:\n> >  \tfriend class ThreadMain;\n> >  \n> >  \tThread *thread_;\n> > -\tbool running_;\n> > +\tbool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n> >  \tpid_t tid_;\n> >  \n> >  \tMutex mutex_;\n> > @@ -160,7 +160,7 @@ private:\n> >  \n> >  \tConditionVariable cv_;\n> >  \tstd::atomic<bool> exit_;\n> > -\tint exitCode_;\n> > +\tint exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n> >  \n> >  \tMessageQueue messages_;\n> >  };\n> > @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)\n> >  \t{\n> >  \t\tMutexLocker locker(data_->mutex_);\n> >  \n> > -\t\tif (duration == utils::duration::max())\n> > -\t\t\tdata_->cv_.wait(locker, [&]() { return !data_->running_; });\n> > -\t\telse\n> > -\t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n> > -\t\t\t\t\t\t\t  [&]() { return !data_->running_; });\n> > +\t\tif (duration == utils::duration::max()) {\n> > +\t\t\tdata_->cv_.wait(\n> > +\t\t\t\tlocker,\n> > +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n> > +\t\t\t\t\treturn !data_->running_;\n> > +\t\t\t\t});\n> > +\t\t} else {\n> > +\t\t\thasFinished = data_->cv_.wait_for(\n> > +\t\t\t\tlocker, duration,\n> > +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n> > +\t\t\t\t\treturn !data_->running_;\n> > +\t\t\t\t});\n> > +\t\t}\n> \n> Let's improve readability a bit:\n> \n> \t\tauto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n> \t\t\treturn !data_->running_;\n> \t\t});\n> \n> \t\tif (duration == utils::duration::max())\n> \t\t\tdata_->cv_.wait(locker, isRunning);\n> \t\telse\n> \t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n> \t\t\t\t\t\t\t  isRunning);\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nActually... Have you tried compiling this series with clang ? :-)\n\n../../src/libcamera/base/thread.cpp:395:9: error: writing variable 'exitCode_' requires holding mutex 'data_->mutex_' exclusively [-Werror,-Wthread-safety-analysis]\n        data_->exitCode_ = code;\n               ^\n1 error generated.\n\nThe most simple fix is to take the lock in Thread::exit(). It however\ngoes against the design goal of not requiring locks for exit(), as shown\nby the exit_ variable being an atomic. I believe the current\nimplementation is safe, although it would probably be worth it\nrevisiting the code to double-check that all necessary memory barriers\nare in place. You could thus drop the LIBCAMERA_TSA_GUARDED_BY\nannotation for exitCode_.\n\nThere's also an error in camera_manager.cpp:\n\n../../src/libcamera/camera_manager.cpp:182:2: error: reading variable 'cameras_' requires holding mutex 'mutex_' [-Werror,-Wthread-safety-analysis]\n        cameras_.clear();\n        ^\n1 error generated.\n\nThis shuld just be a matter of taking the lock in the CameraManager\ndestructor in patch 3/5.\n\n> >  \t}\n> >  \n> >  \tif (thread_.joinable())","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E9E23C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 28 Aug 2022 19:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17ACA61FC0;\n\tSun, 28 Aug 2022 21:18:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 090F161FBD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Aug 2022 21:18:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E86CE5;\n\tSun, 28 Aug 2022 21:18:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661714323;\n\tbh=Olk/CdRPA4iGjGchvKjVo6HwOLPffMNfgYszUnFkLII=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Bdu/ZDY0BT7hEde5u669XGLP7qZxrnftvcHpxK6UpVzmNG5jwGukyk0PzsRDzyxSZ\n\tuKMaI3DSGCUGwB5rUgpSBb/dSyhrVSx1FsPGG8AsAYUz/ono2VBvQECD9Gb/baCNrj\n\tKkmiWnFrnyMO+aUl7KpQYZclhBlnjXrIcLNOFSD1BLNACB41sIMqNTFzzRCSYt7Lm3\n\t34c9ZGcLvvJ8SGShkHwSz0vCEyaYVWetLzlRone1dIJrjPEN+CKrqV3tnzxOaFKBej\n\tBUH3hgqEaORlKhnkkb9+W2cgxQ52TBOE3pUD7+gY0jR66Up5oRZlZMlzTAHQP7QBXh\n\tGmJiiddXM4V4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661714320;\n\tbh=Olk/CdRPA4iGjGchvKjVo6HwOLPffMNfgYszUnFkLII=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=l3nx1MBCatmYzisATQAKPuwIawv0/l0UUJ+eu6lYWFTKV0HZXZX5gcRfVliuEczQ0\n\tdzpeVLKKxB8KL12rnbCAam86asEbv7ZlNeHK4diTVSEgjkhEZKBtOXx7LoAymP2+BZ\n\tyK4ho1M6dBisGY3gaI0JVVol50eIlH4xp4TyELjU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"l3nx1MBC\"; dkim-atps=neutral","Date":"Sun, 28 Aug 2022 22:18:31 +0300","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<Ywu/hwwJmv4EEhfE@pendragon.ideasonboard.com>","References":"<20220620165027.549085-1-umang.jain@ideasonboard.com>\n\t<20220620165027.549085-3-umang.jain@ideasonboard.com>\n\t<Ywu0MJShW/QYxcZb@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<Ywu0MJShW/QYxcZb@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25732,"web_url":"https://patchwork.libcamera.org/comment/25732/","msgid":"<7d2a5e16-dfd6-6453-62a5-05a802119ac2@ideasonboard.com>","date":"2022-11-03T12:38:07","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/29/22 12:48 AM, Laurent Pinchart wrote:\n> On Sun, Aug 28, 2022 at 09:30:08PM +0300, Laurent Pinchart via libcamera-devel wrote:\n>> Hi Umang and Hiro,\n>>\n>> Thank you for the patch.\n>>\n>> On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:\n>>> From: Hirokazu Honda <hiroh@chromium.org>\n>>>\n>>> This annotates member variables of ThreadData by clang thread\n>>> safety annotations.\n>>>\n>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>> ---\n>>>   src/libcamera/base/thread.cpp | 22 +++++++++++++++-------\n>>>   1 file changed, 15 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n>>> index 6bda9d14..2e26b83c 100644\n>>> --- a/src/libcamera/base/thread.cpp\n>>> +++ b/src/libcamera/base/thread.cpp\n>>> @@ -151,7 +151,7 @@ private:\n>>>   \tfriend class ThreadMain;\n>>>   \n>>>   \tThread *thread_;\n>>> -\tbool running_;\n>>> +\tbool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n>>>   \tpid_t tid_;\n>>>   \n>>>   \tMutex mutex_;\n>>> @@ -160,7 +160,7 @@ private:\n>>>   \n>>>   \tConditionVariable cv_;\n>>>   \tstd::atomic<bool> exit_;\n>>> -\tint exitCode_;\n>>> +\tint exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);\n>>>   \n>>>   \tMessageQueue messages_;\n>>>   };\n>>> @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)\n>>>   \t{\n>>>   \t\tMutexLocker locker(data_->mutex_);\n>>>   \n>>> -\t\tif (duration == utils::duration::max())\n>>> -\t\t\tdata_->cv_.wait(locker, [&]() { return !data_->running_; });\n>>> -\t\telse\n>>> -\t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n>>> -\t\t\t\t\t\t\t  [&]() { return !data_->running_; });\n>>> +\t\tif (duration == utils::duration::max()) {\n>>> +\t\t\tdata_->cv_.wait(\n>>> +\t\t\t\tlocker,\n>>> +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n>>> +\t\t\t\t\treturn !data_->running_;\n>>> +\t\t\t\t});\n>>> +\t\t} else {\n>>> +\t\t\thasFinished = data_->cv_.wait_for(\n>>> +\t\t\t\tlocker, duration,\n>>> +\t\t\t\t[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n>>> +\t\t\t\t\treturn !data_->running_;\n>>> +\t\t\t\t});\n>>> +\t\t}\n>> Let's improve readability a bit:\n>>\n>> \t\tauto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {\n>> \t\t\treturn !data_->running_;\n>> \t\t});\n>>\n>> \t\tif (duration == utils::duration::max())\n>> \t\t\tdata_->cv_.wait(locker, isRunning);\n>> \t\telse\n>> \t\t\thasFinished = data_->cv_.wait_for(locker, duration,\n>> \t\t\t\t\t\t\t  isRunning);\n>>\n>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Actually... Have you tried compiling this series with clang ? :-)\n\noops, seems I missed :(\n>\n> ../../src/libcamera/base/thread.cpp:395:9: error: writing variable 'exitCode_' requires holding mutex 'data_->mutex_' exclusively [-Werror,-Wthread-safety-analysis]\n>          data_->exitCode_ = code;\n>                 ^\n> 1 error generated.\n>\n> The most simple fix is to take the lock in Thread::exit(). It however\n> goes against the design goal of not requiring locks for exit(), as shown\n> by the exit_ variable being an atomic. I believe the current\n> implementation is safe, although it would probably be worth it\n> revisiting the code to double-check that all necessary memory barriers\n> are in place. You could thus drop the LIBCAMERA_TSA_GUARDED_BY\n> annotation for exitCode_.\n\nAck.\n>\n> There's also an error in camera_manager.cpp:\n>\n> ../../src/libcamera/camera_manager.cpp:182:2: error: reading variable 'cameras_' requires holding mutex 'mutex_' [-Werror,-Wthread-safety-analysis]\n>          cameras_.clear();\n>          ^\n> 1 error generated.\n>\n> This shuld just be a matter of taking the lock in the CameraManager\n> destructor in patch 3/5.\n\nAcutally cleanup() is not called in the destructor path but in \nCameraManager::Private::run() , so we need to take a lock here (taking a \nlock in CameraManager::Private::run() didn't seem fine).\n\n-       cameras_.clear();\n+       {\n+               MutexLocker locker(mutex_);\n+               cameras_.clear();\n+       }\n+\n\nPosting a new v3 to address these changes.\n>\n>>>   \t}\n>>>   \n>>>   \tif (thread_.joinable())","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7EDABDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  3 Nov 2022 12:38:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26F4C63036;\n\tThu,  3 Nov 2022 13:38:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B930361F42\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  3 Nov 2022 13:38:13 +0100 (CET)","from [192.168.1.108] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65F78589;\n\tThu,  3 Nov 2022 13:38:12 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667479096;\n\tbh=NwufgxoPkr6/zb8vxhzJiiP+2FPuEuoxVu58jq93vn0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=2hSb4frfOOJnbsaV6atotZzC51FMoLYMupg5yywFpSV5DNJmaUIbayCxKHoM5UC9o\n\tyYmnUp0m1oqLBpUXmP38F/5uXa9d/SRQBX2+SajVEByJeJizoPqKB4xMf2pIExv6Yx\n\t54+i/pSMJvbYDUithVICPJhmzIshIGhU2+OzjdjTYCcxUopjtD/bWDi7SZJ+HX1lzN\n\tSe6SjtdXYcr97DAr2PO9+1Ed8jQuuJgVLt5EJm38gGX27gWOtnZKpICijm4KXOQUGa\n\tNm9Hqzz24g7hPpb8xSTKqFieMH1dXt9K4C5oRVC+kYtpd8XpLpARYYwelz9bpifwty\n\t7XCtSoTyWzYWQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667479093;\n\tbh=NwufgxoPkr6/zb8vxhzJiiP+2FPuEuoxVu58jq93vn0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ZW0C9v51ZlDDHW9OSlRbPAZvo/egDD+ii4lTNfhVkRT5vjY/vsBoVjv/OeJ7DtQUL\n\tJYMaEwL5ytuKcPfinvTpfhjkNpn9DLsCsJEhG7dx9uSDyYu0ayZHF5ks1IYGWsIp3h\n\te8Oon9YYz3r/uGoQ1RUVzJiWXJv2yry9BbqWR7Ho="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZW0C9v51\"; dkim-atps=neutral","Message-ID":"<7d2a5e16-dfd6-6453-62a5-05a802119ac2@ideasonboard.com>","Date":"Thu, 3 Nov 2022 18:08:07 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","Content-Language":"en-US","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220620165027.549085-1-umang.jain@ideasonboard.com>\n\t<20220620165027.549085-3-umang.jain@ideasonboard.com>\n\t<Ywu0MJShW/QYxcZb@pendragon.ideasonboard.com>\n\t<Ywu/hwwJmv4EEhfE@pendragon.ideasonboard.com>","In-Reply-To":"<Ywu/hwwJmv4EEhfE@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: base: thread: Apply\n\tclang thread safety annotation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Umang Jain via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Umang Jain <umang.jain@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]