[v3,1/7] cpuidle: Add common init interface and idle functionality

Message ID 1327379854-12403-2-git-send-email-rob.lee@linaro.org
State New
Headers show

Commit Message

Rob Jan. 24, 2012, 4:37 a.m.
The patch adds some cpuidle initialization functionality commonly
duplicated by many platforms.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 drivers/cpuidle/Makefile |    2 +-
 drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h  |   24 +++++++
 3 files changed, 177 insertions(+), 1 deletions(-)
 create mode 100644 drivers/cpuidle/common.c

Comments

Rob Herring Jan. 24, 2012, 2:36 p.m. | #1
On 01/23/2012 10:37 PM, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>

A few cosmetic comments below, but otherwise looks good.

Reviewed-by: Rob Herring <rob.herring@calxeda.com>

> ---
>  drivers/cpuidle/Makefile |    2 +-
>  drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h  |   24 +++++++
>  3 files changed, 177 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/cpuidle/common.c
> 
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 5634f88..2928d93 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -2,4 +2,4 @@
>  # Makefile for cpuidle.
>  #
>  
> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.

2012 now...

> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality
> + * used by many ARM SoC platforms.  Providing this functionality here
> + * reduces the duplication of this code for each ARM platform that uses it.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <linux/hrtimer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <asm/proc-fns.h>
> +
> +static struct cpuidle_device __percpu * common_cpuidle_devices;
> +
> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index);
> +
> +int cpuidle_def_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	cpu_do_idle();
> +	return index;
> +}
> +
> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();
> +
> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));
> +
> +	return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(common_cpuidle_devices);
> +}
> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv		Pointer to your cpuidle_driver object.
> + * @simple		Use the common simple_enter wrapper?

remove the ?

> + * @driver_data_init	Pointer to your platform function to initialize your
> + *			platform specific driver data.  Use NULL if platform
> + *			specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> +	struct cpuidle_device *dev;
> +	struct cpuidle_driver *drv;
> +	int i, cpu_id, ret;
> +
> +	if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);

Using pr_fmt rather than function name is preferred.

> +		return -EINVAL;
> +	}
> +
> +	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> +	if (!drv) {
> +		pr_err("%s: no memory for cpuidle driver\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	*drv = *pdrv;

Perhaps a comment so it is clear you intend this to be struct copy.

> +
> +	for (i = 0; simple && (i < drv->state_count); i++) {
> +		do_idle[i] = drv->states[i].enter;
> +		drv->states[i].enter = simple_enter;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +		goto free_drv;
> +	}
> +
> +	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (common_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		if (driver_data_init)
> +			driver_data_init(dev);
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +uninit:
> +
> +	common_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +
> +	cpuidle_unregister_driver(drv);
> +
> +free_drv:
> +
> +	kfree(drv);
> +
> +	return ret;

Remove all the blank lines in the error handling.

> +}
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..2aa89db 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -56,6 +56,16 @@ struct cpuidle_state {
>  
>  #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>  
> +/* Common ARM WFI state */
> +#define CPUIDLE_ARM_WFI_STATE {\
> +	.enter                  = cpuidle_def_idle,\
> +	.exit_latency           = 2,\
> +	.target_residency       = 1,\
> +	.flags                  = CPUIDLE_FLAG_TIME_VALID,\
> +	.name                   = "WFI",\
> +	.desc                   = "ARM core clock gating (WFI)",\
> +}
> +
>  /**
>   * cpuidle_get_statedata - retrieves private driver state data
>   * @st_usage: the state usage statistics
> @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void);
>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
>  
> +/* provide a default idle function */
> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index);
> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev));
> +extern void common_cpuidle_devices_uninit(void);
> +
>  #else
>  static inline void disable_cpuidle(void) { }
>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { }
>  static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>  {return -ENODEV; }
>  static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
> +static inline int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index)
> +{return -ENODEV; }
> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
> +	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
> +{return -ENODEV; }
> +static inline void common_cpuidle_devices_uninit(void) { }
>  
>  #endif
>
Kevin Hilman Jan. 24, 2012, 8:16 p.m. | #2
Robert Lee <rob.lee@linaro.org> writes:

> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
>
> Signed-off-by: Robert Lee <rob.lee@linaro.org>

[...]

> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();

Is this needed?   arch/arm/kernel/process.c:cpu_idle() already disables IRQs

> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));

I don't think this cast is quite right here, since last_residency is an
int, it needs to be capped, or you'll end up with negative last_residency.

I think you need to do something like is done in poll_idle() in
drivers/cpuidle/cpuidle.c:

	diff = ktime_to_us(ktime_sub(t2, t1));
	if (diff > INT_MAX)
		diff = INT_MAX;


Speaking of poll_idle(), simple_idle() looks quite similar to it.   Maybe
they can be combined?

Kevin
Rob Jan. 24, 2012, 10:43 p.m. | #3
>
> A few cosmetic comments below, but otherwise looks good.
>
> Reviewed-by: Rob Herring <rob.herring@calxeda.com>
>
Rob, thanks for the review.  Agree with your comments and will fix in
the next version.
Rob Jan. 24, 2012, 11:10 p.m. | #4
On Tue, Jan 24, 2012 at 2:16 PM, Kevin Hilman <khilman@ti.com> wrote:
> Robert Lee <rob.lee@linaro.org> writes:
>
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>
> [...]
>
>> +static int simple_enter(struct cpuidle_device *dev,
>> +                            struct cpuidle_driver *drv, int index)
>> +{
>> +     ktime_t time_start, time_end;
>> +
>> +     local_irq_disable();
>
> Is this needed?   arch/arm/kernel/process.c:cpu_idle() already disables IRQs
>

As far as I can tell, all calls into cpuidle_idle_call have already
called this.  I had left it just in case someone wanted to call
cpuidle_idle_call directly, but that probably doesn't make sense.
Perhaps in the comments of cpuidle_idle_call I just list this as a
requirement and remove it from simple_enter.

>> +     time_start = ktime_get();
>> +
>> +     index = do_idle[index](dev, drv, index);
>> +
>> +     time_end = ktime_get();
>> +
>> +     local_irq_enable();
>> +
>> +     dev->last_residency =
>> +             (int)ktime_to_us(ktime_sub(time_end, time_start));
>
> I don't think this cast is quite right here, since last_residency is an
> int, it needs to be capped, or you'll end up with negative last_residency.
>
> I think you need to do something like is done in poll_idle() in
> drivers/cpuidle/cpuidle.c:
>
>        diff = ktime_to_us(ktime_sub(t2, t1));
>        if (diff > INT_MAX)
>                diff = INT_MAX;
>
>

Thanks, I missed that.  Will fix in v4.

> Speaking of poll_idle(), simple_idle() looks quite similar to it.   Maybe
> they can be combined?

Good point.  I'll look into that.

>
> Kevin
>
Mike Turquette Jan. 24, 2012, 11:46 p.m. | #5
On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote:
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality

s/performs //

> +static struct cpuidle_device __percpu * common_cpuidle_devices;

You can use DECLARE_PER_CPU here.

Is there any particular reason to allocate these dynamically?  You can
replace the code above with,

static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices);

I might change the variable name to "cpu_cpuidle_device" in that case
since you are addressing a single CPU when using the per cpu accessor
functions and "common_cpuidle_devices" sounds like an array or a list
or something.  No big deal to keep the current name though.

> +static int simple_enter(struct cpuidle_device *dev,
> +                              struct cpuidle_driver *drv, int index)
> +{
> +       ktime_t time_start, time_end;
> +
> +       local_irq_disable();
> +
> +       time_start = ktime_get();
> +
> +       index = do_idle[index](dev, drv, index);
> +
> +       time_end = ktime_get();
> +
> +       local_irq_enable();
> +
> +       dev->last_residency =
> +               (int)ktime_to_us(ktime_sub(time_end, time_start));

Sometimes an attempt to enter some C-state fails and the do_idle will
return immediately.  What do you think about having do_idle return
-EERROR in this case and the conditionally setting last_residency to
zero in those cases?  The point is that a C-state's total residency
time should not increase in the case where the hardware did not
successfully transition into that C-state.  I've observed many times
where a specific low power state was actually achieved in the hardware
but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps
incrementing (albeit in very tiny increments).  Something like,

if (IS_ERR(index))
        dev->last_residency = 0;
else
        ...

Note: I haven't been through the CPUidle core in a while so maybe the
above suggestion violates some other requirements/assumptions...

> +
> +       return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> +       int cpu_id;
> +       struct cpuidle_device *dev;
> +
> +       for_each_possible_cpu(cpu_id) {
> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +               cpuidle_unregister_device(dev);
> +       }
> +
> +       free_percpu(common_cpuidle_devices);

See note above about statically allocating the per-cpu variables.

> +}
> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv               Pointer to your cpuidle_driver object.
> + * @simple             Use the common simple_enter wrapper?
> + * @driver_data_init   Pointer to your platform function to initialize your
> + *                     platform specific driver data.  Use NULL if platform
> + *                     specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> +                        void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> +       struct cpuidle_device *dev;
> +       struct cpuidle_driver *drv;
> +       int i, cpu_id, ret;
> +
> +       if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
> +               pr_err("%s: Invalid Input\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> +       if (!drv) {
> +               pr_err("%s: no memory for cpuidle driver\n", __func__);
> +               return -ENOMEM;
> +       }
> +
> +       *drv = *pdrv;
> +
> +       for (i = 0; simple && (i < drv->state_count); i++) {
> +               do_idle[i] = drv->states[i].enter;
> +               drv->states[i].enter = simple_enter;
> +       }
> +
> +       ret = cpuidle_register_driver(drv);
> +       if (ret) {
> +               pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +               goto free_drv;
> +       }
> +
> +       common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +       if (common_cpuidle_devices == NULL) {
> +               ret = -ENOMEM;
> +               goto unregister_drv;
> +       }

See note above about statically allocating these.

Regards,
Mike
Daniel Lezcano Jan. 24, 2012, 11:49 p.m. | #6
On 01/24/2012 05:37 AM, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
>
> Signed-off-by: Robert Lee<rob.lee@linaro.org>
> ---

Hi Rob,

nice work. The result is interesting. I have a few comments below.


>   drivers/cpuidle/Makefile |    2 +-
>   drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/cpuidle.h  |   24 +++++++
>   3 files changed, 177 insertions(+), 1 deletions(-)
>   create mode 100644 drivers/cpuidle/common.c
>
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 5634f88..2928d93 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -2,4 +2,4 @@
>   # Makefile for cpuidle.
>   #
>
> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality
> + * used by many ARM SoC platforms.  Providing this functionality here
> + * reduces the duplication of this code for each ARM platform that uses it.
> + */
> +
> +#include<linux/kernel.h>
> +#include<linux/io.h>
> +#include<linux/cpuidle.h>
> +#include<linux/hrtimer.h>
> +#include<linux/err.h>
> +#include<linux/slab.h>
> +#include<asm/proc-fns.h>
> +
> +static struct cpuidle_device __percpu * common_cpuidle_devices;
> +
> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index);
> +
> +int cpuidle_def_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	cpu_do_idle();
> +	return index;
> +}
> +
> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();
> +
> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));
> +
> +	return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(common_cpuidle_devices);
> +}

If the registering sequence aborts, won't cpuidle_unregister_device 
leads to a kernel warning as it could be specified with a cpu which has 
*not* been registered yet ?

Perhaps we should pass the cpuid from where the cpu has failed an do a 
reverse unregister sequence.

void common_cpuidle_devices_uninit(int cpu)
{
         for (cpu--; cpu >= 0; cpu--) {
                 device = &per_cpu(common_cpuidle_devices, cpu);
                 cpuidle_unregister_device(device);
         }
...


> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv		Pointer to your cpuidle_driver object.
> + * @simple		Use the common simple_enter wrapper?
> + * @driver_data_init	Pointer to your platform function to initialize your
> + *			platform specific driver data.  Use NULL if platform
> + *			specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> +	struct cpuidle_device *dev;
> +	struct cpuidle_driver *drv;
> +	int i, cpu_id, ret;
> +
> +	if (!pdrv || pdrv->state_count>  CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> +	if (!drv) {
> +		pr_err("%s: no memory for cpuidle driver\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	*drv = *pdrv;

Rob can you explain why is needed to copy this structure ?

Maybe kmemdup is more adequate here.

drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);

> +
> +	for (i = 0; simple&&  (i<  drv->state_count); i++) {
> +		do_idle[i] = drv->states[i].enter;
> +		drv->states[i].enter = simple_enter;
> +	}

Do we really need a 'simple' parameter ? Is there an idle enter function 
which does not correspond to the 'simple' scheme except omap3/4 ?

Maybe I am wrong but that looks a bit hacky because we are trying to 
override the functions the driver had previously defined and in order to 
prevent to modify the cpuidle.c core and more code.

I am wondering if it is possible to move the usual:

[ local_irq_disable(), getnstimeofday(before), myidle, 
getnstimeofday(after), local_irq_enable(), dev->last_residency = 
after-before, return index ]

to cpuidle.c/cpuidle_idle_call and wrap the
	entered_state = target_state->enter(dev, drv, next_state)
with these simple scheme.

Also I am not sure local_irq_disable is needed because AFAICT the idle 
function is called with the local_irq_disable. For example, the 
intel_idle driver does not do that and assume the enter_idle function is 
called with the local irq disabled.

Looking at the code :

arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable / 
local_irq_enable.

arch/x86/kernel/process_32/64.c : pm_idle is called with 
local_irq_disable but assumes the function will enable local irq

arch/ia64/kernel/process.c : the code assumes the idle function will 
disable/enable the local irq.

etc ...

It seems the code with the different arch is non consistent except there 
is a technical reason I don't know. May be making them consistent will 
help to factor out this part of the code and make the common framework 
more simple.

It is just a suggestion and IMO that could be done later on top of this 
patchset.


> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +		goto free_drv;
> +	}
> +
> +	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (common_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		if (driver_data_init)
> +			driver_data_init(dev);
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +uninit:
> +
> +	common_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +
> +	cpuidle_unregister_driver(drv);
> +
> +free_drv:
> +
> +	kfree(drv);
> +
> +	return ret;
> +}
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..2aa89db 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -56,6 +56,16 @@ struct cpuidle_state {
>
>   #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>
> +/* Common ARM WFI state */
> +#define CPUIDLE_ARM_WFI_STATE {\
> +	.enter                  = cpuidle_def_idle,\
> +	.exit_latency           = 2,\
> +	.target_residency       = 1,\
> +	.flags                  = CPUIDLE_FLAG_TIME_VALID,\
> +	.name                   = "WFI",\
> +	.desc                   = "ARM core clock gating (WFI)",\
> +}
> +
>   /**
>    * cpuidle_get_statedata - retrieves private driver state data
>    * @st_usage: the state usage statistics
> @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void);
>   extern int cpuidle_enable_device(struct cpuidle_device *dev);
>   extern void cpuidle_disable_device(struct cpuidle_device *dev);
>
> +/* provide a default idle function */
> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index);
> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev));
> +extern void common_cpuidle_devices_uninit(void);
> +
>   #else
>   static inline void disable_cpuidle(void) { }
>   static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { }
>   static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>   {return -ENODEV; }
>   static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
> +static inline int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index)
> +{return -ENODEV; }
> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
> +	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
> +{return -ENODEV; }
> +static inline void common_cpuidle_devices_uninit(void) { }
>
>   #endif
>
Rob Jan. 25, 2012, 2:03 a.m. | #7
Mike, thanks for your review.  Comments below.

On Tue, Jan 24, 2012 at 5:46 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote:
>> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
>> new file mode 100644
>> index 0000000..dafa758
>> --- /dev/null
>> +++ b/drivers/cpuidle/common.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Copyright 2011 Freescale Semiconductor, Inc.
>> + * Copyright 2011 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +/*
>> + * This code performs provides some commonly used cpuidle setup functionality
>
> s/performs //
>
>> +static struct cpuidle_device __percpu * common_cpuidle_devices;
>
> You can use DECLARE_PER_CPU here.

Ok.  Do you mean that DECLARE_PER_CPU is preferred in this case?

>
> Is there any particular reason to allocate these dynamically?  You can
> replace the code above with,
>
> static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices);
>

I was thinking of the single kernel with multiple platform support
case.  In that particular case, it seems better to create the number
of device objects you need at run time.

> I might change the variable name to "cpu_cpuidle_device" in that case
> since you are addressing a single CPU when using the per cpu accessor
> functions and "common_cpuidle_devices" sounds like an array or a list
> or something.  No big deal to keep the current name though.
>
>> +static int simple_enter(struct cpuidle_device *dev,
>> +                              struct cpuidle_driver *drv, int index)
>> +{
>> +       ktime_t time_start, time_end;
>> +
>> +       local_irq_disable();
>> +
>> +       time_start = ktime_get();
>> +
>> +       index = do_idle[index](dev, drv, index);
>> +
>> +       time_end = ktime_get();
>> +
>> +       local_irq_enable();
>> +
>> +       dev->last_residency =
>> +               (int)ktime_to_us(ktime_sub(time_end, time_start));
>
> Sometimes an attempt to enter some C-state fails and the do_idle will
> return immediately.  What do you think about having do_idle return
> -EERROR in this case and the conditionally setting last_residency to
> zero in those cases?  The point is that a C-state's total residency
> time should not increase in the case where the hardware did not
> successfully transition into that C-state.  I've observed many times
> where a specific low power state was actually achieved in the hardware
> but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps
> incrementing (albeit in very tiny increments).  Something like,
>
> if (IS_ERR(index))
>        dev->last_residency = 0;
> else
>        ...
>
> Note: I haven't been through the CPUidle core in a while so maybe the
> above suggestion violates some other requirements/assumptions...
>

Good suggestion.  I'll look into adding this to v4.

>> +
>> +       return index;
>> +}
>> +
>> +void common_cpuidle_devices_uninit(void)
>> +{
>> +       int cpu_id;
>> +       struct cpuidle_device *dev;
>> +
>> +       for_each_possible_cpu(cpu_id) {
>> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> +               cpuidle_unregister_device(dev);
>> +       }
>> +
>> +       free_percpu(common_cpuidle_devices);
>
> See note above about statically allocating the per-cpu variables.
>
>> +}
>> +
>> +/**
>> + * common_cpuidle_init() - Provides some commonly used init functionality.
>> + * @pdrv               Pointer to your cpuidle_driver object.
>> + * @simple             Use the common simple_enter wrapper?
>> + * @driver_data_init   Pointer to your platform function to initialize your
>> + *                     platform specific driver data.  Use NULL if platform
>> + *                     specific data is not needed.
>> + *
>> + * Common cpuidle init interface to provide common cpuidle functionality
>> + * used by many platforms.
>> + */
>> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
>> +                        void (*driver_data_init)(struct cpuidle_device *dev))
>> +{
>> +       struct cpuidle_device *dev;
>> +       struct cpuidle_driver *drv;
>> +       int i, cpu_id, ret;
>> +
>> +       if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
>> +               pr_err("%s: Invalid Input\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +
>> +       drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>> +       if (!drv) {
>> +               pr_err("%s: no memory for cpuidle driver\n", __func__);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       *drv = *pdrv;
>> +
>> +       for (i = 0; simple && (i < drv->state_count); i++) {
>> +               do_idle[i] = drv->states[i].enter;
>> +               drv->states[i].enter = simple_enter;
>> +       }
>> +
>> +       ret = cpuidle_register_driver(drv);
>> +       if (ret) {
>> +               pr_err("%s: Failed to register cpuidle driver\n", __func__);
>> +               goto free_drv;
>> +       }
>> +
>> +       common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> +       if (common_cpuidle_devices == NULL) {
>> +               ret = -ENOMEM;
>> +               goto unregister_drv;
>> +       }
>
> See note above about statically allocating these.
>
> Regards,
> Mike
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Jan. 25, 2012, 2:38 a.m. | #8
Daniel, thanks for your review.  I think you and Mike timed sending
your responses :)  Comments below.

On Tue, Jan 24, 2012 at 5:49 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 01/24/2012 05:37 AM, Robert Lee wrote:
>>
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.
>>
>> Signed-off-by: Robert Lee<rob.lee@linaro.org>
>> ---
>
>
> Hi Rob,
>
> nice work. The result is interesting. I have a few comments below.
>
>
>
>>  drivers/cpuidle/Makefile |    2 +-
>>  drivers/cpuidle/common.c |  152
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/cpuidle.h  |   24 +++++++
>>  3 files changed, 177 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/cpuidle/common.c
>>
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index 5634f88..2928d93 100644
>> --- a/drivers/cpuidle/Makefile
>> +++ b/drivers/cpuidle/Makefile
>> @@ -2,4 +2,4 @@
>>  # Makefile for cpuidle.
>>  #
>>
>> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
>> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
>> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
>> new file mode 100644
>> index 0000000..dafa758
>> --- /dev/null
>> +++ b/drivers/cpuidle/common.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Copyright 2011 Freescale Semiconductor, Inc.
>> + * Copyright 2011 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +/*
>> + * This code performs provides some commonly used cpuidle setup
>> functionality
>> + * used by many ARM SoC platforms.  Providing this functionality here
>> + * reduces the duplication of this code for each ARM platform that uses
>> it.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/io.h>
>> +#include<linux/cpuidle.h>
>> +#include<linux/hrtimer.h>
>> +#include<linux/err.h>
>> +#include<linux/slab.h>
>> +#include<asm/proc-fns.h>
>> +
>> +static struct cpuidle_device __percpu * common_cpuidle_devices;
>> +
>> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
>> +                              struct cpuidle_driver *drv, int index);
>> +
>> +int cpuidle_def_idle(struct cpuidle_device *dev,
>> +                              struct cpuidle_driver *drv, int index)
>> +{
>> +       cpu_do_idle();
>> +       return index;
>> +}
>> +
>> +static int simple_enter(struct cpuidle_device *dev,
>> +                              struct cpuidle_driver *drv, int index)
>> +{
>> +       ktime_t time_start, time_end;
>> +
>> +       local_irq_disable();
>> +
>> +       time_start = ktime_get();
>> +
>> +       index = do_idle[index](dev, drv, index);
>> +
>> +       time_end = ktime_get();
>> +
>> +       local_irq_enable();
>> +
>> +       dev->last_residency =
>> +               (int)ktime_to_us(ktime_sub(time_end, time_start));
>> +
>> +       return index;
>> +}
>> +
>> +void common_cpuidle_devices_uninit(void)
>> +{
>> +       int cpu_id;
>> +       struct cpuidle_device *dev;
>> +
>> +       for_each_possible_cpu(cpu_id) {
>> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> +               cpuidle_unregister_device(dev);
>> +       }
>> +
>> +       free_percpu(common_cpuidle_devices);
>> +}
>
>
> If the registering sequence aborts, won't cpuidle_unregister_device leads to
> a kernel warning as it could be specified with a cpu which has *not* been
> registered yet ?
>

I think you may have been looking at cpuidle_unregister_driver.  Here
is cpuidle_unregister_device which seems to handle a device not yet
registered ok:

void cpuidle_unregister_device(struct cpuidle_device *dev)
{
	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();

	if (dev->registered == 0)
		return;
...

> Perhaps we should pass the cpuid from where the cpu has failed an do a
> reverse unregister sequence.
>
> void common_cpuidle_devices_uninit(int cpu)
> {
>        for (cpu--; cpu >= 0; cpu--) {
>                device = &per_cpu(common_cpuidle_devices, cpu);
>                cpuidle_unregister_device(device);
>        }
> ...
>
>
>
>> +
>> +/**
>> + * common_cpuidle_init() - Provides some commonly used init
>> functionality.
>> + * @pdrv               Pointer to your cpuidle_driver object.
>> + * @simple             Use the common simple_enter wrapper?
>> + * @driver_data_init   Pointer to your platform function to initialize
>> your
>> + *                     platform specific driver data.  Use NULL if
>> platform
>> + *                     specific data is not needed.
>> + *
>> + * Common cpuidle init interface to provide common cpuidle functionality
>> + * used by many platforms.
>> + */
>> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
>> +                        void (*driver_data_init)(struct cpuidle_device
>> *dev))
>> +{
>> +       struct cpuidle_device *dev;
>> +       struct cpuidle_driver *drv;
>> +       int i, cpu_id, ret;
>> +
>> +       if (!pdrv || pdrv->state_count>  CPUIDLE_STATE_MAX) {
>> +               pr_err("%s: Invalid Input\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +
>> +       drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>> +       if (!drv) {
>> +               pr_err("%s: no memory for cpuidle driver\n", __func__);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       *drv = *pdrv;
>
>
> Rob can you explain why is needed to copy this structure ?
>

I made the original platform cpuidle_driver objects __initdata so I
need to copy over to the dynamically allocated structure.

> Maybe kmemdup is more adequate here.
>
> drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);
>

Is this preferred by the community over direct structure copies?  Or
is there some other advantage?

>> +
>> +       for (i = 0; simple&&  (i<  drv->state_count); i++) {
>>
>> +               do_idle[i] = drv->states[i].enter;
>> +               drv->states[i].enter = simple_enter;
>> +       }
>
>
> Do we really need a 'simple' parameter ? Is there an idle enter function
> which does not correspond to the 'simple' scheme except omap3/4 ?
>
> Maybe I am wrong but that looks a bit hacky because we are trying to
> override the functions the driver had previously defined and in order to
> prevent to modify the cpuidle.c core and more code.
>
> I am wondering if it is possible to move the usual:
>
> [ local_irq_disable(), getnstimeofday(before), myidle,
> getnstimeofday(after), local_irq_enable(), dev->last_residency =
> after-before, return index ]
>
> to cpuidle.c/cpuidle_idle_call and wrap the
>        entered_state = target_state->enter(dev, drv, next_state)
> with these simple scheme.
>

Yes, I considered the same thing and originally made a version of this
patch with direct changes to cpuidle_idle_call.  But I concluded that
since this common code's main purpose is just to consolidate code
duplication on *some* (but not all) cpuidle implementations, it was
better to create the extra simple_enter wrapper than to add additional
code in cpuidle_idle_call that other platforms don't need.  I'd be
happy to submit a version of this patch with cpuidle_idle_call changes
though and let the community decide.  If anyone else thinks this is a
good or bad idea, please give your input.

> Also I am not sure local_irq_disable is needed because AFAICT the idle
> function is called with the local_irq_disable. For example, the intel_idle
> driver does not do that and assume the enter_idle function is called with
> the local irq disabled.
>
> Looking at the code :
>
> arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable /
> local_irq_enable.
>
> arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable
> but assumes the function will enable local irq
>
> arch/ia64/kernel/process.c : the code assumes the idle function will
> disable/enable the local irq.
>
> etc ...
>

Agree.  I considered this as well but ultimately decided to leave it
in.  I can remove it for the next patch version though.

> It seems the code with the different arch is non consistent except there is
> a technical reason I don't know. May be making them consistent will help to
> factor out this part of the code and make the common framework more simple.
>
> It is just a suggestion and IMO that could be done later on top of this
> patchset.
>
>
>
>> +       ret = cpuidle_register_driver(drv);
>> +       if (ret) {
>> +               pr_err("%s: Failed to register cpuidle driver\n",
>> __func__);
>> +               goto free_drv;
>> +       }
>> +
>> +       common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> +       if (common_cpuidle_devices == NULL) {
>> +               ret = -ENOMEM;
>> +               goto unregister_drv;
>> +       }
>> +
>> +       /* initialize state data for each cpuidle_device */
>> +       for_each_possible_cpu(cpu_id) {
>> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> +               dev->cpu = cpu_id;
>> +               dev->state_count = drv->state_count;
>> +
>> +               if (driver_data_init)
>> +                       driver_data_init(dev);
>> +
>> +               ret = cpuidle_register_device(dev);
>> +               if (ret) {
>> +                       pr_err("%s: Failed to register cpu %u\n",
>> +                               __func__, cpu_id);
>> +                       goto uninit;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +uninit:
>> +
>> +       common_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> +
>> +       cpuidle_unregister_driver(drv);
>> +
>> +free_drv:
>> +
>> +       kfree(drv);
>> +
>> +       return ret;
>> +}
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 712abcc..2aa89db 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -56,6 +56,16 @@ struct cpuidle_state {
>>
>>  #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>>
>> +/* Common ARM WFI state */
>> +#define CPUIDLE_ARM_WFI_STATE {\
>> +       .enter                  = cpuidle_def_idle,\
>> +       .exit_latency           = 2,\
>> +       .target_residency       = 1,\
>> +       .flags                  = CPUIDLE_FLAG_TIME_VALID,\
>> +       .name                   = "WFI",\
>> +       .desc                   = "ARM core clock gating (WFI)",\
>> +}
>> +
>>  /**
>>   * cpuidle_get_statedata - retrieves private driver state data
>>   * @st_usage: the state usage statistics
>> @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void);
>>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
>>
>> +/* provide a default idle function */
>> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
>> +                      struct cpuidle_driver *drv, int index);
>> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
>> +                        void (*driver_data_init)(struct cpuidle_device
>> *dev));
>> +extern void common_cpuidle_devices_uninit(void);
>> +
>>  #else
>>  static inline void disable_cpuidle(void) { }
>>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
>> @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) {
>> }
>>  static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>>  {return -ENODEV; }
>>  static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
>> +static inline int cpuidle_def_idle(struct cpuidle_device *dev,
>> +                      struct cpuidle_driver *drv, int index)
>> +{return -ENODEV; }
>> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
>> +       bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
>> +{return -ENODEV; }
>> +static inline void common_cpuidle_devices_uninit(void) { }
>>
>>  #endif
>>
>
>
> --
>  <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
>
Daniel Lezcano Jan. 25, 2012, 2:52 p.m. | #9
On 01/25/2012 03:38 AM, Rob Lee wrote:
> Daniel, thanks for your review.  I think you and Mike timed sending
> your responses :)  Comments below.

[ ... ]

>>> +       for_each_possible_cpu(cpu_id) {
>>> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>>> +               cpuidle_unregister_device(dev);
>>> +       }
>>> +
>>> +       free_percpu(common_cpuidle_devices);
>>> +}
>>
>>
>> If the registering sequence aborts, won't cpuidle_unregister_device leads to
>> a kernel warning as it could be specified with a cpu which has *not* been
>> registered yet ?
>>
>
> I think you may have been looking at cpuidle_unregister_driver.  Here
> is cpuidle_unregister_device which seems to handle a device not yet
> registered ok:
>
> void cpuidle_unregister_device(struct cpuidle_device *dev)
> {
> 	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
> 	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
>
> 	if (dev->registered == 0)
> 		return;
> ...


Ok, it is harmless. I could have looked at that ... :)

>>> +
>>> +       drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>>> +       if (!drv) {
>>> +               pr_err("%s: no memory for cpuidle driver\n", __func__);
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       *drv = *pdrv;

[ ... ]

>> Rob can you explain why is needed to copy this structure ?
>>
>
> I made the original platform cpuidle_driver objects __initdata so I
> need to copy over to the dynamically allocated structure.

Yes, but why declare a static object to be freed and allocate a new one 
and copy it ? Why don't just use the pdrv parameter of the function ?

>> Maybe kmemdup is more adequate here.
>>
>> drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);
>>
>
> Is this preferred by the community over direct structure copies?  Or
> is there some other advantage?

It does kmalloc + memcpy. And *drv = *pdrv is converted to a memcpy by 
the compiler. So using kmemdup generates the same code as kmalloc + 
memcpy, or kmalloc + *drv = *pdrv

>>> +
>>> +       for (i = 0; simple&&    (i<    drv->state_count); i++) {
>>>
>>> +               do_idle[i] = drv->states[i].enter;
>>> +               drv->states[i].enter = simple_enter;
>>> +       }
>>
>>
>> Do we really need a 'simple' parameter ? Is there an idle enter function
>> which does not correspond to the 'simple' scheme except omap3/4 ?
>>
>> Maybe I am wrong but that looks a bit hacky because we are trying to
>> override the functions the driver had previously defined and in order to
>> prevent to modify the cpuidle.c core and more code.
>>
>> I am wondering if it is possible to move the usual:
>>
>> [ local_irq_disable(), getnstimeofday(before), myidle,
>> getnstimeofday(after), local_irq_enable(), dev->last_residency =
>> after-before, return index ]
>>
>> to cpuidle.c/cpuidle_idle_call and wrap the
>>         entered_state = target_state->enter(dev, drv, next_state)
>> with these simple scheme.
>>
>
> Yes, I considered the same thing and originally made a version of this
> patch with direct changes to cpuidle_idle_call.  But I concluded that
> since this common code's main purpose is just to consolidate code
> duplication on *some* (but not all) cpuidle implementations, it was
> better to create the extra simple_enter wrapper than to add additional
> code in cpuidle_idle_call that other platforms don't need.  I'd be
> happy to submit a version of this patch with cpuidle_idle_call changes
> though and let the community decide.  If anyone else thinks this is a
> good or bad idea, please give your input.

[1]

>> Also I am not sure local_irq_disable is needed because AFAICT the idle
>> function is called with the local_irq_disable. For example, the intel_idle
>> driver does not do that and assume the enter_idle function is called with
>> the local irq disabled.
>>
>> Looking at the code :
>>
>> arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable /
>> local_irq_enable.
>>
>> arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable
>> but assumes the function will enable local irq
>>
>> arch/ia64/kernel/process.c : the code assumes the idle function will
>> disable/enable the local irq.
>>
>> etc ...
>>
>
> Agree.  I considered this as well but ultimately decided to leave it
> in.  I can remove it for the next patch version though.

IMHO, we should wait for what inputs we have in [1]

Thanks
   -- Daniel

Patch

diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 5634f88..2928d93 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -2,4 +2,4 @@ 
 # Makefile for cpuidle.
 #
 
-obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
new file mode 100644
index 0000000..dafa758
--- /dev/null
+++ b/drivers/cpuidle/common.c
@@ -0,0 +1,152 @@ 
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/*
+ * This code performs provides some commonly used cpuidle setup functionality
+ * used by many ARM SoC platforms.  Providing this functionality here
+ * reduces the duplication of this code for each ARM platform that uses it.
+ */
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/cpuidle.h>
+#include <linux/hrtimer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <asm/proc-fns.h>
+
+static struct cpuidle_device __percpu * common_cpuidle_devices;
+
+static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index);
+
+int cpuidle_def_idle(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
+{
+	cpu_do_idle();
+	return index;
+}
+
+static int simple_enter(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
+{
+	ktime_t time_start, time_end;
+
+	local_irq_disable();
+
+	time_start = ktime_get();
+
+	index = do_idle[index](dev, drv, index);
+
+	time_end = ktime_get();
+
+	local_irq_enable();
+
+	dev->last_residency =
+		(int)ktime_to_us(ktime_sub(time_end, time_start));
+
+	return index;
+}
+
+void common_cpuidle_devices_uninit(void)
+{
+	int cpu_id;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(common_cpuidle_devices);
+}
+
+/**
+ * common_cpuidle_init() - Provides some commonly used init functionality.
+ * @pdrv		Pointer to your cpuidle_driver object.
+ * @simple		Use the common simple_enter wrapper?
+ * @driver_data_init	Pointer to your platform function to initialize your
+ *			platform specific driver data.  Use NULL if platform
+ *			specific data is not needed.
+ *
+ * Common cpuidle init interface to provide common cpuidle functionality
+ * used by many platforms.
+ */
+int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
+			 void (*driver_data_init)(struct cpuidle_device *dev))
+{
+	struct cpuidle_device *dev;
+	struct cpuidle_driver *drv;
+	int i, cpu_id, ret;
+
+	if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
+		pr_err("%s: Invalid Input\n", __func__);
+		return -EINVAL;
+	}
+
+	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
+	if (!drv) {
+		pr_err("%s: no memory for cpuidle driver\n", __func__);
+		return -ENOMEM;
+	}
+
+	*drv = *pdrv;
+
+	for (i = 0; simple && (i < drv->state_count); i++) {
+		do_idle[i] = drv->states[i].enter;
+		drv->states[i].enter = simple_enter;
+	}
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("%s: Failed to register cpuidle driver\n", __func__);
+		goto free_drv;
+	}
+
+	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (common_cpuidle_devices == NULL) {
+		ret = -ENOMEM;
+		goto unregister_drv;
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
+		dev->cpu = cpu_id;
+		dev->state_count = drv->state_count;
+
+		if (driver_data_init)
+			driver_data_init(dev);
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("%s: Failed to register cpu %u\n",
+				__func__, cpu_id);
+			goto uninit;
+		}
+	}
+
+	return 0;
+uninit:
+
+	common_cpuidle_devices_uninit();
+
+unregister_drv:
+
+	cpuidle_unregister_driver(drv);
+
+free_drv:
+
+	kfree(drv);
+
+	return ret;
+}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..2aa89db 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -56,6 +56,16 @@  struct cpuidle_state {
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
+/* Common ARM WFI state */
+#define CPUIDLE_ARM_WFI_STATE {\
+	.enter                  = cpuidle_def_idle,\
+	.exit_latency           = 2,\
+	.target_residency       = 1,\
+	.flags                  = CPUIDLE_FLAG_TIME_VALID,\
+	.name                   = "WFI",\
+	.desc                   = "ARM core clock gating (WFI)",\
+}
+
 /**
  * cpuidle_get_statedata - retrieves private driver state data
  * @st_usage: the state usage statistics
@@ -141,6 +151,13 @@  extern void cpuidle_resume_and_unlock(void);
 extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 
+/* provide a default idle function */
+extern int cpuidle_def_idle(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index);
+extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
+			 void (*driver_data_init)(struct cpuidle_device *dev));
+extern void common_cpuidle_devices_uninit(void);
+
 #else
 static inline void disable_cpuidle(void) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
@@ -157,6 +174,13 @@  static inline void cpuidle_resume_and_unlock(void) { }
 static inline int cpuidle_enable_device(struct cpuidle_device *dev)
 {return -ENODEV; }
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
+static inline int cpuidle_def_idle(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index)
+{return -ENODEV; }
+static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
+	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
+{return -ENODEV; }
+static inline void common_cpuidle_devices_uninit(void) { }
 
 #endif