From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751709Ab0BJGyG (ORCPT ); Wed, 10 Feb 2010 01:54:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39645 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750738Ab0BJGyB (ORCPT ); Wed, 10 Feb 2010 01:54:01 -0500 Message-ID: <4B7258AB.2000502@redhat.com> Date: Wed, 10 Feb 2010 14:56:43 +0800 From: Cong Wang User-Agent: Thunderbird 2.0.0.23 (X11/20091001) MIME-Version: 1.0 To: David Rientjes CC: linux-kernel@vger.kernel.org, Xiaotian Feng , Benjamin Herrenschmidt , Dave Jones , cpufreq@vger.kernel.org, Prarit Bhargava , Thomas Renninger , Andrew Morton , Venkatesh Pallipadi Subject: Re: [Patch] cpufreq: fix a deadlock during shutting down References: <20100209091408.29971.63921.sendpatchset@localhost.localdomain> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org David Rientjes wrote: > On Tue, 9 Feb 2010, Amerigo Wang wrote: > >> Benjamin reported that, the machine deadlocks right after printing the >> following when doing a shutdown: >> >> halt/4071 is trying to acquire lock: >> (s_active){++++.+}, at: [] .sysfs_addrm_finish+0x58/0xc0 >> >> but task is already holding lock: >> (&per_cpu(cpu_policy_rwsem, cpu)){+.+.+.}, at: [] .lock_policy_rwsem_write+0x84/0xf4 >> >> which lock already depends on the new lock. >> >> the existing dependency chain (in reverse order) is: >> >> >> >> >> This is because we are trying to kobject_put() a kobject while >> we are holding cpu policy rwsem. So just move kobject_put() >> down after releasing the rwsem. >> >> Totally untested. >> >> Reported-by: Xiaotian Feng >> Reported-by: Benjamin Herrenschmidt >> Signed-off-by: WANG Cong >> Cc: Dave Jones >> Cc: Thomas Renninger >> Cc: Prarit Bhargava >> Cc: Venkatesh Pallipadi >> >> --- >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 67bc2ec..222b35f 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1113,6 +1113,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev) >> unsigned int cpu = sys_dev->id; >> unsigned long flags; >> struct cpufreq_policy *data; >> + struct kobject *kobj; >> #ifdef CONFIG_SMP >> struct sys_device *cpu_sys_dev; >> unsigned int j; >> @@ -1192,7 +1193,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev) >> if (cpufreq_driver->target) >> __cpufreq_governor(data, CPUFREQ_GOV_STOP); >> >> - kobject_put(&data->kobj); >> + kobj = &data->kobj; >> >> /* we need to make sure that the underlying kobj is actually >> * not referenced anymore by anybody before we proceed with > > NACK. > > If you read this comment, it says: > > /* we need to make sure that the underlying kobj is actually > * not referenced anymore by anybody before we proceed with > * unloading. > */ > > That would suggest that the wait_for_completion(&data->kobj_unregister); > would never actually return if you're holding a reference to it in your > patch since it only completes when the last reference is dropped (the > ->release function is cpufreq_sysfs_release()). Oh, my bad. Then this case seems to be more complex... But anyway, this is _not_ a bogus. Thanks.