From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH] OMAP: omap_hwmod: remove locking from hwmod_for_each iterators Date: Wed, 11 Aug 2010 11:24:04 -0700 Message-ID: <87tyn10zzv.fsf@deeprootsystems.com> References: <1281457882-30327-1-git-send-email-khilman@deeprootsystems.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pz0-f46.google.com ([209.85.210.46]:35188 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755134Ab0HKSYG (ORCPT ); Wed, 11 Aug 2010 14:24:06 -0400 Received: by pzk26 with SMTP id 26so153003pzk.19 for ; Wed, 11 Aug 2010 11:24:06 -0700 (PDT) In-Reply-To: (Paul Walmsley's message of "Tue, 10 Aug 2010 23:37:50 -0600 (MDT)") Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Paul Walmsley Cc: linux-omap@vger.kernel.org, b-cousson@ti.com, p-basak2@ti.com Paul Walmsley writes: > Hi Kevin, > > On Tue, 10 Aug 2010, Kevin Hilman wrote: > >> Remove unnecessary locking in the 'for_each' iterators. Any locking should >> be taken care of by using hwmod API functions in the functions called by the >> iterators. >> >> In addition, having locking here causes lockdep to detect possible circular >> dependencies (originally reported by Partha Basak .) >> >> For example, during init (#0 below) you have the hwmod_mutex acquired >> (hwmod_for_each_by_class()) then the dpm_list_mtx acquired >> (device_pm_add()). Later, during suspend the dpm_list_mtx is aquired >> first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired >> (omap_hwmod_idle()). >> >> [ 810.170593] ======================================================= >> [ 810.170593] [ INFO: possible circular locking dependency detected ] >> [ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34 >> [ 810.170654] ------------------------------------------------------- >> [ 810.170654] sh/670 is trying to acquire lock: >> [ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: [] omap_hwmod_idle+0x1c/0x38 >> [ 810.170745] >> [ 810.170745] but task is already holding lock: >> [ 810.170776] (dpm_list_mtx){+.+...}, at: [] dpm_suspend_noirq+0x28/0x188 >> [ 810.170806] >> [ 810.170837] which lock already depends on the new lock. >> [ 810.170837] >> [ 810.170837] >> [ 810.170837] the existing dependency chain (in reverse order) is: >> [ 810.170867] >> [ 810.170867] -> #1 (dpm_list_mtx){+.+...}: >> [ 810.170898] [] lock_acquire+0x60/0x74 >> [ 810.170959] [] mutex_lock_nested+0x58/0x2e4 >> [ 810.170989] [] device_pm_add+0x14/0xcc >> [ 810.171020] [] device_add+0x3b8/0x564 >> [ 810.171051] [] platform_device_add+0x104/0x160 >> [ 810.171112] [] omap_device_build_ss+0x14c/0x1c8 >> [ 810.171142] [] omap_device_build+0x48/0x50 >> [ 810.171173] [] omap2_init_gpio+0xf0/0x15c >> [ 810.171203] [] omap_hwmod_for_each_by_class+0x60/0xa4 >> [ 810.171264] [] do_one_initcall+0x58/0x1b4 >> [ 810.171295] [] kernel_init+0x98/0x150 >> [ 810.171325] [] kernel_thread_exit+0x0/0x8 >> [ 810.171356] >> [ 810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}: >> [ 810.171386] [] __lock_acquire+0x108c/0x1784 >> [ 810.171447] [] lock_acquire+0x60/0x74 >> [ 810.171478] [] mutex_lock_nested+0x58/0x2e4 >> [ 810.171508] [] omap_hwmod_idle+0x1c/0x38 >> [ 810.171539] [] omap_device_idle_hwmods+0x20/0x3c >> [ 810.171600] [] _omap_device_deactivate+0x58/0x14c >> [ 810.171630] [] omap_device_idle+0x4c/0x6c >> [ 810.171661] [] platform_pm_runtime_suspend+0x4c/0x74 >> [ 810.171691] [] __pm_runtime_suspend+0x204/0x34c >> [ 810.171722] [] pm_runtime_suspend+0x20/0x34 >> [ 810.171752] [] platform_pm_runtime_idle+0x8/0x10 >> [ 810.171783] [] __pm_runtime_idle+0x15c/0x198 >> [ 810.171813] [] pm_runtime_idle+0x1c/0x30 >> [ 810.171844] [] platform_pm_suspend_noirq+0x48/0x50 >> [ 810.171875] [] pm_noirq_op+0xa0/0x184 >> [ 810.171905] [] dpm_suspend_noirq+0xac/0x188 >> [ 810.171936] [] suspend_devices_and_enter+0x94/0x1d8 >> [ 810.171966] [] enter_state+0xbc/0x120 >> [ 810.171997] [] state_store+0xa4/0xb8 >> [ 810.172027] [] kobj_attr_store+0x18/0x1c >> [ 810.172088] [] sysfs_write_file+0x10c/0x144 >> [ 810.172119] [] vfs_write+0xb0/0x148 >> [ 810.172149] [] sys_write+0x3c/0x68 >> [ 810.172180] [] ret_fast_syscall+0x0/0x3c > > The intention of the mutex_lock() in the omap_hwmod_for_each*() case is to > protect against changes to omap_hwmod_list during the list iteration. It > is true that omap_hwmod_list is only likely to change very early in boot, > as the hwmods are registered, so perhaps this is not necessary in > practice. But at least theoretically it seems necessary, since I don't > think that list_for_each_entry() is safe if a list addition is in progress > simultaneously. Good point. It wasn't clear to me exactly what the mutex was trying to protect, thanks for the clarification. > On the other hand, taking the mutex during most of the other > omap_hwmod calls, such as > omap_hwmod_{enable,idle,shutdown,enable_clocks,disable_clocks,reset,enable_wakeup,disable_wakeup} > is definitely not needed since those functions do not affect > omap_hwmod_list. Agreed. > The goal of the mutex there was simply to protect > against concurrent calls to those functions. To do that, perhaps the lock > could be moved into the struct omap_hwmod? I believe you suggested this > during our PM discussions in Bengaluru. That would carry a slight memory > space penalty, but would also deserialize hwmod operations on an SMP > system. If you agree, perhaps you might spin a patch for that instead? Yes, will do. Thanks, Kevin