From 137b944e100278d696826cf25c83014ac17473fe Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:48 +0200 Subject: cpuidle: Make it clear that governors cannot be modules cpufreq governors are defined as modules in the code, but the Kconfig options do not allow them to be built as modules. This is not really a problem, but the cpuidle init ordering is: the cpuidle init functions (framework and driver) and then the governors. That leads to some weirdness in the cpuidle framework. Namely, cpuidle_register_device() calls cpuidle_enable_device() which fails at the first attempt, because governors have not been registered yet. When a governor is registered, the framework calls cpuidle_enable_device() again which runs __cpuidle_register_device() only then. Of course, for that to work, the cpuidle_enable_device() return value has to be ignored by cpuidle_register_device(). Instead of having this cyclic call graph and relying on a positive side effects of the hackish back and forth cpuidle_enable_device() calls it is better to fix the cpuidle init ordering. To that end, replace the module init code with postcore_initcall() so we have: * cpuidle framework : core_initcall * cpuidle governors : postcore_initcall * cpuidle drivers : device_initcall and remove the corresponding module exit code as it is dead anyway (governors can't be built as modules). [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/governors/ladder.c | 12 +----------- drivers/cpuidle/governors/menu.c | 12 +----------- 2 files changed, 2 insertions(+), 22 deletions(-) (limited to 'drivers/cpuidle') 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); -- cgit v1.1 From 10b9d3f8a4d5c82bff5b232a0063669dc0e0d725 Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:49 +0200 Subject: cpuidle: Check cpuidle_enable_device() return value We previously changed the ordering of the cpuidle framework initialization so that the governors are registered before the drivers which can register their devices right from the start. Now, we can safely remove the __cpuidle_register_device() call hack in cpuidle_enable_device() and check if the driver has been registered before enabling it. Then, cpuidle_register_device() can consistently check the cpuidle_enable_device() return value when enabling the device. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'drivers/cpuidle') diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index fdc432f..4deed97 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) + 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); -- cgit v1.1 From f89ae89e2790341587f7132f0552c941342115e9 Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:50 +0200 Subject: cpuidle: Fix white space to follow CodingStyle Fix white space in the cpuidle code to follow the rules described in CodingStyle. No changes in behavior should result from this. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/sysfs.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) (limited to 'drivers/cpuidle') 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, "\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); -- cgit v1.1 From 728ce22b696f9f1404a74d7b2279a65933553a1b Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:51 +0200 Subject: cpuidle: Make cpuidle's sysfs directory dynamically allocated 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 create it again. This is not a problem with the current code but future changes will add CPU hotplug support to enable/disable the device, so it will need to remove the sysfs entry like other subsystems do. That won't be possible without this change, because the kobj is a static object which can't be reused for kobj_init_and_add(). Add cpuidle_device_kobj to be allocated dynamically when adding/removing a sysfs entry which is consistent with the other cpuidle's sysfs entries. An added benefit is that the sysfs code is now more self-contained and the includes needed for sysfs can be moved from cpuidle.h directly into sysfs.c so as to reduce the total number of headers dragged along with cpuidle.h. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/sysfs.c | 63 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 15 deletions(-) (limited to 'drivers/cpuidle') 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 #include #include +#include #include #include +#include #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); } -- cgit v1.1 From f6bb51a53a7535c79d6c65862d6b48e83340b337 Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:53 +0200 Subject: cpuidle: Introduce __cpuidle_unregister_device() To reduce code duplication related to the unregistration of cpuidle devices, introduce __cpuidle_unregister_device() and move all of the unregistration code to that function. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle.c | 62 ++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 30 deletions(-) (limited to 'drivers/cpuidle') diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 4deed97..d78c6d8 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); -- cgit v1.1 From 5df0aa7341bd94ca2023a60c64c63faeb6ec209d Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:54 +0200 Subject: cpuidle: Introduce __cpuidle_device_init() Add __cpuidle_device_init() for initializing the cpuidle_device structure. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'drivers/cpuidle') diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d78c6d8..5b63185 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; -- cgit v1.1 From c878a52d3c7cabab5b24460825c24eafd8be7058 Mon Sep 17 00:00:00 2001 From: Daniel Lezcano Date: Wed, 12 Jun 2013 15:08:55 +0200 Subject: cpuidle: Check if device is already registered Make __cpuidle_register_device() check whether or not the device has been registered already and return -EBUSY immediately if that's the case. [rjw: Changelog] Signed-off-by: Daniel Lezcano Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/cpuidle') diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 5b63185..d75040d 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; -- cgit v1.1