cpufreq governor stop and start should be kept in sequence.
If not, there will be unexpected behavior, for example:
we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0.
the normal sequence is as below:
1) Current governor is userspace, one application tries to set
governor to ondemand. it will call __cpufreq_set_policy in which it
will stop userspace governor and then start ondemand governor.
2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
call cpufreq_add_policy_cpu. on which it first stops userspace
governor, and then starts userspace governor.
Now if the sequence of above two cases interleaves, it becames
below sequence:
1) application stops userspace governor
2) hotplug stops userspace governor
3) application starts ondemand governor
4) hotplug starts a governor
in step 4, hotplug is supposed to start userspace governor, but now
the governor has been changed by application to ondemand, so hotplug
starts ondemand governor again !!!!
The solution is: do not allow stop one policy's governor multi-times
Governor stop should only do once for one policy, after it is stopped,
no other governor stop should be executed.
Signed-off-by: Xiaoguang Chen <[email protected]>
---
drivers/cpufreq/cpufreq.c | 9 +++++++++
include/linux/cpufreq.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..b4a2c94 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1562,6 +1562,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
pr_debug("__cpufreq_governor for CPU %u, event %u\n",
policy->cpu, event);
+
+ if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
+ (policy->governor_enabled && (event == CPUFREQ_GOV_START)))
+ return 0;
+
ret = policy->governor->governor(policy, event);
if (!ret) {
@@ -1569,6 +1574,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
policy->governor->initialized++;
else if (event == CPUFREQ_GOV_POLICY_EXIT)
policy->governor->initialized--;
+ else if (event == CPUFREQ_GOV_STOP)
+ policy->governor_enabled = 0;
+ else if (event == CPUFREQ_GOV_START)
+ policy->governor_enabled = 1;
}
/* we keep one module reference alive for
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..c12db73 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -107,6 +107,7 @@ struct cpufreq_policy {
unsigned int policy; /* see above */
struct cpufreq_governor *governor; /* see below */
void *governor_data;
+ int governor_enabled; /* governor start/stop flag */
struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
1.8.0
On 12 June 2013 12:56, Xiaoguang Chen <[email protected]> wrote:
> cpufreq governor stop and start should be kept in sequence.
> If not, there will be unexpected behavior, for example:
>
> we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0.
> the normal sequence is as below:
>
> 1) Current governor is userspace, one application tries to set
> governor to ondemand. it will call __cpufreq_set_policy in which it
> will stop userspace governor and then start ondemand governor.
>
> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
> call cpufreq_add_policy_cpu. on which it first stops userspace
> governor, and then starts userspace governor.
>
> Now if the sequence of above two cases interleaves, it becames
> below sequence:
>
> 1) application stops userspace governor
> 2) hotplug stops userspace governor
> 3) application starts ondemand governor
> 4) hotplug starts a governor
>
> in step 4, hotplug is supposed to start userspace governor, but now
> the governor has been changed by application to ondemand, so hotplug
> starts ondemand governor again !!!!
>
> The solution is: do not allow stop one policy's governor multi-times
> Governor stop should only do once for one policy, after it is stopped,
> no other governor stop should be executed.
>
> Signed-off-by: Xiaoguang Chen <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 9 +++++++++
> include/linux/cpufreq.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2d53f47..b4a2c94 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1562,6 +1562,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>
> pr_debug("__cpufreq_governor for CPU %u, event %u\n",
> policy->cpu, event);
> +
> + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)))
> + return 0;
Few things:
- because __cpufreq_governor() isn't protected by locks, both calls for
stopping governor can reach to this point and race can still happen. So,
both may stop governor.
- Returning zero doesn't seems to be the right thing, as this may cause
STOP called by one user and START called by another. For example,
STOP happened due to governor change. STOP called for hotplug and
0 is returned. START called by hotplug as STOP was successful. :)
Maybe -EBUSY would make more sense here.
2013/6/12 Viresh Kumar <[email protected]>:
> On 12 June 2013 12:56, Xiaoguang Chen <[email protected]> wrote:
>> cpufreq governor stop and start should be kept in sequence.
>> If not, there will be unexpected behavior, for example:
>>
>> we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0.
>> the normal sequence is as below:
>>
>> 1) Current governor is userspace, one application tries to set
>> governor to ondemand. it will call __cpufreq_set_policy in which it
>> will stop userspace governor and then start ondemand governor.
>>
>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
>> call cpufreq_add_policy_cpu. on which it first stops userspace
>> governor, and then starts userspace governor.
>>
>> Now if the sequence of above two cases interleaves, it becames
>> below sequence:
>>
>> 1) application stops userspace governor
>> 2) hotplug stops userspace governor
>> 3) application starts ondemand governor
>> 4) hotplug starts a governor
>>
>> in step 4, hotplug is supposed to start userspace governor, but now
>> the governor has been changed by application to ondemand, so hotplug
>> starts ondemand governor again !!!!
>>
>> The solution is: do not allow stop one policy's governor multi-times
>> Governor stop should only do once for one policy, after it is stopped,
>> no other governor stop should be executed.
>>
>> Signed-off-by: Xiaoguang Chen <[email protected]>
>> ---
>> drivers/cpufreq/cpufreq.c | 9 +++++++++
>> include/linux/cpufreq.h | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 2d53f47..b4a2c94 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1562,6 +1562,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>>
>> pr_debug("__cpufreq_governor for CPU %u, event %u\n",
>> policy->cpu, event);
>> +
>> + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
>> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)))
>> + return 0;
>
> Few things:
> - because __cpufreq_governor() isn't protected by locks, both calls for
> stopping governor can reach to this point and race can still happen. So,
> both may stop governor.
Ok, I'll think about how to protect this.
> - Returning zero doesn't seems to be the right thing, as this may cause
> STOP called by one user and START called by another. For example,
> STOP happened due to governor change. STOP called for hotplug and
> 0 is returned. START called by hotplug as STOP was successful. :)
>
> Maybe -EBUSY would make more sense here.
I thought about return one error code before. but I checked the code
and found that
most of the governor stop/start operations don't check the return
value. I can add error code
here, I think the checking of return value is also needed. May be
another patch can do this.
Thanks
Xiaoguang
On 12 June 2013 14:13, Xiaoguang Chen <[email protected]> wrote:
> I thought about return one error code before. but I checked the code
> and found that
> most of the governor stop/start operations don't check the return
> value. I can add error code
> here, I think the checking of return value is also needed. May be
> another patch can do this.
Yeah.