[PATCH V2 00/16] CPUIdle: Minor cleanups for 3.13

[PATCH V2 00/16] CPUIdle: Minor cleanups for 3.13

[Viresh Kumar]

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

-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 01/16] cpuidle: fix indentation of cpumask

[Viresh Kumar]

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
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 02/16] cpuidle: Fix comments in cpuidle core

[Viresh Kumar]

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--) {
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 03/16] cpuidle: make __cpuidle_get_cpu_driver() inline

[Viresh Kumar]

__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);
 }
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 04/16] cpuidle: make __cpuidle_device_init() return void

[Viresh Kumar]

__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)
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 05/16] cpuidle: make __cpuidle_driver_init() return void

[Viresh Kumar]

__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)
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 06/16] cpuidle: rearrange code in __cpuidle_driver_init()

[Viresh Kumar]

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;
+		}
 	}
 }
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 07/16] cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points

[Viresh Kumar]

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;
 }
 
 /**
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 08/16] cpuidle: merge two if() statements for checking error cases

[Viresh Kumar]

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 */
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 09/16] cpuidle: reduce code duplication inside cpuidle_idle_call()

[Viresh Kumar]

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);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 10/16] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Viresh Kumar]

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);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 11/16] cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock

[Viresh Kumar]

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
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 12/16] cpuidle: use drv instead of cpuidle_driver in show_current_driver()

[Viresh Kumar]

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);
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 13/16] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Viresh Kumar]

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
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 14/16] cpuidle: don't calculate time-diff if entered_state < 0

[Viresh Kumar]

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;
 }
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 15/16] cpuidle: don't call poll_idle_init() for every cpu

[Viresh Kumar]

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;
 }
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 16/16] cpuidle: remove cpuidle_unregister_governor()

[Viresh Kumar]

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
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V2 03/16] cpuidle: make __cpuidle_get_cpu_driver() inline

[Paul Walmsley]

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);
>   }

Re: [PATCH V2 00/16] CPUIdle: Minor cleanups for 3.13

[Kevin Hilman]

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

Re: [PATCH V2 00/16] CPUIdle: Minor cleanups for 3.13

[Viresh Kumar]

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

Re: [PATCH V2 00/16] CPUIdle: Minor cleanups for 3.13

[Viresh Kumar]

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

Re: [PATCH V2 13/16] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Viresh Kumar]

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
>

Re: [PATCH V2 13/16] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Viresh Kumar]

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

Re: [PATCH V2 14/16] cpuidle: don't calculate time-diff if entered_state < 0

[Viresh Kumar]

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
>

Re: [PATCH V2 13/16] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Rafael J. Wysocki]

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
> >>
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 14/16] cpuidle: don't calculate time-diff if entered_state < 0

[Rafael J. Wysocki]

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
> >
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 13/16] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Viresh Kumar]

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