[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Herring]

On 12/08/2011 03:46 PM, Rob Lee wrote:
> Rob, thanks for your thorough review.  Comments and questions below.
> 
> On 6 December 2011 09:06, Rob Herring <robherring2 at gmail.com> wrote:
>> On 12/05/2011 10:38 PM, Robert Lee wrote:
>>> Add commonly used cpuidle init code to avoid unecessary duplication.
>>>
>>> Signed-off-by: Robert Lee <rob.lee at linaro.org>
>>> ---
>>>  arch/arm/common/Makefile       |    1 +
>>>  arch/arm/common/cpuidle.c      |  132 ++++++++++++++++++++++++++++++++++++++++
>>>  arch/arm/include/asm/cpuidle.h |   25 ++++++++
>>
>> Why not move cpuidle drivers to drivers/idle/ ?
>>
> 
> Or to drivers/cpuidle?  I am not sure the reasoning behind a

It would make sense to me that they should be combined. I'm not sure of
the history here.

> drivers/idle directory if everything there is a cpuidle driver
> implementation.  I originally did locate this common cpuidle code in
> drivers/idle/arm_idle.c and put the head file in arch/arm/include/asm
> but this threw a checkpatch warning so I submitted with this placement

What warning?

> to start with.  If the local_fiq_enable/disable calls can be handled
> in a community friendly way for any architecture, then perhaps I can
> just move the header file code to linux/include/cpuidle.h.

Maybe we should think about whether we even need to disable fiq. You
probably don't use low latency interrupt and a high latency low power
mode together.

> Do you have suggestions about making this functionality available for
> any architecture and what is the most community friendly method of
> doing this?  I suppose function declarations for
> local_fiq_enable/disable could be given.  Then, one could either
> define them here as empty functions or could have two idle functions,
> arm_enter_idle and nonarm_enter_idle and the architecture could be
> read or passed in to determine which one to set the cpuidle states'
> enter functions to.

I'm not sure I understand the issue. You can include asm headers and
make things depend on CONFIG_ARM so no other arch builds code with
local_fiq_enable.

The other approach would be to define arch specific cpu_idle functions
for pre and post idle.

>>> +static int (*mach_cpuidle)(struct cpuidle_device *dev,
>>> +                            struct cpuidle_driver *drv,
>>> +                             int index);
>>> +
>>> +static int arm_enter_idle(struct cpuidle_device *dev,
>>> +                            struct cpuidle_driver *drv, int index)
>>
>> I think how this works is backwards. This function is only called if
>> there is a NULL enter function for a state, but mach_cpuidle is global.
>> Ideally, I want to call this function for every state, but call a
>> different mach_cpuidle function for each state. Yes, you could select a
>> specific function for each state within the mach_cpuidle function, but
>> that seems silly to make the per state function global and then have to
>> select a per state function again. And if many users are doing that,
>> then it belongs in the common code.


No comments on this!?


>>> +{
>>> +     ktime_t time_start, time_end;
>>> +
>>> +     local_irq_disable();
>>> +     local_fiq_disable();
>>> +
>>> +     time_start = ktime_get();
>>> +
>>> +     index = mach_cpuidle(dev, drv, index);
>>> +
>>> +     time_end = ktime_get();
>>> +
>>> +     local_fiq_enable();
>>> +     local_irq_enable();
>>> +
>>> +     dev->last_residency =
>>> +             (int)ktime_to_us(ktime_sub(time_end, time_start));
>>> +
>>> +     return index;
>>> +}
>>> +
>>> +void arm_cpuidle_devices_uninit(void)
>>> +{
>>> +     int cpu_id;
>>> +     struct cpuidle_device *dev;
>>> +
>>> +     for_each_possible_cpu(cpu_id) {
>>> +             dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>> +             cpuidle_unregister_device(dev);
>>> +     }
>>> +
>>> +     free_percpu(arm_cpuidle_devices);
>>> +     return;
>>
>> Don't need return statement.
>>
>>> +}
>>> +
>>> +int __init arm_cpuidle_init(struct cpuidle_driver *drv,
>>> +     int (*common_enter)(struct cpuidle_device *dev,
>>> +             struct cpuidle_driver *drv, int index),
>>> +     void *driver_data[])
>>> +{
>>> +     struct cpuidle_device *dev;
>>> +     int i, cpu_id;
>>> +
>>> +     if (drv == NULL) {
>>> +             pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     if (drv->state_count > CPUIDLE_STATE_MAX)
>>> +             pr_err("%s: state count exceeds maximum\n", __func__);
>>
>> return an error?
>>
>> You can collapse these 2 parameter checks down to:
>>
>> if (!drv || (drv->state_count > CPUIDLE_STATE_MAX))
>>
>>> +
>>> +     mach_cpuidle = common_enter;
>>> +
>>> +     /* if state enter function not specified, use common_enter function */
>>> +     for (i = 0; i < drv->state_count; i++) {
>>> +             if (drv->states[i].enter == NULL) {
>>> +                     if (mach_cpuidle == NULL) {
>>
>> Usually !mach_cpuidle is preferred for NULL checks. You can move this
>> check out of the loop.
>>
>>> +                             pr_err("%s: 'enter' function pointer NULL\n",
>>> +                             __func__);
>>> +                             return -EINVAL;
>>> +                     } else
>>> +                             drv->states[i].enter = arm_enter_idle;
>>> +             }
>>> +     }
>>> +
>>> +     if (cpuidle_register_driver(drv)) {
>>> +             pr_err("%s: Failed to register cpuidle driver\n", __func__);
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>>> +     if (arm_cpuidle_devices == NULL) {
>>> +             cpuidle_unregister_driver(drv);
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     /* initialize state data for each cpuidle_device */
>>> +     for_each_possible_cpu(cpu_id) {
>>> +
>>> +             dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>> +             dev->cpu = cpu_id;
>>> +             dev->state_count = drv->state_count;
>>> +
>>> +             if (driver_data)
>>> +                     for (i = 0; i < dev->state_count; i++) {
>>
>> This would be more concise and less indentation:
>>
>> for (i = 0; driver_data, i < dev->state_count; i++)
>>
>>> +                             dev->states_usage[i].driver_data =
>>> +                                                     driver_data[i];
>>> +                     }
>>> +
>>> +             if (cpuidle_register_device(dev)) {
>>> +                     pr_err("%s: Failed to register cpu %u\n",
>>> +                             __func__, cpu_id);
> 
> arm_cpuidle_devices_uninit();
> 
>>> +                     return -ENODEV;
>>
>> Leaking memory here from alloc_percpu.
>>
>> Also, need to unregister driver. It's usually cleaner to use goto's for
>> error clean-up.
>>
> 
> In this case, just adding a call  to arm_cpuidle_devices_uninit()
> seems clean right?
> 
Really you should only undo what you have setup. In most cases uninit
functions just uninit everything unconditionally. This gets a bit messy
when you have loops that can fail.

arm_cpuidle_devices_uninit is not unregistering the driver, so that
needs fixing as well.

Rob