[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code
Rob Lee
rob.lee at linaro.org
Fri Dec 9 10:55:31 EST 2011
On 9 December 2011 07:54, Rob Herring <robherring2 at gmail.com> wrote:
> 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?
>
I'll move things back to drivers/idle and try it again and send the
specific warning later today.
>> 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!?
>
>
I agree with your viewpoint and will fix this in v2. The same goes
for all of your other comments that I made no response to. Should I
post my proposed fix here before submitting v2?
>>>> +{
>>>> + 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
More information about the linux-arm-kernel
mailing list