From patchwork Mon Sep 8 10:56:42 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 36961 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-qa0-f70.google.com (mail-qa0-f70.google.com [209.85.216.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 76A872054D for ; Mon, 8 Sep 2014 10:56:47 +0000 (UTC) Received: by mail-qa0-f70.google.com with SMTP id j7sf41136848qaq.9 for ; Mon, 08 Sep 2014 03:56:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe:content-type; bh=sjg3YbETJ+GGnV84fAXXAd0JMEYpF2pCoJsQ3q5u100=; b=Pm2UAiOhq1PDQbB9yBcVmvffvj+Z4w5gJPw8BCOYoUYF2E5XSA+DVcCpAewDPr4Mn8 M6GswAw3zfOgXsfxd6IWB8mFZqB/+gau2Z9w/Ibhuihjrjafr5HmrynpkDxTY+4fdxZv GU63BqKFiH7H18Wqh7XE7HoUs6t/J8FpDm7fye6XQCZu+lYUu7EMxrZV1WYggGXXnhSD PwLsUkwb5T/qtt8+ZTD9Paj/RqJEFQ8YG6ej2SpIMn+GikP/RK8p6PTnjgopNR2KTgc1 5Mfr/YfpacOxNrkG4nnLtQ7UZ6gkVBN4AJwZK9jnq20g7LG03xREAsnQ39or9BjdtmqE lzew== X-Gm-Message-State: ALoCoQnqwcC3LSrwarVk9jNMWC+U+jDbN/xoa2J4c/32ah9lqHzF4LAV1fpEjSzsBOftYxz9r3cy X-Received: by 10.236.228.138 with SMTP id f10mr16960386yhq.38.1410173807147; Mon, 08 Sep 2014 03:56:47 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.94.231 with SMTP id g94ls1455748qge.74.gmail; Mon, 08 Sep 2014 03:56:47 -0700 (PDT) X-Received: by 10.52.185.168 with SMTP id fd8mr202335vdc.58.1410173807056; Mon, 08 Sep 2014 03:56:47 -0700 (PDT) Received: from mail-vc0-f170.google.com (mail-vc0-f170.google.com [209.85.220.170]) by mx.google.com with ESMTPS id mj9si3750019vcb.41.2014.09.08.03.56.47 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 08 Sep 2014 03:56:47 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.170 as permitted sender) client-ip=209.85.220.170; Received: by mail-vc0-f170.google.com with SMTP id hy4so586755vcb.29 for ; Mon, 08 Sep 2014 03:56:47 -0700 (PDT) X-Received: by 10.220.132.207 with SMTP id c15mr25108539vct.12.1410173806990; Mon, 08 Sep 2014 03:56:46 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.45.67 with SMTP id uj3csp114379vcb; Mon, 8 Sep 2014 03:56:46 -0700 (PDT) X-Received: by 10.68.139.232 with SMTP id rb8mr18964207pbb.20.1410173805670; Mon, 08 Sep 2014 03:56:45 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id k5si17171244pdn.89.2014.09.08.03.56.44 for ; Mon, 08 Sep 2014 03:56:45 -0700 (PDT) Received-SPF: none (google.com: linux-pm-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752989AbaIHK4n (ORCPT + 15 others); Mon, 8 Sep 2014 06:56:43 -0400 Received: from mail-oi0-f46.google.com ([209.85.218.46]:53933 "EHLO mail-oi0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752640AbaIHK4n (ORCPT ); Mon, 8 Sep 2014 06:56:43 -0400 Received: by mail-oi0-f46.google.com with SMTP id g201so1399927oib.33 for ; Mon, 08 Sep 2014 03:56:42 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.236.162 with SMTP id uv2mr30924973obc.12.1410173802610; Mon, 08 Sep 2014 03:56:42 -0700 (PDT) Received: by 10.182.233.170 with HTTP; Mon, 8 Sep 2014 03:56:42 -0700 (PDT) In-Reply-To: <1410164208.6159.18.camel@x200t> References: <1405522398.2348.42.camel@x200t> <1406185912.2406.3.camel@x200t> <1406277769.2399.4.camel@x200t> <1410164208.6159.18.camel@x200t> Date: Mon, 8 Sep 2014 16:26:42 +0530 Message-ID: Subject: Re: PROBLEM: Kernel OOPS and possible system freeze after concurrent writing to cpufreq/scaling_governor (Resend) From: Viresh Kumar To: =?UTF-8?Q?Robert_Sch=C3=B6ne?= Cc: "Rafael J. Wysocki" , "linux-pm@vger.kernel.org" , "Srivatsa S. Bhat" , Prarit Bhargava , Saravana Kannan , Stephen Boyd Sender: linux-pm-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: viresh.kumar@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.170 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On 8 September 2014 13:46, Robert Schöne wrote: > (Sorry for the resend, I forgot to disable my S/MIME signature) > > The patch you suggested did not work, so I introduced a new mutex in the > patch below. Okay, let me apologize first. This thread has taken much longer than I expected, to some level due to me. I have changed my job recently and have been running busy with new assignments, etc.. > I am not happy with adding just another mutex, but it fixes my problem > of changing governors concurrently. So, yeah back to the problem. Can you please try attached patch with the revert earlier suggested? To make it clear again, cherry-pick: 19c7630 and then apply this patch. Let me know if it still doesn't work for you.. I don't have a local setup to test this and so its just compile tested. Cc'd few more people who also reported similar issues. --- viresh >From 250686bfdbb183a9d798b071d32972b65d35c915 Mon Sep 17 00:00:00 2001 Message-Id: <250686bfdbb183a9d798b071d32972b65d35c915.1410173112.git.viresh.kumar@linaro.org> From: Viresh Kumar Date: Mon, 8 Sep 2014 16:01:15 +0530 Subject: [PATCH] cpufreq: Track governor-state with 'policy->governor_state' Even after serializing calls to __cpufreq_governor() there are some races left. The races are around doing the invalid operation during some state of cpufreq governors. For example, while the governor is in CPUFREQ_GOV_POLICY_EXIT state, we can't do CPUFREQ_GOV_START without first doing CPUFREQ_GOV_POLICY_INIT. All these cases weren't handled elegantly in __cpufreq_governor() and so there were enough chances that things may go wrong when governors are changed with multiple thread. This patch renames an existing field 'policy->governor_enabled' with 'policy->governor_state' which can have more values than 0 & 1 now. We maintain the current state of governors for each policy now and reject any invalid operation. Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq.c | 26 +++++++++++++------------- drivers/cpufreq/cpufreq_governor.c | 2 +- include/linux/cpufreq.h | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a7ceae3..c597361 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -935,6 +935,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) struct cpufreq_policy new_policy; int ret = 0; + policy->governor_state = CPUFREQ_GOV_POLICY_EXIT; memcpy(&new_policy, policy, sizeof(*policy)); /* Update governor of new_policy to the governor used before hotplug */ @@ -1976,7 +1977,7 @@ EXPORT_SYMBOL_GPL(cpufreq_driver_target); static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event) { - int ret; + int ret, state; /* Only must be defined when default governor is known to have latency restrictions, like e.g. conservative or ondemand. @@ -2012,19 +2013,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event); mutex_lock(&cpufreq_governor_lock); + state = policy->governor_state; + + /* Check if operation is permitted or not */ if (policy->governor_busy - || (policy->governor_enabled && event == CPUFREQ_GOV_START) - || (!policy->governor_enabled - && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) { + || (state == CPUFREQ_GOV_START && event != CPUFREQ_GOV_LIMITS && event != CPUFREQ_GOV_STOP) + || (state == CPUFREQ_GOV_STOP && event != CPUFREQ_GOV_START && event != CPUFREQ_GOV_POLICY_EXIT) + || (state == CPUFREQ_GOV_POLICY_INIT && event != CPUFREQ_GOV_START && event != CPUFREQ_GOV_POLICY_EXIT) + || (state == CPUFREQ_GOV_POLICY_EXIT && event != CPUFREQ_GOV_POLICY_INIT)) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; } policy->governor_busy = true; - if (event == CPUFREQ_GOV_STOP) - policy->governor_enabled = false; - else if (event == CPUFREQ_GOV_START) - policy->governor_enabled = true; + if (event != CPUFREQ_GOV_LIMITS) + policy->governor_state = event; mutex_unlock(&cpufreq_governor_lock); @@ -2035,13 +2038,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized++; else if (event == CPUFREQ_GOV_POLICY_EXIT) policy->governor->initialized--; - } else { + } else if (event != CPUFREQ_GOV_LIMITS) { /* Restore original values */ mutex_lock(&cpufreq_governor_lock); - if (event == CPUFREQ_GOV_STOP) - policy->governor_enabled = true; - else if (event == CPUFREQ_GOV_START) - policy->governor_enabled = false; + policy->governor_state = state; mutex_unlock(&cpufreq_governor_lock); } diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 1b44496..d173181 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -174,7 +174,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, int i; mutex_lock(&cpufreq_governor_lock); - if (!policy->governor_enabled) + if (policy->governor_state != CPUFREQ_GOV_START) goto out_unlock; if (!all_cpus) { diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index c7aa96b..39ddd3e 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -81,7 +81,7 @@ struct cpufreq_policy { unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data; - bool governor_enabled; /* governor start/stop flag */ + bool governor_state; /* Governor's current state */ bool governor_busy; struct work_struct update; /* if update_policy() needs to be -- 2.0.3.693.g996b0fd