[V3] cpuidle : use percpu cpuidle in the core code

Message ID 1333394401-20701-1-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano April 2, 2012, 7:20 p.m.
The usual cpuidle initialization routines register the driver and
then register a cpuidle device per cpu.

By default, most drivers initialize the device state count with the
driver state count.

We can then add a new function 'cpuidle_register' where we register
the driver and the devices. These devices can be defined in a global
static variable in cpuidle.c. We will be able to factor out and
remove a lot of duplicate lines of code.

As we still have some drivers, with different initialization routines,
we keep 'cpuidle_register_driver' and 'cpuidle_register_device' as low
level initialization routines to do some specific operations on the
cpuidle devices.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h   |    3 +++
 2 files changed, 45 insertions(+), 0 deletions(-)

Comments

Srivatsa S. Bhat April 3, 2012, 5:49 a.m. | #1
On 04/03/2012 12:50 AM, Daniel Lezcano wrote:

> The usual cpuidle initialization routines register the driver and
> then register a cpuidle device per cpu.
> 
> By default, most drivers initialize the device state count with the
> driver state count.
> 
> We can then add a new function 'cpuidle_register' where we register
> the driver and the devices. These devices can be defined in a global
> static variable in cpuidle.c. We will be able to factor out and
> remove a lot of duplicate lines of code.
> 
> As we still have some drivers, with different initialization routines,
> we keep 'cpuidle_register_driver' and 'cpuidle_register_device' as low
> level initialization routines to do some specific operations on the
> cpuidle devices.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>


Looks good to me now.
Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>


Regards,
Srivatsa S. Bhat

> ---
>  drivers/cpuidle/cpuidle.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h   |    3 +++
>  2 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 87411ce..4d1f79b 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -23,6 +23,7 @@
>  #include "cpuidle.h"
> 
>  DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
> +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
> 
>  DEFINE_MUTEX(cpuidle_lock);
>  LIST_HEAD(cpuidle_detected_devices);
> @@ -419,6 +420,47 @@ int cpuidle_register_device(struct cpuidle_device *dev)
> 
>  EXPORT_SYMBOL_GPL(cpuidle_register_device);
> 
> +/*
> + * cpuidle_register : register cpuidle driver and devices
> + * Note this function must be called after smp_init.
> + * @drv : the cpuidle driver
> + * Returns 0 on success, < 0 otherwise
> + */
> +int cpuidle_register(struct cpuidle_driver *drv)
> +{
> +	int ret, cpu, i;
> +	struct cpuidle_device *dev;
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret)
> +		return ret;
> +
> +	for_each_online_cpu(cpu) {
> +		dev = &per_cpu(cpuidle_device, cpu);
> +		dev->cpu = cpu;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret)
> +			goto out_unregister;
> +	}
> +
> +out:
> +	return ret;
> +
> +out_unregister:
> +	for_each_online_cpu(i) {
> +		if (i == cpu)
> +			break;
> +		dev = &per_cpu(cpuidle_device, i);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	cpuidle_unregister_driver(drv);
> +
> +	goto out;
> +}
> +EXPORT_SYMBOL_GPL(cpuidle_register);
> +
>  /**
>   * cpuidle_unregister_device - unregisters a CPU's idle PM feature
>   * @dev: the cpu
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6c26a3d..3475294 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -135,6 +135,7 @@ struct cpuidle_driver {
>  #ifdef CONFIG_CPU_IDLE
>  extern void disable_cpuidle(void);
>  extern int cpuidle_idle_call(void);
> +extern int cpuidle_register(struct cpuidle_driver *drv);
>  extern int cpuidle_register_driver(struct cpuidle_driver *drv);
>  struct cpuidle_driver *cpuidle_get_driver(void);
>  extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
> @@ -154,6 +155,8 @@ extern int cpuidle_play_dead(void);
>  #else
>  static inline void disable_cpuidle(void) { }
>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
> +static inline int cpuidle_register(struct cpuidle_driver *drv)
> +{return -ENODEV; }
>  static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
>  {return -ENODEV; }
>  static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
Daniel Lezcano May 30, 2012, 6:51 a.m. | #2
On 04/03/2012 07:49 AM, Srivatsa S. Bhat wrote:
> On 04/03/2012 12:50 AM, Daniel Lezcano wrote:
> 
>> The usual cpuidle initialization routines register the driver and
>> then register a cpuidle device per cpu.
>>
>> By default, most drivers initialize the device state count with the
>> driver state count.
>>
>> We can then add a new function 'cpuidle_register' where we register
>> the driver and the devices. These devices can be defined in a global
>> static variable in cpuidle.c. We will be able to factor out and
>> remove a lot of duplicate lines of code.
>>
>> As we still have some drivers, with different initialization routines,
>> we keep 'cpuidle_register_driver' and 'cpuidle_register_device' as low
>> level initialization routines to do some specific operations on the
>> cpuidle devices.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> 
> Looks good to me now.
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Hi Len,

could you consider this patch for merging ?

Thanks
  -- Daniel

> 
> 
> Regards,
> Srivatsa S. Bhat
> 
>> ---
>>  drivers/cpuidle/cpuidle.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/cpuidle.h   |    3 +++
>>  2 files changed, 45 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 87411ce..4d1f79b 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -23,6 +23,7 @@
>>  #include "cpuidle.h"
>>
>>  DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
>> +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
>>
>>  DEFINE_MUTEX(cpuidle_lock);
>>  LIST_HEAD(cpuidle_detected_devices);
>> @@ -419,6 +420,47 @@ int cpuidle_register_device(struct cpuidle_device *dev)
>>
>>  EXPORT_SYMBOL_GPL(cpuidle_register_device);
>>
>> +/*
>> + * cpuidle_register : register cpuidle driver and devices
>> + * Note this function must be called after smp_init.
>> + * @drv : the cpuidle driver
>> + * Returns 0 on success, < 0 otherwise
>> + */
>> +int cpuidle_register(struct cpuidle_driver *drv)
>> +{
>> +	int ret, cpu, i;
>> +	struct cpuidle_device *dev;
>> +
>> +	ret = cpuidle_register_driver(drv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for_each_online_cpu(cpu) {
>> +		dev = &per_cpu(cpuidle_device, cpu);
>> +		dev->cpu = cpu;
>> +
>> +		ret = cpuidle_register_device(dev);
>> +		if (ret)
>> +			goto out_unregister;
>> +	}
>> +
>> +out:
>> +	return ret;
>> +
>> +out_unregister:
>> +	for_each_online_cpu(i) {
>> +		if (i == cpu)
>> +			break;
>> +		dev = &per_cpu(cpuidle_device, i);
>> +		cpuidle_unregister_device(dev);
>> +	}
>> +
>> +	cpuidle_unregister_driver(drv);
>> +
>> +	goto out;
>> +}
>> +EXPORT_SYMBOL_GPL(cpuidle_register);
>> +
>>  /**
>>   * cpuidle_unregister_device - unregisters a CPU's idle PM feature
>>   * @dev: the cpu
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 6c26a3d..3475294 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -135,6 +135,7 @@ struct cpuidle_driver {
>>  #ifdef CONFIG_CPU_IDLE
>>  extern void disable_cpuidle(void);
>>  extern int cpuidle_idle_call(void);
>> +extern int cpuidle_register(struct cpuidle_driver *drv);
>>  extern int cpuidle_register_driver(struct cpuidle_driver *drv);
>>  struct cpuidle_driver *cpuidle_get_driver(void);
>>  extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
>> @@ -154,6 +155,8 @@ extern int cpuidle_play_dead(void);
>>  #else
>>  static inline void disable_cpuidle(void) { }
>>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
>> +static inline int cpuidle_register(struct cpuidle_driver *drv)
>> +{return -ENODEV; }
>>  static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
>>  {return -ENODEV; }
>>  static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
> 
>

Patch

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 87411ce..4d1f79b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -23,6 +23,7 @@ 
 #include "cpuidle.h"
 
 DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
+DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
 
 DEFINE_MUTEX(cpuidle_lock);
 LIST_HEAD(cpuidle_detected_devices);
@@ -419,6 +420,47 @@  int cpuidle_register_device(struct cpuidle_device *dev)
 
 EXPORT_SYMBOL_GPL(cpuidle_register_device);
 
+/*
+ * cpuidle_register : register cpuidle driver and devices
+ * Note this function must be called after smp_init.
+ * @drv : the cpuidle driver
+ * Returns 0 on success, < 0 otherwise
+ */
+int cpuidle_register(struct cpuidle_driver *drv)
+{
+	int ret, cpu, i;
+	struct cpuidle_device *dev;
+
+	ret = cpuidle_register_driver(drv);
+	if (ret)
+		return ret;
+
+	for_each_online_cpu(cpu) {
+		dev = &per_cpu(cpuidle_device, cpu);
+		dev->cpu = cpu;
+
+		ret = cpuidle_register_device(dev);
+		if (ret)
+			goto out_unregister;
+	}
+
+out:
+	return ret;
+
+out_unregister:
+	for_each_online_cpu(i) {
+		if (i == cpu)
+			break;
+		dev = &per_cpu(cpuidle_device, i);
+		cpuidle_unregister_device(dev);
+	}
+
+	cpuidle_unregister_driver(drv);
+
+	goto out;
+}
+EXPORT_SYMBOL_GPL(cpuidle_register);
+
 /**
  * cpuidle_unregister_device - unregisters a CPU's idle PM feature
  * @dev: the cpu
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 6c26a3d..3475294 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -135,6 +135,7 @@  struct cpuidle_driver {
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
 extern int cpuidle_idle_call(void);
+extern int cpuidle_register(struct cpuidle_driver *drv);
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
 struct cpuidle_driver *cpuidle_get_driver(void);
 extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
@@ -154,6 +155,8 @@  extern int cpuidle_play_dead(void);
 #else
 static inline void disable_cpuidle(void) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
+static inline int cpuidle_register(struct cpuidle_driver *drv)
+{return -ENODEV; }
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV; }
 static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }