| Message ID | 1355491060-970-2-git-send-email-sivaramn@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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) >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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)
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(-)