[PATCH 1/8] cpuidle: Remove governor module format

[PATCH 1/8] cpuidle: Remove governor module format

[Daniel Lezcano]

The governors are defined as module in the code, but the Kconfig options do not
allow to compile them as module. This is not really a problem but the init
order is: the cpuidle init functions (framework and driver) and then the
governors. That leads to some weirdness in the cpuidle framework because the
function cpuidle_register_device calls cpuidle_enable_device which in turns
fails at the first attempt because no governor is registered. When the
governor is registered, the framework calls cpuidle_enable_device again which
will invoke the __cpuidle_register_device function. Of course, in order to make
this to work, the return code of cpuidle_enable_device is not checked by the
caller in cpuidle_register_device.

Instead of having this cyclic call graph and relying on a positive side effect
of the hackish back and forth call to cpuidle_enable_device, let's fix the
init order for the governor in order to clean up the cpuidle_enable_device
function.

Remove the module init code and replaced it with postcore_initcall, so we have:

 * cpuidle framework : core_initcall
 * cpuidle governors : postcore_initcall
 * cpuidle drivers   : device_initcall

Remove exit module code as it is dead code (governors aren't compiled as
module).

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/ladder.c |   12 +-----------
 drivers/cpuidle/governors/menu.c   |   12 +-----------
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
index 9b78405..9f08e8c 100644
--- a/drivers/cpuidle/governors/ladder.c
+++ b/drivers/cpuidle/governors/ladder.c
@@ -192,14 +192,4 @@ static int __init init_ladder(void)
 	return cpuidle_register_governor(&ladder_governor);
 }
 
-/**
- * exit_ladder - exits the governor
- */
-static void __exit exit_ladder(void)
-{
-	cpuidle_unregister_governor(&ladder_governor);
-}
-
-MODULE_LICENSE("GPL");
-module_init(init_ladder);
-module_exit(exit_ladder);
+postcore_initcall(init_ladder);
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index fe343a0..743138c 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -540,14 +540,4 @@ static int __init init_menu(void)
 	return cpuidle_register_governor(&menu_governor);
 }
 
-/**
- * exit_menu - exits the governor
- */
-static void __exit exit_menu(void)
-{
-	cpuidle_unregister_governor(&menu_governor);
-}
-
-MODULE_LICENSE("GPL");
-module_init(init_menu);
-module_exit(exit_menu);
+postcore_initcall(init_menu);
-- 
1.7.9.5

[PATCH 2/8] cpuidle: Check cpuidle_enable_device succeed

[Daniel Lezcano]

The previous patch changed the order of the framework initialization, the
governors are registered first and then the drivers can register their devices.

We can safely remove the __cpuidle_register_device call hack in the
cpuidle_enable_device function and check if the driver is registered before
enabling it. The cpuidle_register_function can check consistently the return
code when enabling the device.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index fdc432f..52ec46b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -292,15 +292,12 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 	if (!drv || !cpuidle_curr_governor)
 		return -EIO;
 
+	if (dev->registered == 0)
+		return -EINVAL;
+
 	if (!dev->state_count)
 		dev->state_count = drv->state_count;
 
-	if (dev->registered == 0) {
-		ret = __cpuidle_register_device(dev);
-		if (ret)
-			return ret;
-	}
-
 	poll_idle_init(drv);
 
 	ret = cpuidle_add_device_sysfs(dev);
@@ -415,13 +412,17 @@ int cpuidle_register_device(struct cpuidle_device *dev)
 		return ret;
 	}
 
-	cpuidle_enable_device(dev);
+	ret = cpuidle_enable_device(dev);
+	if (ret) {
+		mutex_unlock(&cpuidle_lock);
+		return ret;
+	}
+
 	cpuidle_install_idle_handler();
 
 	mutex_unlock(&cpuidle_lock);
 
 	return 0;
-
 }
 
 EXPORT_SYMBOL_GPL(cpuidle_register_device);
-- 
1.7.9.5

[PATCH 3/8] cpuidle: Fix indentation and conform to Coding Style

[Daniel Lezcano]

This mindless patch does just fix the code to have 80 columns and the pointer
variable format in the parameter functions / declarations to conform the Coding
Style.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/sysfs.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 428754a..7d4448a 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -33,7 +33,8 @@ static ssize_t show_available_governors(struct device *dev,
 
 	mutex_lock(&cpuidle_lock);
 	list_for_each_entry(tmp, &cpuidle_governors, governor_list) {
-		if (i >= (ssize_t) ((PAGE_SIZE/sizeof(char)) - CPUIDLE_NAME_LEN - 2))
+		if (i >= (ssize_t) ((PAGE_SIZE/sizeof(char)) -
+				    CPUIDLE_NAME_LEN - 2))
 			goto out;
 		i += scnprintf(&buf[i], CPUIDLE_NAME_LEN, "%s ", tmp->name);
 	}
@@ -168,11 +169,13 @@ struct cpuidle_attr {
 
 #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)
+
+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);
+	struct cpuidle_attr *cattr = attr_to_cpuidleattr(attr);
 
 	if (cattr->show) {
 		mutex_lock(&cpuidle_lock);
@@ -182,12 +185,12 @@ static ssize_t cpuidle_show(struct kobject * kobj, struct attribute * attr ,char
 	return ret;
 }
 
-static ssize_t cpuidle_store(struct kobject * kobj, struct attribute * attr,
-		     const char * buf, size_t count)
+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);
+	struct cpuidle_attr *cattr = attr_to_cpuidleattr(attr);
 
 	if (cattr->store) {
 		mutex_lock(&cpuidle_lock);
@@ -237,8 +240,8 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
 
 #define define_store_state_ull_function(_name) \
 static ssize_t store_state_##_name(struct cpuidle_state *state, \
-		struct cpuidle_state_usage *state_usage, \
-		const char *buf, size_t size) \
+				   struct cpuidle_state_usage *state_usage, \
+				   const char *buf, size_t size)	\
 { \
 	unsigned long long value; \
 	int err; \
@@ -256,14 +259,16 @@ static ssize_t store_state_##_name(struct cpuidle_state *state, \
 
 #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) \
+				  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) \
+				  struct cpuidle_state_usage *state_usage, \
+				  char *buf)				\
 { \
 	if (state->_name[0] == '\0')\
 		return sprintf(buf, "<null>\n");\
@@ -309,8 +314,9 @@ struct cpuidle_state_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)
+
+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);
@@ -323,8 +329,8 @@ 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)
+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);
@@ -449,8 +455,8 @@ static void cpuidle_driver_sysfs_release(struct kobject *kobj)
 	complete(&driver_kobj->kobj_unregister);
 }
 
-static ssize_t cpuidle_driver_show(struct kobject *kobj, struct attribute * attr,
-				   char * buf)
+static ssize_t cpuidle_driver_show(struct kobject *kobj, struct attribute *attr,
+				   char *buf)
 {
 	int ret = -EIO;
 	struct cpuidle_driver_kobj *driver_kobj = kobj_to_driver_kobj(kobj);
-- 
1.7.9.5

[PATCH 4/8] cpuidle: Make cpuidle's sysfs directory dynamically allocated

[Daniel Lezcano]

The cpuidle sysfs code is designed to have a single instance of per cpu cpuidle
directory. It is not possible to remove the sysfs entry and recreate it. This
is not a problem with the current code but the next patches will add the cpu
hotplug handling to enable/disable the device, thus removing the sysfs entry
like does any other subsystems. Without this patch it is not possible because
the kobj is a static object which can't be reused for kobj_init_and_add.

Create a cpuidle_device_kobj to be allocated dynamically when adding/removing
a sysfs entry. This approach is consistent with the other cpuidle's sysfs
entries.

Another benefit is the sysfs code is more encapsulated and the headers needed
for sysfs are moved from cpuidle.h into the sysfs.c file, preventing useless
header inclusions from cpuidle.h.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/sysfs.c |   63 ++++++++++++++++++++++++++++++++++++-----------
 include/linux/cpuidle.h |    7 +++---
 2 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 7d4448a..8739cc0 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -11,8 +11,10 @@
 #include <linux/sysfs.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
+#include <linux/completion.h>
 #include <linux/capability.h>
 #include <linux/device.h>
+#include <linux/kobject.h>
 
 #include "cpuidle.h"
 
@@ -167,14 +169,27 @@ struct cpuidle_attr {
 #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)
 
+struct cpuidle_device_kobj {
+	struct cpuidle_device *dev;
+	struct completion kobj_unregister;
+	struct kobject kobj;
+};
+
+static inline struct cpuidle_device *to_cpuidle_device(struct kobject *kobj)
+{
+	struct cpuidle_device_kobj *kdev =
+		container_of(kobj, struct cpuidle_device_kobj, kobj);
+
+	return kdev->dev;
+}
+
 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_device *dev = to_cpuidle_device(kobj);
 	struct cpuidle_attr *cattr = attr_to_cpuidleattr(attr);
 
 	if (cattr->show) {
@@ -189,7 +204,7 @@ 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_device *dev = to_cpuidle_device(kobj);
 	struct cpuidle_attr *cattr = attr_to_cpuidleattr(attr);
 
 	if (cattr->store) {
@@ -207,9 +222,10 @@ static const struct sysfs_ops cpuidle_sysfs_ops = {
 
 static void cpuidle_sysfs_release(struct kobject *kobj)
 {
-	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
+	struct cpuidle_device_kobj *kdev =
+		container_of(kobj, struct cpuidle_device_kobj, kobj);
 
-	complete(&dev->kobj_unregister);
+	complete(&kdev->kobj_unregister);
 }
 
 static struct kobj_type ktype_cpuidle = {
@@ -377,6 +393,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
 {
 	int i, ret = -ENOMEM;
 	struct cpuidle_state_kobj *kobj;
+	struct cpuidle_device_kobj *kdev = device->kobj_dev;
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device);
 
 	/* state statistics */
@@ -389,7 +406,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
 		init_completion(&kobj->kobj_unregister);
 
 		ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle,
-					   &device->kobj, "state%d", i);
+					   &kdev->kobj, "state%d", i);
 		if (ret) {
 			kfree(kobj);
 			goto error_state;
@@ -506,6 +523,7 @@ static struct kobj_type ktype_driver_cpuidle = {
 static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
 {
 	struct cpuidle_driver_kobj *kdrv;
+	struct cpuidle_device_kobj *kdev = dev->kobj_dev;
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int ret;
 
@@ -517,7 +535,7 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
 	init_completion(&kdrv->kobj_unregister);
 
 	ret = kobject_init_and_add(&kdrv->kobj, &ktype_driver_cpuidle,
-				   &dev->kobj, "driver");
+				   &kdev->kobj, "driver");
 	if (ret) {
 		kfree(kdrv);
 		return ret;
@@ -586,16 +604,28 @@ void cpuidle_remove_device_sysfs(struct cpuidle_device *device)
  */
 int cpuidle_add_sysfs(struct cpuidle_device *dev)
 {
+	struct cpuidle_device_kobj *kdev;
 	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
 	int error;
 
-	init_completion(&dev->kobj_unregister);
+	kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
+	if (!kdev)
+		return -ENOMEM;
+	kdev->dev = dev;
+	dev->kobj_dev = kdev;
+
+	init_completion(&kdev->kobj_unregister);
+
+	error = kobject_init_and_add(&kdev->kobj, &ktype_cpuidle, &cpu_dev->kobj,
+				   "cpuidle");
+	if (error) {
+		kfree(kdev);
+		return error;
+	}
 
-	error = kobject_init_and_add(&dev->kobj, &ktype_cpuidle, &cpu_dev->kobj,
-				     "cpuidle");
-	if (!error)
-		kobject_uevent(&dev->kobj, KOBJ_ADD);
-	return error;
+	kobject_uevent(&kdev->kobj, KOBJ_ADD);
+
+	return 0;
 }
 
 /**
@@ -604,6 +634,9 @@ int cpuidle_add_sysfs(struct cpuidle_device *dev)
  */
 void cpuidle_remove_sysfs(struct cpuidle_device *dev)
 {
-	kobject_put(&dev->kobj);
-	wait_for_completion(&dev->kobj_unregister);
+	struct cpuidle_device_kobj *kdev = dev->kobj_dev;
+
+	kobject_put(&kdev->kobj);
+	wait_for_completion(&kdev->kobj_unregister);
+	kfree(kdev);
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 0bc4b74..b922db5 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -13,8 +13,6 @@
 
 #include <linux/percpu.h>
 #include <linux/list.h>
-#include <linux/kobject.h>
-#include <linux/completion.h>
 #include <linux/hrtimer.h>
 
 #define CPUIDLE_STATE_MAX	10
@@ -61,6 +59,8 @@ struct cpuidle_state {
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
+struct cpuidle_device_kobj;
+
 struct cpuidle_device {
 	unsigned int		registered:1;
 	unsigned int		enabled:1;
@@ -71,9 +71,8 @@ struct cpuidle_device {
 	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];
 	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
 	struct cpuidle_driver_kobj *kobj_driver;
+	struct cpuidle_device_kobj *kobj_dev;
 	struct list_head 	device_list;
-	struct kobject		kobj;
-	struct completion	kobj_unregister;
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
 	int			safe_state_index;
-- 
1.7.9.5

[PATCH 5/8] cpuidle: Add missing forward declaration

[Daniel Lezcano]

The forward declaration for the cpuidle_state_kobj and the cpuidle_driver_kobj
structures are missing. Let's add them.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/cpuidle.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b922db5..781addc 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -60,6 +60,8 @@ struct cpuidle_state {
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
 struct cpuidle_device_kobj;
+struct cpuidle_state_kobj;
+struct cpuidle_driver_kobj;
 
 struct cpuidle_device {
 	unsigned int		registered:1;
-- 
1.7.9.5

[PATCH 6/8] cpuidle: Encapsulate code in __cpuidle_unregister_device

[Daniel Lezcano]

The same code is used at different places.

Group the common code into a single functions and call this function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |   62 +++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 52ec46b..bdb7180 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -42,8 +42,6 @@ void disable_cpuidle(void)
 	off = 1;
 }
 
-static int __cpuidle_register_device(struct cpuidle_device *dev);
-
 /**
  * cpuidle_play_dead - cpu off-lining
  *
@@ -357,6 +355,15 @@ void cpuidle_disable_device(struct cpuidle_device *dev)
 
 EXPORT_SYMBOL_GPL(cpuidle_disable_device);
 
+static void __cpuidle_unregister_device(struct cpuidle_device *dev)
+{
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	list_del(&dev->device_list);
+	per_cpu(cpuidle_devices, dev->cpu) = NULL;
+	module_put(drv->owner);
+}
+
 /**
  * __cpuidle_register_device - internal register function called before register
  * and enable routines
@@ -374,24 +381,15 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 
 	per_cpu(cpuidle_devices, dev->cpu) = dev;
 	list_add(&dev->device_list, &cpuidle_detected_devices);
-	ret = cpuidle_add_sysfs(dev);
-	if (ret)
-		goto err_sysfs;
 
 	ret = cpuidle_coupled_register_device(dev);
-	if (ret)
-		goto err_coupled;
+	if (ret) {
+		__cpuidle_unregister_device(dev);
+		return ret;
+	}
 
 	dev->registered = 1;
 	return 0;
-
-err_coupled:
-	cpuidle_remove_sysfs(dev);
-err_sysfs:
-	list_del(&dev->device_list);
-	per_cpu(cpuidle_devices, dev->cpu) = NULL;
-	module_put(drv->owner);
-	return ret;
 }
 
 /**
@@ -407,22 +405,30 @@ int cpuidle_register_device(struct cpuidle_device *dev)
 
 	mutex_lock(&cpuidle_lock);
 
-	if ((ret = __cpuidle_register_device(dev))) {
-		mutex_unlock(&cpuidle_lock);
-		return ret;
-	}
+	ret = __cpuidle_register_device(dev);
+	if (ret)
+		goto out_unlock;
+
+	ret = cpuidle_add_sysfs(dev);
+	if (ret)
+		goto out_unregister;
 
 	ret = cpuidle_enable_device(dev);
-	if (ret) {
-		mutex_unlock(&cpuidle_lock);
-		return ret;
-	}
+	if (ret)
+		goto out_sysfs;
 
 	cpuidle_install_idle_handler();
 
+out_unlock:
 	mutex_unlock(&cpuidle_lock);
 
-	return 0;
+	return ret;
+
+out_sysfs:
+	cpuidle_remove_sysfs(dev);
+out_unregister:
+	__cpuidle_unregister_device(dev);
+	goto out_unlock;
 }
 
 EXPORT_SYMBOL_GPL(cpuidle_register_device);
@@ -433,8 +439,6 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device);
  */
 void cpuidle_unregister_device(struct cpuidle_device *dev)
 {
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-
 	if (dev->registered == 0)
 		return;
 
@@ -443,14 +447,12 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
 	cpuidle_disable_device(dev);
 
 	cpuidle_remove_sysfs(dev);
-	list_del(&dev->device_list);
-	per_cpu(cpuidle_devices, dev->cpu) = NULL;
+
+	__cpuidle_unregister_device(dev);
 
 	cpuidle_coupled_unregister_device(dev);
 
 	cpuidle_resume_and_unlock();
-
-	module_put(drv->owner);
 }
 
 EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
-- 
1.7.9.5

[PATCH 7/8] cpuidle: Add a cpuidle_device init function

[Daniel Lezcano]

Create a specific function to initialize the cpuidle_device structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index bdb7180..7c3f625 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -276,7 +276,7 @@ static void poll_idle_init(struct cpuidle_driver *drv) {}
  */
 int cpuidle_enable_device(struct cpuidle_device *dev)
 {
-	int ret, i;
+	int ret;
 	struct cpuidle_driver *drv;
 
 	if (!dev)
@@ -306,12 +306,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 	    (ret = cpuidle_curr_governor->enable(drv, dev)))
 		goto fail_sysfs;
 
-	for (i = 0; i < dev->state_count; i++) {
-		dev->states_usage[i].usage = 0;
-		dev->states_usage[i].time = 0;
-	}
-	dev->last_residency = 0;
-
 	smp_wmb();
 
 	dev->enabled = 1;
@@ -364,6 +358,14 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev)
 	module_put(drv->owner);
 }
 
+static int __cpuidle_device_init(struct cpuidle_device *dev)
+{
+	memset(dev->states_usage, 0, sizeof(dev->states_usage));
+	dev->last_residency = 0;
+
+	return 0;
+}
+
 /**
  * __cpuidle_register_device - internal register function called before register
  * and enable routines
@@ -405,6 +407,10 @@ int cpuidle_register_device(struct cpuidle_device *dev)
 
 	mutex_lock(&cpuidle_lock);
 
+	ret = __cpuidle_device_init(dev);
+	if (ret)
+		goto out_unlock;
+
 	ret = __cpuidle_register_device(dev);
 	if (ret)
 		goto out_unlock;
-- 
1.7.9.5

[PATCH 8/8] cpuidle: Check the device is not already registered

[Daniel Lezcano]

Add a sanity check for cpuidle_register_device by testing if the device
was already registered or not.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7c3f625..59b697877 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -400,13 +400,16 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
  */
 int cpuidle_register_device(struct cpuidle_device *dev)
 {
-	int ret;
+	int ret = -EBUSY;
 
 	if (!dev)
 		return -EINVAL;
 
 	mutex_lock(&cpuidle_lock);
 
+	if (dev->registered)
+		goto out_unlock;
+
 	ret = __cpuidle_device_init(dev);
 	if (ret)
 		goto out_unlock;
-- 
1.7.9.5

Re: [PATCH 1/8] cpuidle: Remove governor module format

[Rafael J. Wysocki]

On Wednesday, June 12, 2013 03:08:48 PM Daniel Lezcano wrote:
> The governors are defined as module in the code, but the Kconfig options do not
> allow to compile them as module. This is not really a problem but the init
> order is: the cpuidle init functions (framework and driver) and then the
> governors. That leads to some weirdness in the cpuidle framework because the
> function cpuidle_register_device calls cpuidle_enable_device which in turns
> fails at the first attempt because no governor is registered. When the
> governor is registered, the framework calls cpuidle_enable_device again which
> will invoke the __cpuidle_register_device function. Of course, in order to make
> this to work, the return code of cpuidle_enable_device is not checked by the
> caller in cpuidle_register_device.
> 
> Instead of having this cyclic call graph and relying on a positive side effect
> of the hackish back and forth call to cpuidle_enable_device, let's fix the
> init order for the governor in order to clean up the cpuidle_enable_device
> function.
> 
> Remove the module init code and replaced it with postcore_initcall, so we have:
> 
>  * cpuidle framework : core_initcall
>  * cpuidle governors : postcore_initcall
>  * cpuidle drivers   : device_initcall
> 
> Remove exit module code as it is dead code (governors aren't compiled as
> module).
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

That's something I'd like to receive much earlier in the cycle (around -rc2).

Honestly, I'm not sure if it's going to make it into 3.11 even if nobody has
any comments.

Thanks,
Rafael


> ---
>  drivers/cpuidle/governors/ladder.c |   12 +-----------
>  drivers/cpuidle/governors/menu.c   |   12 +-----------
>  2 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
> index 9b78405..9f08e8c 100644
> --- a/drivers/cpuidle/governors/ladder.c
> +++ b/drivers/cpuidle/governors/ladder.c
> @@ -192,14 +192,4 @@ static int __init init_ladder(void)
>  	return cpuidle_register_governor(&ladder_governor);
>  }
>  
> -/**
> - * exit_ladder - exits the governor
> - */
> -static void __exit exit_ladder(void)
> -{
> -	cpuidle_unregister_governor(&ladder_governor);
> -}
> -
> -MODULE_LICENSE("GPL");
> -module_init(init_ladder);
> -module_exit(exit_ladder);
> +postcore_initcall(init_ladder);
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index fe343a0..743138c 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -540,14 +540,4 @@ static int __init init_menu(void)
>  	return cpuidle_register_governor(&menu_governor);
>  }
>  
> -/**
> - * exit_menu - exits the governor
> - */
> -static void __exit exit_menu(void)
> -{
> -	cpuidle_unregister_governor(&menu_governor);
> -}
> -
> -MODULE_LICENSE("GPL");
> -module_init(init_menu);
> -module_exit(exit_menu);
> +postcore_initcall(init_menu);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/8] cpuidle: Remove governor module format

[Daniel Lezcano]

On 06/13/2013 12:53 AM, Rafael J. Wysocki wrote:
> On Wednesday, June 12, 2013 03:08:48 PM Daniel Lezcano wrote:
>> The governors are defined as module in the code, but the Kconfig options do not
>> allow to compile them as module. This is not really a problem but the init
>> order is: the cpuidle init functions (framework and driver) and then the
>> governors. That leads to some weirdness in the cpuidle framework because the
>> function cpuidle_register_device calls cpuidle_enable_device which in turns
>> fails at the first attempt because no governor is registered. When the
>> governor is registered, the framework calls cpuidle_enable_device again which
>> will invoke the __cpuidle_register_device function. Of course, in order to make
>> this to work, the return code of cpuidle_enable_device is not checked by the
>> caller in cpuidle_register_device.
>>
>> Instead of having this cyclic call graph and relying on a positive side effect
>> of the hackish back and forth call to cpuidle_enable_device, let's fix the
>> init order for the governor in order to clean up the cpuidle_enable_device
>> function.
>>
>> Remove the module init code and replaced it with postcore_initcall, so we have:
>>
>>  * cpuidle framework : core_initcall
>>  * cpuidle governors : postcore_initcall
>>  * cpuidle drivers   : device_initcall
>>
>> Remove exit module code as it is dead code (governors aren't compiled as
>> module).
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> That's something I'd like to receive much earlier in the cycle (around -rc2).
> 
> Honestly, I'm not sure if it's going to make it into 3.11 even if nobody has
> any comments.

It is ok, choose what is the more convenient to stabilize the kernel.

>> ---
>>  drivers/cpuidle/governors/ladder.c |   12 +-----------
>>  drivers/cpuidle/governors/menu.c   |   12 +-----------
>>  2 files changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
>> index 9b78405..9f08e8c 100644
>> --- a/drivers/cpuidle/governors/ladder.c
>> +++ b/drivers/cpuidle/governors/ladder.c
>> @@ -192,14 +192,4 @@ static int __init init_ladder(void)
>>  	return cpuidle_register_governor(&ladder_governor);
>>  }
>>  
>> -/**
>> - * exit_ladder - exits the governor
>> - */
>> -static void __exit exit_ladder(void)
>> -{
>> -	cpuidle_unregister_governor(&ladder_governor);
>> -}
>> -
>> -MODULE_LICENSE("GPL");
>> -module_init(init_ladder);
>> -module_exit(exit_ladder);
>> +postcore_initcall(init_ladder);
>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>> index fe343a0..743138c 100644
>> --- a/drivers/cpuidle/governors/menu.c
>> +++ b/drivers/cpuidle/governors/menu.c
>> @@ -540,14 +540,4 @@ static int __init init_menu(void)
>>  	return cpuidle_register_governor(&menu_governor);
>>  }
>>  
>> -/**
>> - * exit_menu - exits the governor
>> - */
>> -static void __exit exit_menu(void)
>> -{
>> -	cpuidle_unregister_governor(&menu_governor);
>> -}
>> -
>> -MODULE_LICENSE("GPL");
>> -module_init(init_menu);
>> -module_exit(exit_menu);
>> +postcore_initcall(init_menu);
>>


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog