From patchwork Thu Sep 12 10:10:46 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 19996 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ye0-f200.google.com (mail-ye0-f200.google.com [209.85.213.200]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 3155C25B63 for ; Thu, 12 Sep 2013 10:11:22 +0000 (UTC) Received: by mail-ye0-f200.google.com with SMTP id r3sf11052881yen.11 for ; Thu, 12 Sep 2013 03:11:22 -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:in-reply-to:references:in-reply-to:references :x-original-sender:x-original-authentication-results:precedence :mailing-list:list-id:list-post:list-help:list-archive :list-unsubscribe; bh=JKCqYbTQLamOxsUV56X3xTmlgyJnaP3WpiKLDx+8CZs=; b=kzLAd/gUH+BmBRGXnVDwXEwQd25DpJlrurBMZSSlNIT0YS4a58KwIWPpJYGA8G9Jr3 rWLwNEiJ+VTKjLvDwilpc4yPtPZ2y2niQ2I5LASVsMRK9FKpr+WL86JOvMf5VOEczOIZ nsNS3fUIP6T6wRfqU8DrolOUFQaxiAcHKkQiTAdOU0MdDi7Sqe5ykoQf8pw+CzUZM1FW 5YWSi4zNVunXq4KbsOsldHv/6Aan00Cth40YWvQoV6GFFOMLXdoMjZNsr9JUd4oqfEdb oQHoYlVYg+dOJxCn3XAUaqvthpWkdf4WzIK7rHD19cSIo8hQtACKJUl0xxxYRthxACPH m4hg== X-Gm-Message-State: ALoCoQlJQpfduabvACPD3ELQBIhHrdKseVqzTBIgbRGZTiOldJbNrJt5LeKQwIf93ptQve6VA12f X-Received: by 10.236.176.1 with SMTP id a1mr2467364yhm.10.1378980681840; Thu, 12 Sep 2013 03:11:21 -0700 (PDT) MIME-Version: 1.0 X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.39.104 with SMTP id o8ls612014qek.38.gmail; Thu, 12 Sep 2013 03:11:21 -0700 (PDT) X-Received: by 10.52.108.230 with SMTP id hn6mr724199vdb.28.1378980681623; Thu, 12 Sep 2013 03:11:21 -0700 (PDT) Received: from mail-vb0-f54.google.com (mail-vb0-f54.google.com [209.85.212.54]) by mx.google.com with ESMTPS id dp7si962983ved.35.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 12 Sep 2013 03:11:21 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.212.54 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.212.54; Received: by mail-vb0-f54.google.com with SMTP id q14so7327041vbe.27 for ; Thu, 12 Sep 2013 03:11:21 -0700 (PDT) X-Received: by 10.52.165.131 with SMTP id yy3mr890453vdb.25.1378980681491; Thu, 12 Sep 2013 03:11:21 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.220.174.196 with SMTP id u4csp11319vcz; Thu, 12 Sep 2013 03:11:20 -0700 (PDT) X-Received: by 10.66.88.3 with SMTP id bc3mr8536577pab.48.1378980680284; Thu, 12 Sep 2013 03:11:20 -0700 (PDT) Received: from mail-pd0-f169.google.com (mail-pd0-f169.google.com [209.85.192.169]) by mx.google.com with ESMTPS id yk3si6141323pac.128.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 12 Sep 2013 03:11:20 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.192.169 is neither permitted nor denied by best guess record for domain of viresh.kumar@linaro.org) client-ip=209.85.192.169; Received: by mail-pd0-f169.google.com with SMTP id r10so10563070pdi.0 for ; Thu, 12 Sep 2013 03:11:19 -0700 (PDT) X-Received: by 10.68.245.227 with SMTP id xr3mr586903pbc.182.1378980679846; Thu, 12 Sep 2013 03:11:19 -0700 (PDT) Received: from localhost ([106.197.30.106]) by mx.google.com with ESMTPSA id 7sm9689521paf.22.1969.12.31.16.00.00 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 12 Sep 2013 03:11:18 -0700 (PDT) From: Viresh Kumar To: rjw@sisk.pl Cc: linaro-kernel@lists.linaro.org, patches@linaro.org, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, g.liakhovetski@gmx.de, amit.daniel@samsung.com, kgene.kim@samsung.com, Viresh Kumar Subject: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized Date: Thu, 12 Sep 2013 15:40:46 +0530 Message-Id: <793a5fadad931d04c2adb3a61c64063b7e62d083.1378979483.git.viresh.kumar@linaro.org> X-Mailer: git-send-email 1.7.12.rc2.18.g61b472e In-Reply-To: References: In-Reply-To: References: 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.212.54 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 Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Some part of this patch was pushed in mainline earlier but was then removed due to loopholes in the patch. Those are now fixed and this patch is tested by the people who reported these problems. 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 contiguously. Also, calls to cpufreq_driver_target() or cpufreq_driver->target() must also be serialized. 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, 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 wouldn't be workable anymore... This patch adds some protection against to make transitions serialized. Tested-by: Guennadi Liakhovetski Signed-off-by: Viresh Kumar Tested-by: Stephen Warren --- drivers/cpufreq/cpufreq.c | 83 ++++++++++++++++++++++++++++++++++++ drivers/cpufreq/exynos5440-cpufreq.c | 3 ++ include/linux/cpufreq.h | 1 + 3 files changed, 87 insertions(+) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 43c24aa..f8b0889 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -268,6 +268,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) static void __cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) { + unsigned long flags; + BUG_ON(irqs_disabled()); if (cpufreq_disabled()) @@ -280,6 +282,17 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, switch (state) { case CPUFREQ_PRECHANGE: + write_lock_irqsave(&cpufreq_driver_lock, flags); + if (WARN(policy->transition_ongoing > + cpumask_weight(policy->cpus), + "In middle of another frequency transition\n")) { + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + return; + } + + policy->transition_ongoing++; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + /* detect if the driver reported a value as "old frequency" * which is not equal to what the cpufreq core thinks is * "old frequency". @@ -299,6 +312,16 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, break; case CPUFREQ_POSTCHANGE: + write_lock_irqsave(&cpufreq_driver_lock, flags); + if (WARN(policy->transition_ongoing < 2, + "No frequency transition in progress\n")) { + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + return; + } + + policy->transition_ongoing--; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + adjust_jiffies(CPUFREQ_POSTCHANGE, freqs); pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new, (unsigned long)freqs->cpu); @@ -324,6 +347,20 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy, { for_each_cpu(freqs->cpu, policy->cpus) __cpufreq_notify_transition(policy, freqs, state); + + if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) + && (state == CPUFREQ_POSTCHANGE)) { + unsigned long flags; + + /* + * Some drivers don't send POSTCHANGE notification from their + * ->target() but from some kind of bottom half and so we are + * ending transaction here.. + */ + write_lock_irqsave(&cpufreq_driver_lock, flags); + policy->transition_ongoing--; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + } } EXPORT_SYMBOL_GPL(cpufreq_notify_transition); @@ -1369,8 +1406,33 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq, policy = per_cpu(cpufreq_cpu_data, cpu); read_unlock_irqrestore(&cpufreq_driver_lock, flags); + /* + * The role of this function is to make sure that the CPU frequency we + * use is the same as the CPU is actually running at. Therefore, if a + * CPU frequency change notification is in progress, it will do the + * update anyway, so we have nothing to do here in that case. + */ + write_lock_irqsave(&cpufreq_driver_lock, flags); + if (policy->transition_ongoing) { + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + return; + } + policy->transition_ongoing++; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); + + /* + * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement + * transition_ongoing from POSTCHANGE notifiers. + */ + if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) + return; + + write_lock_irqsave(&cpufreq_driver_lock, flags); + policy->transition_ongoing--; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); } /** @@ -1656,6 +1718,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, { int retval = -EINVAL; unsigned int old_target_freq = target_freq; + unsigned long flags; if (cpufreq_disabled()) return -ENODEV; @@ -1672,9 +1735,29 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, if (target_freq == policy->cur) return 0; + write_lock_irqsave(&cpufreq_driver_lock, flags); + if (policy->transition_ongoing) { + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + return -EBUSY; + } + policy->transition_ongoing++; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + if (cpufreq_driver->target) retval = cpufreq_driver->target(policy, target_freq, relation); + /* + * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement + * transition_ongoing from POSTCHANGE notifiers. + */ + if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) + && (retval == -EINPROGRESS)) + return retval; + + write_lock_irqsave(&cpufreq_driver_lock, flags); + policy->transition_ongoing--; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + return retval; } EXPORT_SYMBOL_GPL(__cpufreq_driver_target); diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c index f44664a..1e391ac 100644 --- a/drivers/cpufreq/exynos5440-cpufreq.c +++ b/drivers/cpufreq/exynos5440-cpufreq.c @@ -251,6 +251,9 @@ static int exynos_target(struct cpufreq_policy *policy, __raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4); } + + /* Mark transition as In-progress */ + ret = -EINPROGRESS; out: mutex_unlock(&cpufreq_lock); return ret; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 3cefb7b..c770bc0 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -85,6 +85,7 @@ struct cpufreq_policy { struct list_head policy_list; struct kobject kobj; struct completion kobj_unregister; + int transition_ongoing; /* Tracks transition status */ }; /* Only for ACPI */