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

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.

[ ... ]

quoted

quoted

+       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 ... :)

quoted

quoted

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

[ ... ]

quoted

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 ?

quoted

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

quoted

quoted

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

quoted

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