Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock

[Juri Lelli]

Hi Viresh,

On 02/02/16 16:27, Viresh Kumar wrote:
> Invalid state-transitions is verified by governor core now and there is
> no need to replicate that in cpufreq core. Also we don't drop
> policy->rwsem anymore, which makes rest of the races go away.

There are still paths where we call __cpufreq_governor() without holding
policy->rwsem, but those should be fixed with my cleanups (that I intend
to refresh and post soon). So, I'm not sure we can safely remove this
yet.

> 
> Simplify code a bit now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/cpufreq/cpufreq.c | 24 ------------------------
>  include/linux/cpufreq.h   |  1 -
>  2 files changed, 25 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5f7e24567e0e..052ad1b9372c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -102,7 +102,6 @@ static LIST_HEAD(cpufreq_governor_list);
>  static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
> -DEFINE_MUTEX(cpufreq_governor_lock);
>  
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
> @@ -1963,21 +1962,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  
>  	pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);
>  
> -	mutex_lock(&cpufreq_governor_lock);
> -	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> -	    || (!policy->governor_enabled
> -	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> -		mutex_unlock(&cpufreq_governor_lock);
> -		return -EBUSY;
> -	}
> -
> -	if (event == CPUFREQ_GOV_STOP)
> -		policy->governor_enabled = false;
> -	else if (event == CPUFREQ_GOV_START)
> -		policy->governor_enabled = true;
> -
> -	mutex_unlock(&cpufreq_governor_lock);
> -
>  	ret = policy->governor->governor(policy, event);

So, __cpufreq_governor() becomes effectively a wrapper around
->governor() calls and governors are left responsible for implementing
the state machine with appropriate checks.

I'm wondering if this approach is completely sane, but what we end up
with your changes should work (and we kill a lock! :)).

Maybe we add a comment somewhere stating exactly how things are meant to
work?

Thanks,

- Juri