| Message ID | 1e4cea25fc1bf43924a39944ca9d1ec0782621e8.1434019473.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + down_write(&policy->rwsem); + } + + if (ret) + 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))) 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); } - return -EINVAL; + return ret; out: pr_debug("governor: change or update limits\n");
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(-)