[PATCH 1/2] cpuidle: fix finding state with min power_usage

[PATCH 1/2] cpuidle: fix finding state with min power_usage

[Sivaram Nair]

Since cpuidle_state.power_usage is a signed value, use INT_MAX (instead
of -1) to init the local copies so that functions that tries to find
cpuidle states with minimum power usage works correctly even if they use
non-negative values.

Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
---
 drivers/cpuidle/cpuidle.c        |    2 +-
 drivers/cpuidle/governors/menu.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8df53dd..fb4a7dd 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -70,7 +70,7 @@ int cpuidle_play_dead(void)
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int i, dead_state = -1;
-	int power_usage = -1;
+	int power_usage = INT_MAX;
 
 	if (!drv)
 		return -ENODEV;
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bd40b94..20ea33a 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -312,7 +312,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
 	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
-	int power_usage = -1;
+	int power_usage = INT_MAX;
 	int i;
 	int multiplier;
 	struct timespec t;
-- 
1.7.9.5

[PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Sivaram Nair]

cpuidle_state->power_usage is signed; so change the corresponding sysfs
ops to output signed value instead of unsigned.

Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
---
 drivers/cpuidle/sysfs.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 3409429..2fc79cd 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -232,6 +232,13 @@ static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
 static ssize_t show_state_##_name(struct cpuidle_state *state, \
 			 struct cpuidle_state_usage *state_usage, char *buf) \
 { \
+	return sprintf(buf, "%d\n", state->_name);\
+}
+
+#define define_show_state_u_function(_name) \
+static ssize_t show_state_##_name(struct cpuidle_state *state, \
+			 struct cpuidle_state_usage *state_usage, char *buf) \
+{ \
 	return sprintf(buf, "%u\n", state->_name);\
 }
 
@@ -270,7 +277,7 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
 	return sprintf(buf, "%s\n", state->_name);\
 }
 
-define_show_state_function(exit_latency)
+define_show_state_u_function(exit_latency)
 define_show_state_function(power_usage)
 define_show_state_ull_function(usage)
 define_show_state_ull_function(time)
-- 
1.7.9.5

Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Rafael J. Wysocki]

On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> cpuidle_state->power_usage is signed; so change the corresponding sysfs
> ops to output signed value instead of unsigned.

What's actually wrong with printing it as an unsigned int?

Rafael


> Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
> ---
>  drivers/cpuidle/sysfs.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 3409429..2fc79cd 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -232,6 +232,13 @@ static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
>  static ssize_t show_state_##_name(struct cpuidle_state *state, \
>  			 struct cpuidle_state_usage *state_usage, char *buf) \
>  { \
> +	return sprintf(buf, "%d\n", state->_name);\
> +}
> +
> +#define define_show_state_u_function(_name) \
> +static ssize_t show_state_##_name(struct cpuidle_state *state, \
> +			 struct cpuidle_state_usage *state_usage, char *buf) \
> +{ \
>  	return sprintf(buf, "%u\n", state->_name);\
>  }
>  
> @@ -270,7 +277,7 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
>  	return sprintf(buf, "%s\n", state->_name);\
>  }
>  
> -define_show_state_function(exit_latency)
> +define_show_state_u_function(exit_latency)
>  define_show_state_function(power_usage)
>  define_show_state_ull_function(usage)
>  define_show_state_ull_function(time)
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Sivaram Nair]

On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > ops to output signed value instead of unsigned.
> 
> What's actually wrong with printing it as an unsigned int?

power_usage could have negative values (for example cpuidle/driver.c
inits this value to -1, -2 etc. when drv->power_specified is not set) and
these shows up badly in the sysfs output.

regards,
Sivaram

Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Rafael J. Wysocki]

On Monday, December 17, 2012 09:38:15 AM Sivaram Nair wrote:
> On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > > ops to output signed value instead of unsigned.
> > 
> > What's actually wrong with printing it as an unsigned int?
> 
> power_usage could have negative values (for example cpuidle/driver.c
> inits this value to -1, -2 etc. when drv->power_specified is not set) and
> these shows up badly in the sysfs output.

Does "badly" mean "as big positive numbers"?

Should we actually print them at all in those case?  Perhaps it'll be better to
make the file appear empty then?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Sivaram Nair]

On Mon, Dec 17, 2012 at 08:56:45AM +0100, Rafael J. Wysocki wrote:
> On Monday, December 17, 2012 09:38:15 AM Sivaram Nair wrote:
> > On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> > > On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > > > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > > > ops to output signed value instead of unsigned.
> > > 
> > > What's actually wrong with printing it as an unsigned int?
> > 
> > power_usage could have negative values (for example cpuidle/driver.c
> > inits this value to -1, -2 etc. when drv->power_specified is not set) and
> > these shows up badly in the sysfs output.
> 
> Does "badly" mean "as big positive numbers"?

Yes (sorry for not being clearer).

> 
> Should we actually print them at all in those case?  Perhaps it'll be better to
> make the file appear empty then?

May be, but why is power_usage signed in the first place? I also noticed
Daniel Lezcano's patches that reduces the scope of this variable. So,
perhaps we can just ignore this change.

-Sivaram

Re: [PATCH 2/2] cpuidle: fix sysfs output for power_usage

[Rafael J. Wysocki]

On Monday, December 17, 2012 10:46:07 AM Sivaram Nair wrote:
> On Mon, Dec 17, 2012 at 08:56:45AM +0100, Rafael J. Wysocki wrote:
> > On Monday, December 17, 2012 09:38:15 AM Sivaram Nair wrote:
> > > On Sat, Dec 15, 2012 at 01:03:02AM +0100, Rafael J. Wysocki wrote:
> > > > On Friday, December 14, 2012 03:17:37 PM Sivaram Nair wrote:
> > > > > cpuidle_state->power_usage is signed; so change the corresponding sysfs
> > > > > ops to output signed value instead of unsigned.
> > > > 
> > > > What's actually wrong with printing it as an unsigned int?
> > > 
> > > power_usage could have negative values (for example cpuidle/driver.c
> > > inits this value to -1, -2 etc. when drv->power_specified is not set) and
> > > these shows up badly in the sysfs output.
> > 
> > Does "badly" mean "as big positive numbers"?
> 
> Yes (sorry for not being clearer).
> 
> > 
> > Should we actually print them at all in those case?  Perhaps it'll be better to
> > make the file appear empty then?
> 
> May be, but why is power_usage signed in the first place?

Please read the comment in driver.c:set_power_states() for the answer. :-)

> I also noticed
> Daniel Lezcano's patches that reduces the scope of this variable. So,
> perhaps we can just ignore this change.

OK

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] cpuidle: fix finding state with min power_usage

[Rik van Riel]

On 12/14/2012 08:17 AM, Sivaram Nair wrote:
> Since cpuidle_state.power_usage is a signed value, use INT_MAX (instead
> of -1) to init the local copies so that functions that tries to find
> cpuidle states with minimum power usage works correctly even if they use
> non-negative values.
>
> Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH 1/2] cpuidle: fix finding state with min power_usage

[Rafael J. Wysocki]

On Friday, December 14, 2012 03:17:36 PM Sivaram Nair wrote:
> Since cpuidle_state.power_usage is a signed value, use INT_MAX (instead
> of -1) to init the local copies so that functions that tries to find
> cpuidle states with minimum power usage works correctly even if they use
> non-negative values.

I'm queuing this up for submission as v3.8 material.

Thanks,
Rafael


> Signed-off-by: Sivaram Nair <sivaramn@nvidia.com>
> ---
>  drivers/cpuidle/cpuidle.c        |    2 +-
>  drivers/cpuidle/governors/menu.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 8df53dd..fb4a7dd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -70,7 +70,7 @@ int cpuidle_play_dead(void)
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int i, dead_state = -1;
> -	int power_usage = -1;
> +	int power_usage = INT_MAX;
>  
>  	if (!drv)
>  		return -ENODEV;
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index bd40b94..20ea33a 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -312,7 +312,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  {
>  	struct menu_device *data = &__get_cpu_var(menu_devices);
>  	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
> -	int power_usage = -1;
> +	int power_usage = INT_MAX;
>  	int i;
>  	int multiplier;
>  	struct timespec t;
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.