On 2 September 2013 02:12, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> Please don't call this a crash, especially not in a patch changelog.
> That is confusing. It's better to say "This triggers the following warning
> sometimes:".
Same mistake for the second time :(
Fixed in my log now..
> OK, so first, I think that this transition_ongoing thing needs to be modified
> and checked under cpufreq_driver_lock. Otherwise this code will be racy,
> because someone may have updated policy->transition_ongoing after we've read
> policy.
Hmm..
> Second, in my opinion the comment should say something like "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." Or is it not correct?
Absolutely correct :)
Check this one (attached too), tested on my thinkpad:
From: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
Date: Sun, 1 Sep 2013 22:19:37 +0530
Subject: [PATCH] cpufreq: fix serialization issues with freq change notifiers
This patch tried to serialize frequency transitions:
commit 7c30ed532cf798a8d924562f2f44d03d7652f7a7
Author: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
Date: Wed Jun 19 10:16:55 2013 +0530
cpufreq: make sure frequency transitions are serialized
But there still are few scenarios where notifications can be done in parallel.
These aren't originated from ->target() but from cpufreq_out_of_sync().
This triggers following warning sometimes:
WARNING: CPU: 0 PID: 14543 at drivers/cpufreq/cpufreq.c:317
__cpufreq_notify_transition+0x238/0x260()
In middle of another frequency transition
<snip>
all Trace:
[<ffffffff81720daa>] dump_stack+0x46/0x58
[<ffffffff8106534c>] warn_slowpath_common+0x8c/0xc0
[<ffffffff815b8560>] ? acpi_cpufreq_target+0x320/0x320
[<ffffffff81065436>] warn_slowpath_fmt+0x46/0x50
[<ffffffff815b1ec8>] __cpufreq_notify_transition+0x238/0x260
[<ffffffff815b33be>] cpufreq_notify_transition+0x3e/0x70
[<ffffffff815b345d>] cpufreq_out_of_sync+0x6d/0xb0
[<ffffffff815b370c>] cpufreq_update_policy+0x10c/0x160
[<ffffffff815b3760>] ? cpufreq_update_policy+0x160/0x160
[<ffffffff81413813>] cpufreq_set_cur_state+0x8c/0xb5
[<ffffffff814138df>] processor_set_cur_state+0xa3/0xcf
[<ffffffff8158e13c>] thermal_cdev_update+0x9c/0xb0
[<ffffffff8159046a>] step_wise_throttle+0x5a/0x90
[<ffffffff8158e21f>] handle_thermal_trip+0x4f/0x140
[<ffffffff8158e377>] thermal_zone_device_update+0x57/0xa0
[<ffffffff81415b36>] acpi_thermal_check+0x2e/0x30
[<ffffffff81415ca0>] acpi_thermal_notify+0x40/0xdc
[<ffffffff813e7dbd>] acpi_device_notify+0x19/0x1b
[<ffffffff813f8241>] acpi_ev_notify_dispatch+0x41/0x5c
[<ffffffff813e3fbe>] acpi_os_execute_deferred+0x25/0x32
[<ffffffff81081060>] process_one_work+0x170/0x4a0
[<ffffffff81082121>] worker_thread+0x121/0x390
[<ffffffff81082000>] ? manage_workers.isra.20+0x170/0x170
[<ffffffff81088fe0>] kthread+0xc0/0xd0
[<ffffffff81088f20>] ? flush_kthread_worker+0xb0/0xb0
[<ffffffff8173582c>] ret_from_fork+0x7c/0xb0
[<ffffffff81088f20>] ? flush_kthread_worker+0xb0/0xb0
This patch modifies cpufreq_out_of_sync() and returns without sending
notifications in case another transition is in progress. 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.
transition_ongoing is now accessed within cpufreq_driver_lock now to avoid any
race conditions.
transition_ongoing is now first incremented from the callers of PRECHANGE
notifier. And is last decremented from POSTCHANGE notifier to mark end of
transfer because some drivers don't send POSTCHANGE notification from their
->target() but from some kind of bottom half, and so this is the only place
which knows when a transition is over.
Reported-by: Alessandro Bono <alessandro.bono@xxxxxxxxx>
Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
---
drivers/cpufreq/cpufreq.c | 54 ++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 49 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index be2e5f4..4aee299 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -274,6 +274,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())
@@ -286,12 +288,16 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
switch (state) {
case CPUFREQ_PRECHANGE:
- if (WARN(policy->transition_ongoing ==
+ write_lock_irqsave(&cpufreq_driver_lock, flags);
+ if (WARN(policy->transition_ongoing >
cpumask_weight(policy->cpus),
- "In middle of another frequency transition\n"))
+ "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
@@ -312,11 +318,15 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
break;
case CPUFREQ_POSTCHANGE:
- if (WARN(!policy->transition_ongoing,
- "No frequency transition in progress\n"))
+ 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,
@@ -343,6 +353,19 @@ void cpufreq_notify_transition(struct
cpufreq_policy *policy,
{
for_each_cpu(freqs->cpu, policy->cpus)
__cpufreq_notify_transition(policy, freqs, state);
+
+ if (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);
@@ -1326,6 +1349,20 @@ 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);
}
@@ -1613,11 +1650,18 @@ 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;
- if (policy->transition_ongoing)
+
+ 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);
/* Make sure that target_freq is within supported range */
if (target_freq > policy->max)
Attachment:
0001-cpufreq-fix-serialization-issues-with-freq-change-no.patch
Description: Binary data
