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.lan@xxxxxxxxx> > >> --- > >> 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 cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
