Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disablespecific C state for debug purpose.

[Valentin, Eduardo]

Hello,


On Mon, Mar 5, 2012 at 12:18 PM, Henrique de Moraes Holschuh
<hmh@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/