| Message ID | 1411023787-11458-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thursday, September 18, 2014 03:03:07 PM Lan Tianyu wrote: > Cpufreq core introduces cpufreq_suspended flag to let cpufreq sysfs nodes > across S2RAM/S2DISK. But the flag is only set in the cpufreq_suspend() > for cpufreq drivers which have target or target_index callback. This > skips intel_pstate driver. This patch is to set the flag before checking > target or target_index callback. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> Viresh, Dirk, this looks like a fix that should go into 3.17, so I'm queuing it up accordingly. Please let me know if that's not correct. > --- > drivers/cpufreq/cpufreq.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d9fdedd..eb9bb78 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1656,6 +1656,8 @@ void cpufreq_suspend(void) > if (!cpufreq_driver) > return; > > + cpufreq_suspended = true; > + > if (!has_target()) > return; > > @@ -1670,8 +1672,6 @@ void cpufreq_suspend(void) > pr_err("%s: Failed to suspend driver: %p\n", __func__, > policy); > } > - > - cpufreq_suspended = true; > } > > /** > @@ -1687,13 +1687,13 @@ void cpufreq_resume(void) > if (!cpufreq_driver) > return; > > + cpufreq_suspended = false; > + > if (!has_target()) > return; > > pr_debug("%s: Resuming Governors\n", __func__); > > - cpufreq_suspended = false; > - > list_for_each_entry(policy, &cpufreq_policy_list, policy_list) { > if (cpufreq_driver->resume && cpufreq_driver->resume(policy)) > pr_err("%s: Failed to resume driver: %p\n", __func__, >
Sorry for being late on this. Was away for a week for Linaro Connect followed by a week of vacations :( On 18 September 2014 00:03, Lan Tianyu <tianyu.lan@intel.com> wrote: > Cpufreq core introduces cpufreq_suspended flag to let cpufreq sysfs nodes > across S2RAM/S2DISK. But the flag is only set in the cpufreq_suspend() > for cpufreq drivers which have target or target_index callback. This > skips intel_pstate driver. This patch is to set the flag before checking > target or target_index callback. Oh yes, this looks to be the right thing to do.. > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > drivers/cpufreq/cpufreq.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d9fdedd..eb9bb78 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1656,6 +1656,8 @@ void cpufreq_suspend(void) > if (!cpufreq_driver) > return; > > + cpufreq_suspended = true; > + > if (!has_target()) > return; > > @@ -1670,8 +1672,6 @@ void cpufreq_suspend(void) > pr_err("%s: Failed to suspend driver: %p\n", __func__, > policy); > } > - > - cpufreq_suspended = true; But this change is buggy.. Because you are updating 'cpufreq_suspended' before actually stopping the governor, any calls to __cpufreq_governor() will be converted to NO-operations because of this in __cpufreq_governor(): /* Don't start any governor operations if we are entering suspend */ if (cpufreq_suspended) return 0; And so the governor's will never stop :( So you need to keep the above line where it was :) -- 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 Monday, September 29, 2014 01:20:44 AM Viresh Kumar wrote: > Sorry for being late on this. Was away for a week for Linaro Connect followed > by a week of vacations :( > > On 18 September 2014 00:03, Lan Tianyu <tianyu.lan@intel.com> wrote: > > Cpufreq core introduces cpufreq_suspended flag to let cpufreq sysfs nodes > > across S2RAM/S2DISK. But the flag is only set in the cpufreq_suspend() > > for cpufreq drivers which have target or target_index callback. This > > skips intel_pstate driver. This patch is to set the flag before checking > > target or target_index callback. > > Oh yes, this looks to be the right thing to do.. > > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > > --- > > drivers/cpufreq/cpufreq.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index d9fdedd..eb9bb78 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1656,6 +1656,8 @@ void cpufreq_suspend(void) > > if (!cpufreq_driver) > > return; > > > > + cpufreq_suspended = true; > > + > > if (!has_target()) > > return; > > > > @@ -1670,8 +1672,6 @@ void cpufreq_suspend(void) > > pr_err("%s: Failed to suspend driver: %p\n", __func__, > > policy); > > } > > - > > - cpufreq_suspended = true; > > But this change is buggy.. Because you are updating 'cpufreq_suspended' > before actually stopping the governor, any calls to __cpufreq_governor() > will be converted to NO-operations because of this in __cpufreq_governor(): > > /* Don't start any governor operations if we are entering suspend */ > if (cpufreq_suspended) > return 0; > > And so the governor's will never stop :( > > So you need to keep the above line where it was :) OK, so please send a fix for that against the Linus' tree.
Sorry later response and just back from vacation. On 2014?09?29? 16:20, Viresh Kumar wrote: > But this change is buggy.. Because you are updating 'cpufreq_suspended' > before actually stopping the governor, any calls to __cpufreq_governor() > will be converted to NO-operations because of this in __cpufreq_governor(): > > /* Don't start any governor operations if we are entering suspend */ > if (cpufreq_suspended) > return 0; > > And so the governor's will never stop :( > > So you need to keep the above line where it was :) > Yes, you are right. Thanks for fixing it.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d9fdedd..eb9bb78 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1656,6 +1656,8 @@ void cpufreq_suspend(void) if (!cpufreq_driver) return; + cpufreq_suspended = true; + if (!has_target()) return; @@ -1670,8 +1672,6 @@ void cpufreq_suspend(void) pr_err("%s: Failed to suspend driver: %p\n", __func__, policy); } - - cpufreq_suspended = true; } /** @@ -1687,13 +1687,13 @@ void cpufreq_resume(void) if (!cpufreq_driver) return; + cpufreq_suspended = false; + if (!has_target()) return; pr_debug("%s: Resuming Governors\n", __func__); - cpufreq_suspended = false; - list_for_each_entry(policy, &cpufreq_policy_list, policy_list) { if (cpufreq_driver->resume && cpufreq_driver->resume(policy)) pr_err("%s: Failed to resume driver: %p\n", __func__,
Cpufreq core introduces cpufreq_suspended flag to let cpufreq sysfs nodes across S2RAM/S2DISK. But the flag is only set in the cpufreq_suspend() for cpufreq drivers which have target or target_index callback. This skips intel_pstate driver. This patch is to set the flag before checking target or target_index callback. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- drivers/cpufreq/cpufreq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)