From patchwork Fri Mar 14 07:43:44 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 26246 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ig0-f200.google.com (mail-ig0-f200.google.com [209.85.213.200]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id AD01A203AC for ; Fri, 14 Mar 2014 07:44:23 +0000 (UTC) Received: by mail-ig0-f200.google.com with SMTP id h18sf7538126igc.3 for ; Fri, 14 Mar 2014 00:44:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:delivered-to:from:to:cc:subject :date:message-id:sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe; bh=gggSYcYTimv1jvPduyJ5+9wN0VZsO8RkI4TOkh8b83A=; b=Tkp6eF/d2TQNYXuqzPU/W9cHDa7nnBJJReUIaNVuSM92PZSDvx1WeYI0aZYcSvjYO1 z+zjxOfLo+PHq1rlxhQO7N0C0PLOY5UKxAGjW5BNiW0nyXfmZ5BWFfhDIbxFUGaZ6Xbg JrJ2oJ+pHMDHIYZyJNbVD2ki784LbJA8V7mOQf9Fuyw8XzBaWbQRF6SCIXfW+za2kOVM 1JHWG6+NZxnhZhXSKjptgZ1XBGEjNlqKFNinX/2q31WN1Rjy+C1cphoANeQ17tV5Jvhl l/Sk8p4yWW2UdqUHMJg+xxCuaxmsJuWpOIX7EaH1Dv9DmEOOhH4yfCkefYEPhgMldHvt DJBw== X-Gm-Message-State: ALoCoQntPSfK2ampiHh8T9cDsKgmR3NPX7e81P0IMfvqWADNPSOWbGujMe8d0wou0LvYNNRaj7P5 X-Received: by 10.182.216.200 with SMTP id os8mr2796930obc.0.1394783062975; Fri, 14 Mar 2014 00:44:22 -0700 (PDT) MIME-Version: 1.0 X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.93.194 with SMTP id d60ls645001qge.22.gmail; Fri, 14 Mar 2014 00:44:22 -0700 (PDT) X-Received: by 10.58.154.10 with SMTP id vk10mr5154252veb.18.1394783062916; Fri, 14 Mar 2014 00:44:22 -0700 (PDT) Received: from mail-vc0-f174.google.com (mail-vc0-f174.google.com [209.85.220.174]) by mx.google.com with ESMTPS id si3si1799026vcb.103.2014.03.14.00.44.22 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 14 Mar 2014 00:44:22 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.220.174 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.220.174; Received: by mail-vc0-f174.google.com with SMTP id ld13so2287310vcb.5 for ; Fri, 14 Mar 2014 00:44:22 -0700 (PDT) X-Received: by 10.221.55.133 with SMTP id vy5mr5079804vcb.17.1394783062788; Fri, 14 Mar 2014 00:44:22 -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.220.78.9 with SMTP id i9csp33976vck; Fri, 14 Mar 2014 00:44:22 -0700 (PDT) X-Received: by 10.68.180.66 with SMTP id dm2mr7405569pbc.143.1394783061800; Fri, 14 Mar 2014 00:44:21 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id dg5si3236739pbc.217.2014.03.14.00.44.21; Fri, 14 Mar 2014 00:44:21 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of cpufreq-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755458AbaCNHoT (ORCPT + 6 others); Fri, 14 Mar 2014 03:44:19 -0400 Received: from mail-we0-f171.google.com ([74.125.82.171]:40039 "EHLO mail-we0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755133AbaCNHoT (ORCPT ); Fri, 14 Mar 2014 03:44:19 -0400 Received: by mail-we0-f171.google.com with SMTP id t61so1788390wes.30 for ; Fri, 14 Mar 2014 00:44:17 -0700 (PDT) X-Received: by 10.194.57.239 with SMTP id l15mr5253386wjq.40.1394783057677; Fri, 14 Mar 2014 00:44:17 -0700 (PDT) Received: from localhost ([213.122.173.131]) by mx.google.com with ESMTPSA id n3sm14384550wix.10.2014.03.14.00.44.14 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 14 Mar 2014 00:44:16 -0700 (PDT) From: Viresh Kumar To: rjw@rjwysocki.net Cc: linaro-kernel@lists.linaro.org, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, srivatsa.bhat@linux.vnet.ibm.com, Viresh Kumar Subject: [RFC V2] cpufreq: make sure frequency transitions are serialized Date: Fri, 14 Mar 2014 13:13:44 +0530 Message-Id: <2efc621827cbd96a05a3d34075154974b4816ecd.1394782795.git.viresh.kumar@linaro.org> X-Mailer: git-send-email 1.7.12.rc2.18.g61b472e Sender: cpufreq-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: cpufreq@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=neutral (google.com: 209.85.220.174 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) 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: , Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice continuously. Following examples show why this is important: Scenario 1: ----------- One thread reading value of cpuinfo_cur_freq, which will call __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition().. And ondemand governor trying to change freq of cpu at the same time and so sending notification via ->target().. Notifiers are not serialized and suppose this is what happened - PRECHANGE Notification for freq A (from cpuinfo_cur_freq) - PRECHANGE Notification for freq B (from target()) - Freq changed by target() to B - POSTCHANGE Notification for freq B - POSTCHANGE Notification for freq A Now the last POSTCHANGE Notification happened for freq A and hardware is currently running at freq B :) Where would we break then?: adjust_jiffies() in cpufreq.c and cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting jiffies).. All loops_per_jiffy based stuff is broken.. Scenario 2: ----------- Governor is changing freq and has called __cpufreq_driver_target(). At the same time we are changing scaling_{min|max}_freq from sysfs, which would eventually end up calling governor's: CPUFREQ_GOV_LIMITS notification, that will also call: __cpufreq_driver_target().. And hence concurrent calls to ->target() And Platform have something like this in their ->target() (Like: cpufreq-cpu0, omap, exynos, etc) A. If new freq is more than old: Increase voltage B. Change freq C. If new freq is less than old: decrease voltage Now, two concurrent calls to target are X and Y, where X is trying to increase freq and Y is trying to decrease it.. And this is the sequence that followed due to races.. X.A: voltage increased for larger freq Y.A: nothing happened here Y.B: freq decreased Y.C: voltage decreased X.B: freq increased X.C: nothing happened.. We ended up setting a freq which is not supported by the voltage we have set.. That will probably make clock to CPU unstable and system might not be workable anymore... This patch adds protection in cpufreq_notify_transition() to make transitions serialized. It runs WARN() if POSTCHANGE notification is sent when we are not in middle of a transition. And PRECHANGE notification is forced to wait while the current transition is in progress. Signed-off-by: Viresh Kumar --- This was discussed earlier here: https://lkml.org/lkml/2013/9/25/402 Where Rafael asked for better fix, as he called the V1 fixes: "quick and dirty". This is another approach, much simpler than previous one. Please see if this looks fine. There is a TODO note in there as I wanted some suggestions on how exactly should we wait for a transition to get over. drivers/cpufreq/cpufreq.c | 39 +++++++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | 2 ++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2677ff1..66bdfff 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -324,6 +324,13 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, } } +static void notify_transition_for_each_cpu(struct cpufreq_policy *policy, + struct cpufreq_freqs *freqs, unsigned int state) +{ + for_each_cpu(freqs->cpu, policy->cpus) + __cpufreq_notify_transition(policy, freqs, state); +} + /** * cpufreq_notify_transition - call notifier chain and adjust_jiffies * on frequency transition. @@ -335,8 +342,35 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) { - for_each_cpu(freqs->cpu, policy->cpus) - __cpufreq_notify_transition(policy, freqs, state); + if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE)) + return notify_transition_for_each_cpu(policy, freqs, state); + + /* Serialize pre-post notifications */ + mutex_lock(&policy->transition_lock); + if (unlikely(WARN_ON(!policy->transition_ongoing && + (state == CPUFREQ_POSTCHANGE)))) { + mutex_unlock(&policy->transition_lock); + return; + } + + if (state == CPUFREQ_PRECHANGE) { + while (policy->transition_ongoing) { + mutex_unlock(&policy->transition_lock); + /* TODO: Can we do something better here? */ + cpu_relax(); + mutex_lock(&policy->transition_lock); + } + + policy->transition_ongoing = true; + mutex_unlock(&policy->transition_lock); + } + + notify_transition_for_each_cpu(policy, freqs, state); + + if (state == CPUFREQ_POSTCHANGE) { + policy->transition_ongoing = false; + mutex_unlock(&policy->transition_lock); + } } EXPORT_SYMBOL_GPL(cpufreq_notify_transition); @@ -983,6 +1017,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void) INIT_LIST_HEAD(&policy->policy_list); init_rwsem(&policy->rwsem); + mutex_init(&policy->transition_lock); return policy; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 31c431e..e5cebce 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -104,6 +104,8 @@ struct cpufreq_policy { * __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem; + bool transition_ongoing; /* Tracks transition status */ + struct mutex transition_lock; }; /* Only for ACPI */