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

[Turquette, Mike]

On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee at 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