From patchwork Wed Mar 19 06:08:45 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 26544 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pd0-f199.google.com (mail-pd0-f199.google.com [209.85.192.199]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id AB94C203C3 for ; Wed, 19 Mar 2014 06:08:51 +0000 (UTC) Received: by mail-pd0-f199.google.com with SMTP id x10sf19875721pdj.6 for ; Tue, 18 Mar 2014 23:08:50 -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=ISP5vmwb1v7fRWcTByL5xhmG/5oos5laxXlN5wNI/S0=; b=gdL/wPQZT2feJZeDh1cKDJ4g7IOuy/P8vYYqsLA3XfI3b3xb7FT+cpvwZ43egmeQ+G ICxNKSFMz50Pes57R4SvXvkK5iIXMzpe10/CBGf7OV4kv0ujUZu/ms6kd3C7Rb8UJqSr 9bz+z7QRMUFk6IJ3Rw+KnuZf9PFqwTGTReXzwDt5Tnn/iPX7ZSe3qS4ZL+yhr8nJjbsa MHMl1aMwtvjzCo/a01pIqltwi7MY6inXUWtsPfu58ignpEvzezUd7L/QDmcdfdoejGqu t1b3JMxC14M5QoPVNJdkSc0PzN3SsBQ2WPZu0XbhrM6dIkhaYU7X9Xri1IE130IlCE8o DsPw== X-Gm-Message-State: ALoCoQmYXEGwJBv9IeSwum1PSFzqsJPcq4XseDwC+RAYczmHlKKs9dd3eNHDGB1pRLg2Qj0rLsxx X-Received: by 10.66.221.137 with SMTP id qe9mr14334828pac.4.1395209330934; Tue, 18 Mar 2014 23:08:50 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.101.12 with SMTP id t12ls2442245qge.1.gmail; Tue, 18 Mar 2014 23:08:50 -0700 (PDT) X-Received: by 10.59.7.170 with SMTP id dd10mr28051605ved.12.1395209330776; Tue, 18 Mar 2014 23:08:50 -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 bf4si4218691vec.46.2014.03.18.23.08.50 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 18 Mar 2014 23:08:50 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.220.170 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.170; Received: by mail-vc0-f170.google.com with SMTP id hu19so8775930vcb.1 for ; Tue, 18 Mar 2014 23:08:50 -0700 (PDT) X-Received: by 10.58.38.166 with SMTP id h6mr7904458vek.22.1395209330684; Tue, 18 Mar 2014 23:08:50 -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 i9csp272454vck; Tue, 18 Mar 2014 23:08:49 -0700 (PDT) X-Received: by 10.68.233.99 with SMTP id tv3mr15264835pbc.163.1395209329300; Tue, 18 Mar 2014 23:08:49 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id yn4si14087520pab.269.2014.03.18.23.08.48; Tue, 18 Mar 2014 23:08:48 -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 S1755292AbaCSGIr (ORCPT + 6 others); Wed, 19 Mar 2014 02:08:47 -0400 Received: from mail-qa0-f47.google.com ([209.85.216.47]:59192 "EHLO mail-qa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755261AbaCSGIp (ORCPT ); Wed, 19 Mar 2014 02:08:45 -0400 Received: by mail-qa0-f47.google.com with SMTP id w5so8007641qac.20 for ; Tue, 18 Mar 2014 23:08:45 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.229.221.1 with SMTP id ia1mr5812075qcb.6.1395209325155; Tue, 18 Mar 2014 23:08:45 -0700 (PDT) Received: by 10.96.22.39 with HTTP; Tue, 18 Mar 2014 23:08:45 -0700 (PDT) In-Reply-To: <532840FD.308@linux.vnet.ibm.com> References: <2efc621827cbd96a05a3d34075154974b4816ecd.1394782795.git.viresh.kumar@linaro.org> <532840FD.308@linux.vnet.ibm.com> Date: Wed, 19 Mar 2014 11:38:45 +0530 Message-ID: Subject: Re: [RFC V2] cpufreq: make sure frequency transitions are serialized From: Viresh Kumar To: "Srivatsa S. Bhat" Cc: "Rafael J. Wysocki" , Lists linaro-kernel , "cpufreq@vger.kernel.org" , "linux-pm@vger.kernel.org" , Linux Kernel Mailing List 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.170 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: , On 18 March 2014 18:20, Srivatsa S. Bhat wrote: > On 03/14/2014 01:13 PM, Viresh Kumar wrote: >> + if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE)) > > Wait a min, when is this condition ever true? I mean, what else can > 'state' ever be, apart from CPUFREQ_PRECHANGE and POSTCHANGE? There were two more 'unused' states available: CPUFREQ_RESUMECHANGE and CPUFREQ_SUSPENDCHANGE I have sent a patch to remove them now and this code would go away.. >> + return notify_transition_for_each_cpu(policy, freqs, state); >> + >> + /* Serialize pre-post notifications */ >> + mutex_lock(&policy->transition_lock); > > Nope, this is definitely not the way to go, IMHO. We should enforce that > the *callers* serialize the transitions, something like this: > > cpufreq_transition_lock(); > > cpufreq_notify_transition(CPUFREQ_PRECHANGE); > > //Perform the frequency change > > cpufreq_notify_transition(CPUFREQ_POSTCHANGE); > > cpufreq_transition_unlock(); > > That's it! > > [ We can either introduce a new "transition" lock or perhaps even reuse > the cpufreq_driver_lock if it fits... but the point is, the _caller_ has > to perform the locking; trying to be smart inside cpufreq_notify_transition() > is a recipe for headache :-( ] > > Is there any problem with this approach due to which you didn't take > this route? I didn't wanted drivers to handle this as core must make sure things are in order. Over that it would have helped by not pasting redundant code everywhere.. Drivers are anyway going to call cpufreq_notify_transition(), why increase burden on them? >> + 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); > > If the caller takes care of the synchronization, we can avoid > these sorts of acrobatics ;-) If we are fine with taking a mutex for the entire transition, then we can avoid above kind of acrobatics by just taking the mutex from PRECHANGE and leaving it at POSTCHANGE.. It will look like this then, hope this looks fine :) More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2677ff1..3b9eac4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -335,8 +335,15 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) { + if (state == CPUFREQ_PRECHANGE) + mutex_lock(&policy->transition_lock); + + /* Send notifications */ for_each_cpu(freqs->cpu, policy->cpus) __cpufreq_notify_transition(policy, freqs, state); + + if (state == CPUFREQ_POSTCHANGE) + mutex_unlock(&policy->transition_lock); } EXPORT_SYMBOL_GPL(cpufreq_notify_transition); @@ -983,6 +990,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..5f9209a 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -104,6 +104,7 @@ struct cpufreq_policy { * __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem; + struct mutex transition_lock; }; -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@vger.kernel.org