[PATCH V4 1/2] cpuidle: simplify multiple driver support

[PATCH V4 1/2] cpuidle: simplify multiple driver support

[Daniel Lezcano]

Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver
support. The code added a couple of new API to register the driver per cpu.
That led to some code complexity to handle the kernel config options when
the multiple driver support is enabled or not, which is not really necessary.
The code has to be compatible when the multiple driver support is not enabled,
and the multiple driver support has to be compatible with the old api.

This patch removes this API, which is not yet used by any driver but needed
for the HMP cpuidle drivers which will come soon, and replaces its usage
by a cpumask pointer in the cpuidle driver structure telling what cpus are
handled by the driver. That let the API cpuidle_[un]register_driver to be used
for the multiple driver support and also the cpuidle_[un]register functions,
added recently in the cpuidle framework.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
[V4]
 - uninitialize per cpu driver variable on error
 - rollback indentation in cpuidle.h
[V3]
 - moved the kerneldoc and comments to a separate patch
[V2]:
 - fixed bad refcount check
 - inverted clockevent notify off order at unregister time

 drivers/cpuidle/cpuidle.c |    4 +-
 drivers/cpuidle/driver.c  |  192 +++++++++++++++++++--------------------------
 include/linux/cpuidle.h   |    6 +-
 3 files changed, 84 insertions(+), 118 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c3a93fe..fdc432f 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -466,7 +466,7 @@ void cpuidle_unregister(struct cpuidle_driver *drv)
 	int cpu;
 	struct cpuidle_device *device;
 
-	for_each_possible_cpu(cpu) {
+	for_each_cpu(cpu, drv->cpumask) {
 		device = &per_cpu(cpuidle_dev, cpu);
 		cpuidle_unregister_device(device);
 	}
@@ -498,7 +498,7 @@ int cpuidle_register(struct cpuidle_driver *drv,
 		return ret;
 	}
 
-	for_each_possible_cpu(cpu) {
+	for_each_cpu(cpu, drv->cpumask) {
 		device = &per_cpu(cpuidle_dev, cpu);
 		device->cpu = cpu;
 
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 8dfaaae..e75fa54 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -18,167 +18,140 @@
 
 DEFINE_SPINLOCK(cpuidle_driver_lock);
 
-static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu);
-static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
+#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
 
-static void cpuidle_setup_broadcast_timer(void *arg)
+static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
+
+static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
 {
-	int cpu = smp_processor_id();
-	clockevents_notify((long)(arg), &cpu);
+	return per_cpu(cpuidle_drivers, cpu);
 }
 
-static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu)
+static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 {
-	int i;
-
-	drv->refcnt = 0;
+	int cpu;
 
-	for (i = drv->state_count - 1; i >= 0 ; i--) {
+	for_each_cpu(cpu, drv->cpumask) {
 
-		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
+		if (drv != __cpuidle_get_cpu_driver(cpu))
 			continue;
 
-		drv->bctimer = 1;
-		on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,
-				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
-		break;
+		per_cpu(cpuidle_drivers, cpu) = NULL;
 	}
 }
 
-static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
+static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 {
-	if (!drv || !drv->state_count)
-		return -EINVAL;
-
-	if (cpuidle_disabled())
-		return -ENODEV;
+	int cpu;
 
-	if (__cpuidle_get_cpu_driver(cpu))
-		return -EBUSY;
+	for_each_cpu(cpu, drv->cpumask) {
 
-	__cpuidle_driver_init(drv, cpu);
+		if (__cpuidle_get_cpu_driver(cpu)) {
+			__cpuidle_unset_driver(drv);
+			return -EBUSY;
+		}
 
-	__cpuidle_set_cpu_driver(drv, cpu);
+		per_cpu(cpuidle_drivers, cpu) = drv;
+	}
 
 	return 0;
 }
 
-static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu)
-{
-	if (drv != __cpuidle_get_cpu_driver(cpu))
-		return;
+#else
 
-	if (!WARN_ON(drv->refcnt > 0))
-		__cpuidle_set_cpu_driver(NULL, cpu);
+static struct cpuidle_driver *cpuidle_curr_driver;
 
-	if (drv->bctimer) {
-		drv->bctimer = 0;
-		on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,
-				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
-	}
+static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
+{
+	return cpuidle_curr_driver;
 }
 
-#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
+static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
+{
+	if (cpuidle_curr_driver)
+		return -EBUSY;
 
-static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
+	cpuidle_curr_driver = drv;
 
-static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
-{
-	per_cpu(cpuidle_drivers, cpu) = drv;
+	return 0;
 }
 
-static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
+static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 {
-	return per_cpu(cpuidle_drivers, cpu);
+	if (drv == cpuidle_curr_driver)
+		cpuidle_curr_driver = NULL;
 }
 
-static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv)
+#endif
+
+static void cpuidle_setup_broadcast_timer(void *arg)
 {
-	int cpu;
-	for_each_present_cpu(cpu)
-		__cpuidle_unregister_driver(drv, cpu);
+	int cpu = smp_processor_id();
+	clockevents_notify((long)(arg), &cpu);
 }
 
-static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv)
+static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
-	int ret = 0;
-	int i, cpu;
+	int i;
 
-	for_each_present_cpu(cpu) {
-		ret = __cpuidle_register_driver(drv, cpu);
-		if (ret)
-			break;
-	}
+	drv->refcnt = 0;
 
-	if (ret)
-		for_each_present_cpu(i) {
-			if (i == cpu)
-				break;
-			__cpuidle_unregister_driver(drv, i);
-		}
+	if (!drv->cpumask)
+		drv->cpumask = (struct cpumask *)cpu_possible_mask;
 
+	for (i = drv->state_count - 1; i >= 0 ; i--) {
 
-	return ret;
+		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
+			continue;
+
+		drv->bctimer = 1;
+		break;
+	}
+
+	return 0;
 }
 
-int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu)
+static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 {
 	int ret;
 
-	spin_lock(&cpuidle_driver_lock);
-	ret = __cpuidle_register_driver(drv, cpu);
-	spin_unlock(&cpuidle_driver_lock);
+	if (!drv || !drv->state_count)
+		return -EINVAL;
 
-	return ret;
-}
+	if (cpuidle_disabled())
+		return -ENODEV;
 
-void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu)
-{
-	spin_lock(&cpuidle_driver_lock);
-	__cpuidle_unregister_driver(drv, cpu);
-	spin_unlock(&cpuidle_driver_lock);
-}
+	ret = __cpuidle_driver_init(drv);
+	if (ret)
+		return ret;
 
-/**
- * cpuidle_register_driver - registers a driver
- * @drv: the driver
- */
-int cpuidle_register_driver(struct cpuidle_driver *drv)
-{
-	int ret;
+	ret = __cpuidle_set_driver(drv);
+	if (ret)
+		return ret;
 
-	spin_lock(&cpuidle_driver_lock);
-	ret = __cpuidle_register_all_cpu_driver(drv);
-	spin_unlock(&cpuidle_driver_lock);
+	if (drv->bctimer)
+		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
+				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
 
-	return ret;
+	return 0;
 }
-EXPORT_SYMBOL_GPL(cpuidle_register_driver);
 
 /**
  * cpuidle_unregister_driver - unregisters a driver
  * @drv: the driver
  */
-void cpuidle_unregister_driver(struct cpuidle_driver *drv)
+static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
-	spin_lock(&cpuidle_driver_lock);
-	__cpuidle_unregister_all_cpu_driver(drv);
-	spin_unlock(&cpuidle_driver_lock);
-}
-EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
-
-#else
-
-static struct cpuidle_driver *cpuidle_curr_driver;
+	if (WARN_ON(drv->refcnt > 0))
+		return;
 
-static inline void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
-{
-	cpuidle_curr_driver = drv;
-}
+	if (drv->bctimer) {
+		drv->bctimer = 0;
+		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
+				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
+	}
 
-static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
-{
-	return cpuidle_curr_driver;
+	__cpuidle_unset_driver(drv);
 }
 
 /**
@@ -187,13 +160,11 @@ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
  */
 int cpuidle_register_driver(struct cpuidle_driver *drv)
 {
-	int ret, cpu;
+	int ret;
 
-	cpu = get_cpu();
 	spin_lock(&cpuidle_driver_lock);
-	ret = __cpuidle_register_driver(drv, cpu);
+	ret = __cpuidle_register_driver(drv);
 	spin_unlock(&cpuidle_driver_lock);
-	put_cpu();
 
 	return ret;
 }
@@ -205,16 +176,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
  */
 void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
-	int cpu;
-
-	cpu = get_cpu();
 	spin_lock(&cpuidle_driver_lock);
-	__cpuidle_unregister_driver(drv, cpu);
+	__cpuidle_unregister_driver(drv);
 	spin_unlock(&cpuidle_driver_lock);
-	put_cpu();
 }
 EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
-#endif
 
 /**
  * cpuidle_get_driver - return the current driver
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8f04062..0bc4b74 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -111,6 +111,9 @@ struct cpuidle_driver {
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
+
+	/* the driver handles the cpus in cpumask */
+	struct cpumask       *cpumask;
 };
 
 #ifdef CONFIG_CPU_IDLE
@@ -135,9 +138,6 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_play_dead(void);
 
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
-extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu);
-extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu);
-
 #else
 static inline void disable_cpuidle(void) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
-- 
1.7.9.5

[PATCH V4 2/2] cpuidle: Comment the driver's framework code

[Daniel Lezcano]

Added kerneldoc and comments for the cpuidle framework driver's code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
[V4]
 - fixed kerneldoc comment format

 drivers/cpuidle/driver.c |  134 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 128 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index e75fa54..4cc5bc4 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -22,11 +22,26 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
 
 static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
 
+/**
+ * __cpuidle_get_cpu_driver - returns the cpuidle driver tied with a cpu
+ * @cpu: the cpu handled by the driver
+ *
+ * Returns a pointer to struct cpuidle_driver, NULL if no driver has been
+ * registered for this driver
+ */
 static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
 {
 	return per_cpu(cpuidle_drivers, cpu);
 }
 
+/**
+ * __cpuidle_unset_driver - set the per cpu variable driver to NULL.
+ * @drv: a valid pointer to a struct cpuidle_driver
+ *
+ * For each cpu belonging to the driver's cpu mask, set the registered
+ * driver to NULL. If the specified driver is different from the registered
+ * one, the registered driver is untouched.
+ */
 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 {
 	int cpu;
@@ -40,6 +55,15 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 	}
 }
 
+/**
+ * __cpuidle_set_driver - assign to the per cpu variable the driver pointer
+ * @drv: a valid pointer to a struct cpuidle_driver
+ *
+ * For each cpus handled by the driver, belonging to the driver's cpumask,
+ * assign to the per cpu variable the driver pointer
+ *
+ * Returns 0 on success, -EBUSY if a driver was already assigned to a cpu.
+ */
 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 {
 	int cpu;
@@ -61,11 +85,24 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 
 static struct cpuidle_driver *cpuidle_curr_driver;
 
+/**
+ * __cpuidle_get_cpu_driver - returns the global cpuidle driver pointer.
+ * @cpu: this parameter is ignored without the multiple driver support
+ *
+ * Returns a pointer to a struct cpuidle_driver, NULL if no driver was
+ * previously registered
+ */
 static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
 {
 	return cpuidle_curr_driver;
 }
 
+/**
+ * __cpuidle_set_driver - assign the global cpuidle driver variable
+ * @drv: a pointer to a struct cpuidle_driver
+ *
+ * Returns 0 on success, -EBUSY if the driver is already registered.
+ */
 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 {
 	if (cpuidle_curr_driver)
@@ -76,6 +113,13 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 	return 0;
 }
 
+/**
+ * __cpuidle_unset_driver - reset the global cpuidle driver variable
+ * @drv: a pointer to a struct cpuidle_driver
+ *
+ * Sets the global cpuidle variable to NULL, if the specified driver
+ * does not match the registered driver, the function will have no effect.
+ */
 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 {
 	if (drv == cpuidle_curr_driver)
@@ -84,21 +128,49 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 
 #endif
 
+/**
+ * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
+ * @arg: a void pointer, actually used to match the smp cross call api but used
+ * as a long with two values:
+ * - CLOCK_EVT_NOTIFY_BROADCAST_ON
+ * - CLOCK_EVT_NOTIFY_BROADCAST_OFF
+ *
+ * Sets the broadcast timer notification for the current cpu. This function
+ * is called per cpu context invoked by a smp cross call. It not supposed to be
+ * called directly.
+ */
 static void cpuidle_setup_broadcast_timer(void *arg)
 {
 	int cpu = smp_processor_id();
 	clockevents_notify((long)(arg), &cpu);
 }
 
+/**
+ * __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 otherwise.
+ */
 static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	int i;
 
 	drv->refcnt = 0;
 
+	/*
+	 * we default here to all cpu possible because if the kernel
+	 * boots with some cpus offline and then we online one of them
+	 * the cpu notifier won't know which driver to assign
+	 */
 	if (!drv->cpumask)
 		drv->cpumask = (struct cpumask *)cpu_possible_mask;
 
+	/*
+	 * we look for the timer stop flag in the different states,
+	 * so know we have to setup the broadcast timer. The loop is
+	 * in reverse order, because usually the deeper state has this
+	 * flag set
+	 */
 	for (i = drv->state_count - 1; i >= 0 ; i--) {
 
 		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
@@ -111,6 +183,21 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 	return 0;
 }
 
+/**
+ * __cpuidle_register_driver: registers the driver
+ * @drv: a valid pointer to a struct cpuidle_driver
+ *
+ * Does some sanity checks, initializes the driver, assigns the driver
+ * to the global cpuidle driver variable(s) and setup the broadcast
+ * timer if the cpuidle driver has some states which shutdown the
+ * local timer.
+ *
+ * Returns 0 on success, a negative error otherwise:
+ *  * -EINVAL if the driver pointer is NULL
+ *  * -EINVAL if the not idle states available
+ *  * -ENODEV if the cpuidle framework is disabled
+ *  * -EBUSY if the driver is already assigned to the global variables
+ */
 static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 {
 	int ret;
@@ -137,8 +224,13 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 }
 
 /**
- * cpuidle_unregister_driver - unregisters a driver
- * @drv: the driver
+ * __cpuidle_unregister_driver - unregister the driver
+ * @drv: a valid pointer to a struct cpuidle_driver
+ *
+ * Checks the driver is no longer in use, resets the global cpuidle
+ * driver variable(s) and disable the timer broadcast notification
+ * mechanism if it was in use.
+ *
  */
 static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
@@ -156,7 +248,13 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
 
 /**
  * cpuidle_register_driver - registers a driver
- * @drv: the driver
+ * @drv: a pointer to a valid struct cpuidle_driver
+ *
+ * Registers the driver by taking a lock to prevent multiple callers
+ * to [un]register a driver at the same time.
+ *
+ * Returns 0 on success, a negative error otherwise (refer to the
+ *  __cpuidle_register_driver).
  */
 int cpuidle_register_driver(struct cpuidle_driver *drv)
 {
@@ -172,7 +270,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
 
 /**
  * cpuidle_unregister_driver - unregisters a driver
- * @drv: the driver
+ * @drv: a pointer to a valid struct cpuidle_driver
+ *
+ * Unregisters a driver by taking a lock to prevent multiple callers
+ * to [un]register a driver at the same time. The specified driver
+ * must match the driver currently registered.
  */
 void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
@@ -183,7 +285,9 @@ void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
 
 /**
- * cpuidle_get_driver - return the current driver
+ * cpuidle_get_driver - returns the driver tied with the current cpu.
+ *
+ * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
  */
 struct cpuidle_driver *cpuidle_get_driver(void)
 {
@@ -199,7 +303,11 @@ struct cpuidle_driver *cpuidle_get_driver(void)
 EXPORT_SYMBOL_GPL(cpuidle_get_driver);
 
 /**
- * cpuidle_get_cpu_driver - return the driver tied with a cpu
+ * cpuidle_get_cpu_driver - returns the driver registered with a cpu.
+ * @dev: a valid pointer to a struct cpuidle_device
+ *
+ * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
+ * for the specified cpu
  */
 struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev)
 {
@@ -210,6 +318,14 @@ struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev)
 }
 EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver);
 
+/**
+ * cpuidle_driver_ref - gets a refcount for the driver
+ *
+ * This function takes a refcount for the driver assigned to the current cpu
+ *
+ * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
+ * for the current cpu
+ */
 struct cpuidle_driver *cpuidle_driver_ref(void)
 {
 	struct cpuidle_driver *drv;
@@ -223,6 +339,12 @@ struct cpuidle_driver *cpuidle_driver_ref(void)
 	return drv;
 }
 
+/**
+ * cpuidle_driver_unref - puts down the refcount for the driver
+ *
+ * This function decrements the refcount for the driver assigned to the current
+ * cpu.
+ */
 void cpuidle_driver_unref(void)
 {
 	struct cpuidle_driver *drv = cpuidle_get_driver();
-- 
1.7.9.5

Re: [PATCH V4 1/2] cpuidle: simplify multiple driver support

[Rafael J. Wysocki]

On Friday, June 07, 2013 11:53:09 PM Daniel Lezcano wrote:
> Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver
> support. The code added a couple of new API to register the driver per cpu.
> That led to some code complexity to handle the kernel config options when
> the multiple driver support is enabled or not, which is not really necessary.
> The code has to be compatible when the multiple driver support is not enabled,
> and the multiple driver support has to be compatible with the old api.
> 
> This patch removes this API, which is not yet used by any driver but needed
> for the HMP cpuidle drivers which will come soon, and replaces its usage
> by a cpumask pointer in the cpuidle driver structure telling what cpus are
> handled by the driver. That let the API cpuidle_[un]register_driver to be used
> for the multiple driver support and also the cpuidle_[un]register functions,
> added recently in the cpuidle framework.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Queued up for 3.11 with a modified changelog.

Thanks,
Rafael


> ---
> [V4]
>  - uninitialize per cpu driver variable on error
>  - rollback indentation in cpuidle.h
> [V3]
>  - moved the kerneldoc and comments to a separate patch
> [V2]:
>  - fixed bad refcount check
>  - inverted clockevent notify off order at unregister time
> 
>  drivers/cpuidle/cpuidle.c |    4 +-
>  drivers/cpuidle/driver.c  |  192 +++++++++++++++++++--------------------------
>  include/linux/cpuidle.h   |    6 +-
>  3 files changed, 84 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index c3a93fe..fdc432f 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -466,7 +466,7 @@ void cpuidle_unregister(struct cpuidle_driver *drv)
>  	int cpu;
>  	struct cpuidle_device *device;
>  
> -	for_each_possible_cpu(cpu) {
> +	for_each_cpu(cpu, drv->cpumask) {
>  		device = &per_cpu(cpuidle_dev, cpu);
>  		cpuidle_unregister_device(device);
>  	}
> @@ -498,7 +498,7 @@ int cpuidle_register(struct cpuidle_driver *drv,
>  		return ret;
>  	}
>  
> -	for_each_possible_cpu(cpu) {
> +	for_each_cpu(cpu, drv->cpumask) {
>  		device = &per_cpu(cpuidle_dev, cpu);
>  		device->cpu = cpu;
>  
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 8dfaaae..e75fa54 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -18,167 +18,140 @@
>  
>  DEFINE_SPINLOCK(cpuidle_driver_lock);
>  
> -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu);
> -static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
>  
> -static void cpuidle_setup_broadcast_timer(void *arg)
> +static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
> +
> +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>  {
> -	int cpu = smp_processor_id();
> -	clockevents_notify((long)(arg), &cpu);
> +	return per_cpu(cpuidle_drivers, cpu);
>  }
>  
> -static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu)
> +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  {
> -	int i;
> -
> -	drv->refcnt = 0;
> +	int cpu;
>  
> -	for (i = drv->state_count - 1; i >= 0 ; i--) {
> +	for_each_cpu(cpu, drv->cpumask) {
>  
> -		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
> +		if (drv != __cpuidle_get_cpu_driver(cpu))
>  			continue;
>  
> -		drv->bctimer = 1;
> -		on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,
> -				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
> -		break;
> +		per_cpu(cpuidle_drivers, cpu) = NULL;
>  	}
>  }
>  
> -static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
> +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  {
> -	if (!drv || !drv->state_count)
> -		return -EINVAL;
> -
> -	if (cpuidle_disabled())
> -		return -ENODEV;
> +	int cpu;
>  
> -	if (__cpuidle_get_cpu_driver(cpu))
> -		return -EBUSY;
> +	for_each_cpu(cpu, drv->cpumask) {
>  
> -	__cpuidle_driver_init(drv, cpu);
> +		if (__cpuidle_get_cpu_driver(cpu)) {
> +			__cpuidle_unset_driver(drv);
> +			return -EBUSY;
> +		}
>  
> -	__cpuidle_set_cpu_driver(drv, cpu);
> +		per_cpu(cpuidle_drivers, cpu) = drv;
> +	}
>  
>  	return 0;
>  }
>  
> -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu)
> -{
> -	if (drv != __cpuidle_get_cpu_driver(cpu))
> -		return;
> +#else
>  
> -	if (!WARN_ON(drv->refcnt > 0))
> -		__cpuidle_set_cpu_driver(NULL, cpu);
> +static struct cpuidle_driver *cpuidle_curr_driver;
>  
> -	if (drv->bctimer) {
> -		drv->bctimer = 0;
> -		on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,
> -				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
> -	}
> +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> +{
> +	return cpuidle_curr_driver;
>  }
>  
> -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> +{
> +	if (cpuidle_curr_driver)
> +		return -EBUSY;
>  
> -static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
> +	cpuidle_curr_driver = drv;
>  
> -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
> -{
> -	per_cpu(cpuidle_drivers, cpu) = drv;
> +	return 0;
>  }
>  
> -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  {
> -	return per_cpu(cpuidle_drivers, cpu);
> +	if (drv == cpuidle_curr_driver)
> +		cpuidle_curr_driver = NULL;
>  }
>  
> -static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv)
> +#endif
> +
> +static void cpuidle_setup_broadcast_timer(void *arg)
>  {
> -	int cpu;
> -	for_each_present_cpu(cpu)
> -		__cpuidle_unregister_driver(drv, cpu);
> +	int cpu = smp_processor_id();
> +	clockevents_notify((long)(arg), &cpu);
>  }
>  
> -static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv)
> +static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>  {
> -	int ret = 0;
> -	int i, cpu;
> +	int i;
>  
> -	for_each_present_cpu(cpu) {
> -		ret = __cpuidle_register_driver(drv, cpu);
> -		if (ret)
> -			break;
> -	}
> +	drv->refcnt = 0;
>  
> -	if (ret)
> -		for_each_present_cpu(i) {
> -			if (i == cpu)
> -				break;
> -			__cpuidle_unregister_driver(drv, i);
> -		}
> +	if (!drv->cpumask)
> +		drv->cpumask = (struct cpumask *)cpu_possible_mask;
>  
> +	for (i = drv->state_count - 1; i >= 0 ; i--) {
>  
> -	return ret;
> +		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
> +			continue;
> +
> +		drv->bctimer = 1;
> +		break;
> +	}
> +
> +	return 0;
>  }
>  
> -int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu)
> +static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
>  	int ret;
>  
> -	spin_lock(&cpuidle_driver_lock);
> -	ret = __cpuidle_register_driver(drv, cpu);
> -	spin_unlock(&cpuidle_driver_lock);
> +	if (!drv || !drv->state_count)
> +		return -EINVAL;
>  
> -	return ret;
> -}
> +	if (cpuidle_disabled())
> +		return -ENODEV;
>  
> -void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu)
> -{
> -	spin_lock(&cpuidle_driver_lock);
> -	__cpuidle_unregister_driver(drv, cpu);
> -	spin_unlock(&cpuidle_driver_lock);
> -}
> +	ret = __cpuidle_driver_init(drv);
> +	if (ret)
> +		return ret;
>  
> -/**
> - * cpuidle_register_driver - registers a driver
> - * @drv: the driver
> - */
> -int cpuidle_register_driver(struct cpuidle_driver *drv)
> -{
> -	int ret;
> +	ret = __cpuidle_set_driver(drv);
> +	if (ret)
> +		return ret;
>  
> -	spin_lock(&cpuidle_driver_lock);
> -	ret = __cpuidle_register_all_cpu_driver(drv);
> -	spin_unlock(&cpuidle_driver_lock);
> +	if (drv->bctimer)
> +		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
> +				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
>  
> -	return ret;
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(cpuidle_register_driver);
>  
>  /**
>   * cpuidle_unregister_driver - unregisters a driver
>   * @drv: the driver
>   */
> -void cpuidle_unregister_driver(struct cpuidle_driver *drv)
> +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> -	spin_lock(&cpuidle_driver_lock);
> -	__cpuidle_unregister_all_cpu_driver(drv);
> -	spin_unlock(&cpuidle_driver_lock);
> -}
> -EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
> -
> -#else
> -
> -static struct cpuidle_driver *cpuidle_curr_driver;
> +	if (WARN_ON(drv->refcnt > 0))
> +		return;
>  
> -static inline void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
> -{
> -	cpuidle_curr_driver = drv;
> -}
> +	if (drv->bctimer) {
> +		drv->bctimer = 0;
> +		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
> +				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
> +	}
>  
> -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> -{
> -	return cpuidle_curr_driver;
> +	__cpuidle_unset_driver(drv);
>  }
>  
>  /**
> @@ -187,13 +160,11 @@ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>   */
>  int cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
> -	int ret, cpu;
> +	int ret;
>  
> -	cpu = get_cpu();
>  	spin_lock(&cpuidle_driver_lock);
> -	ret = __cpuidle_register_driver(drv, cpu);
> +	ret = __cpuidle_register_driver(drv);
>  	spin_unlock(&cpuidle_driver_lock);
> -	put_cpu();
>  
>  	return ret;
>  }
> @@ -205,16 +176,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
>   */
>  void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> -	int cpu;
> -
> -	cpu = get_cpu();
>  	spin_lock(&cpuidle_driver_lock);
> -	__cpuidle_unregister_driver(drv, cpu);
> +	__cpuidle_unregister_driver(drv);
>  	spin_unlock(&cpuidle_driver_lock);
> -	put_cpu();
>  }
>  EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
> -#endif
>  
>  /**
>   * cpuidle_get_driver - return the current driver
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 8f04062..0bc4b74 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -111,6 +111,9 @@ struct cpuidle_driver {
>  	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
>  	int			state_count;
>  	int			safe_state_index;
> +
> +	/* the driver handles the cpus in cpumask */
> +	struct cpumask       *cpumask;
>  };
>  
>  #ifdef CONFIG_CPU_IDLE
> @@ -135,9 +138,6 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev);
>  extern int cpuidle_play_dead(void);
>  
>  extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
> -extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu);
> -extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu);
> -
>  #else
>  static inline void disable_cpuidle(void) { }
>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V4 2/2] cpuidle: Comment the driver's framework code

[Rafael J. Wysocki]

On Friday, June 07, 2013 11:53:10 PM Daniel Lezcano wrote:
> Added kerneldoc and comments for the cpuidle framework driver's code.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Queued up for 3.11 with some modifications.

Thanks,
Rafael


> ---
> [V4]
>  - fixed kerneldoc comment format
> 
>  drivers/cpuidle/driver.c |  134 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index e75fa54..4cc5bc4 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -22,11 +22,26 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
>  
>  static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
>  
> +/**
> + * __cpuidle_get_cpu_driver - returns the cpuidle driver tied with a cpu
> + * @cpu: the cpu handled by the driver
> + *
> + * Returns a pointer to struct cpuidle_driver, NULL if no driver has been
> + * registered for this driver
> + */
>  static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>  {
>  	return per_cpu(cpuidle_drivers, cpu);
>  }
>  
> +/**
> + * __cpuidle_unset_driver - set the per cpu variable driver to NULL.
> + * @drv: a valid pointer to a struct cpuidle_driver
> + *
> + * For each cpu belonging to the driver's cpu mask, set the registered
> + * driver to NULL. If the specified driver is different from the registered
> + * one, the registered driver is untouched.
> + */
>  static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  {
>  	int cpu;
> @@ -40,6 +55,15 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  	}
>  }
>  
> +/**
> + * __cpuidle_set_driver - assign to the per cpu variable the driver pointer
> + * @drv: a valid pointer to a struct cpuidle_driver
> + *
> + * For each cpus handled by the driver, belonging to the driver's cpumask,
> + * assign to the per cpu variable the driver pointer
> + *
> + * Returns 0 on success, -EBUSY if a driver was already assigned to a cpu.
> + */
>  static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  {
>  	int cpu;
> @@ -61,11 +85,24 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  
>  static struct cpuidle_driver *cpuidle_curr_driver;
>  
> +/**
> + * __cpuidle_get_cpu_driver - returns the global cpuidle driver pointer.
> + * @cpu: this parameter is ignored without the multiple driver support
> + *
> + * Returns a pointer to a struct cpuidle_driver, NULL if no driver was
> + * previously registered
> + */
>  static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>  {
>  	return cpuidle_curr_driver;
>  }
>  
> +/**
> + * __cpuidle_set_driver - assign the global cpuidle driver variable
> + * @drv: a pointer to a struct cpuidle_driver
> + *
> + * Returns 0 on success, -EBUSY if the driver is already registered.
> + */
>  static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  {
>  	if (cpuidle_curr_driver)
> @@ -76,6 +113,13 @@ static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  	return 0;
>  }
>  
> +/**
> + * __cpuidle_unset_driver - reset the global cpuidle driver variable
> + * @drv: a pointer to a struct cpuidle_driver
> + *
> + * Sets the global cpuidle variable to NULL, if the specified driver
> + * does not match the registered driver, the function will have no effect.
> + */
>  static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  {
>  	if (drv == cpuidle_curr_driver)
> @@ -84,21 +128,49 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  
>  #endif
>  
> +/**
> + * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
> + * @arg: a void pointer, actually used to match the smp cross call api but used
> + * as a long with two values:
> + * - CLOCK_EVT_NOTIFY_BROADCAST_ON
> + * - CLOCK_EVT_NOTIFY_BROADCAST_OFF
> + *
> + * Sets the broadcast timer notification for the current cpu. This function
> + * is called per cpu context invoked by a smp cross call. It not supposed to be
> + * called directly.
> + */
>  static void cpuidle_setup_broadcast_timer(void *arg)
>  {
>  	int cpu = smp_processor_id();
>  	clockevents_notify((long)(arg), &cpu);
>  }
>  
> +/**
> + * __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 otherwise.
> + */
>  static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>  {
>  	int i;
>  
>  	drv->refcnt = 0;
>  
> +	/*
> +	 * we default here to all cpu possible because if the kernel
> +	 * boots with some cpus offline and then we online one of them
> +	 * the cpu notifier won't know which driver to assign
> +	 */
>  	if (!drv->cpumask)
>  		drv->cpumask = (struct cpumask *)cpu_possible_mask;
>  
> +	/*
> +	 * we look for the timer stop flag in the different states,
> +	 * so know we have to setup the broadcast timer. The loop is
> +	 * in reverse order, because usually the deeper state has this
> +	 * flag set
> +	 */
>  	for (i = drv->state_count - 1; i >= 0 ; i--) {
>  
>  		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
> @@ -111,6 +183,21 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>  	return 0;
>  }
>  
> +/**
> + * __cpuidle_register_driver: registers the driver
> + * @drv: a valid pointer to a struct cpuidle_driver
> + *
> + * Does some sanity checks, initializes the driver, assigns the driver
> + * to the global cpuidle driver variable(s) and setup the broadcast
> + * timer if the cpuidle driver has some states which shutdown the
> + * local timer.
> + *
> + * Returns 0 on success, a negative error otherwise:
> + *  * -EINVAL if the driver pointer is NULL
> + *  * -EINVAL if the not idle states available
> + *  * -ENODEV if the cpuidle framework is disabled
> + *  * -EBUSY if the driver is already assigned to the global variables
> + */
>  static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
>  	int ret;
> @@ -137,8 +224,13 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  }
>  
>  /**
> - * cpuidle_unregister_driver - unregisters a driver
> - * @drv: the driver
> + * __cpuidle_unregister_driver - unregister the driver
> + * @drv: a valid pointer to a struct cpuidle_driver
> + *
> + * Checks the driver is no longer in use, resets the global cpuidle
> + * driver variable(s) and disable the timer broadcast notification
> + * mechanism if it was in use.
> + *
>   */
>  static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> @@ -156,7 +248,13 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  
>  /**
>   * cpuidle_register_driver - registers a driver
> - * @drv: the driver
> + * @drv: a pointer to a valid struct cpuidle_driver
> + *
> + * Registers the driver by taking a lock to prevent multiple callers
> + * to [un]register a driver at the same time.
> + *
> + * Returns 0 on success, a negative error otherwise (refer to the
> + *  __cpuidle_register_driver).
>   */
>  int cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
> @@ -172,7 +270,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
>  
>  /**
>   * cpuidle_unregister_driver - unregisters a driver
> - * @drv: the driver
> + * @drv: a pointer to a valid struct cpuidle_driver
> + *
> + * Unregisters a driver by taking a lock to prevent multiple callers
> + * to [un]register a driver at the same time. The specified driver
> + * must match the driver currently registered.
>   */
>  void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> @@ -183,7 +285,9 @@ void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
>  
>  /**
> - * cpuidle_get_driver - return the current driver
> + * cpuidle_get_driver - returns the driver tied with the current cpu.
> + *
> + * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
>   */
>  struct cpuidle_driver *cpuidle_get_driver(void)
>  {
> @@ -199,7 +303,11 @@ struct cpuidle_driver *cpuidle_get_driver(void)
>  EXPORT_SYMBOL_GPL(cpuidle_get_driver);
>  
>  /**
> - * cpuidle_get_cpu_driver - return the driver tied with a cpu
> + * cpuidle_get_cpu_driver - returns the driver registered with a cpu.
> + * @dev: a valid pointer to a struct cpuidle_device
> + *
> + * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
> + * for the specified cpu
>   */
>  struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev)
>  {
> @@ -210,6 +318,14 @@ struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver);
>  
> +/**
> + * cpuidle_driver_ref - gets a refcount for the driver
> + *
> + * This function takes a refcount for the driver assigned to the current cpu
> + *
> + * Returns a struct cpuidle_driver pointer, or NULL if no driver is registered
> + * for the current cpu
> + */
>  struct cpuidle_driver *cpuidle_driver_ref(void)
>  {
>  	struct cpuidle_driver *drv;
> @@ -223,6 +339,12 @@ struct cpuidle_driver *cpuidle_driver_ref(void)
>  	return drv;
>  }
>  
> +/**
> + * cpuidle_driver_unref - puts down the refcount for the driver
> + *
> + * This function decrements the refcount for the driver assigned to the current
> + * cpu.
> + */
>  void cpuidle_driver_unref(void)
>  {
>  	struct cpuidle_driver *drv = cpuidle_get_driver();
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V4 1/2] cpuidle: simplify multiple driver support

[Viresh Kumar]

On 8 June 2013 03:23, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver
> support. The code added a couple of new API to register the driver per cpu.
> That led to some code complexity to handle the kernel config options when
> the multiple driver support is enabled or not, which is not really necessary.
> The code has to be compatible when the multiple driver support is not enabled,
> and the multiple driver support has to be compatible with the old api.
>
> This patch removes this API, which is not yet used by any driver but needed
> for the HMP cpuidle drivers which will come soon, and replaces its usage
> by a cpumask pointer in the cpuidle driver structure telling what cpus are
> handled by the driver. That let the API cpuidle_[un]register_driver to be used
> for the multiple driver support and also the cpuidle_[un]register functions,
> added recently in the cpuidle framework.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Sorry for jumping onto this thread so late :(
The current code in cpuidle/driver.c contains two definitions of these routines:
__cpuidle_get_cpu_driver() and __cpuidle_{set|unset}_driver(), enclosed
withing #if/else..

These duplicate routines exist only to save some space where we don't
have multiple drivers for a platform, right? So, that we don't waste up
space for per-cpu variables for holding cpuidle_driver..

But what about multi platform kernels? This config option would be enabled
then and we would finally run into the same problem again..

The worst side of this is: We will run separate code paths for a platform
which doesn't have support for multiple drivers in the multiplatform kernel..
Even if it works, it looks a bit wrong design wise..

Either we can have a runtime variable in cpuidle_driver or somewhere else
that will let us know if we want multiple drivers for our platform or not

OR

do the per-cpu stuff for everybody..

I was about to rewrite that stuff and send a patch for it, but then thought
probably its better to discuss it first..

--
viresh

Re: [PATCH V4 1/2] cpuidle: simplify multiple driver support

[Daniel Lezcano]

On 09/18/2013 07:21 AM, Viresh Kumar wrote:
> On 8 June 2013 03:23, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver
>> support. The code added a couple of new API to register the driver per cpu.
>> That led to some code complexity to handle the kernel config options when
>> the multiple driver support is enabled or not, which is not really necessary.
>> The code has to be compatible when the multiple driver support is not enabled,
>> and the multiple driver support has to be compatible with the old api.
>>
>> This patch removes this API, which is not yet used by any driver but needed
>> for the HMP cpuidle drivers which will come soon, and replaces its usage
>> by a cpumask pointer in the cpuidle driver structure telling what cpus are
>> handled by the driver. That let the API cpuidle_[un]register_driver to be used
>> for the multiple driver support and also the cpuidle_[un]register functions,
>> added recently in the cpuidle framework.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Sorry for jumping onto this thread so late :(
> The current code in cpuidle/driver.c contains two definitions of these routines:
> __cpuidle_get_cpu_driver() and __cpuidle_{set|unset}_driver(), enclosed
> withing #if/else..
> 
> These duplicate routines exist only to save some space where we don't
> have multiple drivers for a platform, right? So, that we don't waste up
> space for per-cpu variables for holding cpuidle_driver..

That's right.

> But what about multi platform kernels? This config option would be enabled
> then and we would finally run into the same problem again..
> 
> The worst side of this is: We will run separate code paths for a platform
> which doesn't have support for multiple drivers in the multiplatform kernel..
> Even if it works, it looks a bit wrong design wise..

Where is it wrong in design ? If the multiple driver support is enabled
in the kernel but the driver handles all the cpu, it works.

> Either we can have a runtime variable in cpuidle_driver or somewhere else
> that will let us know if we want multiple drivers for our platform or not
> 
> OR
> 
> do the per-cpu stuff for everybody..

I don't think it is worth to add more code complexity to save an extra
small chunk of memory on ARM. If we don't want to support the multiple
driver, the option is disabled and we use a single variable. That is the
case for x86. If we want to enable it for multiple platforms support on
ARM, then we need per-cpu and we lose a bit of per-cpu memory.

  -- Daniel

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

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

Re: [PATCH V4 1/2] cpuidle: simplify multiple driver support

[Viresh Kumar]

On 25 September 2013 20:32, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Where is it wrong in design ? If the multiple driver support is enabled
> in the kernel but the driver handles all the cpu, it works.

Yeah It works but I don't really support two separate code paths based
on how kernel is compiled.. For example:
Consider any ARM system which doesn't need multiple drivers support
and so must be using the non-per-cpu variable for holding drivers pointer..

Now the same driver is included as part of a multiplatform kernel and
now the same driver/platform is using the per-cpu variables for storing
driver pointer.. Even sysfs will have files with name "driver" to print
drivers name for CPUs..

I know it doesn't break anything but it didn't looked good design wise to me..