Re: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()

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

On 06/20/2015 08:42 AM, Viresh Kumar wrote:
> On 19-06-15, 23:32, Preeti U Murthy wrote:
>> If INIT itself fails, we need to set policy->governor to NULL. I
>> included this in the testing of the patchset.
> 
> Diff from earlier patch:
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index da672b910760..59fa0c1b7922 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2284,14 +2284,23 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>  	old_gov = policy->governor;
>  	/* end old governor */
>  	if (old_gov) {
> -		if(!(ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP))) {
> +		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> +		if (ret) {
> +			/* This can happen due to race with other operations */
> +			pr_debug("%s: Failed to Stop Governor: %s (%d)\n",
> +				 __func__, old_gov->name, ret);
> +			return ret;
> +		} else {
>  			up_write(&policy->rwsem);
>  			ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
>  			down_write(&policy->rwsem);
> -		}
> 
> -		if (ret)
> -			return ret;
> +			if (ret) {
> +				pr_err("%s: Failed to Exit Governor: %s (%d)\n",
> +				       __func__, old_gov->name, ret);
> +				return ret;
> +			}
> +		}
>  	}
> 
>  	/* start new governor */
> @@ -2309,7 +2318,9 @@ 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;
> -		if (!__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT))
> +		if (__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT))
> +			policy->governor = NULL;
> +		else
>  			__cpufreq_governor(policy, CPUFREQ_GOV_START);
>  	}
> 
> 
> New-patch:
> 
> --------------8<-------------------
> 
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Thu, 11 Sep 2014 10:50:48 +0530
> Subject: [PATCH] cpufreq: propagate errors returned from __cpufreq_governor()
> 
> Return codes aren't honored properly in cpufreq_set_policy(). This can
> lead to two problems:
> - wrong errors propagated to sysfs
> - we try to do next state-change even if the previous one failed
> 
> cpufreq_governor_dbs() now returns proper errors on all invalid
> state-transition requests and this code should honor that.
> 
> Reviewed-and-Tested-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b612411655f9..59fa0c1b7922 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2284,16 +2284,29 @@ 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);
> +		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> +		if (ret) {
> +			/* This can happen due to race with other operations */
> +			pr_debug("%s: Failed to Stop Governor: %s (%d)\n",
> +				 __func__, old_gov->name, ret);
> +			return ret;
> +		} else {
> +			up_write(&policy->rwsem);
> +			ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
> +			down_write(&policy->rwsem);
> +
> +			if (ret) {
> +				pr_err("%s: Failed to Exit Governor: %s (%d)\n",
> +				       __func__, old_gov->name, 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 +2318,13 @@ 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))
> +			policy->governor = NULL;
> +		else
> +			__cpufreq_governor(policy, CPUFREQ_GOV_START);
>  	}
> 
> -	return -EINVAL;
> +	return ret;
> 
>   out:
>  	pr_debug("governor: change or update limits\n");
> 

There are some checkpatch errors on this patch.

Regards
Preeti U Murthy

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in