Even after serializing calls to __cpufreq_governor() there are some races left. The races are around doing the invalid operation during some state of cpufreq governors. For example, while the governor is in CPUFREQ_GOV_POLICY_EXIT state, we can't do CPUFREQ_GOV_START without first doing CPUFREQ_GOV_POLICY_INIT.
All these cases weren't handled elegantly in __cpufreq_governor() and so there were enough chances that things may go wrong when governors are changed with multiple thread in parallel.
This patch renames an existing field 'policy->governor_enabled' as 'policy->governor_state' which can have values other than 0 & 1 now. Its type is also changed to 'int' from 'bool'.
We maintain the current state of governors for each policy now and reject any invalid operation.
Reported-and-tested-by: Robert Schöne robert.schoene@tu-dresden.de Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 26 +++++++++++++------------- drivers/cpufreq/cpufreq_governor.c | 2 +- include/linux/cpufreq.h | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a7ceae3..c597361 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -935,6 +935,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) struct cpufreq_policy new_policy; int ret = 0;
+ policy->governor_state = CPUFREQ_GOV_POLICY_EXIT; memcpy(&new_policy, policy, sizeof(*policy));
/* Update governor of new_policy to the governor used before hotplug */ @@ -1976,7 +1977,7 @@ EXPORT_SYMBOL_GPL(cpufreq_driver_target); static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event) { - int ret; + int ret, state;
/* Only must be defined when default governor is known to have latency restrictions, like e.g. conservative or ondemand. @@ -2012,19 +2013,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock); + state = policy->governor_state; + + /* Check if operation is permitted or not */ if (policy->governor_busy - || (policy->governor_enabled && event == CPUFREQ_GOV_START) - || (!policy->governor_enabled - && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) { + || (state == CPUFREQ_GOV_START && event != CPUFREQ_GOV_LIMITS && event != CPUFREQ_GOV_STOP) + || (state == CPUFREQ_GOV_STOP && event != CPUFREQ_GOV_START && event != CPUFREQ_GOV_POLICY_EXIT) + || (state == CPUFREQ_GOV_POLICY_INIT && event != CPUFREQ_GOV_START && event != CPUFREQ_GOV_POLICY_EXIT) + || (state == CPUFREQ_GOV_POLICY_EXIT && event != CPUFREQ_GOV_POLICY_INIT)) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; }
policy->governor_busy = true; - if (event == CPUFREQ_GOV_STOP) - policy->governor_enabled = false; - else if (event == CPUFREQ_GOV_START) - policy->governor_enabled = true; + if (event != CPUFREQ_GOV_LIMITS) + policy->governor_state = event;
mutex_unlock(&cpufreq_governor_lock);
@@ -2035,13 +2038,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized++; else if (event == CPUFREQ_GOV_POLICY_EXIT) policy->governor->initialized--; - } else { + } else if (event != CPUFREQ_GOV_LIMITS) { /* Restore original values */ mutex_lock(&cpufreq_governor_lock); - if (event == CPUFREQ_GOV_STOP) - policy->governor_enabled = true; - else if (event == CPUFREQ_GOV_START) - policy->governor_enabled = false; + policy->governor_state = state; mutex_unlock(&cpufreq_governor_lock); }
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 1b44496..d173181 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -174,7 +174,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, int i;
mutex_lock(&cpufreq_governor_lock); - if (!policy->governor_enabled) + if (policy->governor_state != CPUFREQ_GOV_START) goto out_unlock;
if (!all_cpus) { diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index c7aa96b..fb20fc5 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -81,7 +81,7 @@ struct cpufreq_policy { unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data; - bool governor_enabled; /* governor start/stop flag */ + int governor_state; /* Governor's current state */ bool governor_busy;
struct work_struct update; /* if update_policy() needs to be