On Saturday, November 16, 2013 11:59:59 AM Lan Tianyu wrote:
> On 11/16/2013 08:38 AM, Rafael J. Wysocki wrote:
> > On Friday, November 15, 2013 04:15:34 PM Lan Tianyu wrote:
> >> Currently, governor of nonboot cpus will be put to EXIT when system
> >> suspend.
> >> Since all these cpus will be unplugged and the governor usage_count
> >> decreases
> >> to zero. The governor data and its sysfs interfaces will be freed or
> >> released.
> >> This makes user config of these governors loss during suspend and resume.
> >
> > First off, do we have a pointer to a bug report related to that?
> >
>
> No, I found this bug when I tried to resolve other similar bug.
> https://bugzilla.kernel.org/show_bug.cgi?id=63081. I still have no idea
> about bug 63081 and asked reporter to try this patch.
>
> > Second, what does need to be done to reproduce this problem?
> >
>
> Defaultly, all cpus use ondemand governor after bootup. Change one
> non-boot cpu's governor to conservative,
Well, why would anyone want to do that? Just out of curiosity ...
> modify conservative config via sysfs interface and then do system suspend.
> After resume, the config of conservative is reset. On my machine, all cpus
> have owen policy.
So this is acpi-cpufreq, right?
The patch looks basically OK to me, but ->
> >> This doesn't happen on the governor covering boot cpu because it isn't
> >> unplugged during system suspend.
> >>
> >> To fix this issue, skipping governor exit during system suspend and check
> >> policy governor data to determine whether the governor is really needed
> >> to be initialized when do init. If not, return EALREADY to indicate the
> >> governor has been initialized and should do nothing. __cpufreq_governor()
> >> convert EALREADY to 0 as return value for INIT event since governor is
> >> still under INIT state and can do START operation.
> >>
> >> Signed-off-by: Lan Tianyu <tianyu....@intel.com>
> >> ---
> >> Fix some typos
> >>
> >> drivers/cpufreq/cpufreq.c | 5 ++++-
> >> drivers/cpufreq/cpufreq_governor.c | 13 ++++++++++++-
> >> 2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index 02d534d..38f2e4a 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1239,7 +1239,7 @@ static int __cpufreq_remove_dev_finish(struct device
> >> *dev,
> >>
> >> /* If cpu is last user of policy, free policy */
> >> if (cpus == 1) {
> >> - if (has_target()) {
> >> + if (has_target() && !frozen) {
> >> ret = __cpufreq_governor(policy,
> >> CPUFREQ_GOV_POLICY_EXIT);
> >> if (ret) {
> >> @@ -1822,6 +1822,9 @@ static int __cpufreq_governor(struct cpufreq_policy
> >> *policy,
> >> ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> >> module_put(policy->governor->owner);
> >>
> >> + if ((event == CPUFREQ_GOV_POLICY_INIT) && ret == -EALREADY)
> >> + ret = 0;
> >> +
-> I'd prefer this check to be combined with the one done to determine whether
or not we need to do the module_put(). Something like
if (event == CPUFREQ_GOV_POLICY_EXIT && ret) {
module_put(policy->governor->owner);
if (ret == -EALREADY)
return 0;
} else if (event == CPUFREQ_GOV_POLICY_EXIT && !ret) {
module_put(policy->governor->owner);
}
Thanks!
> >> return ret;
> >> }
> >>
> >> diff --git a/drivers/cpufreq/cpufreq_governor.c
> >> b/drivers/cpufreq/cpufreq_governor.c
> >> index 0806c31..ddb93af 100644
> >> --- a/drivers/cpufreq/cpufreq_governor.c
> >> +++ b/drivers/cpufreq/cpufreq_governor.c
> >> @@ -204,9 +204,20 @@ int cpufreq_governor_dbs(struct cpufreq_policy
> >> *policy,
> >>
> >> switch (event) {
> >> case CPUFREQ_GOV_POLICY_INIT:
> >> + /*
> >> + * In order to keep governor data across suspend/resume,
> >> + * Governor doesn't exit when suspend and will be
> >> + * reinitialized when resume. Here check policy governor
> >> + * data to determine whether the governor has been exited.
> >> + * If not, return EALREADY.
> >> + */
> >> if (have_governor_per_policy()) {
> >> - WARN_ON(dbs_data);
> >> + if (dbs_data)
> >> + return -EALREADY;
> >> } else if (dbs_data) {
> >> + if (policy->governor_data == dbs_data)
> >> + return -EALREADY;
> >> +
> >> dbs_data->usage_count++;
> >> policy->governor_data = dbs_data;
> >> return 0;
> >>
>
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
