Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751154AbaABDPj (ORCPT ); Wed, 1 Jan 2014 22:15:39 -0500 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:21520 "EHLO mx0a-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750712AbaABDPh (ORCPT ); Wed, 1 Jan 2014 22:15:37 -0500 From: To: , CC: , , , Jane Li Subject: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled Date: Thu, 2 Jan 2014 11:14:42 +0800 Message-ID: <1388632482-16921-1-git-send-email-jiel@marvell.com> X-Mailer: git-send-email 1.7.9.5 MIME-Version: 1.0 Content-Type: text/plain X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.11.87,1.0.14,0.0.0000 definitions=2014-01-02_02:2014-01-01,2014-01-02,1970-01-01 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=7.0.1-1305240000 definitions=main-1401010220 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6161 Lines: 135 From: Jane Li When a CPU is hot removed we'll cancel all the delayed work items via gov_cancel_work(). Sometimes the delayed work function determines that it should adjust the delay for all other CPUs that the policy is managing. If this scenario occurs, the canceling CPU will cancel its own work but queue up the other CPUs works to run. Commit 3617f2(cpufreq: Fix timer/workqueue corruption due to double queueing) has tried to fix this, but reading governor_enabled is not protected by cpufreq_governor_lock. Even though od_dbs_timer() checks governor_enabled before gov_queue_work(), this scenario may occur. For example: CPU0 CPU1 ---- ---- cpu_down() ... __cpufreq_remove_dev() od_dbs_timer() __cpufreq_governor() policy->governor_enabled policy->governor_enabled = false; cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: gov_cancel_work(dbs_data, policy); cpu0 work is canceled timer is canceled cpu1 work is canceled gov_queue_work(*, *, true); cpu0 work queued cpu1 work queued cpu2 work queued ... cpu1 work is canceled cpu2 work is canceled ... At the end of the GOV_STOP case cpu0 still has a work queued to run although the code is expecting all of the works to be canceled. __cpufreq_remove_dev() will then proceed to re-initialize all the other CPUs works except for the CPU that is going down. The CPUFREQ_GOV_START case in cpufreq_governor_dbs() will trample over the queued work and debugobjects will spit out a warning: WARNING: at lib/debugobjects.c:260 debug_print_object+0x94/0xbc() ODEBUG: init active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x14 Modules linked in: CPU: 1 PID: 1205 Comm: sh Tainted: G W 3.10.0 #200 [] (unwind_backtrace+0x0/0xf8) from [] (show_stack+0x10/0x14) [] (show_stack+0x10/0x14) from [] (warn_slowpath_common+0x4c/0x68) [] (warn_slowpath_common+0x4c/0x68) from [] (warn_slowpath_fmt+0x30/0x40) [] (warn_slowpath_fmt+0x30/0x40) from [] (debug_print_object+0x94/0xbc) [] (debug_print_object+0x94/0xbc) from [] (__debug_object_init+0xc8/0x3c0) [] (__debug_object_init+0xc8/0x3c0) from [] (init_timer_key+0x20/0x104) [] (init_timer_key+0x20/0x104) from [] (cpufreq_governor_dbs+0x1dc/0x68c) [] (cpufreq_governor_dbs+0x1dc/0x68c) from [] (__cpufreq_governor+0x80/0x1b0) [] (__cpufreq_governor+0x80/0x1b0) from [] (__cpufreq_remove_dev.isra.12+0x22c/0x380) [] (__cpufreq_remove_dev.isra.12+0x22c/0x380) from [] (cpufreq_cpu_callback+0x48/0x5c) [] (cpufreq_cpu_callback+0x48/0x5c) from [] (notifier_call_chain+0x44/0x84) [] (notifier_call_chain+0x44/0x84) from [] (__cpu_notify+0x2c/0x48) [] (__cpu_notify+0x2c/0x48) from [] (_cpu_down+0x80/0x258) [] (_cpu_down+0x80/0x258) from [] (cpu_down+0x28/0x3c) [] (cpu_down+0x28/0x3c) from [] (store_online+0x30/0x74) [] (store_online+0x30/0x74) from [] (dev_attr_store+0x18/0x24) [] (dev_attr_store+0x18/0x24) from [] (sysfs_write_file+0x100/0x180) [] (sysfs_write_file+0x100/0x180) from [] (vfs_write+0xbc/0x184) [] (vfs_write+0xbc/0x184) from [] (SyS_write+0x40/0x68) [] (SyS_write+0x40/0x68) from [] (ret_fast_syscall+0x0/0x48) In gov_queue_work(), lock cpufreq_governor_lock before gov_queue_work, and unlock it after __gov_queue_work(). In this way, governor_enabled is guaranteed not changed in gov_queue_work(). Signed-off-by: Jane Li --- drivers/cpufreq/cpufreq.c | 2 +- drivers/cpufreq/cpufreq_governor.c | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 16d7b4a..185c9f5 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -39,7 +39,7 @@ static struct cpufreq_driver *cpufreq_driver; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback); static DEFINE_RWLOCK(cpufreq_driver_lock); -static DEFINE_MUTEX(cpufreq_governor_lock); +DEFINE_MUTEX(cpufreq_governor_lock); static LIST_HEAD(cpufreq_policy_list); #ifdef CONFIG_HOTPLUG_CPU diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index e6be635..485ee0d 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -22,6 +22,8 @@ #include "cpufreq_governor.h" +extern struct mutex cpufreq_governor_lock; + static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data) { if (have_governor_per_policy()) @@ -119,8 +121,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, { int i; - if (!policy->governor_enabled) + mutex_lock(&cpufreq_governor_lock); + if (!policy->governor_enabled) { + mutex_unlock(&cpufreq_governor_lock); return; + } if (!all_cpus) { /* @@ -135,6 +140,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, for_each_cpu(i, policy->cpus) __gov_queue_work(i, dbs_data, delay); } + mutex_unlock(&cpufreq_governor_lock); } EXPORT_SYMBOL_GPL(gov_queue_work); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/