Re: Subject: [PATCH] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

Re: Subject: [PATCH] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

[Deepthi Dharwar]

Hi,

On 03/05/2012 08:52 AM, Liu, ShuoX wrote:

>>> It's useful to help developers debug some issues.
>>
>> It would help developers more if it was documented a bit.  As it
>> stands, they'd be lucky if they even knew it existed.
>>
>> Looky:
>>
>> akpm:/usr/src/linux-3.3-rc5> grep -ril cpuidle Documentation
>> Documentation/trace/events-power.txt
>> Documentation/ABI/testing/sysfs-devices-system-cpu
>> Documentation/kernel-parameters.txt
>> Documentation/00-INDEX
>> Documentation/cpuidle/core.txt
>> Documentation/cpuidle/sysfs.txt
>> Documentation/cpuidle/driver.txt
>> Documentation/cpuidle/governor.txt
> 
> Thanks Andrew's advice and Yanmin's review. Here is the new patch. 
> 
> From: ShuoX Liu <shuox.liu@intel.com>
> 
> Some C states of new CPU might be not good. One reason is BIOS might configure
> them incorrectly. To help developers root cause it quickly, the patch adds a
> new sysfs entry, so developers could disable specific C state manually.
> 
> In addition, C state might have much impact on performance tuning, as it takes
> much time to enter/exit C states, which might delay interrupt processing. With
> the new debug option, developers could check if a deep C state could  impact
> performance and how much impact it could cause.
> 
> Also add this option in Documentation/cpuidle/sysfs.txt.



Enabling/Disabling particular C-states from sysfs entry is really
helpful for cpuidle developers for general debugging and performance
tuning for not just x86 but for all platforms that support cpuidle,
like arm, powerpc etc .

>

> Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
> Reviewed-by: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> ---
>  Documentation/cpuidle/sysfs.txt  |    5 +++++
>  drivers/cpuidle/cpuidle.c        |    1 +
>  drivers/cpuidle/governors/menu.c |    5 ++++-
>  drivers/cpuidle/sysfs.c          |   34 ++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h          |    1 +
>  5 files changed, 45 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/cpuidle/sysfs.txt b/Documentation/cpuidle/sysfs.txt
> index 50d7b16..635424f 100644
> --- a/Documentation/cpuidle/sysfs.txt
> +++ b/Documentation/cpuidle/sysfs.txt
> @@ -36,6 +36,7 @@ drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
>  /sys/devices/system/cpu/cpu0/cpuidle/state0:
>  total 0
>  -r--r--r-- 1 root root 4096 Feb  8 10:42 desc
> +-r--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


Please update the documentation, rw fields are valid
for disable flag
/sys/devices/system/cpu/cpu2/cpuidle/state1 # ls -l
total 0
-r--r--r-- 1 root root 65536 Mar  5 11:28 desc
-rw-r--r-- 1 root root 65536 Mar  5 11:28 disable
-r--r--r-- 1 root root 65536 Mar  5 11:28 latency
-r--r--r-- 1 root root 65536 Mar  5 11:28 name
-r--r--r-- 1 root root 65536 Mar  5 11:28 power
-r--r--r-- 1 root root 65536 Mar  5 11:28 time
-r--r--r-- 1 root root 65536 Mar  5 11:28 usage
> @@ -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
> +-r--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
> @@ -54,6 +56,7 @@ total 0
>  /sys/devices/system/cpu/cpu0/cpuidle/state2:
>  total 0
>  -r--r--r-- 1 root root 4096 Feb  8 10:42 desc
> +-r--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
> @@ -63,6 +66,7 @@ total 0
>  /sys/devices/system/cpu/cpu0/cpuidle/state3:
>  total 0
>  -r--r--r-- 1 root root 4096 Feb  8 10:42 desc
> +-r--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
> @@ -72,6 +76,7 @@ total 0
>  
>  
>  * desc : Small description about the idle state (string)
> +* disable : Option to disable this idle state (bool)
>  * latency : Latency to exit out of this idle state (in microseconds)
>  * name : Name of the idle state (string)
>  * power : Power consumed while in this idle state (in milliwatts)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..7d66d3e 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
>  	state->power_usage = -1;
>  	state->flags = 0;
>  	state->enter = poll_idle;
> +	state->disable = 0;
>  }
>  #else
>  static void poll_idle_init(struct cpuidle_driver *drv) {}
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index ad09526..5c17ca1 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	 * We want to default to C1 (hlt), not to busy polling
>  	 * unless the timer is happening really really soon.
>  	 */
> -	if (data->expected_us > 5)
> +	if (data->expected_us > 5 &&
> +		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
>  		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>  
>  	/*
> @@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
>  		struct cpuidle_state *s = &drv->states[i];
>  
> +		if (s->disable)
> +			continue;
>  		if (s->target_residency > data->predicted_us)
>  			continue;
>  		if (s->exit_latency > latency_req)
> 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; \
> +}
> +
>  #define define_show_state_ull_function(_name) \
>  static ssize_t show_state_##_name(struct cpuidle_state *state, \
>  			struct cpuidle_state_usage *state_usage, char *buf) \
> @@ -251,6 +267,8 @@ define_show_state_ull_function(usage)
>  define_show_state_ull_function(time)
>  define_show_state_str_function(name)
>  define_show_state_str_function(desc)
> +define_show_state_function(disable)
> +define_store_state_function(disable)
>  
>  define_one_state_ro(name, show_state_name);
>  define_one_state_ro(desc, show_state_desc);
> @@ -258,6 +276,7 @@ define_one_state_ro(latency, show_state_exit_latency);
>  define_one_state_ro(power, show_state_power_usage);
>  define_one_state_ro(usage, show_state_usage);
>  define_one_state_ro(time, show_state_time);
> +define_one_state_rw(disable, show_state_disable, store_state_disable);
>  
>  static struct attribute *cpuidle_state_default_attrs[] = {
>  	&attr_name.attr,
> @@ -266,6 +285,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
>  	&attr_power.attr,
>  	&attr_usage.attr,
>  	&attr_time.attr,
> +	&attr_disable.attr,
>  	NULL
>  };
>  
> @@ -287,8 +307,22 @@ static ssize_t cpuidle_state_show(struct kobject * kobj,
>  	return ret;
>  }
>  
> +static ssize_t cpuidle_state_store(struct kobject *kobj,
> +	struct attribute *attr, const char *buf, size_t size)
> +{
> +	int ret = -EIO;
> +	struct cpuidle_state *state = kobj_to_state(kobj);
> +	struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
> +
> +	if (cattr->store)
> +		ret = cattr->store(state, buf, size);
> +
> +	return ret;
> +}
> +
>  static const struct sysfs_ops cpuidle_state_sysfs_ops = {
>  	.show = cpuidle_state_show,
> +	.store = cpuidle_state_store,
>  };
>  
>  static void cpuidle_state_sysfs_release(struct kobject *kobj)
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..eb150a5 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -45,6 +45,7 @@ struct cpuidle_state {
>  	unsigned int	exit_latency; /* in US */
>  	unsigned int	power_usage; /* in mW */
>  	unsigned int	target_residency; /* in US */
> +	unsigned int    disable;
>  
>  	int (*enter)	(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
> 


Reviewed-and-Tested-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>

Cheers,
Deepthi

[PATCH V3] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

[ShuoX Liu]

On 2012年03月05日 14:16, Deepthi Dharwar wrote:
> Hi,
>
> On 03/05/2012 08:52 AM, Liu, ShuoX wrote:
>
>>>> It's useful to help developers debug some issues.
>>>
>>> It would help developers more if it was documented a bit.  As it
>>> stands, they'd be lucky if they even knew it existed.
>>>
>>> Looky:
>>>
>>> akpm:/usr/src/linux-3.3-rc5>  grep -ril cpuidle Documentation
>>> Documentation/trace/events-power.txt
>>> Documentation/ABI/testing/sysfs-devices-system-cpu
>>> Documentation/kernel-parameters.txt
>>> Documentation/00-INDEX
>>> Documentation/cpuidle/core.txt
>>> Documentation/cpuidle/sysfs.txt
>>> Documentation/cpuidle/driver.txt
>>> Documentation/cpuidle/governor.txt
>>
>> Thanks Andrew's advice and Yanmin's review. Here is the new patch.
>>
>> From: ShuoX Liu<shuox.liu@intel.com>
>>
>> Some C states of new CPU might be not good. One reason is BIOS might configure
>> them incorrectly. To help developers root cause it quickly, the patch adds a
>> new sysfs entry, so developers could disable specific C state manually.
>>
>> In addition, C state might have much impact on performance tuning, as it takes
>> much time to enter/exit C states, which might delay interrupt processing. With
>> the new debug option, developers could check if a deep C state could  impact
>> performance and how much impact it could cause.
>>
>> Also add this option in Documentation/cpuidle/sysfs.txt.
>
>
>
> Enabling/Disabling particular C-states from sysfs entry is really
> helpful for cpuidle developers for general debugging and performance
> tuning for not just x86 but for all platforms that support cpuidle,
> like arm, powerpc etc .
>
>>
>
>> Signed-off-by: ShuoX Liu<shuox.liu@intel.com>
>> Reviewed-by: Yanmin Zhang<yanmin_zhang@linux.intel.com>

>
> Please update the documentation, rw fields are valid
> for disable flag
> /sys/devices/system/cpu/cpu2/cpuidle/state1 # ls -l
> total 0
I will do so.

>>   static void cpuidle_state_sysfs_release(struct kobject *kobj)
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 712abcc..eb150a5 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -45,6 +45,7 @@ struct cpuidle_state {
>>        unsigned int    exit_latency; /* in US */
>>        unsigned int    power_usage; /* in mW */
>>        unsigned int    target_residency; /* in US */
>> +     unsigned int    disable;
>>
>>        int (*enter)    (struct cpuidle_device *dev,
>>                        struct cpuidle_driver *drv,
>>

Thanks Deepthi. below is the new patch.

From: ShuoX Liu <shuox.liu@intel.com>

Some C states of new CPU might be not good. One reason is BIOS might 
configure
them incorrectly. To help developers root cause it quickly, the patch adds a
new sysfs entry, so developers could disable specific C state manually.

In addition, C state might have much impact on performance tuning, as it 
takes
much time to enter/exit C states, which might delay interrupt 
processing. With
the new debug option, developers could check if a deep C state could  impact
performance and how much impact it could cause.

Also add this option in Documentation/cpuidle/sysfs.txt.

Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com>
Reviewed-and-Tested-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
---
  Documentation/cpuidle/sysfs.txt  |    5 +++++
  drivers/cpuidle/cpuidle.c        |    1 +
  drivers/cpuidle/governors/menu.c |    5 ++++-
  drivers/cpuidle/sysfs.c          |   34 ++++++++++++++++++++++++++++++++++
  include/linux/cpuidle.h          |    1 +
  5 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/Documentation/cpuidle/sysfs.txt 
b/Documentation/cpuidle/sysfs.txt
index 50d7b16..9d28a34 100644
--- a/Documentation/cpuidle/sysfs.txt
+++ b/Documentation/cpuidle/sysfs.txt
@@ -36,6 +36,7 @@ drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
  /sys/devices/system/cpu/cpu0/cpuidle/state0:
  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
@@ -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
@@ -54,6 +56,7 @@ total 0
  /sys/devices/system/cpu/cpu0/cpuidle/state2:
  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
@@ -63,6 +66,7 @@ total 0
  /sys/devices/system/cpu/cpu0/cpuidle/state3:
  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
@@ -72,6 +76,7 @@ total 0


  * desc : Small description about the idle state (string)
+* disable : Option to disable this idle state (bool)
  * latency : Latency to exit out of this idle state (in microseconds)
  * name : Name of the idle state (string)
  * power : Power consumed while in this idle state (in milliwatts)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..7d66d3e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
  	state->power_usage = -1;
  	state->flags = 0;
  	state->enter = poll_idle;
+	state->disable = 0;
  }
  #else
  static void poll_idle_init(struct cpuidle_driver *drv) {}
diff --git a/drivers/cpuidle/governors/menu.c 
b/drivers/cpuidle/governors/menu.c
index ad09526..5c17ca1 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, 
struct cpuidle_device *dev)
  	 * We want to default to C1 (hlt), not to busy polling
  	 * unless the timer is happening really really soon.
  	 */
-	if (data->expected_us > 5)
+	if (data->expected_us > 5 &&
+		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
  		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;

  	/*
@@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, 
struct cpuidle_device *dev)
  	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
  		struct cpuidle_state *s = &drv->states[i];

+		if (s->disable)
+			continue;
  		if (s->target_residency > data->predicted_us)
  			continue;
  		if (s->exit_latency > latency_req)
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; \
+}
+
  #define define_show_state_ull_function(_name) \
  static ssize_t show_state_##_name(struct cpuidle_state *state, \
  			struct cpuidle_state_usage *state_usage, char *buf) \
@@ -251,6 +267,8 @@ define_show_state_ull_function(usage)
  define_show_state_ull_function(time)
  define_show_state_str_function(name)
  define_show_state_str_function(desc)
+define_show_state_function(disable)
+define_store_state_function(disable)

  define_one_state_ro(name, show_state_name);
  define_one_state_ro(desc, show_state_desc);
@@ -258,6 +276,7 @@ define_one_state_ro(latency, show_state_exit_latency);
  define_one_state_ro(power, show_state_power_usage);
  define_one_state_ro(usage, show_state_usage);
  define_one_state_ro(time, show_state_time);
+define_one_state_rw(disable, show_state_disable, store_state_disable);

  static struct attribute *cpuidle_state_default_attrs[] = {
  	&attr_name.attr,
@@ -266,6 +285,7 @@ static struct attribute 
*cpuidle_state_default_attrs[] = {
  	&attr_power.attr,
  	&attr_usage.attr,
  	&attr_time.attr,
+	&attr_disable.attr,
  	NULL
  };

@@ -287,8 +307,22 @@ static ssize_t cpuidle_state_show(struct kobject * 
kobj,
  	return ret;
  }

+static ssize_t cpuidle_state_store(struct kobject *kobj,
+	struct attribute *attr, const char *buf, size_t size)
+{
+	int ret = -EIO;
+	struct cpuidle_state *state = kobj_to_state(kobj);
+	struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
+
+	if (cattr->store)
+		ret = cattr->store(state, buf, size);
+
+	return ret;
+}
+
  static const struct sysfs_ops cpuidle_state_sysfs_ops = {
  	.show = cpuidle_state_show,
+	.store = cpuidle_state_store,
  };

  static void cpuidle_state_sysfs_release(struct kobject *kobj)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..eb150a5 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -45,6 +45,7 @@ struct cpuidle_state {
  	unsigned int	exit_latency; /* in US */
  	unsigned int	power_usage; /* in mW */
  	unsigned int	target_residency; /* in US */
+	unsigned int    disable;

  	int (*enter)	(struct cpuidle_device *dev,
  			struct cpuidle_driver *drv,
-- 

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

[Henrique de Moraes Holschuh]

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)?

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

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

[Valentin, Eduardo]

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

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

[Yanmin Zhang]

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.

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

[Greg KH]

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

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

[Yanmin Zhang]

On Mon, 2012-03-05 at 21:22 -0800, Greg KH wrote:
> 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 :)
Greg,

Sorry. I might mislead you.

Basically, we could move all the entries of cpuidle from sysfs to debugfs. But such
moving would change KBI. There might be many scripts used by end users to parse the
data. If we change them to debugfs, the scripts wouldn't work and users would
complain.

What's your opinion about the KBI consistence?

Yanmin

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

[Greg KH]

On Tue, Mar 06, 2012 at 01:51:18PM +0800, Yanmin Zhang wrote:
> On Mon, 2012-03-05 at 21:22 -0800, Greg KH wrote:
> > 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 :)
> Greg,
> 
> Sorry. I might mislead you.
> 
> Basically, we could move all the entries of cpuidle from sysfs to debugfs. But such
> moving would change KBI. There might be many scripts used by end users to parse the
> data. If we change them to debugfs, the scripts wouldn't work and users would
> complain.
> 
> What's your opinion about the KBI consistence?

These files all are debugging files, right?  So they should be moved,
especially as the violate the "one value per file" rule of sysfs.

Do you know of any tools using these files?  I have never heard of them,
and I was told we should move these files years ago.  So I don't think
there should be any api issues.

But, if there are, we need to know what they are, and work to preserve
them.  The only way to find out is to move them :)

thanks,

greg k-h

[PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.

[ShuoX Liu]

On 2012年03月09日 02:01, Greg KH wrote:
> On Wed, Mar 07, 2012 at 09:00:51AM +0800, Yanmin Zhang wrote:
>> On Tue, 2012-03-06 at 06:39 -0800, Greg KH wrote:
>>> On Tue, Mar 06, 2012 at 01:51:18PM +0800, Yanmin Zhang wrote:
>>>> On Mon, 2012-03-05 at 21:22 -0800, Greg KH wrote:
>>>>> 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 :)
>>>> Greg,
>>>>
>>>> Sorry. I might mislead you.
>>>>
>>>> Basically, we could move all the entries of cpuidle from sysfs to debugfs. But such
>>>> moving would change KBI. There might be many scripts used by end users to parse the
>>>> data. If we change them to debugfs, the scripts wouldn't work and users would
>>>> complain.
>>>>
>>>> What's your opinion about the KBI consistence?
>>>
>>> These files all are debugging files, right?  So they should be moved,
>>> especially as the violate the "one value per file" rule of sysfs.
>>>
>>> Do you know of any tools using these files?  I have never heard of them,
>>> and I was told we should move these files years ago.  So I don't think
>>> there should be any api issues.
>>>
>>> But, if there are, we need to know what they are, and work to preserve
>>> them.  The only way to find out is to move them :)
>> We would move all cpuidle entries to debugfs firstly.
>> 1) Create the similar directory tree on debugfs: cpu/cpuXXX, but just have cpuidle subdirectory;
>> 2) Move our cpuidle->disable patch to debugfs;
>> sysfs cpuXXX has other sub directories. We don't move them this time.
>>
>> The new directory under debugfs becomes:
>> cpu---cpu0---cpuidle
>> 	|
>> 	|
>>        cpu1---cpuidle
>> 	|
>>
>> Is it ok?
>
> Looks fine to me.
>
> greg k-h

I created a series to do this.

[PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
[PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for 
debug purpose.
[PATCH 3/3] cpupower: Update the cpupower tool for new debugfs entries 
of cpuidle.

Below are patches.
---
From: ShuoX Liu <shuox.liu@intel.com>

This patch move cpuidle sysfs entry into debugfs. That is
/sys/devices/system/cpu/cpuXX/cpuidle will be changed to
$(debugfs_mount_dir)/cpu/cpuXX/cpuidle.

Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com>
---
  Documentation/cpuidle/debugfs.txt |   63 ++++++++++
  Documentation/cpuidle/sysfs.txt   |   59 ---------
  drivers/base/cpu.c                |   14 ++
  drivers/cpuidle/Makefile          |    1 +
  drivers/cpuidle/cpuidle.c         |   23 +---
  drivers/cpuidle/cpuidle.h         |   29 ++++-
  drivers/cpuidle/debugfs.c         |  176 +++++++++++++++++++++++++++
  drivers/cpuidle/sysfs.c           |  239 
-------------------------------------
  include/linux/cpuidle.h           |   10 +-
  9 files changed, 292 insertions(+), 322 deletions(-)
  create mode 100644 Documentation/cpuidle/debugfs.txt
  create mode 100644 drivers/cpuidle/debugfs.c

diff --git a/Documentation/cpuidle/debugfs.txt 
b/Documentation/cpuidle/debugfs.txt
new file mode 100644
index 0000000..7724a69
--- /dev/null
+++ b/Documentation/cpuidle/debugfs.txt
@@ -0,0 +1,63 @@
+
+		Supporting multiple CPU idle levels in kernel
+
+				cpuidle debugfs
+
+Per logical CPU specific cpuidle information are under
+/sys/kernel/debug/cpu/cpuX/cpuidle
+(when your system mounted debugfs at /sys/kernel/debug)
+for each online cpu X
+
+--------------------------------------------------------------------------------
+# ls -lR /sys/kernel/debug/cpu/cpu0/cpuidle/
+/sys/kernel/debug/cpu/cpu0/cpuidle/:
+total 0
+drwxr-xr-x 2 root root 0 Feb  8 10:42 state0
+drwxr-xr-x 2 root root 0 Feb  8 10:42 state1
+drwxr-xr-x 2 root root 0 Feb  8 10:42 state2
+drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
+
+/sys/kernel/debug/cpu/cpu0/cpuidle/state0:
+total 0
+-r--r--r-- 1 root root 4096 Feb  8 10:42 desc
+-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
+-r--r--r-- 1 root root 4096 Feb  8 10:42 time
+-r--r--r-- 1 root root 4096 Feb  8 10:42 usage
+
+/sys/kernel/debug/cpu/cpu0/cpuidle/state1:
+total 0
+-r--r--r-- 1 root root 4096 Feb  8 10:42 desc
+-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
+-r--r--r-- 1 root root 4096 Feb  8 10:42 time
+-r--r--r-- 1 root root 4096 Feb  8 10:42 usage
+
+/sys/kernel/debug/cpu/cpu0/cpuidle/state2:
+total 0
+-r--r--r-- 1 root root 4096 Feb  8 10:42 desc
+-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
+-r--r--r-- 1 root root 4096 Feb  8 10:42 time
+-r--r--r-- 1 root root 4096 Feb  8 10:42 usage
+
+/sys/kernel/debug/cpu/cpu0/cpuidle/state3:
+total 0
+-r--r--r-- 1 root root 4096 Feb  8 10:42 desc
+-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
+-r--r--r-- 1 root root 4096 Feb  8 10:42 time
+-r--r--r-- 1 root root 4096 Feb  8 10:42 usage
+--------------------------------------------------------------------------------
+
+
+* desc : Small description about the idle state (string)
+* latency : Latency to exit out of this idle state (in microseconds)
+* name : Name of the idle state (string)
+* power : Power consumed while in this idle state (in milliwatts)
+* time : Total time spent in this idle state (in microseconds)
+* usage : Number of times this state was entered (count)
diff --git a/Documentation/cpuidle/sysfs.txt 
b/Documentation/cpuidle/sysfs.txt
index 50d7b16..588e40b 100644
--- a/Documentation/cpuidle/sysfs.txt
+++ b/Documentation/cpuidle/sysfs.txt
@@ -18,62 +18,3 @@ following objects are visible instead.
  * current_governor
  In this case users can switch the governor at run time by writing
  to current_governor.
-
-
-Per logical CPU specific cpuidle information are under
-/sys/devices/system/cpu/cpuX/cpuidle
-for each online cpu X
-
---------------------------------------------------------------------------------
-# ls -lR /sys/devices/system/cpu/cpu0/cpuidle/
-/sys/devices/system/cpu/cpu0/cpuidle/:
-total 0
-drwxr-xr-x 2 root root 0 Feb  8 10:42 state0
-drwxr-xr-x 2 root root 0 Feb  8 10:42 state1
-drwxr-xr-x 2 root root 0 Feb  8 10:42 state2
-drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
-
-/sys/devices/system/cpu/cpu0/cpuidle/state0:
-total 0
--r--r--r-- 1 root root 4096 Feb  8 10:42 desc
--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
--r--r--r-- 1 root root 4096 Feb  8 10:42 time
--r--r--r-- 1 root root 4096 Feb  8 10:42 usage
-
-/sys/devices/system/cpu/cpu0/cpuidle/state1:
-total 0
--r--r--r-- 1 root root 4096 Feb  8 10:42 desc
--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
--r--r--r-- 1 root root 4096 Feb  8 10:42 time
--r--r--r-- 1 root root 4096 Feb  8 10:42 usage
-
-/sys/devices/system/cpu/cpu0/cpuidle/state2:
-total 0
--r--r--r-- 1 root root 4096 Feb  8 10:42 desc
--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
--r--r--r-- 1 root root 4096 Feb  8 10:42 time
--r--r--r-- 1 root root 4096 Feb  8 10:42 usage
-
-/sys/devices/system/cpu/cpu0/cpuidle/state3:
-total 0
--r--r--r-- 1 root root 4096 Feb  8 10:42 desc
--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
--r--r--r-- 1 root root 4096 Feb  8 10:42 time
--r--r--r-- 1 root root 4096 Feb  8 10:42 usage
---------------------------------------------------------------------------------
-
-
-* desc : Small description about the idle state (string)
-* latency : Latency to exit out of this idle state (in microseconds)
-* name : Name of the idle state (string)
-* power : Power consumed while in this idle state (in milliwatts)
-* time : Total time spent in this idle state (in microseconds)
-* usage : Number of times this state was entered (count)
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4dabf50..cc4fa6f 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -12,6 +12,7 @@
  #include <linux/node.h>
  #include <linux/gfp.h>
  #include <linux/percpu.h>
+#include <linux/debugfs.h>

  #include "base.h"

@@ -22,6 +23,9 @@ struct bus_type cpu_subsys = {
  EXPORT_SYMBOL_GPL(cpu_subsys);

  static DEFINE_PER_CPU(struct device *, cpu_sys_devices);
+static struct dentry *cpu_debugfs_root;
+DEFINE_PER_CPU(struct dentry *, cpu_debugfs);
+EXPORT_PER_CPU_SYMBOL_GPL(cpu_debugfs);

  #ifdef CONFIG_HOTPLUG_CPU
  static ssize_t show_online(struct device *dev,
@@ -71,6 +75,8 @@ void unregister_cpu(struct cpu *cpu)
  {
  	int logical_cpu = cpu->dev.id;

+	debugfs_remove_recursive(per_cpu(cpu_debugfs, logical_cpu));
+
  	unregister_cpu_under_node(logical_cpu, cpu_to_node(logical_cpu));

  	device_remove_file(&cpu->dev, &dev_attr_online);
@@ -238,6 +244,7 @@ static void cpu_device_release(struct device *dev)
  int __cpuinit register_cpu(struct cpu *cpu, int num)
  {
  	int error;
+	struct dentry **debugfs = &per_cpu(cpu_debugfs, num);

  	cpu->node_id = cpu_to_node(num);
  	memset(&cpu->dev, 0x00, sizeof(struct device));
@@ -251,6 +258,9 @@ int __cpuinit register_cpu(struct cpu *cpu, int num)
  		per_cpu(cpu_sys_devices, num) = &cpu->dev;
  	if (!error)
  		register_cpu_under_node(num, cpu_to_node(num));
+	if (!error && cpu_debugfs_root)
+		*debugfs = debugfs_create_dir(dev_name(&cpu->dev),
+						cpu_debugfs_root);

  #ifdef CONFIG_KEXEC
  	if (!error)
@@ -323,4 +333,8 @@ void __init cpu_dev_init(void)
  #if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
  	sched_create_sysfs_power_savings_entries(cpu_subsys.dev_root);
  #endif
+
+	cpu_debugfs_root = debugfs_create_dir("cpu", NULL);
+	if (cpu_debugfs_root == ERR_PTR(-ENODEV))
+		cpu_debugfs_root = NULL;
  }
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 5634f88..a4640ed 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -3,3 +3,4 @@
  #

  obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-$(CONFIG_DEBUG_FS) += debugfs.o
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..b4946bc 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -228,12 +228,9 @@ int cpuidle_enable_device(struct cpuidle_device *dev)

  	poll_idle_init(cpuidle_get_driver());

-	if ((ret = cpuidle_add_state_sysfs(dev)))
-		return ret;
-
  	if (cpuidle_curr_governor->enable &&
  	    (ret = cpuidle_curr_governor->enable(cpuidle_get_driver(), dev)))
-		goto fail_sysfs;
+		return ret;

  	for (i = 0; i < dev->state_count; i++) {
  		dev->states_usage[i].usage = 0;
@@ -243,15 +240,13 @@ int cpuidle_enable_device(struct cpuidle_device *dev)

  	smp_wmb();

+	cpuidle_add_state_debugfs(dev);
+
  	dev->enabled = 1;

  	enabled_devices++;
-	return 0;

-fail_sysfs:
-	cpuidle_remove_state_sysfs(dev);
-
-	return ret;
+	return 0;
  }

  EXPORT_SYMBOL_GPL(cpuidle_enable_device);
@@ -275,7 +270,7 @@ void cpuidle_disable_device(struct cpuidle_device *dev)
  	if (cpuidle_curr_governor->disable)
  		cpuidle_curr_governor->disable(cpuidle_get_driver(), dev);

-	cpuidle_remove_state_sysfs(dev);
+	cpuidle_remove_state_debugfs(dev);
  	enabled_devices--;
  }

@@ -290,7 +285,6 @@ EXPORT_SYMBOL_GPL(cpuidle_disable_device);
   */
  static int __cpuidle_register_device(struct cpuidle_device *dev)
  {
-	int ret;
  	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
  	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();

@@ -303,10 +297,7 @@ static int __cpuidle_register_device(struct 
cpuidle_device *dev)

  	per_cpu(cpuidle_devices, dev->cpu) = dev;
  	list_add(&dev->device_list, &cpuidle_detected_devices);
-	if ((ret = cpuidle_add_sysfs(cpu_dev))) {
-		module_put(cpuidle_driver->owner);
-		return ret;
-	}
+	cpuidle_add_debugfs(cpu_dev);

  	dev->registered = 1;
  	return 0;
@@ -354,7 +345,7 @@ void cpuidle_unregister_device(struct cpuidle_device 
*dev)

  	cpuidle_disable_device(dev);

-	cpuidle_remove_sysfs(cpu_dev);
+	cpuidle_remove_debugfs(cpu_dev);
  	list_del(&dev->device_list);
  	wait_for_completion(&dev->kobj_unregister);
  	per_cpu(cpuidle_devices, dev->cpu) = NULL;
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index 7db1866..890545c 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -25,9 +25,30 @@ extern int cpuidle_switch_governor(struct 
cpuidle_governor *gov);
  /* sysfs */
  extern int cpuidle_add_interface(struct device *dev);
  extern void cpuidle_remove_interface(struct device *dev);
-extern int cpuidle_add_state_sysfs(struct cpuidle_device *device);
-extern void cpuidle_remove_state_sysfs(struct cpuidle_device *device);
-extern int cpuidle_add_sysfs(struct device *dev);
-extern void cpuidle_remove_sysfs(struct device *dev);
+
+/* debugfs */
+#ifdef CONFIG_DEBUG_FS
+DECLARE_PER_CPU(struct dentry *, cpu_debugfs);
+extern int cpuidle_add_state_debugfs(struct cpuidle_device *device);
+extern void cpuidle_remove_state_debugfs(struct cpuidle_device *device);
+extern int cpuidle_add_debugfs(struct device *dev);
+extern void cpuidle_remove_debugfs(struct device *dev);
+#else
+static inline int cpuidle_add_state_debugfs(struct cpuidle_device *device)
+{
+	return 0;
+}
+static inline
+void cpuidle_remove_state_debugfs(struct cpuidle_device *device)
+{
+}
+static inline int cpuidle_add_debugfs(struct device *cpu_dev)
+{
+	return 0;
+}
+static inline void cpuidle_remove_debugfs(struct device *cpu_dev)
+{
+}
+#endif

  #endif /* __DRIVER_CPUIDLE_H */
diff --git a/drivers/cpuidle/debugfs.c b/drivers/cpuidle/debugfs.c
new file mode 100644
index 0000000..67ddc44
--- /dev/null
+++ b/drivers/cpuidle/debugfs.c
@@ -0,0 +1,176 @@
+/*
+ * debugfs.c - debugfs support
+ *
+ * (C) 2006-2007 Shaohua Li <shaohua.li@intel.com>
+ * (C) 2012 ShuoX Liu <shuox.liu@intel.com>
+ *
+ * This code is licenced under the GPL.
+ */
+
+#include <linux/kernel.h>
+#include <linux/cpuidle.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+
+#include "cpuidle.h"
+
+static int state_open_generic(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t state_name_read(struct file *f, char __user *buf,
+				size_t count, loff_t *off)
+{
+	struct cpuidle_state *state = f->private_data;
+	char buffer[CPUIDLE_NAME_LEN];
+	int len;
+
+	len = snprintf(buffer, sizeof(buffer), "%s\n", state->name);
+
+	return simple_read_from_buffer(buf, count, off, buffer, len);
+}
+
+static const struct file_operations state_name_fops = {
+	.open	= state_open_generic,
+	.read	= state_name_read,
+};
+
+static ssize_t state_desc_read(struct file *f, char __user *buf,
+				size_t count, loff_t *off)
+{
+	struct cpuidle_state *state = f->private_data;
+	char buffer[CPUIDLE_DESC_LEN];
+	int len;
+
+	len = snprintf(buffer, sizeof(buffer), "%s\n", state->desc);
+
+	return simple_read_from_buffer(buf, count, off, buffer, len);
+}
+
+static const struct file_operations state_desc_fops = {
+	.open	= state_open_generic,
+	.read	= state_desc_read,
+};
+
+static void debugfs_add_state_attrs(struct cpuidle_dev_state *dev_state)
+{
+	struct dentry *parent = dev_state->debugfs;
+	struct cpuidle_state *state = dev_state->state;
+	struct cpuidle_state_usage *state_usage = dev_state->state_usage;
+
+	if (!parent)
+		return;
+
+	if (!debugfs_create_file("name", S_IRUGO,
+				parent, state, &state_name_fops))
+		goto error;
+
+	if (!debugfs_create_file("desc", S_IRUGO,
+				parent, state, &state_desc_fops))
+		goto error;
+
+	if (!debugfs_create_u32("latency", S_IRUGO,
+				parent, &state->exit_latency))
+		goto error;
+
+	if (!debugfs_create_u32("power", S_IRUGO,
+				parent, &state->power_usage))
+		goto error;
+
+	if (!debugfs_create_u64("usage", S_IRUGO,
+				parent, &state_usage->usage))
+		goto error;
+
+	if (!debugfs_create_u64("time", S_IRUGO,
+				parent, &state_usage->time))
+		goto error;
+
+	return;
+error:
+	debugfs_remove_recursive(parent);
+}
+
+static inline void cpuidle_free_dev_state(struct cpuidle_device 
*device, int i)
+{
+	struct cpuidle_dev_state *dev_state = device->dev_states[i];
+
+	if (!dev_state)
+		return ;
+	debugfs_remove_recursive(dev_state->debugfs);
+	kfree(dev_state);
+	dev_state = NULL;
+}
+
+int cpuidle_add_state_debugfs(struct cpuidle_device *device)
+{
+	int i, ret = -ENOMEM;
+	char buf[16];
+	struct cpuidle_dev_state *dev_state;
+	struct cpuidle_driver *drv = cpuidle_get_driver();
+	struct dentry *parent = device->debugfs;
+
+	if (!parent)
+		return -ENOENT;
+	/* state statistics */
+	for (i = 0; i < device->state_count; i++) {
+		dev_state =
+			kzalloc(sizeof(struct cpuidle_dev_state), GFP_KERNEL);
+		if (!dev_state)
+			goto error;
+		dev_state->state = &drv->states[i];
+		dev_state->state_usage = &device->states_usage[i];
+
+		sprintf(buf, "state%d", i);
+		dev_state->debugfs = debugfs_create_dir(buf, parent);
+		if (!dev_state->debugfs) {
+			kfree(dev_state);
+			dev_state = NULL;
+			goto error;
+		}
+		dev_state->dev = device;
+		device->dev_states[i] = dev_state;
+		debugfs_add_state_attrs(dev_state);
+	}
+
+	return 0;
+
+error:
+	for (i = i - 1; i >= 0; i--)
+		cpuidle_free_dev_state(device, i);
+	return ret;
+}
+
+void cpuidle_remove_state_debugfs(struct cpuidle_device *device)
+{
+	int i;
+
+	for (i = 0; i < device->state_count; i++)
+		cpuidle_free_dev_state(device, i);
+}
+
+int cpuidle_add_debugfs(struct device *cpu_dev)
+{
+	int cpu = cpu_dev->id;
+	struct cpuidle_device *device;
+	struct dentry *parent = per_cpu(cpu_debugfs, cpu);
+
+	if (!parent)
+		return -ENOENT;
+	device = per_cpu(cpuidle_devices, cpu);
+	device->debugfs = debugfs_create_dir("cpuidle", parent);
+	if (!device->debugfs)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void cpuidle_remove_debugfs(struct device *cpu_dev)
+{
+	int cpu = cpu_dev->id;
+	struct cpuidle_device *dev;
+
+	dev = per_cpu(cpuidle_devices, cpu);
+	debugfs_remove_recursive(dev->debugfs);
+}
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 3fe41fe..dd597a9 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -152,242 +152,3 @@ void cpuidle_remove_interface(struct device *dev)
  {
  	sysfs_remove_group(&dev->kobj, &cpuidle_attr_group);
  }
-
-struct cpuidle_attr {
-	struct attribute attr;
-	ssize_t (*show)(struct cpuidle_device *, char *);
-	ssize_t (*store)(struct cpuidle_device *, const char *, size_t count);
-};
-
-#define define_one_ro(_name, show) \
-	static struct cpuidle_attr attr_##_name = __ATTR(_name, 0444, show, NULL)
-#define define_one_rw(_name, show, store) \
-	static struct cpuidle_attr attr_##_name = __ATTR(_name, 0644, show, store)
-
-#define kobj_to_cpuidledev(k) container_of(k, struct cpuidle_device, kobj)
-#define attr_to_cpuidleattr(a) container_of(a, struct cpuidle_attr, attr)
-static ssize_t cpuidle_show(struct kobject * kobj, struct attribute * 
attr ,char * buf)
-{
-	int ret = -EIO;
-	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
-	struct cpuidle_attr * cattr = attr_to_cpuidleattr(attr);
-
-	if (cattr->show) {
-		mutex_lock(&cpuidle_lock);
-		ret = cattr->show(dev, buf);
-		mutex_unlock(&cpuidle_lock);
-	}
-	return ret;
-}
-
-static ssize_t cpuidle_store(struct kobject * kobj, struct attribute * 
attr,
-		     const char * buf, size_t count)
-{
-	int ret = -EIO;
-	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
-	struct cpuidle_attr * cattr = attr_to_cpuidleattr(attr);
-
-	if (cattr->store) {
-		mutex_lock(&cpuidle_lock);
-		ret = cattr->store(dev, buf, count);
-		mutex_unlock(&cpuidle_lock);
-	}
-	return ret;
-}
-
-static const struct sysfs_ops cpuidle_sysfs_ops = {
-	.show = cpuidle_show,
-	.store = cpuidle_store,
-};
-
-static void cpuidle_sysfs_release(struct kobject *kobj)
-{
-	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
-
-	complete(&dev->kobj_unregister);
-}
-
-static struct kobj_type ktype_cpuidle = {
-	.sysfs_ops = &cpuidle_sysfs_ops,
-	.release = cpuidle_sysfs_release,
-};
-
-struct cpuidle_state_attr {
-	struct attribute attr;
-	ssize_t (*show)(struct cpuidle_state *, \
-					struct cpuidle_state_usage *, char *);
-	ssize_t (*store)(struct cpuidle_state *, const char *, size_t);
-};
-
-#define define_one_state_ro(_name, show) \
-static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0444, 
show, NULL)
-
-#define define_show_state_function(_name) \
-static ssize_t show_state_##_name(struct cpuidle_state *state, \
-			 struct cpuidle_state_usage *state_usage, char *buf) \
-{ \
-	return sprintf(buf, "%u\n", state->_name);\
-}
-
-#define define_show_state_ull_function(_name) \
-static ssize_t show_state_##_name(struct cpuidle_state *state, \
-			struct cpuidle_state_usage *state_usage, char *buf) \
-{ \
-	return sprintf(buf, "%llu\n", state_usage->_name);\
-}
-
-#define define_show_state_str_function(_name) \
-static ssize_t show_state_##_name(struct cpuidle_state *state, \
-			struct cpuidle_state_usage *state_usage, char *buf) \
-{ \
-	if (state->_name[0] == '\0')\
-		return sprintf(buf, "<null>\n");\
-	return sprintf(buf, "%s\n", state->_name);\
-}
-
-define_show_state_function(exit_latency)
-define_show_state_function(power_usage)
-define_show_state_ull_function(usage)
-define_show_state_ull_function(time)
-define_show_state_str_function(name)
-define_show_state_str_function(desc)
-
-define_one_state_ro(name, show_state_name);
-define_one_state_ro(desc, show_state_desc);
-define_one_state_ro(latency, show_state_exit_latency);
-define_one_state_ro(power, show_state_power_usage);
-define_one_state_ro(usage, show_state_usage);
-define_one_state_ro(time, show_state_time);
-
-static struct attribute *cpuidle_state_default_attrs[] = {
-	&attr_name.attr,
-	&attr_desc.attr,
-	&attr_latency.attr,
-	&attr_power.attr,
-	&attr_usage.attr,
-	&attr_time.attr,
-	NULL
-};
-
-#define kobj_to_state_obj(k) container_of(k, struct cpuidle_state_kobj, 
kobj)
-#define kobj_to_state(k) (kobj_to_state_obj(k)->state)
-#define kobj_to_state_usage(k) (kobj_to_state_obj(k)->state_usage)
-#define attr_to_stateattr(a) container_of(a, struct cpuidle_state_attr, 
attr)
-static ssize_t cpuidle_state_show(struct kobject * kobj,
-	struct attribute * attr ,char * buf)
-{
-	int ret = -EIO;
-	struct cpuidle_state *state = kobj_to_state(kobj);
-	struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj);
-	struct cpuidle_state_attr * cattr = attr_to_stateattr(attr);
-
-	if (cattr->show)
-		ret = cattr->show(state, state_usage, buf);
-
-	return ret;
-}
-
-static const struct sysfs_ops cpuidle_state_sysfs_ops = {
-	.show = cpuidle_state_show,
-};
-
-static void cpuidle_state_sysfs_release(struct kobject *kobj)
-{
-	struct cpuidle_state_kobj *state_obj = kobj_to_state_obj(kobj);
-
-	complete(&state_obj->kobj_unregister);
-}
-
-static struct kobj_type ktype_state_cpuidle = {
-	.sysfs_ops = &cpuidle_state_sysfs_ops,
-	.default_attrs = cpuidle_state_default_attrs,
-	.release = cpuidle_state_sysfs_release,
-};
-
-static inline void cpuidle_free_state_kobj(struct cpuidle_device 
*device, int i)
-{
-	kobject_put(&device->kobjs[i]->kobj);
-	wait_for_completion(&device->kobjs[i]->kobj_unregister);
-	kfree(device->kobjs[i]);
-	device->kobjs[i] = NULL;
-}
-
-/**
- * cpuidle_add_driver_sysfs - adds driver-specific sysfs attributes
- * @device: the target device
- */
-int cpuidle_add_state_sysfs(struct cpuidle_device *device)
-{
-	int i, ret = -ENOMEM;
-	struct cpuidle_state_kobj *kobj;
-	struct cpuidle_driver *drv = cpuidle_get_driver();
-
-	/* state statistics */
-	for (i = 0; i < device->state_count; i++) {
-		kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL);
-		if (!kobj)
-			goto error_state;
-		kobj->state = &drv->states[i];
-		kobj->state_usage = &device->states_usage[i];
-		init_completion(&kobj->kobj_unregister);
-
-		ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle, 
&device->kobj,
-					   "state%d", i);
-		if (ret) {
-			kfree(kobj);
-			goto error_state;
-		}
-		kobject_uevent(&kobj->kobj, KOBJ_ADD);
-		device->kobjs[i] = kobj;
-	}
-
-	return 0;
-
-error_state:
-	for (i = i - 1; i >= 0; i--)
-		cpuidle_free_state_kobj(device, i);
-	return ret;
-}
-
-/**
- * cpuidle_remove_driver_sysfs - removes driver-specific sysfs attributes
- * @device: the target device
- */
-void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
-{
-	int i;
-
-	for (i = 0; i < device->state_count; i++)
-		cpuidle_free_state_kobj(device, i);
-}
-
-/**
- * cpuidle_add_sysfs - creates a sysfs instance for the target device
- * @dev: the target device
- */
-int cpuidle_add_sysfs(struct device *cpu_dev)
-{
-	int cpu = cpu_dev->id;
-	struct cpuidle_device *dev;
-	int error;
-
-	dev = per_cpu(cpuidle_devices, cpu);
-	error = kobject_init_and_add(&dev->kobj, &ktype_cpuidle, &cpu_dev->kobj,
-				     "cpuidle");
-	if (!error)
-		kobject_uevent(&dev->kobj, KOBJ_ADD);
-	return error;
-}
-
-/**
- * cpuidle_remove_sysfs - deletes a sysfs instance on the target device
- * @dev: the target device
- */
-void cpuidle_remove_sysfs(struct device *cpu_dev)
-{
-	int cpu = cpu_dev->id;
-	struct cpuidle_device *dev;
-
-	dev = per_cpu(cpuidle_devices, cpu);
-	kobject_put(&dev->kobj);
-}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..f605d28 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -76,11 +76,11 @@ cpuidle_set_statedata(struct cpuidle_state_usage 
*st_usage, void *data)
  	st_usage->driver_data = data;
  }

-struct cpuidle_state_kobj {
+struct cpuidle_dev_state {
  	struct cpuidle_state *state;
  	struct cpuidle_state_usage *state_usage;
-	struct completion kobj_unregister;
-	struct kobject kobj;
+	struct cpuidle_device *dev;
+	struct dentry *debugfs;
  };

  struct cpuidle_device {
@@ -91,12 +91,14 @@ struct cpuidle_device {
  	int			last_residency;
  	int			state_count;
  	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];
-	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
+	struct cpuidle_dev_state	*dev_states[CPUIDLE_STATE_MAX];

  	struct list_head 	device_list;
  	struct kobject		kobj;
  	struct completion	kobj_unregister;
  	void			*governor_data;
+
+	struct dentry		*debugfs;
  };

  DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
-- 
1.7.1

[PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for debug purpose.

[ShuoX Liu]

From: ShuoX Liu <shuox.liu@intel.com>

Some C states of new CPU might be not good. One reason is BIOS might 
configure
them incorrectly. To help developers root cause it quickly, the patch adds a
new debugfs entry, so developers could disable specific C state manually.

In addition, C state might have much impact on performance tuning, as it 
takes
much time to enter/exit C states, which might delay interrupt 
processing. With
the new debug option, developers could check if a deep C state could impact
performance and how much impact it could cause.

Also add this option in Documentation/cpuidle/debugfs.txt.

Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com>
---
  Documentation/cpuidle/debugfs.txt |    5 +++++
  drivers/cpuidle/cpuidle.c         |    1 +
  drivers/cpuidle/debugfs.c         |    4 ++++
  drivers/cpuidle/governors/menu.c  |    5 ++++-
  include/linux/cpuidle.h           |    1 +
  5 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/cpuidle/debugfs.txt 
b/Documentation/cpuidle/debugfs.txt
index 7724a69..ca393ba 100644
--- a/Documentation/cpuidle/debugfs.txt
+++ b/Documentation/cpuidle/debugfs.txt
@@ -20,6 +20,7 @@ drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
  /sys/kernel/debug/cpu/cpu0/cpuidle/state0:
  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
@@ -29,6 +30,7 @@ total 0
  /sys/kernel/debug/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
@@ -38,6 +40,7 @@ total 0
  /sys/kernel/debug/cpu/cpu0/cpuidle/state2:
  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
@@ -47,6 +50,7 @@ total 0
  /sys/kernel/debug/cpu/cpu0/cpuidle/state3:
  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
@@ -56,6 +60,7 @@ total 0


  * desc : Small description about the idle state (string)
+* disable : Option to disable this idle state (bool)
  * latency : Latency to exit out of this idle state (in microseconds)
  * name : Name of the idle state (string)
  * power : Power consumed while in this idle state (in milliwatts)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index b4946bc..fdaadb3 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
  	state->power_usage = -1;
  	state->flags = 0;
  	state->enter = poll_idle;
+	state->disable = 0;
  }
  #else
  static void poll_idle_init(struct cpuidle_driver *drv) {}
diff --git a/drivers/cpuidle/debugfs.c b/drivers/cpuidle/debugfs.c
index 67ddc44..197ce72 100644
--- a/drivers/cpuidle/debugfs.c
+++ b/drivers/cpuidle/debugfs.c
@@ -87,6 +87,10 @@ static void debugfs_add_state_attrs(struct 
cpuidle_dev_state *dev_state)
  				parent, &state_usage->time))
  		goto error;

+	if (!debugfs_create_bool("disable", S_IRUGO | S_IWUSR,
+				parent, &state->disable))
+		goto error;
+
  	return;
  error:
  	debugfs_remove_recursive(parent);
diff --git a/drivers/cpuidle/governors/menu.c 
b/drivers/cpuidle/governors/menu.c
index ad09526..5c17ca1 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, 
struct cpuidle_device *dev)
  	 * We want to default to C1 (hlt), not to busy polling
  	 * unless the timer is happening really really soon.
  	 */
-	if (data->expected_us > 5)
+	if (data->expected_us > 5 &&
+		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
  		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;

  	/*
@@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, 
struct cpuidle_device *dev)
  	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
  		struct cpuidle_state *s = &drv->states[i];

+		if (s->disable)
+			continue;
  		if (s->target_residency > data->predicted_us)
  			continue;
  		if (s->exit_latency > latency_req)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index f605d28..a85877d 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -45,6 +45,7 @@ struct cpuidle_state {
  	unsigned int	exit_latency; /* in US */
  	unsigned int	power_usage; /* in mW */
  	unsigned int	target_residency; /* in US */
+	unsigned int    disable;

  	int (*enter)	(struct cpuidle_device *dev,
  			struct cpuidle_driver *drv,
-- 
1.7.1

Re: [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for debug purpose.

[Matthew Garrett]

On Mon, Mar 12, 2012 at 05:21:33PM +0800, ShuoX Liu wrote:

> In addition, C state might have much impact on performance tuning,
> as it takes
> much time to enter/exit C states, which might delay interrupt
> processing. With
> the new debug option, developers could check if a deep C state could impact
> performance and how much impact it could cause.

Would anyone doing performance tuning not be using the cpu_dma_latency 
interface?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 3/3] cpupower: Update the cpupower tool for new debugfs entries of cpuidle.

[ShuoX Liu]

From: ShuoX Liu <shuox.liu@intel.com>

sys entries of each CPU's cpuidle states has been moved to debugfs, so
we should update the cpupower tool.

Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
---
  tools/power/cpupower/Makefile                      |    3 +-
  tools/power/cpupower/man/cpupower-monitor.1        |    4 +-
  tools/power/cpupower/utils/cpuidle-info.c          |   27 ++--
  tools/power/cpupower/utils/helpers/debugfs.c       |  224 
++++++++++++++++++++
  tools/power/cpupower/utils/helpers/debugfs.h       |   21 ++
  tools/power/cpupower/utils/helpers/sysfs.c         |  177 ---------------
  tools/power/cpupower/utils/helpers/sysfs.h         |   12 -
  .../cpupower/utils/idle_monitor/cpuidle_sysfs.c    |   11 +-
  8 files changed, 269 insertions(+), 210 deletions(-)
  create mode 100644 tools/power/cpupower/utils/helpers/debugfs.c
  create mode 100644 tools/power/cpupower/utils/helpers/debugfs.h

diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
index e8a03ac..d9b0507 100644
--- a/tools/power/cpupower/Makefile
+++ b/tools/power/cpupower/Makefile
@@ -114,7 +114,8 @@ CFLAGS += -DVERSION=\"$(VERSION)\" 
-DPACKAGE=\"$(PACKAGE)\" \
  		-DPACKAGE_BUGREPORT=\"$(PACKAGE_BUGREPORT)\" -D_GNU_SOURCE

  UTIL_OBJS =  utils/helpers/amd.o utils/helpers/topology.o 
utils/helpers/msr.o \
-	utils/helpers/sysfs.o utils/helpers/misc.o utils/helpers/cpuid.o \
+	utils/helpers/sysfs.o utils/helpers/debugfs.o utils/helpers/misc.o \
+	utils/helpers/cpuid.o \
  	utils/helpers/pci.o utils/helpers/bitmask.o \
  	utils/idle_monitor/nhm_idle.o utils/idle_monitor/snb_idle.o \
  	utils/idle_monitor/amd_fam14h_idle.o utils/idle_monitor/cpuidle_sysfs.o \
diff --git a/tools/power/cpupower/man/cpupower-monitor.1 
b/tools/power/cpupower/man/cpupower-monitor.1
index d5cfa26..1cb2951 100644
--- a/tools/power/cpupower/man/cpupower-monitor.1
+++ b/tools/power/cpupower/man/cpupower-monitor.1
@@ -79,7 +79,7 @@ Increase verbosity if the binary was compiled with the 
DEBUG option set.
  .SH MONITOR DESCRIPTIONS
  .SS "Idle_Stats"
  Shows statistics of the cpuidle kernel subsystem. Values are retrieved 
from
-/sys/devices/system/cpu/cpu*/cpuidle/state*/.
+$(debugfs_root)/cpu/cpu*/cpuidle/state*/.
  The kernel updates these values every time an idle state is entered or
  left. Therefore there can be some inaccuracy when cores are in an idle
  state for some time when the measure starts or ends. In worst case it 
can happen
@@ -165,7 +165,7 @@ http://www.intel.com/products/processor/manuals
  .ta
  .nf
  /dev/cpu/*/msr
-/sys/devices/system/cpu/cpu*/cpuidle/state*/.
+$(debugfs_root)/cpu/cpu*/cpuidle/state*/.
  .fi

  .SH "SEE ALSO"
diff --git a/tools/power/cpupower/utils/cpuidle-info.c 
b/tools/power/cpupower/utils/cpuidle-info.c
index b028267..17df26a 100644
--- a/tools/power/cpupower/utils/cpuidle-info.c
+++ b/tools/power/cpupower/utils/cpuidle-info.c
@@ -16,6 +16,7 @@

  #include "helpers/helpers.h"
  #include "helpers/sysfs.h"
+#include "helpers/debugfs.h"
  #include "helpers/bitmask.h"

  #define LINE_LEN 10
@@ -27,7 +28,7 @@ static void cpuidle_cpu_output(unsigned int cpu, int 
verbose)

  	printf(_ ("Analyzing CPU %d:\n"), cpu);

-	idlestates = sysfs_get_idlestate_count(cpu);
+	idlestates = debugfs_get_idlestate_count(cpu);
  	if (idlestates == 0) {
  		printf(_("CPU %u: No idle states\n"), cpu);
  		return;
@@ -35,7 +36,7 @@ static void cpuidle_cpu_output(unsigned int cpu, int 
verbose)
  		printf(_("CPU %u: Can't read idle state info\n"), cpu);
  		return;
  	}
-	tmp = sysfs_get_idlestate_name(cpu, idlestates - 1);
+	tmp = debugfs_get_idlestate_name(cpu, idlestates - 1);
  	if (!tmp) {
  		printf(_("Could not determine max idle state %u\n"),
  		       idlestates - 1);
@@ -46,7 +47,7 @@ static void cpuidle_cpu_output(unsigned int cpu, int 
verbose)

  	printf(_("Available idle states:"));
  	for (idlestate = 1; idlestate < idlestates; idlestate++) {
-		tmp = sysfs_get_idlestate_name(cpu, idlestate);
+		tmp = debugfs_get_idlestate_name(cpu, idlestate);
  		if (!tmp)
  			continue;
  		printf(" %s", tmp);
@@ -58,24 +59,24 @@ static void cpuidle_cpu_output(unsigned int cpu, int 
verbose)
  		return;

  	for (idlestate = 1; idlestate < idlestates; idlestate++) {
-		tmp = sysfs_get_idlestate_name(cpu, idlestate);
+		tmp = debugfs_get_idlestate_name(cpu, idlestate);
  		if (!tmp)
  			continue;
  		printf("%s:\n", tmp);
  		free(tmp);

-		tmp = sysfs_get_idlestate_desc(cpu, idlestate);
+		tmp = debugfs_get_idlestate_desc(cpu, idlestate);
  		if (!tmp)
  			continue;
  		printf(_("Flags/Description: %s\n"), tmp);
  		free(tmp);

  		printf(_("Latency: %lu\n"),
-		       sysfs_get_idlestate_latency(cpu, idlestate));
+		       debugfs_get_idlestate_latency(cpu, idlestate));
  		printf(_("Usage: %lu\n"),
-		       sysfs_get_idlestate_usage(cpu, idlestate));
+		       debugfs_get_idlestate_usage(cpu, idlestate));
  		printf(_("Duration: %llu\n"),
-		       sysfs_get_idlestate_time(cpu, idlestate));
+		       debugfs_get_idlestate_time(cpu, idlestate));
  	}
  	printf("\n");
  }
@@ -108,7 +109,7 @@ static void proc_cpuidle_cpu_output(unsigned int cpu)
  	long max_allowed_cstate = 2000000000;
  	int cstates, cstate;

-	cstates = sysfs_get_idlestate_count(cpu);
+	cstates = debugfs_get_idlestate_count(cpu);
  	if (cstates == 0) {
  		/*
  		 * Go on and print same useless info as you'd see with
@@ -131,11 +132,11 @@ static void proc_cpuidle_cpu_output(unsigned int cpu)
  			 "type[C%d] "), cstate, cstate);
  		printf(_("promotion[--] demotion[--] "));
  		printf(_("latency[%03lu] "),
-		       sysfs_get_idlestate_latency(cpu, cstate));
+		       debugfs_get_idlestate_latency(cpu, cstate));
  		printf(_("usage[%08lu] "),
-		       sysfs_get_idlestate_usage(cpu, cstate));
-		printf(_("duration[%020Lu] \n"),
-		       sysfs_get_idlestate_time(cpu, cstate));
+		       debugfs_get_idlestate_usage(cpu, cstate));
+		printf(_("duration[%020Lu]\n"),
+		       debugfs_get_idlestate_time(cpu, cstate));
  	}
  }

diff --git a/tools/power/cpupower/utils/helpers/debugfs.c 
b/tools/power/cpupower/utils/helpers/debugfs.c
new file mode 100644
index 0000000..1e12e8b
--- /dev/null
+++ b/tools/power/cpupower/utils/helpers/debugfs.c
@@ -0,0 +1,224 @@
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "helpers/debugfs.h"
+
+char debugfs_root[DEBUGFS_PATH_MAX + 1] = "/sys/kernel/debug";
+
+const char *get_debugfs_root()
+{
+	FILE *fp;
+	char type[100];
+
+	fp = fopen("/proc/mounts", "r");
+	if (fp == NULL)
+		return NULL;
+
+	while (fscanf(fp, "%*s %" STR(DEBUGFS_PATH_MAX) "s %99s %*s %*d %*d\n",
+				debugfs_root, type) == 2)
+		if (strcmp(type, "debugfs") == 0)
+			break;
+	fclose(fp);
+
+	if (strcmp(type, "debugfs") != 0)
+		return NULL;
+
+	return debugfs_root;
+}
+
+/* CPUidle idlestate specific $(debugfs_root)/cpu/cpuX/cpuidle/ access */
+
+/*
+ * helper function to read file from $(debugfs_root) into given buffer
+ * fname is a relative path under "cpuX/cpuidle/stateX/" dir
+ * cstates starting with 0, C0 is not counted as cstate.
+ * This means if you want C1 info, pass 0 as idlestate param
+ */
+unsigned int debugfs_idlestate_read_file(unsigned int cpu,
+				unsigned int idlestate, const char *fname,
+				char *buf, size_t buflen)
+{
+	char path[DEBUGFS_PATH_MAX];
+	int fd;
+	ssize_t numread;
+
+	if (get_debugfs_root() == NULL)
+		return 0;
+
+	snprintf(path, sizeof(path), "%s/cpu/" "cpu%u/cpuidle/state%u/%s",
+		 debugfs_root, cpu, idlestate, fname);
+
+	fd = open(path, O_RDONLY);
+	if (fd == -1)
+		return 0;
+
+	numread = read(fd, buf, buflen - 1);
+	if (numread < 1) {
+		close(fd);
+		return 0;
+	}
+
+	buf[numread] = '\0';
+	close(fd);
+
+	return (unsigned int) numread;
+}
+
+/* read access to files which contain one numeric value */
+
+enum idlestate_value {
+	IDLESTATE_USAGE,
+	IDLESTATE_POWER,
+	IDLESTATE_LATENCY,
+	IDLESTATE_TIME,
+	MAX_IDLESTATE_VALUE_FILES
+};
+
+static const char *idlestate_value_files[MAX_IDLESTATE_VALUE_FILES] = {
+	[IDLESTATE_USAGE] = "usage",
+	[IDLESTATE_POWER] = "power",
+	[IDLESTATE_LATENCY] = "latency",
+	[IDLESTATE_TIME]  = "time",
+};
+
+static unsigned long long debugfs_idlestate_get_one_value(unsigned int cpu,
+						     unsigned int idlestate,
+						     enum idlestate_value which)
+{
+	unsigned long long value;
+	unsigned int len;
+	char linebuf[255];
+	char *endp;
+
+	if (which >= MAX_IDLESTATE_VALUE_FILES)
+		return 0;
+
+	len = debugfs_idlestate_read_file(cpu, idlestate,
+					idlestate_value_files[which],
+					linebuf, sizeof(linebuf));
+	if (len == 0)
+		return 0;
+
+	value = strtoull(linebuf, &endp, 0);
+
+	if (endp == linebuf || errno == ERANGE)
+		return 0;
+
+	return value;
+}
+
+/* read access to files which contain one string */
+
+enum idlestate_string {
+	IDLESTATE_DESC,
+	IDLESTATE_NAME,
+	MAX_IDLESTATE_STRING_FILES
+};
+
+static const char *idlestate_string_files[MAX_IDLESTATE_STRING_FILES] = {
+	[IDLESTATE_DESC] = "desc",
+	[IDLESTATE_NAME] = "name",
+};
+
+
+static char *debugfs_idlestate_get_one_string(unsigned int cpu,
+					unsigned int idlestate,
+					enum idlestate_string which)
+{
+	char linebuf[255];
+	char *result;
+	unsigned int len;
+
+	if (which >= MAX_IDLESTATE_STRING_FILES)
+		return NULL;
+
+	len = debugfs_idlestate_read_file(cpu, idlestate,
+					idlestate_string_files[which],
+					linebuf, sizeof(linebuf));
+	if (len == 0)
+		return NULL;
+
+	result = strdup(linebuf);
+	if (result == NULL)
+		return NULL;
+
+	if (result[strlen(result) - 1] == '\n')
+		result[strlen(result) - 1] = '\0';
+
+	return result;
+}
+
+unsigned long debugfs_get_idlestate_latency(unsigned int cpu,
+					unsigned int idlestate)
+{
+	return debugfs_idlestate_get_one_value(cpu, idlestate,
+						IDLESTATE_LATENCY);
+}
+
+unsigned long debugfs_get_idlestate_usage(unsigned int cpu,
+					unsigned int idlestate)
+{
+	return debugfs_idlestate_get_one_value(cpu, idlestate,
+						IDLESTATE_USAGE);
+}
+
+unsigned long long debugfs_get_idlestate_time(unsigned int cpu,
+					unsigned int idlestate)
+{
+	return debugfs_idlestate_get_one_value(cpu, idlestate,
+						IDLESTATE_TIME);
+}
+
+char *debugfs_get_idlestate_name(unsigned int cpu, unsigned int idlestate)
+{
+	return debugfs_idlestate_get_one_string(cpu, idlestate,
+						IDLESTATE_NAME);
+}
+
+char *debugfs_get_idlestate_desc(unsigned int cpu, unsigned int idlestate)
+{
+	return debugfs_idlestate_get_one_string(cpu, idlestate,
+						IDLESTATE_DESC);
+}
+
+/*
+ * Returns number of supported C-states of CPU core cpu
+ * Negativ in error case
+ * Zero if cpuidle does not export any C-states
+ */
+int debugfs_get_idlestate_count(unsigned int cpu)
+{
+	char file[DEBUGFS_PATH_MAX];
+	struct stat statbuf;
+	int idlestates = 1;
+
+	if (get_debugfs_root() == NULL) {
+		perror("Please make sure debugfs has been mounted!\n");
+		return 0;
+	}
+
+	snprintf(file, DEBUGFS_PATH_MAX, "%s/cpu/" "cpu%u/cpuidle",
+					debugfs_root, cpu);
+	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
+		return -ENODEV;
+
+	snprintf(file, DEBUGFS_PATH_MAX, "%s/cpu/" "cpu%u/cpuidle/state0",
+					debugfs_root, cpu);
+	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
+		return 0;
+
+	while (stat(file, &statbuf) == 0 && S_ISDIR(statbuf.st_mode)) {
+		snprintf(file, DEBUGFS_PATH_MAX, "%s/cpu/"
+				"cpu%u/cpuidle/state%d",
+				debugfs_root, cpu, idlestates);
+		idlestates++;
+	}
+	idlestates--;
+	return idlestates;
+}
diff --git a/tools/power/cpupower/utils/helpers/debugfs.h 
b/tools/power/cpupower/utils/helpers/debugfs.h
new file mode 100644
index 0000000..8db8afc
--- /dev/null
+++ b/tools/power/cpupower/utils/helpers/debugfs.h
@@ -0,0 +1,21 @@
+#ifndef __CPUPOWER_HELPERS_DEBUGFS_H__
+#define __CPUPOWER_HELPERS_DEBUGFS_H__
+
+#define _STR(x) #x
+#define STR(x) _STR(x)
+
+#define DEBUGFS_PATH_MAX 255
+
+extern unsigned long debugfs_get_idlestate_latency(unsigned int cpu,
+						unsigned int idlestate);
+extern unsigned long debugfs_get_idlestate_usage(unsigned int cpu,
+					unsigned int idlestate);
+extern unsigned long long debugfs_get_idlestate_time(unsigned int cpu,
+						unsigned int idlestate);
+extern char *debugfs_get_idlestate_name(unsigned int cpu,
+				unsigned int idlestate);
+extern char *debugfs_get_idlestate_desc(unsigned int cpu,
+				unsigned int idlestate);
+extern int debugfs_get_idlestate_count(unsigned int cpu);
+
+#endif /* __CPUPOWER_HELPERS_DEBUGFS_H__ */
diff --git a/tools/power/cpupower/utils/helpers/sysfs.c 
b/tools/power/cpupower/utils/helpers/sysfs.c
index c634302..e97a58e 100644
--- a/tools/power/cpupower/utils/helpers/sysfs.c
+++ b/tools/power/cpupower/utils/helpers/sysfs.c
@@ -106,181 +106,6 @@ int sysfs_is_cpu_online(unsigned int cpu)
  	return value;
  }

-/* CPUidle idlestate specific /sys/devices/system/cpu/cpuX/cpuidle/ 
access */
-
-/*
- * helper function to read file from /sys into given buffer
- * fname is a relative path under "cpuX/cpuidle/stateX/" dir
- * cstates starting with 0, C0 is not counted as cstate.
- * This means if you want C1 info, pass 0 as idlestate param
- */
-unsigned int sysfs_idlestate_read_file(unsigned int cpu, unsigned int 
idlestate,
-			     const char *fname, char *buf, size_t buflen)
-{
-	char path[SYSFS_PATH_MAX];
-	int fd;
-	ssize_t numread;
-
-	snprintf(path, sizeof(path), PATH_TO_CPU "cpu%u/cpuidle/state%u/%s",
-		 cpu, idlestate, fname);
-
-	fd = open(path, O_RDONLY);
-	if (fd == -1)
-		return 0;
-
-	numread = read(fd, buf, buflen - 1);
-	if (numread < 1) {
-		close(fd);
-		return 0;
-	}
-
-	buf[numread] = '\0';
-	close(fd);
-
-	return (unsigned int) numread;
-}
-
-/* read access to files which contain one numeric value */
-
-enum idlestate_value {
-	IDLESTATE_USAGE,
-	IDLESTATE_POWER,
-	IDLESTATE_LATENCY,
-	IDLESTATE_TIME,
-	MAX_IDLESTATE_VALUE_FILES
-};
-
-static const char *idlestate_value_files[MAX_IDLESTATE_VALUE_FILES] = {
-	[IDLESTATE_USAGE] = "usage",
-	[IDLESTATE_POWER] = "power",
-	[IDLESTATE_LATENCY] = "latency",
-	[IDLESTATE_TIME]  = "time",
-};
-
-static unsigned long long sysfs_idlestate_get_one_value(unsigned int cpu,
-						     unsigned int idlestate,
-						     enum idlestate_value which)
-{
-	unsigned long long value;
-	unsigned int len;
-	char linebuf[MAX_LINE_LEN];
-	char *endp;
-
-	if (which >= MAX_IDLESTATE_VALUE_FILES)
-		return 0;
-
-	len = sysfs_idlestate_read_file(cpu, idlestate,
-					idlestate_value_files[which],
-					linebuf, sizeof(linebuf));
-	if (len == 0)
-		return 0;
-
-	value = strtoull(linebuf, &endp, 0);
-
-	if (endp == linebuf || errno == ERANGE)
-		return 0;
-
-	return value;
-}
-
-/* read access to files which contain one string */
-
-enum idlestate_string {
-	IDLESTATE_DESC,
-	IDLESTATE_NAME,
-	MAX_IDLESTATE_STRING_FILES
-};
-
-static const char *idlestate_string_files[MAX_IDLESTATE_STRING_FILES] = {
-	[IDLESTATE_DESC] = "desc",
-	[IDLESTATE_NAME] = "name",
-};
-
-
-static char *sysfs_idlestate_get_one_string(unsigned int cpu,
-					unsigned int idlestate,
-					enum idlestate_string which)
-{
-	char linebuf[MAX_LINE_LEN];
-	char *result;
-	unsigned int len;
-
-	if (which >= MAX_IDLESTATE_STRING_FILES)
-		return NULL;
-
-	len = sysfs_idlestate_read_file(cpu, idlestate,
-					idlestate_string_files[which],
-					linebuf, sizeof(linebuf));
-	if (len == 0)
-		return NULL;
-
-	result = strdup(linebuf);
-	if (result == NULL)
-		return NULL;
-
-	if (result[strlen(result) - 1] == '\n')
-		result[strlen(result) - 1] = '\0';
-
-	return result;
-}
-
-unsigned long sysfs_get_idlestate_latency(unsigned int cpu,
-					unsigned int idlestate)
-{
-	return sysfs_idlestate_get_one_value(cpu, idlestate, IDLESTATE_LATENCY);
-}
-
-unsigned long sysfs_get_idlestate_usage(unsigned int cpu,
-					unsigned int idlestate)
-{
-	return sysfs_idlestate_get_one_value(cpu, idlestate, IDLESTATE_USAGE);
-}
-
-unsigned long long sysfs_get_idlestate_time(unsigned int cpu,
-					unsigned int idlestate)
-{
-	return sysfs_idlestate_get_one_value(cpu, idlestate, IDLESTATE_TIME);
-}
-
-char *sysfs_get_idlestate_name(unsigned int cpu, unsigned int idlestate)
-{
-	return sysfs_idlestate_get_one_string(cpu, idlestate, IDLESTATE_NAME);
-}
-
-char *sysfs_get_idlestate_desc(unsigned int cpu, unsigned int idlestate)
-{
-	return sysfs_idlestate_get_one_string(cpu, idlestate, IDLESTATE_DESC);
-}
-
-/*
- * Returns number of supported C-states of CPU core cpu
- * Negativ in error case
- * Zero if cpuidle does not export any C-states
- */
-int sysfs_get_idlestate_count(unsigned int cpu)
-{
-	char file[SYSFS_PATH_MAX];
-	struct stat statbuf;
-	int idlestates = 1;
-
-
-	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpuidle");
-	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
-		return -ENODEV;
-
-	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpu%u/cpuidle/state0", cpu);
-	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
-		return 0;
-
-	while (stat(file, &statbuf) == 0 && S_ISDIR(statbuf.st_mode)) {
-		snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU
-			 "cpu%u/cpuidle/state%d", cpu, idlestates);
-		idlestates++;
-	}
-	idlestates--;
-	return idlestates;
-}
-
  /* CPUidle general /sys/devices/system/cpu/cpuidle/ sysfs access ********/

  /*
@@ -297,8 +122,6 @@ static unsigned int sysfs_cpuidle_read_file(const 
char *fname, char *buf,
  	return sysfs_read_file(path, buf, buflen);
  }

-
-
  /* read access to files which contain one string */

  enum cpuidle_string {
diff --git a/tools/power/cpupower/utils/helpers/sysfs.h 
b/tools/power/cpupower/utils/helpers/sysfs.h
index 8cb797b..d189777 100644
--- a/tools/power/cpupower/utils/helpers/sysfs.h
+++ b/tools/power/cpupower/utils/helpers/sysfs.h
@@ -9,18 +9,6 @@ extern unsigned int sysfs_read_file(const char *path, 
char *buf, size_t buflen);

  extern int sysfs_is_cpu_online(unsigned int cpu);

-extern unsigned long sysfs_get_idlestate_latency(unsigned int cpu,
-						unsigned int idlestate);
-extern unsigned long sysfs_get_idlestate_usage(unsigned int cpu,
-					unsigned int idlestate);
-extern unsigned long long sysfs_get_idlestate_time(unsigned int cpu,
-						unsigned int idlestate);
-extern char *sysfs_get_idlestate_name(unsigned int cpu,
-				unsigned int idlestate);
-extern char *sysfs_get_idlestate_desc(unsigned int cpu,
-				unsigned int idlestate);
-extern int sysfs_get_idlestate_count(unsigned int cpu);
-
  extern char *sysfs_get_cpuidle_governor(void);
  extern char *sysfs_get_cpuidle_driver(void);

diff --git a/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c 
b/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
index bcd22a1..a10fec9 100644
--- a/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
+++ b/tools/power/cpupower/utils/idle_monitor/cpuidle_sysfs.c
@@ -12,6 +12,7 @@
  #include <limits.h>

  #include "helpers/sysfs.h"
+#include "helpers/debugfs.h"
  #include "helpers/helpers.h"
  #include "idle_monitor/cpupower-monitor.h"

@@ -51,7 +52,7 @@ static int cpuidle_start(void)
  		for (state = 0; state < cpuidle_sysfs_monitor.hw_states_num;
  		     state++) {
  			previous_count[cpu][state] =
-				sysfs_get_idlestate_time(cpu, state);
+				debugfs_get_idlestate_time(cpu, state);
  			dprint("CPU %d - State: %d - Val: %llu\n",
  			       cpu, state, previous_count[cpu][state]);
  		}
@@ -70,7 +71,7 @@ static int cpuidle_stop(void)
  		for (state = 0; state < cpuidle_sysfs_monitor.hw_states_num;
  		     state++) {
  			current_count[cpu][state] =
-				sysfs_get_idlestate_time(cpu, state);
+				debugfs_get_idlestate_time(cpu, state);
  			dprint("CPU %d - State: %d - Val: %llu\n",
  			       cpu, state, previous_count[cpu][state]);
  		}
@@ -132,13 +133,13 @@ static struct cpuidle_monitor *cpuidle_register(void)
  	char *tmp;

  	/* Assume idle state count is the same for all CPUs */
-	cpuidle_sysfs_monitor.hw_states_num = sysfs_get_idlestate_count(0);
+	cpuidle_sysfs_monitor.hw_states_num = debugfs_get_idlestate_count(0);

  	if (cpuidle_sysfs_monitor.hw_states_num <= 0)
  		return NULL;

  	for (num = 0; num < cpuidle_sysfs_monitor.hw_states_num; num++) {
-		tmp = sysfs_get_idlestate_name(0, num);
+		tmp = debugfs_get_idlestate_name(0, num);
  		if (tmp == NULL)
  			continue;

@@ -146,7 +147,7 @@ static struct cpuidle_monitor *cpuidle_register(void)
  		strncpy(cpuidle_cstates[num].name, tmp, CSTATE_NAME_LEN - 1);
  		free(tmp);

-		tmp = sysfs_get_idlestate_desc(0, num);
+		tmp = debugfs_get_idlestate_desc(0, num);
  		if (tmp == NULL)
  			continue;
  		strncpy(cpuidle_cstates[num].desc, tmp,	CSTATE_DESC_LEN - 1);
-- 
1.7.1

Re: [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.

[Greg KH]

On Mon, Mar 12, 2012 at 05:19:36PM +0800, ShuoX Liu wrote:
> I created a series to do this.
> 
> [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
> [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state
> for debug purpose.
> [PATCH 3/3] cpupower: Update the cpupower tool for new debugfs
> entries of cpuidle.
> 
> Below are patches.

The patch is line-wrapped and can not be applied :(

> --- /dev/null
> +++ b/drivers/cpuidle/debugfs.c
> @@ -0,0 +1,176 @@
> +/*
> + * debugfs.c - debugfs support
> + *
> + * (C) 2006-2007 Shaohua Li <shaohua.li@intel.com>
> + * (C) 2012 ShuoX Liu <shuox.liu@intel.com>
> + *
> + * This code is licenced under the GPL.

What version of the GPL?

thanks,

greg k-h

Re: [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.

[ShuoX Liu]

On 2012年03月13日 01:22, Greg KH wrote:

> On Mon, Mar 12, 2012 at 05:19:36PM +0800, ShuoX Liu wrote:
>> I created a series to do this.
>>
>> [PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs.
>> [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state
>> for debug purpose.
>> [PATCH 3/3] cpupower: Update the cpupower tool for new debugfs
>> entries of cpuidle.
>>
>> Below are patches.
> 
> The patch is line-wrapped and can not be applied :(

Sorry that i made a mistake. I sent email with fresh email-client which
has the text-wrapping default. I will configure it.
I will send the patch in good format when needed.

>> --- /dev/null
>> +++ b/drivers/cpuidle/debugfs.c
>> @@ -0,0 +1,176 @@
>> +/*
>> + * debugfs.c - debugfs support
>> + *
>> + * (C) 2006-2007 Shaohua Li <shaohua.li@intel.com>
>> + * (C) 2012 ShuoX Liu <shuox.liu@intel.com>
>> + *
>> + * This code is licenced under the GPL.
> 
> What version of the GPL?

GPLv2

> 
> thanks,
> 
> greg k-h


thanks,
Shuo

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

[Mark Brown]

On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:

> Do you know of any tools using these files?  I have never heard of them,
> and I was told we should move these files years ago.  So I don't think
> there should be any api issues.

powertop uses them.

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

[Greg KH]

On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> 
> > Do you know of any tools using these files?  I have never heard of them,
> > and I was told we should move these files years ago.  So I don't think
> > there should be any api issues.
> 
> powertop uses them.

Ok, then we can't move them all.

We should then just move the ones that have multiple lines, as I'm
pretty sure powertop doesn't use them, right?

thanks,

greg k-h

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

[Yanmin Zhang]

On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > 
> > > Do you know of any tools using these files?  I have never heard of them,
> > > and I was told we should move these files years ago.  So I don't think
> > > there should be any api issues.
> > 
> > powertop uses them.
> 
> Ok, then we can't move them all.
> 
> We should then just move the ones that have multiple lines, as I'm
> pretty sure powertop doesn't use them, right?
All sys files under cpu/cpuXXX/cpuidle have single line. If we move
some files to debugfs and keep others under sysfs, users might be confused.

Should we go back to the 1st version which just adds the new entry to
sysfs?

In addition, should we move powertop to tools/?

Yanmin

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

[Greg KH]

On Tue, Mar 13, 2012 at 09:36:34AM +0800, Yanmin Zhang wrote:
> On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> > On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > > 
> > > > Do you know of any tools using these files?  I have never heard of them,
> > > > and I was told we should move these files years ago.  So I don't think
> > > > there should be any api issues.
> > > 
> > > powertop uses them.
> > 
> > Ok, then we can't move them all.
> > 
> > We should then just move the ones that have multiple lines, as I'm
> > pretty sure powertop doesn't use them, right?
> All sys files under cpu/cpuXXX/cpuidle have single line. If we move
> some files to debugfs and keep others under sysfs, users might be confused.
> 
> Should we go back to the 1st version which just adds the new entry to
> sysfs?

Wait, I thought this whole thing started when we wanted to move the
files that had multiple lines out of sysfs?

If none of these do, and they all are being used by tools already, then
fine, they should stay.

But for some reason, I thought there was a problem here.  Perhaps that
was in the cpufreq code?

confused,

greg k-h

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

[Yanmin Zhang]

On Tue, 2012-03-13 at 12:29 -0700, Greg KH wrote:
> On Tue, Mar 13, 2012 at 09:36:34AM +0800, Yanmin Zhang wrote:
> > On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> > > On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > > > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > > > 
> > > > > Do you know of any tools using these files?  I have never heard of them,
> > > > > and I was told we should move these files years ago.  So I don't think
> > > > > there should be any api issues.
> > > > 
> > > > powertop uses them.
> > > 
> > > Ok, then we can't move them all.
> > > 
> > > We should then just move the ones that have multiple lines, as I'm
> > > pretty sure powertop doesn't use them, right?
> > All sys files under cpu/cpuXXX/cpuidle have single line. If we move
> > some files to debugfs and keep others under sysfs, users might be confused.
> > 
> > Should we go back to the 1st version which just adds the new entry to
> > sysfs?
> 
> Wait, I thought this whole thing started when we wanted to move the
> files that had multiple lines out of sysfs?
No. Liu Shuo's patch adds a new entry under cpu/cpuXXX/cpuidle and users
can disable specific C state.
A gentleman raised that if we should move it to debugfs, then you suggested
to move all files under cpu/cpuXXX/cpuidle to debugfs.

> 
> If none of these do, and they all are being used by tools already, then
> fine, they should stay.
Agree.

> 
> But for some reason, I thought there was a problem here.  Perhaps that
> was in the cpufreq code?
I checked cpufreq quickly. Every file has single-line, but some have
multiple-fields.

We would send a new patch based on sysfs as the new entry has
single line.

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

[Greg KH]

On Wed, Mar 14, 2012 at 08:55:00AM +0800, Yanmin Zhang wrote:
> On Tue, 2012-03-13 at 12:29 -0700, Greg KH wrote:
> > On Tue, Mar 13, 2012 at 09:36:34AM +0800, Yanmin Zhang wrote:
> > > On Mon, 2012-03-12 at 12:29 -0700, Greg KH wrote:
> > > > On Mon, Mar 12, 2012 at 06:11:51PM +0000, Mark Brown wrote:
> > > > > On Tue, Mar 06, 2012 at 06:39:35AM -0800, Greg KH wrote:
> > > > > 
> > > > > > Do you know of any tools using these files?  I have never heard of them,
> > > > > > and I was told we should move these files years ago.  So I don't think
> > > > > > there should be any api issues.
> > > > > 
> > > > > powertop uses them.
> > > > 
> > > > Ok, then we can't move them all.
> > > > 
> > > > We should then just move the ones that have multiple lines, as I'm
> > > > pretty sure powertop doesn't use them, right?
> > > All sys files under cpu/cpuXXX/cpuidle have single line. If we move
> > > some files to debugfs and keep others under sysfs, users might be confused.
> > > 
> > > Should we go back to the 1st version which just adds the new entry to
> > > sysfs?
> > 
> > Wait, I thought this whole thing started when we wanted to move the
> > files that had multiple lines out of sysfs?
> No. Liu Shuo's patch adds a new entry under cpu/cpuXXX/cpuidle and users
> can disable specific C state.
> A gentleman raised that if we should move it to debugfs, then you suggested
> to move all files under cpu/cpuXXX/cpuidle to debugfs.
> 
> > 
> > If none of these do, and they all are being used by tools already, then
> > fine, they should stay.
> Agree.
> 
> > 
> > But for some reason, I thought there was a problem here.  Perhaps that
> > was in the cpufreq code?
> I checked cpufreq quickly. Every file has single-line, but some have
> multiple-fields.
> 
> We would send a new patch based on sysfs as the new entry has
> single line.

Then that's fine, I'm very sorry for getting this wrong the first time
and causing you extra work.  I owe you all the drink of your choice the
next time I see you.

greg k-h

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

[ShuoX Liu]

From: ShuoX Liu <shuox.liu@intel.com>

Some C states of new CPU might be not good. One reason is BIOS might configure
them incorrectly. To help developers root cause it quickly, the patch adds a
new sysfs entry, so developers could disable specific C state manually.

In addition, C state might have much impact on performance tuning, as it takes
much time to enter/exit C states, which might delay interrupt processing. With
the new debug option, developers could check if a deep C state could  impact
performance and how much impact it could cause.

Also add this option in Documentation/cpuidle/sysfs.txt.

Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com>
Reviewed-and-Tested-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
---
Thanks for all comments in previous mails. Here is the new patch.

---
 Documentation/cpuidle/sysfs.txt  |    5 +++++
 drivers/cpuidle/cpuidle.c        |    1 +
 drivers/cpuidle/governors/menu.c |    5 ++++-
 drivers/cpuidle/sysfs.c          |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h          |    1 +
 5 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/Documentation/cpuidle/sysfs.txt b/Documentation/cpuidle/sysfs.txt
index 50d7b16..9d28a34 100644
--- a/Documentation/cpuidle/sysfs.txt
+++ b/Documentation/cpuidle/sysfs.txt
@@ -36,6 +36,7 @@ drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
 /sys/devices/system/cpu/cpu0/cpuidle/state0:
 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
@@ -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
@@ -54,6 +56,7 @@ total 0
 /sys/devices/system/cpu/cpu0/cpuidle/state2:
 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
@@ -63,6 +66,7 @@ total 0
 /sys/devices/system/cpu/cpu0/cpuidle/state3:
 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
@@ -72,6 +76,7 @@ total 0
 
 
 * desc : Small description about the idle state (string)
+* disable : Option to disable this idle state (bool)
 * latency : Latency to exit out of this idle state (in microseconds)
 * name : Name of the idle state (string)
 * power : Power consumed while in this idle state (in milliwatts)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..7d66d3e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
 	state->power_usage = -1;
 	state->flags = 0;
 	state->enter = poll_idle;
+	state->disable = 0;
 }
 #else
 static void poll_idle_init(struct cpuidle_driver *drv) {}
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index ad09526..5c17ca1 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	 * We want to default to C1 (hlt), not to busy polling
 	 * unless the timer is happening really really soon.
 	 */
-	if (data->expected_us > 5)
+	if (data->expected_us > 5 &&
+		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
 		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
 
 	/*
@@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 
+		if (s->disable)
+			continue;
 		if (s->target_residency > data->predicted_us)
 			continue;
 		if (s->exit_latency > latency_req)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 3fe41fe..f0fe0f5 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -11,6 +11,7 @@
 #include <linux/sysfs.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
+#include <linux/capability.h>
 
 #include "cpuidle.h"
 
@@ -222,6 +223,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 +233,21 @@ 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) \
+{ \
+	long value; \
+	if (!capable(CAP_SYS_ADMIN)) \
+		return -EPERM; \
+	kstrtol(buf, 0, &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}
+
 #define define_show_state_ull_function(_name) \
 static ssize_t show_state_##_name(struct cpuidle_state *state, \
 			struct cpuidle_state_usage *state_usage, char *buf) \
@@ -251,6 +270,8 @@ define_show_state_ull_function(usage)
 define_show_state_ull_function(time)
 define_show_state_str_function(name)
 define_show_state_str_function(desc)
+define_show_state_function(disable)
+define_store_state_function(disable)
 
 define_one_state_ro(name, show_state_name);
 define_one_state_ro(desc, show_state_desc);
@@ -258,6 +279,7 @@ define_one_state_ro(latency, show_state_exit_latency);
 define_one_state_ro(power, show_state_power_usage);
 define_one_state_ro(usage, show_state_usage);
 define_one_state_ro(time, show_state_time);
+define_one_state_rw(disable, show_state_disable, store_state_disable);
 
 static struct attribute *cpuidle_state_default_attrs[] = {
 	&attr_name.attr,
@@ -266,6 +288,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
 	&attr_power.attr,
 	&attr_usage.attr,
 	&attr_time.attr,
+	&attr_disable.attr,
 	NULL
 };
 
@@ -287,8 +310,22 @@ static ssize_t cpuidle_state_show(struct kobject * kobj,
 	return ret;
 }
 
+static ssize_t cpuidle_state_store(struct kobject *kobj,
+	struct attribute *attr, const char *buf, size_t size)
+{
+	int ret = -EIO;
+	struct cpuidle_state *state = kobj_to_state(kobj);
+	struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
+
+	if (cattr->store)
+		ret = cattr->store(state, buf, size);
+
+	return ret;
+}
+
 static const struct sysfs_ops cpuidle_state_sysfs_ops = {
 	.show = cpuidle_state_show,
+	.store = cpuidle_state_store,
 };
 
 static void cpuidle_state_sysfs_release(struct kobject *kobj)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..eb150a5 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -45,6 +45,7 @@ struct cpuidle_state {
 	unsigned int	exit_latency; /* in US */
 	unsigned int	power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
+	unsigned int    disable;
 
 	int (*enter)	(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
-- 
1.7.1

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

[Yanmin Zhang]

On Wed, 2012-03-14 at 11:24 +0800, ShuoX Liu wrote:
> From: ShuoX Liu <shuox.liu@intel.com>
> 
> Some C states of new CPU might be not good. One reason is BIOS might configure
> them incorrectly. To help developers root cause it quickly, the patch adds a
> new sysfs entry, so developers could disable specific C state manually.
> 
> In addition, C state might have much impact on performance tuning, as it takes
> much time to enter/exit C states, which might delay interrupt processing. With
> the new debug option, developers could check if a deep C state could  impact
> performance and how much impact it could cause.
> 
> Also add this option in Documentation/cpuidle/sysfs.txt.
Andrew,

Would you like to merge the new patch into your testing tree? Shuo revised the old
patches based on all comments from community.

Deepthi confirms the patch is useful.

Thanks,
Yanmin

> 
> Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
> Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com>
> Reviewed-and-Tested-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> ---
> Thanks for all comments in previous mails. Here is the new patch.
> 
> ---
>  Documentation/cpuidle/sysfs.txt  |    5 +++++
>  drivers/cpuidle/cpuidle.c        |    1 +
>  drivers/cpuidle/governors/menu.c |    5 ++++-
>  drivers/cpuidle/sysfs.c          |   37 +++++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h          |    1 +
>  5 files changed, 48 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/cpuidle/sysfs.txt b/Documentation/cpuidle/sysfs.txt
> index 50d7b16..9d28a34 100644
> --- a/Documentation/cpuidle/sysfs.txt
> +++ b/Documentation/cpuidle/sysfs.txt
> @@ -36,6 +36,7 @@ drwxr-xr-x 2 root root 0 Feb  8 10:42 state3
>  /sys/devices/system/cpu/cpu0/cpuidle/state0:
>  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
> @@ -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
> @@ -54,6 +56,7 @@ total 0
>  /sys/devices/system/cpu/cpu0/cpuidle/state2:
>  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
> @@ -63,6 +66,7 @@ total 0
>  /sys/devices/system/cpu/cpu0/cpuidle/state3:
>  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
> @@ -72,6 +76,7 @@ total 0
>  
> 
>  * desc : Small description about the idle state (string)
> +* disable : Option to disable this idle state (bool)
>  * latency : Latency to exit out of this idle state (in microseconds)
>  * name : Name of the idle state (string)
>  * power : Power consumed while in this idle state (in milliwatts)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..7d66d3e 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
>  	state->power_usage = -1;
>  	state->flags = 0;
>  	state->enter = poll_idle;
> +	state->disable = 0;
>  }
>  #else
>  static void poll_idle_init(struct cpuidle_driver *drv) {}
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index ad09526..5c17ca1 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	 * We want to default to C1 (hlt), not to busy polling
>  	 * unless the timer is happening really really soon.
>  	 */
> -	if (data->expected_us > 5)
> +	if (data->expected_us > 5 &&
> +		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
>  		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>  
>  	/*
> @@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
>  		struct cpuidle_state *s = &drv->states[i];
>  
> +		if (s->disable)
> +			continue;
>  		if (s->target_residency > data->predicted_us)
>  			continue;
>  		if (s->exit_latency > latency_req)
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 3fe41fe..f0fe0f5 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -11,6 +11,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/slab.h>
>  #include <linux/cpu.h>
> +#include <linux/capability.h>
>  
>  #include "cpuidle.h"
>  
> @@ -222,6 +223,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 +233,21 @@ 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) \
> +{ \
> +	long value; \
> +	if (!capable(CAP_SYS_ADMIN)) \
> +		return -EPERM; \
> +	kstrtol(buf, 0, &value); \
> +	if (value) \
> +		state->disable = 1; \
> +	else \
> +		state->disable = 0; \
> +	return size; \
> +}
> +
>  #define define_show_state_ull_function(_name) \
>  static ssize_t show_state_##_name(struct cpuidle_state *state, \
>  			struct cpuidle_state_usage *state_usage, char *buf) \
> @@ -251,6 +270,8 @@ define_show_state_ull_function(usage)
>  define_show_state_ull_function(time)
>  define_show_state_str_function(name)
>  define_show_state_str_function(desc)
> +define_show_state_function(disable)
> +define_store_state_function(disable)
>  
>  define_one_state_ro(name, show_state_name);
>  define_one_state_ro(desc, show_state_desc);
> @@ -258,6 +279,7 @@ define_one_state_ro(latency, show_state_exit_latency);
>  define_one_state_ro(power, show_state_power_usage);
>  define_one_state_ro(usage, show_state_usage);
>  define_one_state_ro(time, show_state_time);
> +define_one_state_rw(disable, show_state_disable, store_state_disable);
>  
>  static struct attribute *cpuidle_state_default_attrs[] = {
>  	&attr_name.attr,
> @@ -266,6 +288,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
>  	&attr_power.attr,
>  	&attr_usage.attr,
>  	&attr_time.attr,
> +	&attr_disable.attr,
>  	NULL
>  };
>  
> @@ -287,8 +310,22 @@ static ssize_t cpuidle_state_show(struct kobject * kobj,
>  	return ret;
>  }
>  
> +static ssize_t cpuidle_state_store(struct kobject *kobj,
> +	struct attribute *attr, const char *buf, size_t size)
> +{
> +	int ret = -EIO;
> +	struct cpuidle_state *state = kobj_to_state(kobj);
> +	struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
> +
> +	if (cattr->store)
> +		ret = cattr->store(state, buf, size);
> +
> +	return ret;
> +}
> +
>  static const struct sysfs_ops cpuidle_state_sysfs_ops = {
>  	.show = cpuidle_state_show,
> +	.store = cpuidle_state_store,
>  };
>  
>  static void cpuidle_state_sysfs_release(struct kobject *kobj)
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..eb150a5 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -45,6 +45,7 @@ struct cpuidle_state {
>  	unsigned int	exit_latency; /* in US */
>  	unsigned int	power_usage; /* in mW */
>  	unsigned int	target_residency; /* in US */
> +	unsigned int    disable;
>  
>  	int (*enter)	(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,

Re: [PATCH v4] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

[Andrew Morton]

On Wed, 14 Mar 2012 11:24:40 +0800
ShuoX Liu <shuox.liu@intel.com> wrote:

> From: ShuoX Liu <shuox.liu@intel.com>
> 
> Some C states of new CPU might be not good. One reason is BIOS might configure
> them incorrectly. To help developers root cause it quickly, the patch adds a
> new sysfs entry, so developers could disable specific C state manually.
> 
> In addition, C state might have much impact on performance tuning, as it takes
> much time to enter/exit C states, which might delay interrupt processing. With
> the new debug option, developers could check if a deep C state could  impact
> performance and how much impact it could cause.
> 
> Also add this option in Documentation/cpuidle/sysfs.txt.
> 
>
> ...
>
> +#define define_one_state_rw(_name, show, store) \
> +static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
>
> ...
>

> +#define define_store_state_function(_name) \
> +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> +		const char *buf, size_t size) \
> +{ \
> +	long value; \
> +	if (!capable(CAP_SYS_ADMIN)) \
> +		return -EPERM; \

Is the capability check required?  The 0644 permissions aren't sufficient?

> +	kstrtol(buf, 0, &value); \
> +	if (value) \
> +		state->disable = 1; \
> +	else \
> +		state->disable = 0; \
> +	return size; \
> +}
>
> ...
>

Re: [PATCH v4] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

[Henrique de Moraes Holschuh]

On Fri, 16 Mar 2012, Andrew Morton wrote:
> > +#define define_store_state_function(_name) \
> > +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> > +		const char *buf, size_t size) \
> > +{ \
> > +	long value; \
> > +	if (!capable(CAP_SYS_ADMIN)) \
> > +		return -EPERM; \
> 
> Is the capability check required?  The 0644 permissions aren't sufficient?

That depends.  Without capable(), restricted root (one which had its
capabilities dropped) can disable idle states.

If you want to restrict something to "root only", IMHO it should be
using capable(), as restricted root really doesn't qualify for "root
only" things.

If you wanted to restrict it to "owner only" OTOH, then yes, the
capable() check (especially with the very coarse set of capabilities we
currently have) might not be desireable.

However, if we had a power-management capability, it would be best to
use that one instead of CAP_SYS_ADMIN (aka "the new root" as LWN called
it).

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

Re: [PATCH v4] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

[Andrew Morton]

On Wed, 14 Mar 2012 11:24:40 +0800
ShuoX Liu <shuox.liu@intel.com> wrote:

> From: ShuoX Liu <shuox.liu@intel.com>
> 
> Some C states of new CPU might be not good. One reason is BIOS might configure
> them incorrectly. To help developers root cause it quickly, the patch adds a
> new sysfs entry, so developers could disable specific C state manually.
> 
> In addition, C state might have much impact on performance tuning, as it takes
> much time to enter/exit C states, which might delay interrupt processing. With
> the new debug option, developers could check if a deep C state could  impact
> performance and how much impact it could cause.
> 
> ...
>
> +#define define_store_state_function(_name) \
> +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> +		const char *buf, size_t size) \
> +{ \
> +	long value; \
> +	if (!capable(CAP_SYS_ADMIN)) \
> +		return -EPERM; \
> +	kstrtol(buf, 0, &value); \
> +	if (value) \
> +		state->disable = 1; \
> +	else \
> +		state->disable = 0; \
> +	return size; \
> +}

drivers/cpuidle/sysfs.c: In function 'store_state_disable':
drivers/cpuidle/sysfs.c:274: warning: ignoring return value of 'kstrtol', declared with attribute warn_unused_result

The check is there for a reason - the kernel shouldn't silently accept
random garbage inputs.

--- a/drivers/cpuidle/sysfs.c~cpuidle-add-a-sysfs-entry-to-disable-specific-c-state-for-debug-purpose-fix
+++ a/drivers/cpuidle/sysfs.c
@@ -238,9 +238,12 @@ static ssize_t store_state_##_name(struc
 		const char *buf, size_t size) \
 { \
 	long value; \
+	int err; \
 	if (!capable(CAP_SYS_ADMIN)) \
 		return -EPERM; \
-	kstrtol(buf, 0, &value); \
+	err = kstrtol(buf, 0, &value); \
+	if (err) \
+		return err; \
 	if (value) \
 		state->disable = 1; \
 	else \
_

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

[Yanmin Zhang]

On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh 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...
Sorry. Could you elaborate it?

Basically, CPU initialization codes (ACPI on x86) checks the capabilities and
creates the C states in kernel. If we find there are such C states under
/sys/devices/system/cpu/cpuXXX/cpuidle/, we assume CPU supports them.
So here we wouldn't check them again.

> 
> 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)?
We would use strict_strtol to parse the value in next version.

Thanks for your kind comments!

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

[Henrique de Moraes Holschuh]

On Tue, 06 Mar 2012, Yanmin Zhang wrote:
> On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh 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...
> Sorry. Could you elaborate it?

Sure.  Should any user be able to disable a C state, therefore causing
the system to consume more power?

I am pretty sure the answer is NO, in which case you should check for
the appropriate user credentials before you allow a write to these
"debug" controls to succeed.  "capability" here is one of the CAP_*
capabilities tested through capable(), which are supposed to limit even
root.

> > 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)?
> We would use strict_strtol to parse the value in next version.

Thanks!

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

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

[Yanmin Zhang]

On Mon, 2012-03-12 at 21:42 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 06 Mar 2012, Yanmin Zhang wrote:
> > On Mon, 2012-03-05 at 07:18 -0300, Henrique de Moraes Holschuh 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...
> > Sorry. Could you elaborate it?
> 
> Sure.  Should any user be able to disable a C state, therefore causing
> the system to consume more power?
Here we use the simple way to check access. Only root could change it.

> 
> I am pretty sure the answer is NO, in which case you should check for
> the appropriate user credentials before you allow a write to these
> "debug" controls to succeed.  "capability" here is one of the CAP_*
> capabilities tested through capable(), which are supposed to limit even
> root.
We would add below check.
 
        if (!capable(CAP_SYS_ADMIN))
                return -EPERM;

> 
> > > 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)?
> > We would use strict_strtol to parse the value in next version.
> 
> Thanks!
> 

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

[Henrique de Moraes Holschuh]

On Tue, 13 Mar 2012, Yanmin Zhang wrote:
> > > > > +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...
> > > Sorry. Could you elaborate it?
> > 
> > Sure.  Should any user be able to disable a C state, therefore causing
> > the system to consume more power?
> Here we use the simple way to check access. Only root could change it.

Yea, but capabilities are supposed to constrain root as well :-)

> > I am pretty sure the answer is NO, in which case you should check for
> > the appropriate user credentials before you allow a write to these
> > "debug" controls to succeed.  "capability" here is one of the CAP_*
> > capabilities tested through capable(), which are supposed to limit even
> > root.
> We would add below check.
>  
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;

Looks good.

Thanks!

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