From patchwork Fri Dec 27 09:30:51 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: jiel@marvell.com X-Patchwork-Id: 3411521 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0A43FC02DC for ; Fri, 27 Dec 2013 09:32:08 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 04C632015E for ; Fri, 27 Dec 2013 09:32:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DC1872013A for ; Fri, 27 Dec 2013 09:32:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754159Ab3L0Jbr (ORCPT ); Fri, 27 Dec 2013 04:31:47 -0500 Received: from mx0b-0016f401.pphosted.com ([67.231.156.173]:4370 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754134Ab3L0Jbp (ORCPT ); Fri, 27 Dec 2013 04:31:45 -0500 Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.14.5/8.14.5) with SMTP id rBR9VdH9029577; Fri, 27 Dec 2013 01:31:39 -0800 Received: from sc-owa01.marvell.com ([199.233.58.136]) by mx0b-0016f401.pphosted.com with ESMTP id 1h0e8w50c9-12 (version=TLSv1/SSLv3 cipher=RC4-MD5 bits=128 verify=NOT); Fri, 27 Dec 2013 01:31:39 -0800 Received: from maili.marvell.com (10.93.76.43) by sc-owa01.marvell.com (10.93.76.21) with Microsoft SMTP Server id 8.3.327.1; Fri, 27 Dec 2013 01:31:37 -0800 Received: from localhost (unknown [10.38.36.182]) by maili.marvell.com (Postfix) with ESMTP id 5FA721CCDC0; Fri, 27 Dec 2013 01:31:37 -0800 (PST) From: To: , CC: , , , Jane Li Subject: [PATCH] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled Date: Fri, 27 Dec 2013 17:30:51 +0800 Message-ID: <1388136651-21883-1-git-send-email-jiel@marvell.com> X-Mailer: git-send-email 1.7.9.5 MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.11.87, 1.0.14, 0.0.0000 definitions=2013-12-27_01:2013-12-24, 2013-12-27, 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-1312270015 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 | 1 - drivers/cpufreq/cpufreq_governor.c | 6 +++++- include/linux/cpufreq.h | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..50601f6 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -39,7 +39,6 @@ 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); 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..11b9f0b 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -119,8 +119,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 +138,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); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index dc196bb..4faafe7 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -254,6 +254,7 @@ struct cpufreq_driver { int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); +static DEFINE_MUTEX(cpufreq_governor_lock); const char *cpufreq_get_current_driver(void);