Re: [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

Yanmin Zhang
2012-03-06T01:05:09Z

On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 05 Mar 2012, ShuoX Liu wrote:
> > @@ -45,6 +46,7 @@ total 0
> > /sys/devices/system/cpu/cpu0/cpuidle/state1:
> > total 0
> > -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> > +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> > -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> > -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> > -r--r--r-- 1 root root 4096 Feb 8 10:42 power
>
> ...
>
> > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> > index 3fe41fe..1eae29a 100644
> > --- a/drivers/cpuidle/sysfs.c
> > +++ b/drivers/cpuidle/sysfs.c
> > @@ -222,6 +222,9 @@ struct cpuidle_state_attr {
> > #define define_one_state_ro(_name, show) \
> > static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0444,
> > show, NULL)
> >
> > +#define define_one_state_rw(_name, show, store) \
> > +static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644,
> > show, store)
> > +
> > #define define_show_state_function(_name) \
> > static ssize_t show_state_##_name(struct cpuidle_state *state, \
> > struct cpuidle_state_usage *state_usage, char *buf) \
> > @@ -229,6 +232,19 @@ static ssize_t show_state_##_name(struct
> > cpuidle_state *state, \
> > return sprintf(buf, "%u\n", state->_name);\
> > }
> >
> > +#define define_store_state_function(_name) \
> > +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> > + const char *buf, size_t size) \
> > +{ \
> > + int value; \
> > + sscanf(buf, "%d", &value); \
> > + if (value) \
> > + state->disable = 1; \
> > + else \
> > + state->disable = 0; \
> > + return size; \
> > +}
>
> Isn't this missing a check for capabilities? Disabling cpuidle states is
> not something random Joe (and IMHO that does mean random capability-
> restricted Joe root) should be doing...
Sorry. Could you elaborate it?

Basically, CPU initialization codes (ACPI on x86) checks the capabilities and
creates the C states in kernel. If we find there are such C states under
/sys/devices/system/cpu/cpuXXX/cpuidle/, we assume CPU supports them.
So here we wouldn't check them again.

>
> Also, maybe it would be best to use one of the lib helpers to parse that
> value, so that it will be less annoying to userspace (trim blanks, complain
> if there is trailing junk after trimming, etc)?
We would use strict_strtol to parse the value in next version.

Thanks for your kind comments!