From patchwork Fri Jul 20 18:14:38 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 1222101 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 4175B3FD4F for ; Fri, 20 Jul 2012 18:14:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753291Ab2GTSOm (ORCPT ); Fri, 20 Jul 2012 14:14:42 -0400 Received: from wolverine01.qualcomm.com ([199.106.114.254]:11666 "EHLO wolverine01.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752591Ab2GTSOk (ORCPT ); Fri, 20 Jul 2012 14:14:40 -0400 X-IronPort-AV: E=McAfee;i="5400,1158,6778"; a="212825183" Received: from pdmz-ns-mip.qualcomm.com (HELO mostmsg01.qualcomm.com) ([199.106.114.10]) by wolverine01.qualcomm.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 20 Jul 2012 11:14:41 -0700 Received: from sboyd-linux.qualcomm.com (pdmz-ns-snip_218_1.qualcomm.com [192.168.218.1]) by mostmsg01.qualcomm.com (Postfix) with ESMTPA id 1B9FD10004AC; Fri, 20 Jul 2012 11:14:40 -0700 (PDT) From: Stephen Boyd To: "Rafael J . Wysocki" Cc: linux-kernel@vger.kernel.org, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org Subject: [PATCHv2] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch Date: Fri, 20 Jul 2012 11:14:38 -0700 Message-Id: <1342808078-5317-1-git-send-email-sboyd@codeaurora.org> X-Mailer: git-send-email 1.7.11.2.249.g31c7954 In-Reply-To: <201207201205.36635.rjw@sisk.pl> Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org Running one program that continuously hotplugs and replugs a cpu concurrently with another program that continuously writes to the scaling_setspeed node eventually deadlocks with: ============================================= [ INFO: possible recursive locking detected ] 3.4.0 #37 Tainted: G W --------------------------------------------- filemonkey/122 is trying to acquire lock: (s_active#13){++++.+}, at: [] sysfs_remove_dir+0x9c/0xb4 but task is already holding lock: (s_active#13){++++.+}, at: [] sysfs_write_file+0xe8/0x140 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(s_active#13); lock(s_active#13); *** DEADLOCK *** May be due to missing lock nesting notation 2 locks held by filemonkey/122: #0: (&buffer->mutex){+.+.+.}, at: [] sysfs_write_file+0x28/0x140 #1: (s_active#13){++++.+}, at: [] sysfs_write_file+0xe8/0x140 stack backtrace: [] (unwind_backtrace+0x0/0x120) from [] (validate_chain+0x6f8/0x1054) [] (validate_chain+0x6f8/0x1054) from [] (__lock_acquire+0x81c/0x8d8) [] (__lock_acquire+0x81c/0x8d8) from [] (lock_acquire+0x18c/0x1e8) [] (lock_acquire+0x18c/0x1e8) from [] (sysfs_addrm_finish+0xd0/0x180) [] (sysfs_addrm_finish+0xd0/0x180) from [] (sysfs_remove_dir+0x9c/0xb4) [] (sysfs_remove_dir+0x9c/0xb4) from [] (kobject_del+0x10/0x38) [] (kobject_del+0x10/0x38) from [] (kobject_release+0xf0/0x194) [] (kobject_release+0xf0/0x194) from [] (cpufreq_cpu_put+0xc/0x24) [] (cpufreq_cpu_put+0xc/0x24) from [] (store+0x6c/0x74) [] (store+0x6c/0x74) from [] (sysfs_write_file+0x10c/0x140) [] (sysfs_write_file+0x10c/0x140) from [] (vfs_write+0xb0/0x128) [] (vfs_write+0xb0/0x128) from [] (sys_write+0x3c/0x68) [] (sys_write+0x3c/0x68) from [] (ret_fast_syscall+0x0/0x3c) This is because store() in cpufreq.c indirectly calls kobject_get() via cpufreq_cpu_get() and is the last one to call kobject_put() via cpufreq_cpu_put(). Sysfs code should not call kobject_get() or kobject_put() directly (see the comment around sysfs_schedule_callback() for more information). Fix this deadlock by introducing two new functions: struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu) void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data) which do the same thing as cpufreq_cpu_{get,put}() but don't call kobject functions. To easily trigger this deadlock you can insert an msleep() with a reasonably large value right after the fail label at the bottom of the store() function in cpufreq.c and then write scaling_setspeed in one task and offline the cpu in another. The first task will hang and be detected by the hung task detector. Signed-off-by: Stephen Boyd --- Changes from v1: - switch sysfs arg to bool - remove patch from commit text drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 7f2f149..fb8a527 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -138,7 +138,7 @@ void disable_cpufreq(void) static LIST_HEAD(cpufreq_governor_list); static DEFINE_MUTEX(cpufreq_governor_mutex); -struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) +static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs) { struct cpufreq_policy *data; unsigned long flags; @@ -162,7 +162,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) if (!data) goto err_out_put_module; - if (!kobject_get(&data->kobj)) + if (!sysfs && !kobject_get(&data->kobj)) goto err_out_put_module; spin_unlock_irqrestore(&cpufreq_driver_lock, flags); @@ -175,16 +175,35 @@ err_out_unlock: err_out: return NULL; } + +struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) +{ + return __cpufreq_cpu_get(cpu, false); +} EXPORT_SYMBOL_GPL(cpufreq_cpu_get); +static struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu) +{ + return __cpufreq_cpu_get(cpu, true); +} -void cpufreq_cpu_put(struct cpufreq_policy *data) +static void __cpufreq_cpu_put(struct cpufreq_policy *data, bool sysfs) { - kobject_put(&data->kobj); + if (!sysfs) + kobject_put(&data->kobj); module_put(cpufreq_driver->owner); } + +void cpufreq_cpu_put(struct cpufreq_policy *data) +{ + __cpufreq_cpu_put(data, false); +} EXPORT_SYMBOL_GPL(cpufreq_cpu_put); +static void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data) +{ + __cpufreq_cpu_put(data, true); +} /********************************************************************* * EXTERNALLY AFFECTING FREQUENCY CHANGES * @@ -617,7 +636,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf) struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); ssize_t ret = -EINVAL; - policy = cpufreq_cpu_get(policy->cpu); + policy = cpufreq_cpu_get_sysfs(policy->cpu); if (!policy) goto no_policy; @@ -631,7 +650,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf) unlock_policy_rwsem_read(policy->cpu); fail: - cpufreq_cpu_put(policy); + cpufreq_cpu_put_sysfs(policy); no_policy: return ret; } @@ -642,7 +661,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); ssize_t ret = -EINVAL; - policy = cpufreq_cpu_get(policy->cpu); + policy = cpufreq_cpu_get_sysfs(policy->cpu); if (!policy) goto no_policy; @@ -656,7 +675,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, unlock_policy_rwsem_write(policy->cpu); fail: - cpufreq_cpu_put(policy); + cpufreq_cpu_put_sysfs(policy); no_policy: return ret; }