Re: [Update PATCH 1/1] Cpufreq: Make governor data on nonboot cpus across system suspend/resume

[Lan Tianyu <tianyu.lan@intel.com>]

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, 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.


>> 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@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;
>> +
>>   	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;
>>


-- 
Best Regards
Tianyu Lan