[PATCH] return error when failing to set minfreq

[PATCH] return error when failing to set minfreq

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

Hello,
I just stumbled on this bug/feature, this is how to reproduce it:

# echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
# echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
# echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
# cpufreq-info -p
450000 450000 powersave
# echo 1800000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq ; echo $?
0
# cpufreq-info -p             
450000 450000 powersave

Here it is. The kernel refuses to set a min_freq higher than the
max_freq but it allows a max_freq lower than min_freq (lowering min_freq
also).

I'm unsure if things could be improved here.  The offending instruction
is in cpufreq_verify_within_limits (cpufreq.h:241):

	if (policy->min > policy->max)
                policy->min = policy->max;

This behaviour is pretty straightforward (but undocumented) and it
doesn't return an error altough failing to accomplish the requested
action (set min_freq).
The problem (IMO) is basically that userspace is not allowed to set a
full policy atomically while the kernel always does that thus it must
enforce an ordering on operations.

The attached patch returns -EINVAL if trying to increase frequencies
starting from scaling_min_freq and documents the correct ordering of
writes.

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

[-- Attachment #2: cpufreq_error_when_increasing_minfreq_before_maxfreq.diff --]
[-- Type: text/plain, Size: 1263 bytes --]

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 29b2fa5..a78967f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1349,6 +1349,11 @@ static int __cpufreq_set_policy(struct c
 
 	memcpy(&policy->cpuinfo, &data->cpuinfo, sizeof(struct cpufreq_cpuinfo));
 
+	if (policy->min > data->min && policy->min > policy->max) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
 	/* verify the cpu speed can be set within this limit */
 	ret = cpufreq_driver->verify(policy);
 	if (ret)
diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
index 7fedc00..555c8cf 100644
--- a/Documentation/cpu-freq/user-guide.txt
+++ b/Documentation/cpu-freq/user-guide.txt
@@ -153,10 +153,13 @@ scaling_governor,		and by "echoing" the 
 				that some governors won't load - they only
 				work on some specific architectures or
 				processors.
-scaling_min_freq and 
+scaling_min_freq and
 scaling_max_freq		show the current "policy limits" (in
 				kHz). By echoing new values into these
 				files, you can change these limits.
+				NOTE: when setting a policy you need to
+				first set scaling_max_freq, then
+				scaling_min_freq.
 
 
 If you have selected the "userspace" governor which allows you to

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] return error when failing to set minfreq

[Mattia Dongili]

Hello,

no comment on the following?

On Fri, May 26, 2006 at 12:34:04AM +0200, Mattia Dongili wrote:
> Hello,
> I just stumbled on this bug/feature, this is how to reproduce it:
> 
> # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> # echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> # cpufreq-info -p
> 450000 450000 powersave
> # echo 1800000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq ; echo $?
> 0
> # cpufreq-info -p             
> 450000 450000 powersave
> 
> Here it is. The kernel refuses to set a min_freq higher than the
> max_freq but it allows a max_freq lower than min_freq (lowering min_freq
> also).
> 
> I'm unsure if things could be improved here.  The offending instruction
> is in cpufreq_verify_within_limits (cpufreq.h:241):
> 
> 	if (policy->min > policy->max)
>                 policy->min = policy->max;
> 
> This behaviour is pretty straightforward (but undocumented) and it
> doesn't return an error altough failing to accomplish the requested
> action (set min_freq).
> The problem (IMO) is basically that userspace is not allowed to set a
> full policy atomically while the kernel always does that thus it must
> enforce an ordering on operations.
> 
> The attached patch returns -EINVAL if trying to increase frequencies
> starting from scaling_min_freq and documents the correct ordering of
> writes.
> 
> Signed-off-by: Mattia Dongili <malattia@linux.it>
> -- 
> mattia
> :wq!

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 29b2fa5..a78967f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1349,6 +1349,11 @@ static int __cpufreq_set_policy(struct c
>  
>  	memcpy(&policy->cpuinfo, &data->cpuinfo, sizeof(struct cpufreq_cpuinfo));
>  
> +	if (policy->min > data->min && policy->min > policy->max) {
> +		ret = -EINVAL;
> +		goto error_out;
> +	}
> +
>  	/* verify the cpu speed can be set within this limit */
>  	ret = cpufreq_driver->verify(policy);
>  	if (ret)
> diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
> index 7fedc00..555c8cf 100644
> --- a/Documentation/cpu-freq/user-guide.txt
> +++ b/Documentation/cpu-freq/user-guide.txt
> @@ -153,10 +153,13 @@ scaling_governor,		and by "echoing" the 
>  				that some governors won't load - they only
>  				work on some specific architectures or
>  				processors.
> -scaling_min_freq and 
> +scaling_min_freq and
>  scaling_max_freq		show the current "policy limits" (in
>  				kHz). By echoing new values into these
>  				files, you can change these limits.
> +				NOTE: when setting a policy you need to
> +				first set scaling_max_freq, then
> +				scaling_min_freq.
>  
>  
>  If you have selected the "userspace" governor which allows you to

> _______________________________________________
> Cpufreq mailing list
> Cpufreq@lists.linux.org.uk
> http://lists.linux.org.uk/mailman/listinfo/cpufreq

-- 
mattia
:wq!

Re: [PATCH] return error when failing to set minfreq

[Dominik Brodowski]

On Mon, Jun 05, 2006 at 08:31:48PM +0200, Mattia Dongili wrote:
> Hello,
> 
> no comment on the following?
> 
> On Fri, May 26, 2006 at 12:34:04AM +0200, Mattia Dongili wrote:
> > Hello,
> > I just stumbled on this bug/feature, this is how to reproduce it:
> > 
> > # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> > # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> > # echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> > # cpufreq-info -p
> > 450000 450000 powersave
> > # echo 1800000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq ; echo $?
> > 0
> > # cpufreq-info -p             
> > 450000 450000 powersave
> > 
> > Here it is. The kernel refuses to set a min_freq higher than the
> > max_freq but it allows a max_freq lower than min_freq (lowering min_freq
> > also).
> > 
> > I'm unsure if things could be improved here.  The offending instruction
> > is in cpufreq_verify_within_limits (cpufreq.h:241):
> > 
> > 	if (policy->min > policy->max)
> >                 policy->min = policy->max;
> > 
> > This behaviour is pretty straightforward (but undocumented) and it
> > doesn't return an error altough failing to accomplish the requested
> > action (set min_freq).
> > The problem (IMO) is basically that userspace is not allowed to set a
> > full policy atomically while the kernel always does that thus it must
> > enforce an ordering on operations.
> > 
> > The attached patch returns -EINVAL if trying to increase frequencies
> > starting from scaling_min_freq and documents the correct ordering of
> > writes.
> > 
> > Signed-off-by: Mattia Dongili <malattia@linux.it>

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>

Thanks,
	Dominik

[PATCH] return error when failing to set minfreq

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

Hello,

I'm resending this patch that probably has been lost in deep space.
It got the signed-off by Dominik before:
http://lists.linux.org.uk/mailman/private/cpufreq/2006-June/005916.html
The same message contains a detailed explanation of the problem it
fixes.
Rediffed against 2.6.17-mm6.

Description:
return -EINVAL if trying to increase frequencies starting from
scaling_min_freq and documents the correct ordering of writes.

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

[-- Attachment #2: cpufreq_error_when_increasing_minfreq_before_maxfreq.diff --]
[-- Type: text/plain, Size: 1263 bytes --]

diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
index 7fedc00..555c8cf 100644
--- a/Documentation/cpu-freq/user-guide.txt
+++ b/Documentation/cpu-freq/user-guide.txt
@@ -153,10 +153,13 @@ scaling_governor,		and by "echoing" the 
 				that some governors won't load - they only
 				work on some specific architectures or
 				processors.
-scaling_min_freq and 
+scaling_min_freq and
 scaling_max_freq		show the current "policy limits" (in
 				kHz). By echoing new values into these
 				files, you can change these limits.
+				NOTE: when setting a policy you need to
+				first set scaling_max_freq, then
+				scaling_min_freq.
 
 
 If you have selected the "userspace" governor which allows you to
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1ba4039..deea3e0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1348,6 +1348,11 @@ static int __cpufreq_set_policy(struct c
 
 	memcpy(&policy->cpuinfo, &data->cpuinfo, sizeof(struct cpufreq_cpuinfo));
 
+	if (policy->min > data->min && policy->min > policy->max) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
 	/* verify the cpu speed can be set within this limit */
 	ret = cpufreq_driver->verify(policy);
 	if (ret)

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] return error when failing to set minfreq

[Dave Jones]

On Wed, Jul 05, 2006 at 11:12:20PM +0200, Mattia Dongili wrote:

 > I'm resending this patch that probably has been lost in deep space.
 > It got the signed-off by Dominik before:
 > http://lists.linux.org.uk/mailman/private/cpufreq/2006-June/005916.html
 > The same message contains a detailed explanation of the problem it
 > fixes.
 > Rediffed against 2.6.17-mm6.

Applied.
(After de-MIME'ing your mail by hand, which is possibly why it got
 dropped last time, though I usually send out grumbles when that happens
 as git-applymbox barfs on MIMEs.)

		Dave

-- 
http://www.codemonkey.org.uk