[PATCH] cpufreq: use last policy after online

[PATCH] cpufreq: use last policy after online

[Srinivas Pandruvada]

For cpufreq drivers which use setpolicy interface, after offline->online
the policy is set to default. This can be reproduced by setting the
default policy of intel_pstate or longrun to ondemand and then change to
"performance". After offline and online, the setpolicy will be called with
the policy=ondemand.
For drivers using governors this condition is handled by storing
last_governor, during offline and restoring during online. The same should
be done for drivers using setpolicy interface. Storing last_policy during
offline and restoring during online.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/cpufreq.c | 14 ++++++++++----
 include/linux/cpufreq.h   |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a83c995..8412ce5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -976,10 +976,14 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 
 	new_policy.governor = gov;
 
-	/* Use the default policy if its valid. */
-	if (cpufreq_driver->setpolicy)
-		cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
-
+	/* Use the default policy if there is no last_policy. */
+	if (cpufreq_driver->setpolicy) {
+		if (policy->last_policy)
+			new_policy.policy = policy->last_policy;
+		else
+			cpufreq_parse_governor(gov->name, &new_policy.policy,
+					       NULL);
+	}
 	/* set default policy */
 	return cpufreq_set_policy(policy, &new_policy);
 }
@@ -1330,6 +1334,8 @@ static void cpufreq_offline_prepare(unsigned int cpu)
 		if (has_target())
 			strncpy(policy->last_governor, policy->governor->name,
 				CPUFREQ_NAME_LEN);
+		else
+			policy->last_policy = policy->policy;
 	} else if (cpu == policy->cpu) {
 		/* Nominate new CPU */
 		policy->cpu = cpumask_any(policy->cpus);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index ef4c5b1..177c768 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -77,6 +77,7 @@ struct cpufreq_policy {
 	unsigned int		suspend_freq; /* freq to set during suspend */
 
 	unsigned int		policy; /* see above */
+	unsigned int		last_policy; /* policy before unplug */
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	bool			governor_enabled; /* governor start/stop flag */
-- 
1.9.3

Re: [PATCH] cpufreq: use last policy after online

[Rafael J. Wysocki]

On Tuesday, December 01, 2015 04:52:14 PM Srinivas Pandruvada wrote:
> For cpufreq drivers which use setpolicy interface, after offline->online
> the policy is set to default. This can be reproduced by setting the
> default policy of intel_pstate or longrun to ondemand and then change to
> "performance". After offline and online, the setpolicy will be called with
> the policy=ondemand.
> For drivers using governors this condition is handled by storing
> last_governor, during offline and restoring during online. The same should
> be done for drivers using setpolicy interface. Storing last_policy during
> offline and restoring during online.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

I guess this is urgent, right?

Is it a regression fix or has it just never worked?

> ---
>  drivers/cpufreq/cpufreq.c | 14 ++++++++++----
>  include/linux/cpufreq.h   |  1 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a83c995..8412ce5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -976,10 +976,14 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
>  
>  	new_policy.governor = gov;
>  
> -	/* Use the default policy if its valid. */
> -	if (cpufreq_driver->setpolicy)
> -		cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
> -
> +	/* Use the default policy if there is no last_policy. */
> +	if (cpufreq_driver->setpolicy) {
> +		if (policy->last_policy)
> +			new_policy.policy = policy->last_policy;
> +		else
> +			cpufreq_parse_governor(gov->name, &new_policy.policy,
> +					       NULL);
> +	}
>  	/* set default policy */
>  	return cpufreq_set_policy(policy, &new_policy);
>  }
> @@ -1330,6 +1334,8 @@ static void cpufreq_offline_prepare(unsigned int cpu)
>  		if (has_target())
>  			strncpy(policy->last_governor, policy->governor->name,
>  				CPUFREQ_NAME_LEN);
> +		else
> +			policy->last_policy = policy->policy;
>  	} else if (cpu == policy->cpu) {
>  		/* Nominate new CPU */
>  		policy->cpu = cpumask_any(policy->cpus);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index ef4c5b1..177c768 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -77,6 +77,7 @@ struct cpufreq_policy {
>  	unsigned int		suspend_freq; /* freq to set during suspend */
>  
>  	unsigned int		policy; /* see above */
> +	unsigned int		last_policy; /* policy before unplug */
>  	struct cpufreq_governor	*governor; /* see below */
>  	void			*governor_data;
>  	bool			governor_enabled; /* governor start/stop flag */
> 

Thanks,
Rafael

Re: [PATCH] cpufreq: use last policy after online

[Viresh Kumar]

On 01-12-15, 16:52, Srinivas Pandruvada wrote:
> For cpufreq drivers which use setpolicy interface, after offline->online
> the policy is set to default. This can be reproduced by setting the
> default policy of intel_pstate or longrun to ondemand and then change to
> "performance". After offline and online, the setpolicy will be called with
> the policy=ondemand.
> For drivers using governors this condition is handled by storing
> last_governor, during offline and restoring during online. The same should
> be done for drivers using setpolicy interface. Storing last_policy during
> offline and restoring during online.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/cpufreq.c | 14 ++++++++++----
>  include/linux/cpufreq.h   |  1 +
>  2 files changed, 11 insertions(+), 4 deletions(-)

Okay, the problem looks real. But I think it should be solved a bit
differently.

Try partly reverting the below commit, so that we can use user_policy
instead.

88dc43849587 ("cpufreq: remove redundant 'policy' field from
user_policy")

-- 
viresh

Re: [PATCH] cpufreq: use last policy after online

[Viresh Kumar]

On 02-12-15, 03:05, Rafael J. Wysocki wrote:
> On Tuesday, December 01, 2015 04:52:14 PM Srinivas Pandruvada wrote:
> > For cpufreq drivers which use setpolicy interface, after offline->online
> > the policy is set to default. This can be reproduced by setting the
> > default policy of intel_pstate or longrun to ondemand and then change to
> > "performance". After offline and online, the setpolicy will be called with
> > the policy=ondemand.
> > For drivers using governors this condition is handled by storing
> > last_governor, during offline and restoring during online. The same should
> > be done for drivers using setpolicy interface. Storing last_policy during
> > offline and restoring during online.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> I guess this is urgent, right?
> 
> Is it a regression fix or has it just never worked?

I tried to look at history and I am not sure if it worked earlier or
not.

-- 
viresh

Re: [PATCH] cpufreq: use last policy after online

[Srinivas Pandruvada]

On Wed, 2015-12-02 at 08:23 +0530, Viresh Kumar wrote:
> On 01-12-15, 16:52, Srinivas Pandruvada wrote:
> > For cpufreq drivers which use setpolicy interface, after offline->online
> > the policy is set to default. This can be reproduced by setting the
> > default policy of intel_pstate or longrun to ondemand and then change to
> > "performance". After offline and online, the setpolicy will be called with
> > the policy=ondemand.
> > For drivers using governors this condition is handled by storing
> > last_governor, during offline and restoring during online. The same should
> > be done for drivers using setpolicy interface. Storing last_policy during
> > offline and restoring during online.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c | 14 ++++++++++----
> >  include/linux/cpufreq.h   |  1 +
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> Okay, the problem looks real. But I think it should be solved a bit
> differently.
> 
> Try partly reverting the below commit, so that we can use user_policy
> instead.
We have to store somewhere. I can, but my approach matching what using
governors do to store and restore policy. In this way we have consistent
processing for has_target() and setpolicy() drivers.
> 
> 88dc43849587 ("cpufreq: remove redundant 'policy' field from
> user_policy")
> 

Re: [PATCH] cpufreq: use last policy after online

[Srinivas Pandruvada]

On Wed, 2015-12-02 at 03:05 +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 01, 2015 04:52:14 PM Srinivas Pandruvada wrote:
> > For cpufreq drivers which use setpolicy interface, after offline->online
> > the policy is set to default. This can be reproduced by setting the
> > default policy of intel_pstate or longrun to ondemand and then change to
> > "performance". After offline and online, the setpolicy will be called with
> > the policy=ondemand.
> > For drivers using governors this condition is handled by storing
> > last_governor, during offline and restoring during online. The same should
> > be done for drivers using setpolicy interface. Storing last_policy during
> > offline and restoring during online.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> I guess this is urgent, right?
I don't think. I tried couple of old kernels, same issue.

Thanks,
Srinivas
> 
> Is it a regression fix or has it just never worked?
> 
> > ---
> >  drivers/cpufreq/cpufreq.c | 14 ++++++++++----
> >  include/linux/cpufreq.h   |  1 +
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index a83c995..8412ce5 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -976,10 +976,14 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  
> >  	new_policy.governor = gov;
> >  
> > -	/* Use the default policy if its valid. */
> > -	if (cpufreq_driver->setpolicy)
> > -		cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
> > -
> > +	/* Use the default policy if there is no last_policy. */
> > +	if (cpufreq_driver->setpolicy) {
> > +		if (policy->last_policy)
> > +			new_policy.policy = policy->last_policy;
> > +		else
> > +			cpufreq_parse_governor(gov->name, &new_policy.policy,
> > +					       NULL);
> > +	}
> >  	/* set default policy */
> >  	return cpufreq_set_policy(policy, &new_policy);
> >  }
> > @@ -1330,6 +1334,8 @@ static void cpufreq_offline_prepare(unsigned int cpu)
> >  		if (has_target())
> >  			strncpy(policy->last_governor, policy->governor->name,
> >  				CPUFREQ_NAME_LEN);
> > +		else
> > +			policy->last_policy = policy->policy;
> >  	} else if (cpu == policy->cpu) {
> >  		/* Nominate new CPU */
> >  		policy->cpu = cpumask_any(policy->cpus);
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index ef4c5b1..177c768 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -77,6 +77,7 @@ struct cpufreq_policy {
> >  	unsigned int		suspend_freq; /* freq to set during suspend */
> >  
> >  	unsigned int		policy; /* see above */
> > +	unsigned int		last_policy; /* policy before unplug */
> >  	struct cpufreq_governor	*governor; /* see below */
> >  	void			*governor_data;
> >  	bool			governor_enabled; /* governor start/stop flag */
> > 
> 
> Thanks,
> Rafael
> 

Re: [PATCH] cpufreq: use last policy after online

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:05:27 PM Srinivas Pandruvada wrote:
> On Wed, 2015-12-02 at 03:05 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 01, 2015 04:52:14 PM Srinivas Pandruvada wrote:
> > > For cpufreq drivers which use setpolicy interface, after offline->online
> > > the policy is set to default. This can be reproduced by setting the
> > > default policy of intel_pstate or longrun to ondemand and then change to
> > > "performance". After offline and online, the setpolicy will be called with
> > > the policy=ondemand.
> > > For drivers using governors this condition is handled by storing
> > > last_governor, during offline and restoring during online. The same should
> > > be done for drivers using setpolicy interface. Storing last_policy during
> > > offline and restoring during online.
> > > 
> > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > 
> > I guess this is urgent, right?
> I don't think. I tried couple of old kernels, same issue.

OK, thanks for checking!

Rafael

Re: [PATCH] cpufreq: use last policy after online

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 08:56:45 AM Srinivas Pandruvada wrote:
> On Wed, 2015-12-02 at 08:23 +0530, Viresh Kumar wrote:
> > On 01-12-15, 16:52, Srinivas Pandruvada wrote:
> > > For cpufreq drivers which use setpolicy interface, after offline->online
> > > the policy is set to default. This can be reproduced by setting the
> > > default policy of intel_pstate or longrun to ondemand and then change to
> > > "performance". After offline and online, the setpolicy will be called with
> > > the policy=ondemand.
> > > For drivers using governors this condition is handled by storing
> > > last_governor, during offline and restoring during online. The same should
> > > be done for drivers using setpolicy interface. Storing last_policy during
> > > offline and restoring during online.
> > > 
> > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > ---
> > >  drivers/cpufreq/cpufreq.c | 14 ++++++++++----
> > >  include/linux/cpufreq.h   |  1 +
> > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > Okay, the problem looks real. But I think it should be solved a bit
> > differently.
> > 
> > Try partly reverting the below commit, so that we can use user_policy
> > instead.
> We have to store somewhere. I can, but my approach matching what using
> governors do to store and restore policy. In this way we have consistent
> processing for has_target() and setpolicy() drivers.

Agreed, consistency matters.

OK, patch queued up for 4.4-rc, thanks!

Rafael