On 06/11/2015 04:21 PM, Viresh Kumar wrote:
There can be races where the request has come to a wrong state. For example INIT followed by STOP (instead of START) or START followed by EXIT (instead of STOP).
Also make sure, once we have started canceling queued works, we don't queue any new works. That can lead to the case where the work-handler finds many data structures are freed and so NULL pointer exceptions.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq_governor.c | 56 ++++++++++++++++++++++++++++++-------- drivers/cpufreq/cpufreq_governor.h | 1 + 2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index aa24aa9a9eb3..ee2e19a1218a 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -169,8 +169,12 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, unsigned int delay, bool all_cpus) {
struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(policy->cpu); int i;
if (!cdbs->ccdbs->enabled)
return;
policy->governor_enabled is already doing this job. Why this additional check ?
- mutex_lock(&cpufreq_governor_lock); if (!policy->governor_enabled) goto out_unlock;
@@ -234,6 +238,8 @@ static void dbs_timer(struct work_struct *work) bool modify_all = true;
mutex_lock(&dbs_data->cdata->mutex);
- if (!cdbs->ccdbs->enabled)
goto unlock;
This should not trigger at all if we get the entries into cpufreq_governor_dbs() fixed. I don't like the idea of adding checks/locks in places where it can be avoided.
if (dbs_data->cdata->governor == GOV_CONSERVATIVE) { struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; @@ -251,6 +257,7 @@ static void dbs_timer(struct work_struct *work) delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all); gov_queue_work(dbs_data, policy, delay, modify_all);
+unlock: mutex_unlock(&dbs_data->cdata->mutex); }
@@ -376,10 +383,15 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy, return ret; }
-static void cpufreq_governor_exit(struct cpufreq_policy *policy,
struct dbs_data *dbs_data)+static int cpufreq_governor_exit(struct cpufreq_policy *policy,
struct dbs_data *dbs_data){ struct common_dbs_data *cdata = dbs_data->cdata;
- struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(policy->cpu);
- /* STOP should have been called by now */
This is not true, atleast in the races that I have seen. The problem is not about STOP not being called before an EXIT. It is about a START being called after a STOP and before an EXIT. The comment should ideally be "The policy is active, stop it before exit" or similar.
- if (cdbs->ccdbs->enabled)
return -EBUSY;
And.. in such a scenario, we must not be aborting EXIT; rather it must cancel the queued work and successfully exit the policy. An EXIT is a more urgent operation than START, given its call sites. Also an EXIT will not leave the cpufreq governors in a limbo state, it is bound to restart a new policy or quit a policy if the last cpu goes down. A racing START operation however is typically from a call site referencing an older policy. Its better to abort this than the EXIT operation.
It may mean a user is trying to switch governors, and the exit operation is quitting the old governor as a result. A START from a cpufreq_remove_dev_finish() racing in just before this is no reason to prevent switching governors.
policy->governor_data = NULL; if (!--dbs_data->usage_count) { @@ -395,6 +407,8 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy, free_ccdbs(policy, cdata); kfree(dbs_data); }
- return 0;
}
static int cpufreq_governor_start(struct cpufreq_policy *policy, @@ -409,6 +423,10 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, if (!policy->cur) return -EINVAL;
- /* START shouldn't be already called */
- if (ccdbs->enabled)
return -EBUSY;
Why not reuse policy->governor_enabled in each of these places ?
Regards Preeti U Murthy