Hi Rafael,
These are V2 of my CPUIdle cleanup series.. Few patches are dropped as they required further modifications. Last one is rewritten as suggested by Daniel. Most of them are already Acked by Daniel.
Viresh Kumar (16): cpuidle: fix indentation of cpumask cpuidle: Fix comments in cpuidle core cpuidle: make __cpuidle_get_cpu_driver() inline cpuidle: make __cpuidle_device_init() return void cpuidle: make __cpuidle_driver_init() return void cpuidle: rearrange code in __cpuidle_driver_init() cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points cpuidle: merge two if() statements for checking error cases cpuidle: reduce code duplication inside cpuidle_idle_call() cpuidle: replace multiline statements with single line in cpuidle_idle_call() cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock cpuidle: use drv instead of cpuidle_driver in show_current_driver() cpuidle: free all state kobjects from cpuidle_free_state_kobj() cpuidle: don't calculate time-diff if entered_state < 0 cpuidle: don't call poll_idle_init() for every cpu cpuidle: remove cpuidle_unregister_governor()
Documentation/cpuidle/governor.txt | 1 - drivers/cpuidle/coupled.c | 2 +- drivers/cpuidle/cpuidle.c | 95 ++++++++++---------------------------- drivers/cpuidle/driver.c | 69 ++++++++++++++++++++------- drivers/cpuidle/governor.c | 43 ----------------- drivers/cpuidle/sysfs.c | 30 ++++++------ include/linux/cpuidle.h | 8 +--- 7 files changed, 94 insertions(+), 154 deletions(-)
cpumask is indented using spaces instead of tabs. Fix it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/cpuidle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 781addc..c082425 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -114,7 +114,7 @@ struct cpuidle_driver { int safe_state_index;
/* the driver handles the cpus in cpumask */ - struct cpumask *cpumask; + struct cpumask *cpumask; };
#ifdef CONFIG_CPU_IDLE
Few comments in cpuidle core files have trivial mistakes. This patch fixes them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/coupled.c | 2 +- drivers/cpuidle/cpuidle.c | 2 +- drivers/cpuidle/driver.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index f8a8636..e952936 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -147,7 +147,7 @@ static cpumask_t cpuidle_coupled_poked; * has returned from this function, the barrier is immediately available for * reuse. * - * The atomic variable a must be initialized to 0 before any cpu calls + * The atomic variable must be initialized to 0 before any cpu calls * this function, will be reset to 0 before any cpu returns from this function. * * Must only be called from within a coupled idle state handler diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d75040d..8827c02 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -516,7 +516,7 @@ int cpuidle_register(struct cpuidle_driver *drv,
#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED /* - * On multiplatform for ARM, the coupled idle states could + * On multiplatform for ARM, the coupled idle states could be * enabled in the kernel even if the cpuidle driver does not * use it. Note, coupled_cpus is a struct copy. */ diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 6e11701..ced1df6 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -56,7 +56,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) }
/** - * __cpuidle_set_driver - set per CPU driver variables the the given driver. + * __cpuidle_set_driver - set per CPU driver variables for the given driver. * @drv: a valid pointer to a struct cpuidle_driver * * For each CPU in the driver's cpumask, unset the registered driver per CPU @@ -132,7 +132,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer * @arg: a void pointer used to match the SMP cross call API * - * @arg is used as a value of type 'long' with on of the two values: + * @arg is used as a value of type 'long' with one of the two values: * - CLOCK_EVT_NOTIFY_BROADCAST_ON * - CLOCK_EVT_NOTIFY_BROADCAST_OFF * @@ -169,7 +169,7 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv) /* * Look for the timer stop flag in the different states, so that we know * if the broadcast timer has to be set up. The loop is in the reverse - * order, because usually on of the the deeper states has this flag set. + * order, because usually one of the deeper states have this flag set. */ for (i = drv->state_count - 1; i >= 0 ; i--) {
__cpuidle_get_cpu_driver() is a single line function and so deserves to be marked inline.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index ced1df6..25455e8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); * Returns a pointer to struct cpuidle_driver or NULL if no driver has been * registered for @cpu. */ -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { return per_cpu(cpuidle_drivers, cpu); }
Hi
a comment on this one (and any similar patch)
On 10/03/2013 08:56 AM, Viresh Kumar wrote:
__cpuidle_get_cpu_driver() is a single line function and so deserves to be marked inline.
In general, this is a violation of Documentation/CodingStyle - see Chapter 15.
Unless this produces a significant benefit, it's probably best to just let the compiler do this if it wants.
- Paul
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index ced1df6..25455e8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
- Returns a pointer to struct cpuidle_driver or NULL if no driver has been
- registered for @cpu.
*/ -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { return per_cpu(cpuidle_drivers, cpu); }
__cpuidle_device_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8827c02..211e504 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -358,12 +358,10 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev) module_put(drv->owner); }
-static int __cpuidle_device_init(struct cpuidle_device *dev) +static void __cpuidle_device_init(struct cpuidle_device *dev) { memset(dev->states_usage, 0, sizeof(dev->states_usage)); dev->last_residency = 0; - - return 0; }
/** @@ -410,9 +408,7 @@ int cpuidle_register_device(struct cpuidle_device *dev) if (dev->registered) goto out_unlock;
- ret = __cpuidle_device_init(dev); - if (ret) - goto out_unlock; + __cpuidle_device_init(dev);
ret = __cpuidle_register_device(dev); if (ret)
__cpuidle_driver_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 25455e8..ebc6b0f 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -149,10 +149,8 @@ static void cpuidle_setup_broadcast_timer(void *arg) /** * __cpuidle_driver_init - initialize the driver's internal data * @drv: a valid pointer to a struct cpuidle_driver - * - * Returns 0 on success, a negative error code otherwise. */ -static int __cpuidle_driver_init(struct cpuidle_driver *drv) +static void __cpuidle_driver_init(struct cpuidle_driver *drv) { int i;
@@ -179,8 +177,6 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv) drv->bctimer = 1; break; } - - return 0; }
/** @@ -206,9 +202,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) if (cpuidle_disabled()) return -ENODEV;
- ret = __cpuidle_driver_init(drv); - if (ret) - return ret; + __cpuidle_driver_init(drv);
ret = __cpuidle_set_driver(drv); if (ret)
This is trivial patch that just reorders few statements in __cpuidle_driver_init() routine, so that we don't need both 'continue' and 'break' in the for loop. Functionally it shouldn't change anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index ebc6b0f..58077a9 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -170,12 +170,10 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) * order, because usually one of the deeper states have this flag set. */ for (i = drv->state_count - 1; i >= 0 ; i--) { - - if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) - continue; - - drv->bctimer = 1; - break; + if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) { + drv->bctimer = 1; + break; + } } }
This patch rearranges __cpuidle_register_device() a bit in order to reduce the number of exit points of this function.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 211e504..8c91bad 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -383,13 +383,12 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) list_add(&dev->device_list, &cpuidle_detected_devices);
ret = cpuidle_coupled_register_device(dev); - if (ret) { + if (ret) __cpuidle_unregister_device(dev); - return ret; - } + else + dev->registered = 1;
- dev->registered = 1; - return 0; + return ret; }
/**
Both return same error message and so better write them in a single line.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8c91bad..518b542 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -119,10 +119,7 @@ int cpuidle_idle_call(void) struct cpuidle_driver *drv; int next_state, entered_state;
- if (off) - return -ENODEV; - - if (!initialized) + if (off || !initialized) return -ENODEV;
/* check if the device is ready */
We are doing this twice in cpuidle_idle_call() routine: drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP
Would be better if we actually store this in a local variable and use that. That would remove code duplication as well as make this piece of code run fast (in case compiler wasn't able to optimize it earlier)
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 518b542..ffc637a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -118,6 +118,7 @@ int cpuidle_idle_call(void) struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); struct cpuidle_driver *drv; int next_state, entered_state; + bool broadcast;
if (off || !initialized) return -ENODEV; @@ -141,7 +142,9 @@ int cpuidle_idle_call(void)
trace_cpu_idle_rcuidle(next_state, dev->cpu);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP; + + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
@@ -151,7 +154,7 @@ int cpuidle_idle_call(void) else entered_state = cpuidle_enter_state(dev, drv, next_state);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
Few statements in cpuidle_idle_call() are broken into multiple lines, whereas they can actually come in a single line. Convert those to single line.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ffc637a..b45257a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -145,8 +145,7 @@ int cpuidle_idle_call(void) broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
if (broadcast) - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, - &dev->cpu); + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
if (cpuidle_state_is_coupled(dev, drv, next_state)) entered_state = cpuidle_enter_state_coupled(dev, drv, @@ -155,8 +154,7 @@ int cpuidle_idle_call(void) entered_state = cpuidle_enter_state(dev, drv, next_state);
if (broadcast) - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, - &dev->cpu); + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
cpuidle_driver_lock is taken correctly at most of the places but at few places calls to cpuidle_get_driver() are done from outside of this lock.
Fix them by calling cpuidle_get_driver() after taking cpuidle_driver_lock.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 3 ++- drivers/cpuidle/sysfs.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 58077a9..662d934 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -338,10 +338,11 @@ struct cpuidle_driver *cpuidle_driver_ref(void) */ void cpuidle_driver_unref(void) { - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock);
+ drv = cpuidle_get_driver(); if (drv && !WARN_ON(drv->refcnt <= 0)) drv->refcnt--;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 8739cc0..a022393 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,9 +52,10 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret; - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); + struct cpuidle_driver *cpuidle_driver;
spin_lock(&cpuidle_driver_lock); + cpuidle_driver = cpuidle_get_driver(); if (cpuidle_driver) ret = sprintf(buf, "%s\n", cpuidle_driver->name); else
Instances of "struct cpuidle_driver *" are consistently named as "drv" in cpuidle core. Its broken only at one place: show_current_driver().
Fix it for consistency.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index a022393..e918b6d 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,12 +52,12 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret; - struct cpuidle_driver *cpuidle_driver; + struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock); - cpuidle_driver = cpuidle_get_driver(); - if (cpuidle_driver) - ret = sprintf(buf, "%s\n", cpuidle_driver->name); + drv = cpuidle_get_driver(); + if (drv) + ret = sprintf(buf, "%s\n", drv->name); else ret = sprintf(buf, "none\n"); spin_unlock(&cpuidle_driver_lock);
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, };
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, + int count) { - kobject_put(&device->kobjs[i]->kobj); - wait_for_completion(&device->kobjs[i]->kobj_unregister); - kfree(device->kobjs[i]); - device->kobjs[i] = NULL; + int i; + + for (i = 0; i < count; i++) { + kobject_put(&device->kobjs[i]->kobj); + wait_for_completion(&device->kobjs[i]->kobj_unregister); + kfree(device->kobjs[i]); + device->kobjs[i] = NULL; + } }
/** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0;
error_state: - for (i = i - 1; i >= 0; i--) - cpuidle_free_state_kobj(device, i); + cpuidle_free_state_kobj(device, i); return ret; }
@@ -430,10 +434,7 @@ error_state: */ static 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_free_state_kobj(device, device->state_count); }
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Hi Rafael,
You missed applying this one?
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, };
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
int count){
kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;
int i;for (i = 0; i < count; i++) {kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;}}
/** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0;
error_state:
for (i = i - 1; i >= 0; i--)cpuidle_free_state_kobj(device, i);
cpuidle_free_state_kobj(device, i); return ret;}
@@ -430,10 +434,7 @@ error_state: */ static 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_free_state_kobj(device, device->state_count);}
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
1.7.12.rc2.18.g61b472e
Fixing Rafael's email id..
On 20 November 2013 13:33, Viresh Kumar viresh.kumar@linaro.org wrote:
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Hi Rafael,
You missed applying this one?
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, };
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
int count){
kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;
int i;for (i = 0; i < count; i++) {kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;}}
/** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0;
error_state:
for (i = i - 1; i >= 0; i--)cpuidle_free_state_kobj(device, i);
cpuidle_free_state_kobj(device, i); return ret;}
@@ -430,10 +434,7 @@ error_state: */ static 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_free_state_kobj(device, device->state_count);}
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
1.7.12.rc2.18.g61b472e
On Wednesday, November 20, 2013 01:33:53 PM Viresh Kumar wrote:
Fixing Rafael's email id..
On 20 November 2013 13:33, Viresh Kumar viresh.kumar@linaro.org wrote:
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Hi Rafael,
You missed applying this one?
I might. Care to resend?
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, };
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
int count){
kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;
int i;for (i = 0; i < count; i++) {kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;}}
/** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0;
error_state:
for (i = i - 1; i >= 0; i--)cpuidle_free_state_kobj(device, i);
cpuidle_free_state_kobj(device, i); return ret;}
@@ -430,10 +434,7 @@ error_state: */ static 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_free_state_kobj(device, device->state_count);}
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
1.7.12.rc2.18.g61b472e
On 21 November 2013 06:35, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Wednesday, November 20, 2013 01:33:53 PM Viresh Kumar wrote:
Fixing Rafael's email id..
On 20 November 2013 13:33, Viresh Kumar viresh.kumar@linaro.org wrote:
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Hi Rafael,
You missed applying this one?
I might. Care to resend?
Sent..
If entered_state < 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
And so move calculation of diff also inside the "if" statement.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@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 b45257a..1a6e9f5 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
struct cpuidle_state *target_state = &drv->states[index]; ktime_t time_start, time_end; - s64 diff;
time_start = ktime_get();
entered_state = target_state->enter(dev, drv, index);
- time_end = ktime_get(); + if (entered_state >= 0) { + s64 diff;
- local_irq_enable(); + time_end = ktime_get(); + diff = ktime_to_us(ktime_sub(time_end, time_start));
- diff = ktime_to_us(ktime_sub(time_end, time_start)); - if (diff > INT_MAX) - diff = INT_MAX; + if (diff > INT_MAX) + diff = INT_MAX;
- dev->last_residency = (int) diff; + dev->last_residency = (int) diff;
- if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. @@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, dev->last_residency = 0; }
+ local_irq_enable(); + return entered_state; }
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
If entered_state < 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
And so move calculation of diff also inside the "if" statement.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
Missed applying this one too?
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index b45257a..1a6e9f5 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
struct cpuidle_state *target_state = &drv->states[index]; ktime_t time_start, time_end;
s64 diff; time_start = ktime_get(); entered_state = target_state->enter(dev, drv, index);time_end = ktime_get();
if (entered_state >= 0) {s64 diff;
local_irq_enable();
time_end = ktime_get();diff = ktime_to_us(ktime_sub(time_end, time_start));
diff = ktime_to_us(ktime_sub(time_end, time_start));if (diff > INT_MAX)diff = INT_MAX;
if (diff > INT_MAX)diff = INT_MAX;
dev->last_residency = (int) diff;
dev->last_residency = (int) diff;
if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code.@@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, dev->last_residency = 0; }
local_irq_enable();return entered_state;}
-- 1.7.12.rc2.18.g61b472e
On Wednesday, November 20, 2013 01:34:45 PM Viresh Kumar wrote:
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
If entered_state < 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
And so move calculation of diff also inside the "if" statement.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
Missed applying this one too?
No, I didn't. I just thought it wasn't worth it ...
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index b45257a..1a6e9f5 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
struct cpuidle_state *target_state = &drv->states[index]; ktime_t time_start, time_end;
s64 diff; time_start = ktime_get(); entered_state = target_state->enter(dev, drv, index);time_end = ktime_get();
if (entered_state >= 0) {s64 diff;
local_irq_enable();
time_end = ktime_get();diff = ktime_to_us(ktime_sub(time_end, time_start));
diff = ktime_to_us(ktime_sub(time_end, time_start));if (diff > INT_MAX)diff = INT_MAX;
if (diff > INT_MAX)diff = INT_MAX;
dev->last_residency = (int) diff;
dev->last_residency = (int) diff;
if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code.@@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, dev->last_residency = 0; }
local_irq_enable();return entered_state;}
-- 1.7.12.rc2.18.g61b472e
poll_idle_init() just initializes drv->states[0] and so that is required to be done only once for each driver. Currently it is called from cpuidle_enable_device() which is called for every CPU that the driver supports. And that is not required. Move it to a better place and call it from __cpuidle_register_driver() so that it is initialized only once.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 41 ----------------------------------------- drivers/cpuidle/driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 1a6e9f5..1caf95d 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -227,45 +227,6 @@ void cpuidle_resume(void) mutex_unlock(&cpuidle_lock); }
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX -static int poll_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) -{ - ktime_t t1, t2; - s64 diff; - - t1 = ktime_get(); - local_irq_enable(); - while (!need_resched()) - cpu_relax(); - - t2 = ktime_get(); - diff = ktime_to_us(ktime_sub(t2, t1)); - if (diff > INT_MAX) - diff = INT_MAX; - - dev->last_residency = (int) diff; - - return index; -} - -static void poll_idle_init(struct cpuidle_driver *drv) -{ - struct cpuidle_state *state = &drv->states[0]; - - snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); - snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); - state->exit_latency = 0; - state->target_residency = 0; - state->power_usage = -1; - state->flags = 0; - state->enter = poll_idle; - state->disabled = false; -} -#else -static void poll_idle_init(struct cpuidle_driver *drv) {} -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */ - /** * cpuidle_enable_device - enables idle PM for a CPU * @dev: the CPU @@ -295,8 +256,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) if (!dev->state_count) dev->state_count = drv->state_count;
- poll_idle_init(drv); - ret = cpuidle_add_device_sysfs(dev); if (ret) return ret; diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 662d934..d5b5294 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -10,6 +10,7 @@
#include <linux/mutex.h> #include <linux/module.h> +#include <linux/sched.h> #include <linux/cpuidle.h> #include <linux/cpumask.h> #include <linux/clockchips.h> @@ -177,6 +178,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) } }
+#ifdef CONFIG_ARCH_HAS_CPU_RELAX +static int poll_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + ktime_t t1, t2; + s64 diff; + + t1 = ktime_get(); + local_irq_enable(); + while (!need_resched()) + cpu_relax(); + + t2 = ktime_get(); + diff = ktime_to_us(ktime_sub(t2, t1)); + if (diff > INT_MAX) + diff = INT_MAX; + + dev->last_residency = (int) diff; + + return index; +} + +static void poll_idle_init(struct cpuidle_driver *drv) +{ + struct cpuidle_state *state = &drv->states[0]; + + snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); + snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); + state->exit_latency = 0; + state->target_residency = 0; + state->power_usage = -1; + state->flags = 0; + state->enter = poll_idle; + state->disabled = false; +} +#else +static void poll_idle_init(struct cpuidle_driver *drv) {} +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */ + /** * __cpuidle_register_driver: register the driver * @drv: a valid pointer to a struct cpuidle_driver @@ -210,6 +250,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
+ poll_idle_init(drv); + return 0; }
cpuidle_unregister_governor() and cpuidle_replace_governor() routines aren't used anymore and so can be removed. These were used by cpufreq governors earlier, which can't be compiled in as modules anymore and so these are useless.
Suggested-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Documentation/cpuidle/governor.txt | 1 - drivers/cpuidle/governor.c | 43 -------------------------------------- include/linux/cpuidle.h | 6 ------ 3 files changed, 50 deletions(-)
diff --git a/Documentation/cpuidle/governor.txt b/Documentation/cpuidle/governor.txt index 12c6bd5..d9020f5 100644 --- a/Documentation/cpuidle/governor.txt +++ b/Documentation/cpuidle/governor.txt @@ -25,5 +25,4 @@ kernel configuration and platform will be selected by cpuidle.
Interfaces: extern int cpuidle_register_governor(struct cpuidle_governor *gov); -extern void cpuidle_unregister_governor(struct cpuidle_governor *gov); struct cpuidle_governor diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c index ea2f8e7..ca89412 100644 --- a/drivers/cpuidle/governor.c +++ b/drivers/cpuidle/governor.c @@ -96,46 +96,3 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
return ret; } - -/** - * cpuidle_replace_governor - find a replacement governor - * @exclude_rating: the rating that will be skipped while looking for - * new governor. - */ -static struct cpuidle_governor *cpuidle_replace_governor(int exclude_rating) -{ - struct cpuidle_governor *gov; - struct cpuidle_governor *ret_gov = NULL; - unsigned int max_rating = 0; - - list_for_each_entry(gov, &cpuidle_governors, governor_list) { - if (gov->rating == exclude_rating) - continue; - if (gov->rating > max_rating) { - max_rating = gov->rating; - ret_gov = gov; - } - } - - return ret_gov; -} - -/** - * cpuidle_unregister_governor - unregisters a governor - * @gov: the governor - */ -void cpuidle_unregister_governor(struct cpuidle_governor *gov) -{ - if (!gov) - return; - - mutex_lock(&cpuidle_lock); - if (gov == cpuidle_curr_governor) { - struct cpuidle_governor *new_gov; - new_gov = cpuidle_replace_governor(gov->rating); - cpuidle_switch_governor(new_gov); - } - list_del(&gov->governor_list); - mutex_unlock(&cpuidle_lock); -} - diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index c082425..50fcbb0 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -195,16 +195,10 @@ struct cpuidle_governor { };
#ifdef CONFIG_CPU_IDLE - extern int cpuidle_register_governor(struct cpuidle_governor *gov); -extern void cpuidle_unregister_governor(struct cpuidle_governor *gov); - #else - static inline int cpuidle_register_governor(struct cpuidle_governor *gov) {return 0;} -static inline void cpuidle_unregister_governor(struct cpuidle_governor *gov) { } - #endif
#ifdef CONFIG_ARCH_HAS_CPU_RELAX
Viresh Kumar viresh.kumar@linaro.org writes:
Hi Rafael,
These are V2 of my CPUIdle cleanup series.. Few patches are dropped as they required further modifications. Last one is rewritten as suggested by Daniel. Most of them are already Acked by Daniel.
I gave these a spin on OMAP3 and both full-chip retention and off-mode during idle are still working fine.
Tested-by: Kevin Hilman khilman@linaro.org
Kevin
Viresh Kumar (16): cpuidle: fix indentation of cpumask cpuidle: Fix comments in cpuidle core cpuidle: make __cpuidle_get_cpu_driver() inline cpuidle: make __cpuidle_device_init() return void cpuidle: make __cpuidle_driver_init() return void cpuidle: rearrange code in __cpuidle_driver_init() cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points cpuidle: merge two if() statements for checking error cases cpuidle: reduce code duplication inside cpuidle_idle_call() cpuidle: replace multiline statements with single line in cpuidle_idle_call() cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock cpuidle: use drv instead of cpuidle_driver in show_current_driver() cpuidle: free all state kobjects from cpuidle_free_state_kobj() cpuidle: don't calculate time-diff if entered_state < 0 cpuidle: don't call poll_idle_init() for every cpu cpuidle: remove cpuidle_unregister_governor()
Documentation/cpuidle/governor.txt | 1 - drivers/cpuidle/coupled.c | 2 +- drivers/cpuidle/cpuidle.c | 95 ++++++++++---------------------------- drivers/cpuidle/driver.c | 69 ++++++++++++++++++++------- drivers/cpuidle/governor.c | 43 ----------------- drivers/cpuidle/sysfs.c | 30 ++++++------ include/linux/cpuidle.h | 8 +--- 7 files changed, 94 insertions(+), 154 deletions(-)
On 4 October 2013 02:37, Kevin Hilman khilman@linaro.org wrote:
I gave these a spin on OMAP3 and both full-chip retention and off-mode during idle are still working fine.
Tested-by: Kevin Hilman khilman@linaro.org
Thanks a lot !!
On 3 October 2013 21:26, Viresh Kumar viresh.kumar@linaro.org wrote:
These are V2 of my CPUIdle cleanup series.. Few patches are dropped as they required further modifications. Last one is rewritten as suggested by Daniel. Most of them are already Acked by Daniel.
Viresh Kumar (16): cpuidle: fix indentation of cpumask cpuidle: Fix comments in cpuidle core cpuidle: make __cpuidle_get_cpu_driver() inline cpuidle: make __cpuidle_device_init() return void cpuidle: make __cpuidle_driver_init() return void cpuidle: rearrange code in __cpuidle_driver_init() cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points cpuidle: merge two if() statements for checking error cases cpuidle: reduce code duplication inside cpuidle_idle_call() cpuidle: replace multiline statements with single line in cpuidle_idle_call() cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock cpuidle: use drv instead of cpuidle_driver in show_current_driver() cpuidle: free all state kobjects from cpuidle_free_state_kobj() cpuidle: don't calculate time-diff if entered_state < 0 cpuidle: don't call poll_idle_init() for every cpu cpuidle: remove cpuidle_unregister_governor()
Hi Rafael,
Any chance this can get in 3.13 ??
linaro-kernel@lists.linaro.org
-
Kevin Hilman -
Paul Walmsley -
Rafael J. Wysocki -
Viresh Kumar