From 27e289dce29764e488c1e13e9aa6950cad1f4aab Mon Sep 17 00:00:00 2001 From: Stratos Karafotis Date: Fri, 25 Apr 2014 23:15:23 +0300 Subject: cpufreq: Introduce macros for cpufreq_frequency_table iteration Many cpufreq drivers need to iterate over the cpufreq_frequency_table for various tasks. This patch introduces two macros which can be used for iteration over cpufreq_frequency_table keeping a common coding style across drivers: - cpufreq_for_each_entry: iterate over each entry of the table - cpufreq_for_each_valid_entry: iterate over each entry that contains a valid frequency. It should have no functional changes. Signed-off-by: Stratos Karafotis Acked-by: Lad, Prabhakar Acked-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index abda6609d3e7..a517da996aaf 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -237,6 +237,17 @@ void cpufreq_cpu_put(struct cpufreq_policy *policy) } EXPORT_SYMBOL_GPL(cpufreq_cpu_put); +bool cpufreq_next_valid(struct cpufreq_frequency_table **pos) +{ + while ((*pos)->frequency != CPUFREQ_TABLE_END) + if ((*pos)->frequency != CPUFREQ_ENTRY_INVALID) + return true; + else + (*pos)++; + return false; +} +EXPORT_SYMBOL_GPL(cpufreq_next_valid); + /********************************************************************* * EXTERNALLY AFFECTING FREQUENCY CHANGES * *********************************************************************/ -- cgit v1.2.3 From ca654dc3a93d3b47dddc0c24a98043060bbb256b Mon Sep 17 00:00:00 2001 From: "Srivatsa S. Bhat" Date: Mon, 5 May 2014 12:52:39 +0530 Subject: cpufreq: Catch double invocations of cpufreq_freq_transition_begin/end Some cpufreq drivers were redundantly invoking the _begin() and _end() APIs around frequency transitions, and this double invocation (one from the cpufreq core and the other from the cpufreq driver) used to result in a self-deadlock, leading to system hangs during boot. (The _begin() API makes contending callers wait until the previous invocation is complete. Hence, the cpufreq driver would end up waiting on itself!). Now all such drivers have been fixed, but debugging this issue was not very straight-forward (even lockdep didn't catch this). So let us add a debug infrastructure to the cpufreq core to catch such issues more easily in the future. We add a new field called 'transition_task' to the policy structure, to keep track of the task which is performing the frequency transition. Using this field, we make note of this task during _begin() and print a warning if we find a case where the same task is calling _begin() again, before completing the previous frequency transition using the corresponding _end(). We have left out ASYNC_NOTIFICATION drivers from this debug infrastructure for 2 reasons: 1. At the moment, we have no way to avoid a particular scenario where this debug infrastructure can emit false-positive warnings for such drivers. The scenario is depicted below: Task A Task B /* 1st freq transition */ Invoke _begin() { ... ... } Change the frequency /* 2nd freq transition */ Invoke _begin() { ... //waiting for B to ... //finish _end() for ... //the 1st transition ... | Got interrupt for successful ... | change of frequency (1st one). ... | ... | /* 1st freq transition */ ... | Invoke _end() { ... | ... ... V } ... ... } This scenario is actually deadlock-free because, once Task A changes the frequency, it is Task B's responsibility to invoke the corresponding _end() for the 1st frequency transition. Hence it is perfectly legal for Task A to go ahead and attempt another frequency transition in the meantime. (Of course it won't be able to proceed until Task B finishes the 1st _end(), but this doesn't cause a deadlock or a hang). The debug infrastructure cannot handle this scenario and will treat it as a deadlock and print a warning. To avoid this, we exclude such drivers from the purview of this code. 2. Luckily, we don't _need_ this infrastructure for ASYNC_NOTIFICATION drivers at all! The cpufreq core does not automatically invoke the _begin() and _end() APIs during frequency transitions in such drivers. Thus, the driver alone is responsible for invoking _begin()/_end() and hence there shouldn't be any conflicts which lead to double invocations. So, we can skip these drivers, since the probability that such drivers will hit this problem is extremely low, as outlined above. Signed-off-by: Srivatsa S. Bhat Acked-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 14 ++++++++++++++ include/linux/cpufreq.h | 1 + 2 files changed, 15 insertions(+) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a517da996aaf..bfe82b63875f 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -365,6 +365,18 @@ static void cpufreq_notify_post_transition(struct cpufreq_policy *policy, void cpufreq_freq_transition_begin(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs) { + + /* + * Catch double invocations of _begin() which lead to self-deadlock. + * ASYNC_NOTIFICATION drivers are left out because the cpufreq core + * doesn't invoke _begin() on their behalf, and hence the chances of + * double invocations are very low. Moreover, there are scenarios + * where these checks can emit false-positive warnings in these + * drivers; so we avoid that by skipping them altogether. + */ + WARN_ON(!(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) + && current == policy->transition_task); + wait: wait_event(policy->transition_wait, !policy->transition_ongoing); @@ -376,6 +388,7 @@ wait: } policy->transition_ongoing = true; + policy->transition_task = current; spin_unlock(&policy->transition_lock); @@ -392,6 +405,7 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy, cpufreq_notify_post_transition(policy, freqs, transition_failed); policy->transition_ongoing = false; + policy->transition_task = NULL; wake_up(&policy->transition_wait); } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 77a5fa191502..f3822f836e14 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -110,6 +110,7 @@ struct cpufreq_policy { bool transition_ongoing; /* Tracks transition status */ spinlock_t transition_lock; wait_queue_head_t transition_wait; + struct task_struct *transition_task; /* Task which is doing the transition */ }; /* Only for ACPI */ -- cgit v1.2.3 From 5eeaf1f1897372590105f155c6a7110b3fa36aef Mon Sep 17 00:00:00 2001 From: Stratos Karafotis Date: Wed, 7 May 2014 19:33:33 +0300 Subject: cpufreq: Fix build error on some platforms that use cpufreq_for_each_* On platforms that use cpufreq_for_each_* macros, build fails if CONFIG_CPU_FREQ=n, e.g. ARM/shmobile/koelsch/non-multiplatform: drivers/built-in.o: In function `clk_round_parent': clkdev.c:(.text+0xcf168): undefined reference to `cpufreq_next_valid' drivers/built-in.o: In function `clk_rate_table_find': clkdev.c:(.text+0xcf820): undefined reference to `cpufreq_next_valid' make[3]: *** [vmlinux] Error 1 Fix this making cpufreq_next_valid function inline and move it to cpufreq.h. Fixes: 27e289dce297 (cpufreq: Introduce macros for cpufreq_frequency_table iteration) Reported-and-tested-by: Geert Uytterhoeven Signed-off-by: Stratos Karafotis Acked-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 11 ----------- include/linux/cpufreq.h | 11 +++++++++-- 2 files changed, 9 insertions(+), 13 deletions(-) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index bfe82b63875f..a05c92198b9f 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -237,17 +237,6 @@ void cpufreq_cpu_put(struct cpufreq_policy *policy) } EXPORT_SYMBOL_GPL(cpufreq_cpu_put); -bool cpufreq_next_valid(struct cpufreq_frequency_table **pos) -{ - while ((*pos)->frequency != CPUFREQ_TABLE_END) - if ((*pos)->frequency != CPUFREQ_ENTRY_INVALID) - return true; - else - (*pos)++; - return false; -} -EXPORT_SYMBOL_GPL(cpufreq_next_valid); - /********************************************************************* * EXTERNALLY AFFECTING FREQUENCY CHANGES * *********************************************************************/ diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 9d803b529ac2..3f458896d45c 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -489,8 +489,15 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev, } #endif - -bool cpufreq_next_valid(struct cpufreq_frequency_table **pos); +static inline bool cpufreq_next_valid(struct cpufreq_frequency_table **pos) +{ + while ((*pos)->frequency != CPUFREQ_TABLE_END) + if ((*pos)->frequency != CPUFREQ_ENTRY_INVALID) + return true; + else + (*pos)++; + return false; +} /* * cpufreq_for_each_entry - iterate over a cpufreq_frequency_table -- cgit v1.2.3 From 8d65775d17941d6d41f5913fc6a99a134c588e01 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 21 May 2014 14:29:29 +0530 Subject: cpufreq: handle calls to ->target_index() in separate routine Handling calls to ->target_index() has got complex over time and might become more complex. So, its better to take target_index() bits out in another routine __target_index() for better code readability. Shouldn't have any functional impact. Tested-by: Stephen Warren Reviewed-by: Doug Anderson Signed-off-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 56 ++++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 23 deletions(-) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a05c92198b9f..ae11dd51f81d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1816,12 +1816,43 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); * GOVERNORS * *********************************************************************/ +static int __target_index(struct cpufreq_policy *policy, + struct cpufreq_frequency_table *freq_table, int index) +{ + struct cpufreq_freqs freqs; + int retval = -EINVAL; + bool notify; + + notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); + + if (notify) { + freqs.old = policy->cur; + freqs.new = freq_table[index].frequency; + freqs.flags = 0; + + pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", + __func__, policy->cpu, freqs.old, freqs.new); + + cpufreq_freq_transition_begin(policy, &freqs); + } + + retval = cpufreq_driver->target_index(policy, index); + if (retval) + pr_err("%s: Failed to change cpu frequency: %d\n", __func__, + retval); + + if (notify) + cpufreq_freq_transition_end(policy, &freqs, retval); + + return retval; +} + int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) { - int retval = -EINVAL; unsigned int old_target_freq = target_freq; + int retval = -EINVAL; if (cpufreq_disabled()) return -ENODEV; @@ -1848,8 +1879,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, retval = cpufreq_driver->target(policy, target_freq, relation); else if (cpufreq_driver->target_index) { struct cpufreq_frequency_table *freq_table; - struct cpufreq_freqs freqs; - bool notify; int index; freq_table = cpufreq_frequency_get_table(policy->cpu); @@ -1870,26 +1899,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, goto out; } - notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION); - - if (notify) { - freqs.old = policy->cur; - freqs.new = freq_table[index].frequency; - freqs.flags = 0; - - pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n", - __func__, policy->cpu, freqs.old, freqs.new); - - cpufreq_freq_transition_begin(policy, &freqs); - } - - retval = cpufreq_driver->target_index(policy, index); - if (retval) - pr_err("%s: Failed to change cpu frequency: %d\n", - __func__, retval); - - if (notify) - cpufreq_freq_transition_end(policy, &freqs, retval); + retval = __target_index(policy, freq_table, index); } out: -- cgit v1.2.3