On 10/17/2012 08:43 PM, Julius Werner wrote:
This is specific to the acpi and should be handled in the processor_idle.c file instead of the cpuidle core code.
Could be the function 'acpi_processor_cst_has_changed' the right place to set a dummy power value for the power in the new C-state ?
Thanks for your feedback. I think it wouldn't be wise to split the dummy power value logic over two places, but I could submit a patch that makes set_power_states globally accessible and calls it from acpi_processor_cst_has_changed instead.
No please, do not export this function. That will add more confusion on the acpi code and more generally in the cpuidle core code.
IIUC, a new state is inserted/deleted and we will set the entire array of states to setup the power.
You have the acpi_processor_cst_has_changed function calling the acpi_processor_setup_cpuidle_states function. It seems to be a good candidate to setup the power of the new state. All the states are filled again AFAICS under the lock.
However, I do not think this should really be ACPI specific. It applies to any cpuidle driver that wants to change its idle states at runtime. Currently only the ACPI one does, but the future might bring others that would run into the same problem. I also think that set_power_states fits much better into cpuidle_enable_device conceptually anyway (right next to poll_idle_init which also does state initialization).
The states are now part of the cpuidle driver and the set_power_states should remain to this file. The dynamic C-states brought some complexity in the acpi code and honestly this code is very confusing.
Maybe one day, that would make sense but until then I am in favor of keeping the arch specific bits in the drivers, especially when they are *hum* so "complex".
poll_idle_init looks hackish for me and probably move it to the arch would also make sense.
Thanks -- Daniel