lkml.org 
[lkml]   [2012]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
    On Tue, Mar 06, 2012 at 09:54:45AM +0800, Yanmin Zhang wrote:
    > 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.

    I totally agree, they all need to move out of sysfs.

    > 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.

    They should all be moved there, that will remove any confusion :)

    thanks,

    greg k-h


    \
     
     \ /
      Last update: 2012-03-06 06:31    [W:0.028 / U:0.204 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site