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);
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);
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);
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;
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;
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);
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;
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;
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);
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);
linaro-kernel@lists.linaro.org
-
Daniel Lezcano -
Rafael J. Wysocki