From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755264AbdKBIbK (ORCPT ); Thu, 2 Nov 2017 04:31:10 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:46052 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751578AbdKBIbF (ORCPT ); Thu, 2 Nov 2017 04:31:05 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20171102083103euoutp025d27ceaabd880dfe433acc42008338fe~zN6C2EQfO2722227222euoutp02k X-AuditID: cbfec7f5-f79d06d0000031c7-3a-59fad7c6ca6b Subject: Re: [PATCH 2/2] media: s5p-mfc: fix lock confection - request_firmware() once and keep state To: Shuah Khan , kyungmin.park@samsung.com, kamil@wypas.org, jtp.park@samsung.com, mchehab@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org From: Andrzej Hajda Message-id: Date: Thu, 02 Nov 2017 09:31:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset="utf-8" Content-transfer-encoding: 7bit Content-language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHebezc46jk6ep+WCSMYgw8AYWB28YCe2bRV/ULzryoEvn5Rwn KkTTct6ihSnqMspKq1Ep81rE0ilueM3MzEQhlTBrIaml4aVtZ4Lffs/zf/7v8/5fXlIsM0r8 SFVWHstlKTPluBTrGtwaDxqc/pcYujR0ghl4OSViGmxlEma05CfBmBY/SZjJN404c6utU8K0 mLZFTO36KhZLKkzGClzR/uS6okM/RyhudxiRYs10XPFxoJi4iCdJo1LZTFU+y4XEpEjTdTdn sZwvEQWNzyoJLdIHVSIPEuhweFVyHxf4KLyfb3WwlJTRzQjGm7oJoVhD8KvVjPYdtm0jJggt CEoe1Lst3xDU9c0RzikvOg2azB9ETsGb1iL4WmoVOwUxzcKS/ZFrIU4Hwk77jIspOgYWNsok Tsbok1DVvODq+9AJYO1/ioSZI7B5dx5zsgedAnMTP3DhzEBYXq/GBA6A9hd29y5fuFE647oq 0Is4DOqbxEKGOOgxG93sBSvWDkJgf6go7xMJhioEv/U2QihqEOyu1rsdkdBvnZAIKw5DdVed o086+hSU62TCiAK0uzvuBzsHtpo290suILAP9xF3UIDhQCLDgRSGAykMB1I8RJgRebMaXp3G 8meCeaWa12SlBV/JVpuQ4wcN71o3elDzYIQF0SSSH6J6T/1LlEmU+Xyh2oKAFMu9qRWbo0Wl KguLWC47mdNksrwFHSMxuS8VnaRLlNFpyjw2g2VzWG5fFZEeflrk6Xk53lPRuMdUduKX2HB8 q2ejPnTPzOZwU1szqvXe8362Jdvm1T+pKvPjPG3kaOF0dPfYcuzQWSouN6zoMzVSU6DK1VgT nkfN+ms9+AbvSDk1YrqW8W5xUh0Sr/rrM2rXcbUNiAsZsyQHqC8UgfJeT/pr4q1MVxdUUfxd jvHpyrDTYo5X/gePdW48PQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrNLMWRmVeSWpSXmKPExsVy+t/xq7pHr/+KNNj30dDiyNqrTBYzT7Sz WpxtesNusenxNVaLy7vmsFn0bNjKarFs0x8mi6lfPrA4cHhsWtXJ5rF5Sb3Hlv677B59W1Yx enzeJOdx5UgjewBbFJdNSmpOZllqkb5dAldGW8ttloJbVhVzVnSxNzD263YxcnJICJhInPiz igXCFpO4cG89WxcjF4eQwBJGiRMHV7CCJIQEnjFK3JweBmILC6RLLNx3iQmkSESggVHiwvMF 7CAJZoFUiRkfH7NDdD9ilGg/3gU2lk1AU+Lv5ptsIDavgJ3Eo6/tYFNZBFQlupc+AouLCkRI PG9+zwpRIyjxY/I9sF5OgQSJYyuWAdkcQAvUJaZMyYXYJS+xec1bZghbXKK59SbLBEbBWUi6 ZyF0zELSMQtJxwJGllWMIqmlxbnpucWGesWJucWleel6yfm5mxiBUbLt2M/NOxgvbQw+xCjA wajEw3tA/VekEGtiWXFl7iFGCQ5mJRHeVyeAQrwpiZVVqUX58UWlOanFhxilOViUxHl796yO FBJITyxJzU5NLUgtgskycXBKNTDO554xp//zDK8liQvnfLguzXWfb3nJ62+pxz3+7f6qHX/0 bf7s4Ht3jX0+1M2ftKvxmeOBBRo3p5e//1XAcPOBy7SF33+7m8x581bhrE69+PyPbCr6tqwp 83w5Xm1+v/nn+9OJoiuW/bhnsj7tJVNK9VPTi83aCWb5uz752b29HqPx71PFliMHlJRYijMS DbWYi4oTAX+VT5OOAgAA X-CMS-MailID: 20171102083102eucas1p24774907ce25f5151f4686026b876a3f0 X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20171006213016epcas3p20e34abea60ca43f7c3f79a68fc7a38d7 X-RootMTR: 20171006213016epcas3p20e34abea60ca43f7c3f79a68fc7a38d7 References: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06.10.2017 23:30, Shuah Khan wrote: > Driver calls request_firmware() whenever the device is opened for the > first time. As the device gets opened and closed, dev->num_inst == 1 > is true several times. This is not necessary since the firmware is saved > in the fw_buf. s5p_mfc_load_firmware() copies the buffer returned by > the request_firmware() to dev->fw_buf. > > fw_buf sticks around until it gets released from s5p_mfc_remove(), hence > there is no need to keep requesting firmware and copying it to fw_buf. > > This might have been overlooked when changes are made to free fw_buf from > the device release interface s5p_mfc_release(). > > Fix s5p_mfc_load_firmware() to call request_firmware() once and keep state. > Change _probe() to load firmware once fw_buf has been allocated. > > s5p_mfc_open() and it continues to call s5p_mfc_load_firmware() and init > hardware which is the step where firmware is written to the device. > > This addresses the mfc_mutex contention due to repeated request_firmware() > calls from open() in the following circular locking warning: > > [ 552.194115] qtdemux0:sink/2710 is trying to acquire lock: > [ 552.199488] (&dev->mfc_mutex){+.+.}, at: [] s5p_mfc_mmap+0x28/0xd4 [s5p_mfc] > [ 552.207459] > but task is already holding lock: > [ 552.213264] (&mm->mmap_sem){++++}, at: [] vm_mmap_pgoff+0x44/0xb8 > [ 552.220284] > which lock already depends on the new lock. > > [ 552.228429] > the existing dependency chain (in reverse order) is: > [ 552.235881] > -> #2 (&mm->mmap_sem){++++}: > [ 552.241259] __might_fault+0x80/0xb0 > [ 552.245331] filldir64+0xc0/0x2f8 > [ 552.249144] call_filldir+0xb0/0x14c > [ 552.253214] ext4_readdir+0x768/0x90c > [ 552.257374] iterate_dir+0x74/0x168 > [ 552.261360] SyS_getdents64+0x7c/0x1a0 > [ 552.265608] ret_fast_syscall+0x0/0x28 > [ 552.269850] > -> #1 (&type->i_mutex_dir_key#2){++++}: > [ 552.276180] down_read+0x48/0x90 > [ 552.279904] lookup_slow+0x74/0x178 > [ 552.283889] walk_component+0x1a4/0x2e4 > [ 552.288222] link_path_walk+0x174/0x4a0 > [ 552.292555] path_openat+0x68/0x944 > [ 552.296541] do_filp_open+0x60/0xc4 > [ 552.300528] file_open_name+0xe4/0x114 > [ 552.304772] filp_open+0x28/0x48 > [ 552.308499] kernel_read_file_from_path+0x30/0x78 > [ 552.313700] _request_firmware+0x3ec/0x78c > [ 552.318291] request_firmware+0x3c/0x54 > [ 552.322642] s5p_mfc_load_firmware+0x54/0x150 [s5p_mfc] > [ 552.328358] s5p_mfc_open+0x4e4/0x550 [s5p_mfc] > [ 552.333394] v4l2_open+0xa0/0x104 [videodev] > [ 552.338137] chrdev_open+0xa4/0x18c > [ 552.342121] do_dentry_open+0x208/0x310 > [ 552.346454] path_openat+0x28c/0x944 > [ 552.350526] do_filp_open+0x60/0xc4 > [ 552.354512] do_sys_open+0x118/0x1c8 > [ 552.358586] ret_fast_syscall+0x0/0x28 > [ 552.362830] > -> #0 (&dev->mfc_mutex){+.+.}: > -> #0 (&dev->mfc_mutex){+.+.}: > [ 552.368379] lock_acquire+0x6c/0x88 > [ 552.372364] __mutex_lock+0x68/0xa34 > [ 552.376437] mutex_lock_interruptible_nested+0x1c/0x24 > [ 552.382086] s5p_mfc_mmap+0x28/0xd4 [s5p_mfc] > [ 552.386939] v4l2_mmap+0x54/0x88 [videodev] > [ 552.391601] mmap_region+0x3a8/0x638 > [ 552.395673] do_mmap+0x330/0x3a4 > [ 552.399400] vm_mmap_pgoff+0x90/0xb8 > [ 552.403472] SyS_mmap_pgoff+0x90/0xc0 > [ 552.407632] ret_fast_syscall+0x0/0x28 > [ 552.411876] > other info that might help us debug this: > > [ 552.419848] Chain exists of: > &dev->mfc_mutex --> &type->i_mutex_dir_key#2 --> &mm->mmap_sem > > [ 552.431200] Possible unsafe locking scenario: > > [ 552.437092] CPU0 CPU1 > [ 552.441598] ---- ---- > [ 552.446104] lock(&mm->mmap_sem); > [ 552.449484] lock(&type->i_mutex_dir_key#2); > [ 552.456329] lock(&mm->mmap_sem); > [ 552.462222] lock(&dev->mfc_mutex); > [ 552.465775] > *** DEADLOCK *** I am not 100% but it looks like false positive. Could you describe scenario when it deadlocks? > > Signed-off-by: Shuah Khan > --- > drivers/media/platform/s5p-mfc/s5p_mfc.c | 4 ++++ > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 3 +++ > drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 5 +++++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c > index 1afde50..4c253fb 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c > @@ -1315,6 +1315,10 @@ static int s5p_mfc_probe(struct platform_device *pdev) > goto err_dma; > } > > + ret = s5p_mfc_load_firmware(dev); > + if (ret) > + mfc_err("Failed to load FW - try loading from open()\n"); > + What is the point of adding it? It will produce error log in case filesystem is not yet mounted, and as I remember it was the reason to put fw load to open callback. Regards Andrzej > mutex_init(&dev->mfc_mutex); > init_waitqueue_head(&dev->queue); > dev->hw_lock = 0; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > index 4220914..76119a8 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > @@ -290,6 +290,8 @@ struct s5p_mfc_priv_buf { > * @mfc_cmds: cmd structure holding HW commands function pointers > * @mfc_regs: structure holding MFC registers > * @fw_ver: loaded firmware sub-version > + * @fw_get_done flag set when request_firmware() is complete and > + * copied into fw_buf > * risc_on: flag indicates RISC is on or off > * > */ > @@ -336,6 +338,7 @@ struct s5p_mfc_dev { > struct s5p_mfc_hw_cmds *mfc_cmds; > const struct s5p_mfc_regs *mfc_regs; > enum s5p_mfc_fw_ver fw_ver; > + bool fw_get_done; > bool risc_on; /* indicates if RISC is on or off */ > }; > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c > index f064a0d1..ca57936 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c > @@ -55,6 +55,9 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev *dev) > * into kernel. */ > mfc_debug_enter(); > > + if (dev->fw_get_done) > + return 0; > + > if (!dev->fw_buf.virt) { > mfc_err("MFC firmware is not allocated\n"); > return -EINVAL; > @@ -82,6 +85,7 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev *dev) > } > memcpy(dev->fw_buf.virt, fw_blob->data, fw_blob->size); > wmb(); > + dev->fw_get_done = true; > release_firmware(fw_blob); > mfc_debug_leave(); > return 0; > @@ -93,6 +97,7 @@ int s5p_mfc_release_firmware(struct s5p_mfc_dev *dev) > /* Before calling this function one has to make sure > * that MFC is no longer processing */ > s5p_mfc_release_priv_buf(dev, &dev->fw_buf); > + dev->fw_get_done = false; > return 0; > } >