Messages in this thread | | | Subject | Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose. | From | Yanmin Zhang <> | Date | Tue, 06 Mar 2012 09:54:45 +0800 |
| |
On Mon, 2012-03-05 at 14:20 +0200, Valentin, Eduardo wrote: > Hello, > > > On Mon, Mar 5, 2012 at 12:18 PM, Henrique de Moraes Holschuh > <hmh@hmh.eng.br> 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... > > > > 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)? > > I may be jumping the thread in the middle but, if it is for debug > purposes, as states the subject, shouldn't this entry go to debugfs > instead of sysfs? I know cpuidle has all the infrastructure there to > simply add another sysfs entry, but if the intent is to create a debug > capability, then I'd say it fits under debugfs instead. Adding Greg > KH here, as I suppose he may have strong opinion on using sysfs for > debugging. Thanks for the comments.
IMHO, all entries under cpuidle directory are for debug purpose. End users shouldn't care about them. If we rewrite codes around all the entries, I strongly agree that we need move them to debugfs.
Here, we just add a new entry under same directory. If we create it under debugfs, we need create the similar directory tree, which is a duplicate effort. In addition, users might be confused that why we separate the entries under sysfs and debugfs.
> > Of course, if you are targeting user space control over C states on > production systems, then it's a different thing then. Although we say it's mostly for debug purpose used by developers, end users could use it on production systems. For example, they might get a new machine and installed an official Linux OS on it. They could do some tuning without recompiling the kernel when recompiling is impossible.
| |