Hi Viresh,
On 02/02/16 16:27, Viresh Kumar wrote:
Invalid state-transitions is verified by governor core now and there is no need to replicate that in cpufreq core. Also we don't drop policy->rwsem anymore, which makes rest of the races go away.
There are still paths where we call __cpufreq_governor() without holding policy->rwsem, but those should be fixed with my cleanups (that I intend to refresh and post soon). So, I'm not sure we can safely remove this yet.
Simplify code a bit now.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 24 ------------------------ include/linux/cpufreq.h | 1 - 2 files changed, 25 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5f7e24567e0e..052ad1b9372c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list); static struct cpufreq_driver *cpufreq_driver; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_RWLOCK(cpufreq_driver_lock); -DEFINE_MUTEX(cpufreq_governor_lock); /* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended; @@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
- mutex_lock(&cpufreq_governor_lock);
- if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
|| (!policy->governor_enabled&& (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {mutex_unlock(&cpufreq_governor_lock);return -EBUSY;- }
- if (event == CPUFREQ_GOV_STOP)
policy->governor_enabled = false;- else if (event == CPUFREQ_GOV_START)
policy->governor_enabled = true;- mutex_unlock(&cpufreq_governor_lock);
- ret = policy->governor->governor(policy, event);
So, __cpufreq_governor() becomes effectively a wrapper around ->governor() calls and governors are left responsible for implementing the state machine with appropriate checks.
I'm wondering if this approach is completely sane, but what we end up with your changes should work (and we kill a lock! :)).
Maybe we add a comment somewhere stating exactly how things are meant to work?
Thanks,
- Juri