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

[Daniel Lezcano]

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

-- 
  <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