Re: [PATCH 11/12] cpufreq: propagate errors returned from __cpufreq_governor()

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

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