| Message ID | 1449017534-6127-1-git-send-email-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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")
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.
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") > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)