Re: [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Yue Hu <zbestahu@gmail.com>]

On Mon, 29 Apr 2019 10:45:07 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 26-04-19, 14:31, Yue Hu wrote:
> > From: Yue Hu <huyue2@yulong.com>
> > 
> > In cpufreq_init_policy() we will check if there's last_governor for target
> > and setpolicy type. However last_governor is set only if has_target() is
> > true in cpufreq_offline(). That means find last_governor for setpolicy
> > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > callback is set in cpufreq_set_policy().
> > 
> > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > Let's add a new helper function to avoid it. Also fix a little comment.
> > 
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> > v2: fix ->setplicy typo.
> > 
> >  drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-------------------
> >  1 file changed, 40 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0322cce..b822a3e 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -578,6 +578,20 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> >  	return NULL;
> >  }
> >  
> > +static int cpufreq_parse_static_governor(char *str_governor,
> > +					struct cpufreq_policy *policy)
> > +{
> > +	if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> > +		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> > +		return 0;
> > +	}
> > +	if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> > +		policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > +		return 0;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  /**
> >   * cpufreq_parse_governor - parse a governor string
> >   */
> > @@ -585,15 +599,7 @@ static int cpufreq_parse_governor(char *str_governor,
> >  				  struct cpufreq_policy *policy)
> >  {  
> 
> There were only two callers of cpufreq_parse_governor() and one of them you have
> removed already.
> 
> >  	if (cpufreq_driver->setpolicy) {
> > -		if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> > -			policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> > -			return 0;
> > -		}
> > -
> > -		if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> > -			policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > -			return 0;
> > -		}
> > +		return cpufreq_parse_static_governor(str_governor, policy);  
> 
> So I will rather remove above completely from here and let
> cpufreq_parse_governor() governor only handle !set_policy.

So, also need update store_scaling_governor() as below if doing that above, right?

        if (cpufreq_driver->setpolicy) {
                if (cpufreq_parse_static_governor(str_governor, &new_policy))
                        return -EINVAL;
        } else {
                if (cpufreq_parse_governor(str_governor, &new_policy))
                        return -EINVAL;
        } 

Moreover, change cpufreq_parse_static_governor() to cpufreq_parse_generic_policy()
should be better?

> 
> >  	} else {
> >  		struct cpufreq_governor *t;
> >  
> > @@ -1020,32 +1026,40 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
> >  
> >  static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  {
> > -	struct cpufreq_governor *gov = NULL;
> > +	struct cpufreq_governor *gov = NULL, *def_gov = NULL;
> >  	struct cpufreq_policy new_policy;
> >  
> >  	memcpy(&new_policy, policy, sizeof(*policy));
> >  
> > -	/* Update governor of new_policy to the governor used before hotplug */
> > -	gov = find_governor(policy->last_governor);
> > -	if (gov) {
> > -		pr_debug("Restoring governor %s for cpu %d\n",
> > +	def_gov = cpufreq_default_governor();
> > +
> > +	if (has_target()) {
> > +		/*
> > +		 * Update governor of new_policy to the governor used before
> > +		 * hotplug
> > +		 */
> > +		gov = find_governor(policy->last_governor);
> > +		if (gov)  
> 
> You must use {} here in the if block as well for two reasons:
> - Below pr_debug is across multiple lines
> - And else part already uses {}
>

Ok, fix it in v3.
 
> > +			pr_debug("Restoring governor %s for cpu %d\n",
> >  				policy->governor->name, policy->cpu);
> > +		else {
> > +			if (!def_gov)
> > +				return -ENODATA;
> > +			gov = def_gov;
> > +		}
> > +		new_policy.governor = gov;
> >  	} else {
> > -		gov = cpufreq_default_governor();
> > -		if (!gov)
> > -			return -ENODATA;
> > -	}
> > -
> > -	new_policy.governor = gov;
> > -
> > -	/* Use the default policy if there is no last_policy. */
> > -	if (cpufreq_driver->setpolicy) {
> > +		/* Use the default policy if there is no last_policy. */
> >  		if (policy->last_policy)
> >  			new_policy.policy = policy->last_policy;
> > -		else
> > -			cpufreq_parse_governor(gov->name, &new_policy);
> > +		else {
> > +			if (!def_gov)
> > +				return -ENODATA;
> > +			cpufreq_parse_static_governor(def_gov->name,
> > +						      &new_policy);
> > +		}
> >  	}
> > -	/* set default policy */
> > +	/* Set new policy */  
> 
> Just drop the comment and make it a blank line.

Ok, fix it in v3.

> 
> >  	return cpufreq_set_policy(policy, &new_policy);
> >  }
> >  
> > -- 
> > 1.9.1  
>