On 06/11/2015 04:21 PM, Viresh Kumar wrote:
At few places in cpufreq_set_policy() either we aren't checking return errors of __cpufreq_governor() or aren't returning them as is to the callers. This sometimes propagates wrong errors to sysfs OR we try to do more operations even if we have failed.
Now that we return -EBUSY from __cpufreq_governor() on invalid requests, there are more chances of hitting these errors. Lets fix them by checking and returning errors properly.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b612411655f9..da672b910760 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2284,16 +2284,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor; /* end old governor */ if (old_gov) {
__cpufreq_governor(policy, CPUFREQ_GOV_STOP);up_write(&policy->rwsem);__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);down_write(&policy->rwsem);
if(!(ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP))) {up_write(&policy->rwsem);ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
From my comments in the previous patches, EXIT should always succeed. In
that case we only need to take care of STOP. So we can perhaps do a
ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP) if (!ret) .. .. ?
It looks better this way.
down_write(&policy->rwsem);}if (ret)
How about a pr_debug("Failed to stop old governor %s", policy->gov->name) here ?
return ret;}
/* start new governor */ policy->governor = new_policy->governor;
- if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT)) {
if (!__cpufreq_governor(policy, CPUFREQ_GOV_START))
- if (!(ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT))) {
if (!(ret = __cpufreq_governor(policy, CPUFREQ_GOV_START)))
Do we really need to capture the return values here ? If there are errors we fall down to return EINVAL. Will this not be a valid error condition?
goto out; up_write(&policy->rwsem);@@ -2305,11 +2309,11 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, pr_debug("starting governor %s failed\n", policy->governor->name); if (old_gov) { policy->governor = old_gov;
__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT);__cpufreq_governor(policy, CPUFREQ_GOV_START);
if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT))__cpufreq_governor(policy, CPUFREQ_GOV_START);
I would suggest printing a debug message if INIT fails and calling __cpufreq_governor(POLICY_EXIT) if START fails. And EXIT is not supposed to fail. This will leave the cpufreq governor in a sane state.
}
- return -EINVAL;
return ret;
out: pr_debug("governor: change or update limits\n");
Regards Preeti U Murthy