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

Open Source and information security mailing list archives

Message-ID: <CAGF5oy_BUytjeDa8Oc62pRabaEqwqKCFuVJuzLd-XNZg34ne2w@mail.gmail.com>
Date:	Mon, 5 Mar 2012 14:20:46 +0200
From:	"Valentin, Eduardo" <eduardo.valentin@...com>
To:	Henrique de Moraes Holschuh <hmh@....eng.br>
Cc:	ShuoX Liu <shuox.liu@...el.com>,
	"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>,
	"Brown, Len" <len.brown@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-pm@...ts.linux-foundation.org" 
	<linux-pm@...ts.linux-foundation.org>, Ingo Molnar <mingo@...e.hu>,
	greg@...ah.com
Subject: Re: [linux-pm] [PATCH V3] cpuidle: Add a sysfs entry to disable
 specific C state for debug purpose.

Hello,

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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives