From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752579AbbJFI4I (ORCPT ); Tue, 6 Oct 2015 04:56:08 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:53279 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765AbbJFI4D (ORCPT ); Tue, 6 Oct 2015 04:56:03 -0400 X-AuditID: cbfec7f5-f794b6d000001495-ea-56138ca1c2cc Subject: Re: [PATCH v4 4/4] usb: dwc2: refactor common low-level hw code to platform.c To: Felipe Balbi , John Youn , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" References: <20151001155947.GD4469@saruman.tx.rr.com> <1443771918-19075-1-git-send-email-m.szyprowski@samsung.com> <2B3535C5ECE8B5419E3ECBE30077290901DC3876E9@US01WEMBX2.internal.synopsys.com> <874mi4gale.fsf@saruman.tx.rr.com> Cc: Robert Baldyga , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz From: Marek Szyprowski Message-id: <56138C9F.7040609@samsung.com> Date: Tue, 06 Oct 2015 10:55:59 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-version: 1.0 In-reply-to: <874mi4gale.fsf@saruman.tx.rr.com> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrDLMWRmVeSWpSXmKPExsVy+t/xa7oLe4TDDKYuZ7XYOGM9q8XB+/UW E5b+ZLF4/cLQ4vKuOWwWM87vY7JYtKyV2eLB4Z3sDhwefVtWMXps2f+Z0eP4je1MHp83yQWw RHHZpKTmZJalFunbJXBlfGg9z15w0Lxi6azpLA2Mm3S7GDk5JARMJJpbXjJC2GISF+6tZ+ti 5OIQEljKKPH48GRWkISQwHNGic+TJUBsYYEIiU2L9rKCFIkIdDFJzJvTwgzR8ZJR4vfEV2Dt zAKTGSVWn1gG1s4mYCjR9baLDcTmFdCS6P/wjxnEZhFQlViy9hxYXFQgRuL9plWMEDWCEj8m 32MBsTkF9CWarm8Am8MsYCbx5eVhKFteYvOat8wTGAVmIWmZhaRsFpKyBYzMqxhFU0uTC4qT 0nON9IoTc4tL89L1kvNzNzFCwvzrDsalx6wOMQpwMCrx8ErcFAoTYk0sK67MPcQowcGsJML7 k0s4TIg3JbGyKrUoP76oNCe1+BCjNAeLkjjvzF3vQ4QE0hNLUrNTUwtSi2CyTBycUg2MPs5W XE0h9QmrG/gfbZ11MMVfSjQ1dsM5lfk2lrPFw07sc4wN0JnNq3z8nilf9b/JcxLfVpztY9Mw s3I2njPtcaRn0Zud4hFtzz/eW6aw+nyjT7vlMycheeEZRxoWRCgyFgexmd5I5RHg//F/8sZ5 JcYnJ9xSkdQtkHg6l3HnEqV8oSz15dZKLMUZiYZazEXFiQDpFN0xbwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On 2015-10-06 01:27, Felipe Balbi wrote: > John Youn writes: > > Hi, > >> On 10/2/2015 12:45 AM, Marek Szyprowski wrote: >>> DWC2 module on some platforms needs three additional hardware >>> resources: phy controller, clock and power supply. All of them must be >>> enabled/activated to properly initialize and operate. This was initially >>> handled in s3c-hsotg driver, which has been converted to 'gadget' part >>> of dwc2 driver. Unfortunately, not all of this code got moved to common >>> platform code, what resulted in accessing DWC2 registers without >>> enabling low-level hardware resources. This fails for example on Exynos >>> SoCs. This patch moves all the code for managing those resources to >>> common platform.c file and provides convenient wrappers for controlling >>> them. >>> >>> Signed-off-by: Marek Szyprowski >>> --- >>> Changelog: >>> v4: >>> - fixed broken conditional compilation and adjusted comments in dwc2_hsotg >>> structure documentation >>> >>> v3: >>> - rebased onto latest 'testing/next' from Felipe Balbi (includes >>> s3c_hsotg -> dwc2 rename) >>> >>> v2: >>> - moved setting of ll_hw_enabled flag to enable/disable functions, >>> as suggested by John Youn >>> - moved setting of phy width to dwc2_lowlevel_init function >>> --- >>> drivers/usb/dwc2/core.h | 24 +++-- >>> drivers/usb/dwc2/gadget.c | 193 ++++-------------------------------- >>> drivers/usb/dwc2/platform.c | 234 +++++++++++++++++++++++++++++++++++++------- >>> 3 files changed, 228 insertions(+), 223 deletions(-) >>> >> Hi Marek, >> >> I still see lockdep warnings. >> >> Any ideas about these? >> >> >> [ 1618.179611] ====================================================== >> [ 1618.179612] [ INFO: possible circular locking dependency detected ] >> [ 1618.179613] 4.3.0-rc3-snps-00125-g744fd93 #28 Not tainted >> [ 1618.179614] ------------------------------------------------------- >> [ 1618.179615] modprobe/2658 is trying to acquire lock: >> [ 1618.179616] (&hsotg->init_mutex){+.+.+.}, at: [] dwc2_hsotg_udc_start+0x5c/0x200 [dwc2] >> [ 1618.179622] >> [ 1618.179622] but task is already holding lock: >> [ 1618.179623] (udc_lock){+.+.+.}, at: [] usb_gadget_probe_driver+0x3a/0xd0 [udc_core] >> [ 1618.179627] >> [ 1618.179627] which lock already depends on the new lock. >> [ 1618.179627] >> [ 1618.179628] >> [ 1618.179628] the existing dependency chain (in reverse order) is: >> [ 1618.179629] >> [ 1618.179629] -> #1 (udc_lock){+.+.+.}: >> [ 1618.179631] [] lock_acquire+0xdd/0x1f0 >> [ 1618.179635] [] mutex_lock_nested+0x76/0x3e0 >> [ 1618.179638] [] usb_add_gadget_udc_release+0x187/0x240 [udc_core] >> [ 1618.179640] [] usb_add_gadget_udc+0x10/0x20 [udc_core] >> [ 1618.179642] [] dwc2_gadget_init+0x47c/0x580 [dwc2] >> [ 1618.179645] [] dwc2_driver_probe+0x422/0x4b0 [dwc2] >> [ 1618.179648] [] platform_drv_probe+0x34/0x90 >> [ 1618.179650] [] driver_probe_device+0x224/0x480 >> [ 1618.179652] [] __device_attach_driver+0x71/0xa0 >> [ 1618.179654] [] bus_for_each_drv+0x5d/0x90 >> [ 1618.179655] [] __device_attach+0xbf/0x140 >> [ 1618.179657] [] device_initial_probe+0x13/0x20 >> [ 1618.179658] [] bus_probe_device+0xa3/0xb0 >> [ 1618.179660] [] device_add+0x40d/0x690 >> [ 1618.179661] [] platform_device_add+0x111/0x270 >> [ 1618.179663] [] dwc2_pci_probe+0xe8/0x1d2 [dwc2_pci] >> [ 1618.179665] [] local_pci_probe+0x45/0xa0 >> [ 1618.179668] [] pci_device_probe+0xe1/0x130 >> [ 1618.179669] [] driver_probe_device+0x224/0x480 >> [ 1618.179671] [] __driver_attach+0x88/0x90 >> [ 1618.179672] [] bus_for_each_dev+0x66/0xa0 >> [ 1618.179674] [] driver_attach+0x1e/0x20 >> [ 1618.179675] [] bus_add_driver+0x1ee/0x280 >> [ 1618.179677] [] driver_register+0x60/0xe0 >> [ 1618.179678] [] __pci_register_driver+0x60/0x70 >> [ 1618.179680] [] 0xffffffffc000601e >> [ 1618.179681] [] do_one_initcall+0xb3/0x200 >> [ 1618.179684] [] do_init_module+0x5f/0x1e7 >> [ 1618.179687] [] load_module+0x21a8/0x2840 >> [ 1618.179689] [] SyS_finit_module+0x9a/0xc0 >> [ 1618.179691] [] entry_SYSCALL_64_fastpath+0x16/0x7a >> [ 1618.179693] >> [ 1618.179693] -> #0 (&hsotg->init_mutex){+.+.+.}: >> [ 1618.179695] [] __lock_acquire+0x1d35/0x1db0 >> [ 1618.179697] [] lock_acquire+0xdd/0x1f0 >> [ 1618.179698] [] mutex_lock_nested+0x76/0x3e0 >> [ 1618.179700] [] dwc2_hsotg_udc_start+0x5c/0x200 [dwc2] >> [ 1618.179703] [] udc_bind_to_driver+0xa4/0x100 [udc_core] >> [ 1618.179705] [] usb_gadget_probe_driver+0x7a/0xd0 [udc_core] >> [ 1618.179707] [] usb_composite_probe+0xa4/0xc0 [libcomposite] >> [ 1618.179709] [] msg_init+0x10/0x1000 [g_mass_storage] >> [ 1618.179711] [] do_one_initcall+0xb3/0x200 >> [ 1618.179713] [] do_init_module+0x5f/0x1e7 >> [ 1618.179714] [] load_module+0x21a8/0x2840 >> [ 1618.179716] [] SyS_finit_module+0x9a/0xc0 >> [ 1618.179717] [] entry_SYSCALL_64_fastpath+0x16/0x7a >> [ 1618.179719] >> [ 1618.179719] other info that might help us debug this: >> [ 1618.179719] >> [ 1618.179720] Possible unsafe locking scenario: >> [ 1618.179720] >> [ 1618.179721] CPU0 CPU1 >> [ 1618.179722] ---- ---- >> [ 1618.179722] lock(udc_lock); >> [ 1618.179723] lock(&hsotg->init_mutex); > It seems like init_mutex is completely unnecessary in this driver. In > fact, why are you trying to hold a mutex while inside a spinlock ? init_mutex is a leftover from the time, when s3c-hsotg driver didn't implement proper pull up/down control and emulated it by enabling disabling phy. It can be removed now. btw, the possible lockup pointed by John is an interaction of two mutexes, not a case of taking mutex under a spinlock. I will send an updated patch in a few minutes. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland