lkml.org 
[lkml]   [2012]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.
    From
    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.

    Of course, if you are targeting user space control over C states on
    production systems, then it's a different thing then.

    >
    > --
    >  "One disk to rule them all, One disk to find them. One disk to bring
    >  them all and in the darkness grind them. In the Land of Redmond
    >  where the shadows lie." -- The Silicon Valley Tarot
    >  Henrique Holschuh
    > _______________________________________________
    > linux-pm mailing list
    > linux-pm@lists.linux-foundation.org
    > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm



    --

    Eduardo Valentin
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-03-05 13:23    [W:3.473 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site