[PATCH] If max_freq got reduced (e.g. by _PPC) a write to sysfs scaling_governor let cpufreq core stuck at low max_freq for ever

[PATCH] If max_freq got reduced (e.g. by _PPC) a write to sysfs scaling_governor let cpufreq core stuck at low max_freq for ever

[Thomas Renninger]

Subject: Max freq stucks at low freq if reduced by _PPC and sysfs gov access

The problem is reproducable by(if machine is limiting freqs via BIOS):
 - Unplugging AC -> max freq gets limited
 - echo ${governor} >/sys/.../cpufreq/scaling_governor (policy->user_data.max
   gets overridden with policy->max and will never come up again.)

This patch exchanges the cpufreq_set_policy call to __cpufreq_set_policy 
in store_scaling_governor and duplicates most of it's functionality but does
not override user_data.max.

 drivers/cpufreq/cpufreq.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

Signed-off-by: Thomas Renninger <trenn@suse.de>

Index: linux-2.6.16/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6.16.orig/drivers/cpufreq/cpufreq.c
+++ linux-2.6.16/drivers/cpufreq/cpufreq.c
@@ -402,7 +402,7 @@ static ssize_t show_scaling_governor (st
 	return -EINVAL;
 }
 
-
+static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_policy *policy);
 /**
  * store_scaling_governor - store policy for the specified CPU
  */
@@ -413,19 +413,34 @@ static ssize_t store_scaling_governor (s
 	char	str_governor[16];
 	struct cpufreq_policy new_policy;
 
+	mutex_lock(&policy->lock);
 	ret = cpufreq_get_policy(&new_policy, policy->cpu);
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = sscanf (buf, "%15s", str_governor);
-	if (ret != 1)
-		return -EINVAL;
+	if (ret != 1){
+		ret =  -EINVAL;
+		goto error;
+	}
 
-	if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
-		return -EINVAL;
+	if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor)){
+		ret =  -EINVAL;
+		goto error;
+	}
+	/* Do not use cpufreq_set_policy here or the user_policy.max
+	   will be wrongly overridden */
+	ret = __cpufreq_set_policy(policy, &new_policy);
+
+	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
+	mutex_unlock(&policy->lock);
+	cpufreq_cpu_put(policy);
 
-	ret = cpufreq_set_policy(&new_policy);
 	return ret ? ret : count;
+ error:
+	mutex_unlock(&policy->lock);
+	return ret;
 }
 
 /**

Re: [PATCH] If max_freq got reduced (e.g. by _PPC) a write to sysfs scaling_governor let cpufreq core stuck at low max_freq for ever

[Thomas Renninger]

The previous patch had bugs (locking and refcount).

This one could also be related to the latest DELL reports.
But they only slip into this if a user prog (e.g. powersave daemon does when
AC got (un) plugged due to a scheme change) echos something to 
/sys/../cpufreq/scaling_governor
while the frequencies got limited by BIOS.

This one works:

Subject: Max freq stucks at low freq if reduced by _PPC and sysfs gov access

The problem is reproducable by(if machine is limiting freqs via BIOS):
 - Unplugging AC -> max freq gets limited
 - echo ${governor} >/sys/.../cpufreq/scaling_governor (policy->user_data.max
   gets overridden with policy->max and will never come up again.)

This patch exchanged the cpufreq_set_policy call to __cpufreq_set_policy and
duplicated it's functionality but did not override user_data.max.
The same happens with overridding min/max values. If freqs are limited and
you override the min freq value, the max freq global value will also get
stuck to the limited freq, even if BIOS allows all freqs again.
Last scenario does only happen if BIOS does not reduce the frequency
to the lowest value (should never happen, just for correctness...)

 drivers/cpufreq/cpufreq.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

Signed-off-by: Thomas Renninger <trenn@suse.de>

Index: linux-2.6.16/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6.16.orig/drivers/cpufreq/cpufreq.c
+++ linux-2.6.16/drivers/cpufreq/cpufreq.c
@@ -350,6 +350,8 @@ show_one(scaling_min_freq, min);
 show_one(scaling_max_freq, max);
 show_one(scaling_cur_freq, cur);
 
+static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_policy *policy);
+
 /**
  * cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs write access
  */
@@ -368,7 +370,10 @@ static ssize_t store_##file_name					\
 	if (ret != 1)							\
 		return -EINVAL;						\
 									\
-	ret = cpufreq_set_policy(&new_policy);				\
+	mutex_lock(&policy->lock);					\
+	ret = __cpufreq_set_policy(policy, &new_policy);		\
+	policy->user_policy.object = policy->object;			\
+	mutex_unlock(&policy->lock);					\
 									\
 	return ret ? ret : count;					\
 }
@@ -424,7 +429,15 @@ static ssize_t store_scaling_governor (s
 	if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
 		return -EINVAL;
 
-	ret = cpufreq_set_policy(&new_policy);
+	/* Do not use cpufreq_set_policy here or the user_policy.max
+	   will be wrongly overridden */
+	mutex_lock(&policy->lock);
+	ret = __cpufreq_set_policy(policy, &new_policy);
+
+	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
+	mutex_unlock(&policy->lock);
+
 	return ret ? ret : count;
 }

RE: [PATCH] If max_freq got reduced (e.g. by _PPC) a write to sysfs scaling_governor let cpufreq core stuck at low max_freq for ever

[Pallipadi, Venkatesh]

Looks good. 
Dave: can you add this patch.

I think we can mark cpufreq_set_policy() for removal as it is only
getting used by deprecated acpi /proc/interface. Probably mark it with
ACPI_CPUFREQ_PROC_INTF?

Thanks,
Venki

>-----Original Message-----
>From: Thomas Renninger [mailto:trenn@suse.de] 
>Sent: Thursday, April 13, 2006 6:14 AM
>To: cpufreq@lists.linux.org.uk
>Cc: Dave Jones; Pallipadi, Venkatesh
>Subject: Re: [PATCH] If max_freq got reduced (e.g. by _PPC) a 
>write to sysfs scaling_governor let cpufreq core stuck at low 
>max_freq for ever
>
>The previous patch had bugs (locking and refcount).
>
>This one could also be related to the latest DELL reports.
>But they only slip into this if a user prog (e.g. powersave 
>daemon does when
>AC got (un) plugged due to a scheme change) echos something to 
>/sys/../cpufreq/scaling_governor
>while the frequencies got limited by BIOS.
>
>This one works:
>
>Subject: Max freq stucks at low freq if reduced by _PPC and 
>sysfs gov access
>
>The problem is reproducable by(if machine is limiting freqs via BIOS):
> - Unplugging AC -> max freq gets limited
> - echo ${governor} >/sys/.../cpufreq/scaling_governor 
>(policy->user_data.max
>   gets overridden with policy->max and will never come up again.)
>
>This patch exchanged the cpufreq_set_policy call to 
>__cpufreq_set_policy and
>duplicated it's functionality but did not override user_data.max.
>The same happens with overridding min/max values. If freqs are 
>limited and
>you override the min freq value, the max freq global value 
>will also get
>stuck to the limited freq, even if BIOS allows all freqs again.
>Last scenario does only happen if BIOS does not reduce the frequency
>to the lowest value (should never happen, just for correctness...)
>
> drivers/cpufreq/cpufreq.c |   17 +++++++++++++++--
> 1 files changed, 15 insertions(+), 2 deletions(-)
>
>Signed-off-by: Thomas Renninger <trenn@suse.de>
>
>Index: linux-2.6.16/drivers/cpufreq/cpufreq.c
>===================================================================
>--- linux-2.6.16.orig/drivers/cpufreq/cpufreq.c
>+++ linux-2.6.16/drivers/cpufreq/cpufreq.c
>@@ -350,6 +350,8 @@ show_one(scaling_min_freq, min);
> show_one(scaling_max_freq, max);
> show_one(scaling_cur_freq, cur);
> 
>+static int __cpufreq_set_policy(struct cpufreq_policy *data, 
>struct cpufreq_policy *policy);
>+
> /**
>  * cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs 
>write access
>  */
>@@ -368,7 +370,10 @@ static ssize_t store_##file_name		
>			\
> 	if (ret != 1)						
>	\
> 		return -EINVAL;					
>	\
> 								
>	\
>-	ret = cpufreq_set_policy(&new_policy);			
>	\
>+	mutex_lock(&policy->lock);				
>	\
>+	ret = __cpufreq_set_policy(policy, &new_policy);	
>	\
>+	policy->user_policy.object = policy->object;		
>	\
>+	mutex_unlock(&policy->lock);				
>	\
> 								
>	\
> 	return ret ? ret : count;				
>	\
> }
>@@ -424,7 +429,15 @@ static ssize_t store_scaling_governor (s
> 	if (cpufreq_parse_governor(str_governor, 
>&new_policy.policy, &new_policy.governor))
> 		return -EINVAL;
> 
>-	ret = cpufreq_set_policy(&new_policy);
>+	/* Do not use cpufreq_set_policy here or the user_policy.max
>+	   will be wrongly overridden */
>+	mutex_lock(&policy->lock);
>+	ret = __cpufreq_set_policy(policy, &new_policy);
>+
>+	policy->user_policy.policy = policy->policy;
>+	policy->user_policy.governor = policy->governor;
>+	mutex_unlock(&policy->lock);
>+
> 	return ret ? ret : count;
> }
> 
>