Re: [PATCH] cpuidle: reinitialize power_usage values when adding/removing C-states

2012-10-18 Thread Daniel Lezcano 
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

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpuidle: reinitialize power_usage values when adding/removing C-states

2012-10-17 Thread Julius Werner 
> 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.

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

Let me know what you think.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpuidle: reinitialize power_usage values when adding/removing C-states

2012-10-17 Thread Daniel Lezcano 
On 10/17/2012 12:31 PM, Daniel Lezcano wrote:
> On 10/17/2012 12:39 AM, Julius Werner wrote:
>> When cpuidle drivers do not supply explicit power_usage values,
>> cpuidle/driver.c inserts dummy values instead. When a running processor
>> dynamically gains new C-states (e.g. after ACPI events), the power_usage
>> values of those states will stay uninitialized, and cpuidle governors
>> will never choose to enter them.
>>
>> This patch moves the dummy value initialization from
>> cpuidle_register_driver to cpuidle_enable_device, which drivers such as
>> acpi/processor_idle.c will call again when they add or remove C-states.
>> Tested and verified on an Acer AC700 Chromebook, which supports the C3
>> state only when it switches from AC to battery power.
>>
>> Signed-off-by: Julius Werner 
>> ---
> 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 ?

btw, good catch ! :)



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpuidle: reinitialize power_usage values when adding/removing C-states

2012-10-17 Thread Daniel Lezcano 
On 10/17/2012 12:39 AM, Julius Werner wrote:
> When cpuidle drivers do not supply explicit power_usage values,
> cpuidle/driver.c inserts dummy values instead. When a running processor
> dynamically gains new C-states (e.g. after ACPI events), the power_usage
> values of those states will stay uninitialized, and cpuidle governors
> will never choose to enter them.
> 
> This patch moves the dummy value initialization from
> cpuidle_register_driver to cpuidle_enable_device, which drivers such as
> acpi/processor_idle.c will call again when they add or remove C-states.
> Tested and verified on an Acer AC700 Chromebook, which supports the C3
> state only when it switches from AC to battery power.
> 
> Signed-off-by: Julius Werner 
> ---

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 ?




>  drivers/cpuidle/cpuidle.c |   24 
>  drivers/cpuidle/driver.c  |   25 -
>  2 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..bef3a31 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -298,6 +298,28 @@ static void poll_idle_init(struct cpuidle_driver *drv)
>  static void poll_idle_init(struct cpuidle_driver *drv) {}
>  #endif /* CONFIG_ARCH_HAS_CPU_RELAX */
>  
> +static void set_power_states(struct cpuidle_driver *drv)
> +{
> + int i;
> +
> + /*
> +  * cpuidle driver should set the drv->power_specified bit
> +  * before registering if the driver provides
> +  * power_usage numbers.
> +  *
> +  * If power_specified is not set,
> +  * we fill in power_usage with decreasing values as the
> +  * cpuidle code has an implicit assumption that state Cn
> +  * uses less power than C(n-1).
> +  *
> +  * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
> +  * an power value of -1.  So we use -2, -3, etc, for other
> +  * c-states.
> +  */
> + for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
> + drv->states[i].power_usage = -1 - i;
> +}
> +
>  /**
>   * cpuidle_enable_device - enables idle PM for a CPU
>   * @dev: the CPU
> @@ -330,6 +352,8 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
>   cpuidle_enter_tk : cpuidle_enter;
>  
>   poll_idle_init(drv);
> + if (!drv->power_specified)
> + set_power_states(drv);
>  
>   if ((ret = cpuidle_add_state_sysfs(dev)))
>   return ret;
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 87db387..caaed27 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -18,28 +18,6 @@ static struct cpuidle_driver *cpuidle_curr_driver;
>  DEFINE_SPINLOCK(cpuidle_driver_lock);
>  int cpuidle_driver_refcount;
>  
> -static void set_power_states(struct cpuidle_driver *drv)
> -{
> - int i;
> -
> - /*
> -  * cpuidle driver should set the drv->power_specified bit
> -  * before registering if the driver provides
> -  * power_usage numbers.
> -  *
> -  * If power_specified is not set,
> -  * we fill in power_usage with decreasing values as the
> -  * cpuidle code has an implicit assumption that state Cn
> -  * uses less power than C(n-1).
> -  *
> -  * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
> -  * an power value of -1.  So we use -2, -3, etc, for other
> -  * c-states.
> -  */
> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
> - drv->states[i].power_usage = -1 - i;
> -}
> -
>  /**
>   * cpuidle_register_driver - registers a driver
>   * @drv: the driver
> @@ -58,9 +36,6 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
>   return -EBUSY;
>   }
>  
> - if (!drv->power_specified)
> - set_power_states(drv);
> -
>   cpuidle_curr_driver = drv;
>  
>   spin_unlock(&cpuidle_driver_lock);


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] cpuidle: reinitialize power_usage values when adding/removing C-states

2012-10-16 Thread Julius Werner 
When cpuidle drivers do not supply explicit power_usage values,
cpuidle/driver.c inserts dummy values instead. When a running processor
dynamically gains new C-states (e.g. after ACPI events), the power_usage
values of those states will stay uninitialized, and cpuidle governors
will never choose to enter them.

This patch moves the dummy value initialization from
cpuidle_register_driver to cpuidle_enable_device, which drivers such as
acpi/processor_idle.c will call again when they add or remove C-states.
Tested and verified on an Acer AC700 Chromebook, which supports the C3
state only when it switches from AC to battery power.

Signed-off-by: Julius Werner 
---
 drivers/cpuidle/cpuidle.c |   24 
 drivers/cpuidle/driver.c  |   25 -
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f15b85..bef3a31 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -298,6 +298,28 @@ static void poll_idle_init(struct cpuidle_driver *drv)
 static void poll_idle_init(struct cpuidle_driver *drv) {}
 #endif /* CONFIG_ARCH_HAS_CPU_RELAX */
 
+static void set_power_states(struct cpuidle_driver *drv)
+{
+   int i;
+
+   /*
+* cpuidle driver should set the drv->power_specified bit
+* before registering if the driver provides
+* power_usage numbers.
+*
+* If power_specified is not set,
+* we fill in power_usage with decreasing values as the
+* cpuidle code has an implicit assumption that state Cn
+* uses less power than C(n-1).
+*
+* With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
+* an power value of -1.  So we use -2, -3, etc, for other
+* c-states.
+*/
+   for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
+   drv->states[i].power_usage = -1 - i;
+}
+
 /**
  * cpuidle_enable_device - enables idle PM for a CPU
  * @dev: the CPU
@@ -330,6 +352,8 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
cpuidle_enter_tk : cpuidle_enter;
 
poll_idle_init(drv);
+   if (!drv->power_specified)
+   set_power_states(drv);
 
if ((ret = cpuidle_add_state_sysfs(dev)))
return ret;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 87db387..caaed27 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -18,28 +18,6 @@ static struct cpuidle_driver *cpuidle_curr_driver;
 DEFINE_SPINLOCK(cpuidle_driver_lock);
 int cpuidle_driver_refcount;
 
-static void set_power_states(struct cpuidle_driver *drv)
-{
-   int i;
-
-   /*
-* cpuidle driver should set the drv->power_specified bit
-* before registering if the driver provides
-* power_usage numbers.
-*
-* If power_specified is not set,
-* we fill in power_usage with decreasing values as the
-* cpuidle code has an implicit assumption that state Cn
-* uses less power than C(n-1).
-*
-* With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
-* an power value of -1.  So we use -2, -3, etc, for other
-* c-states.
-*/
-   for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
-   drv->states[i].power_usage = -1 - i;
-}
-
 /**
  * cpuidle_register_driver - registers a driver
  * @drv: the driver
@@ -58,9 +36,6 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
return -EBUSY;
}
 
-   if (!drv->power_specified)
-   set_power_states(drv);
-
cpuidle_curr_driver = drv;
 
spin_unlock(&cpuidle_driver_lock);
-- 
1.7.8.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/