Re: [PATCH V2] PM / Domains: Remove cpuidle attach

[Rafael J. Wysocki]

On Tuesday, September 01, 2015 08:37:49 PM Daniel Lezcano wrote:
> The power domains code allows to tie a cpuidle state with a power domain.
> 
> Preventing the cpuidle framework to enter a specific idle state by disabling
> from the power domain framework is a good idea. Unfortunately, the current
> implementation has some gaps with a SMP system and a complex cpuidle
> implementation. Enabling a power domain wakes up all the cpus even if a cpu
> does not belong to the power domain.
> 
> There is some work to do a logical representation with the power domains of
> the hardware dependencies (eg. a cpu belongs to a power domains, these power
> domains belong to a higher power domain for a cluster, etc ...). A new code
> relying on the genpd hierarchy to disable the idle states would make more
> sense.
> 
> As the unique user of this code has been removed, let's wipe out this code
> to prevent new user and to have a clean place to put a new implementation.
> 
> Signed-off-by: Daniel Lezcano 

What tree is this supposed to apply to?


> ---
>  drivers/base/power/domain.c | 154 
> 
>  include/linux/pm_domain.h   |  27 
>  2 files changed, 181 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0ee43c1..f468627 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -171,19 +171,6 @@ static void genpd_set_active(struct generic_pm_domain 
> *genpd)
>   genpd->status = GPD_STATE_ACTIVE;
>  }
>  
> -static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
> -{
> - s64 usecs64;
> -
> - if (!genpd->cpuidle_data)
> - return;
> -
> - usecs64 = genpd->power_on_latency_ns;
> - do_div(usecs64, NSEC_PER_USEC);
> - usecs64 += genpd->cpuidle_data->saved_exit_latency;
> - genpd->cpuidle_data->idle_state->exit_latency = usecs64;
> -}
> -
>  static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>   ktime_t time_start;
> @@ -207,7 +194,6 @@ static int genpd_power_on(struct generic_pm_domain 
> *genpd, bool timed)
>  
>   genpd->power_on_latency_ns = elapsed_ns;
>   genpd->max_off_time_changed = true;
> - genpd_recalc_cpu_exit_latency(genpd);
>   pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>genpd->name, "on", elapsed_ns);
>  
> @@ -280,13 +266,6 @@ static int __pm_genpd_poweron(struct generic_pm_domain 
> *genpd)
>   return 0;
>   }
>  
> - if (genpd->cpuidle_data) {
> - cpuidle_pause_and_lock();
> - genpd->cpuidle_data->idle_state->disabled = true;
> - cpuidle_resume_and_unlock();
> - goto out;
> - }
> -
>   /*
>* The list is guaranteed not to change while the loop below is being
>* executed, unless one of the masters' .power_on() callbacks fiddles
> @@ -595,21 +574,6 @@ static int pm_genpd_poweroff(struct generic_pm_domain 
> *genpd)
>   }
>   }
>  
> - if (genpd->cpuidle_data) {
> - /*
> -  * If cpuidle_data is set, cpuidle should turn the domain off
> -  * when the CPU in it is idle.  In that case we don't decrement
> -  * the subdomain counts of the master domains, so that power is
> -  * not removed from the current domain prematurely as a result
> -  * of cutting off the masters' power.
> -  */
> - genpd->status = GPD_STATE_POWER_OFF;
> - cpuidle_pause_and_lock();
> - genpd->cpuidle_data->idle_state->disabled = false;
> - cpuidle_resume_and_unlock();
> - goto out;
> - }
> -
>   if (genpd->power_off) {
>   if (atomic_read(>sd_count) > 0) {
>   ret = -EBUSY;
> @@ -1725,124 +1689,6 @@ int pm_genpd_remove_subdomain(struct 
> generic_pm_domain *genpd,
>   return ret;
>  }
>  
> -/**
> - * pm_genpd_attach_cpuidle - Connect the given PM domain with cpuidle.
> - * @genpd: PM domain to be connected with cpuidle.
> - * @state: cpuidle state this domain can disable/enable.
> - *
> - * Make a PM domain behave as though it contained a CPU core, that is, 
> instead
> - * of calling its power down routine it will enable the given cpuidle state 
> so
> - * that the cpuidle subsystem can power it down (if possible and desirable).
> - */
> -int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
> -{
> - struct cpuidle_driver *cpuidle_drv;
> - struct gpd_cpuidle_data *cpuidle_data;
> - struct cpuidle_state *idle_state;
> - int ret = 0;
> -
> - if (IS_ERR_OR_NULL(genpd) || state < 0)
> - return -EINVAL;
> -
> - genpd_acquire_lock(genpd);
> -
> - if (genpd->cpuidle_data) {
> - ret = -EEXIST;
> - goto out;
> - }
> - cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> - if 

Re: [PATCH V2] PM / Domains: Remove cpuidle attach

[Rafael J. Wysocki]

On Tuesday, September 01, 2015 08:37:49 PM Daniel Lezcano wrote:
> The power domains code allows to tie a cpuidle state with a power domain.
> 
> Preventing the cpuidle framework to enter a specific idle state by disabling
> from the power domain framework is a good idea. Unfortunately, the current
> implementation has some gaps with a SMP system and a complex cpuidle
> implementation. Enabling a power domain wakes up all the cpus even if a cpu
> does not belong to the power domain.
> 
> There is some work to do a logical representation with the power domains of
> the hardware dependencies (eg. a cpu belongs to a power domains, these power
> domains belong to a higher power domain for a cluster, etc ...). A new code
> relying on the genpd hierarchy to disable the idle states would make more
> sense.
> 
> As the unique user of this code has been removed, let's wipe out this code
> to prevent new user and to have a clean place to put a new implementation.
> 
> Signed-off-by: Daniel Lezcano 

What tree is this supposed to apply to?


> ---
>  drivers/base/power/domain.c | 154 
> 
>  include/linux/pm_domain.h   |  27 
>  2 files changed, 181 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0ee43c1..f468627 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -171,19 +171,6 @@ static void genpd_set_active(struct generic_pm_domain 
> *genpd)
>   genpd->status = GPD_STATE_ACTIVE;
>  }
>  
> -static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
> -{
> - s64 usecs64;
> -
> - if (!genpd->cpuidle_data)
> - return;
> -
> - usecs64 = genpd->power_on_latency_ns;
> - do_div(usecs64, NSEC_PER_USEC);
> - usecs64 += genpd->cpuidle_data->saved_exit_latency;
> - genpd->cpuidle_data->idle_state->exit_latency = usecs64;
> -}
> -
>  static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>   ktime_t time_start;
> @@ -207,7 +194,6 @@ static int genpd_power_on(struct generic_pm_domain 
> *genpd, bool timed)
>  
>   genpd->power_on_latency_ns = elapsed_ns;
>   genpd->max_off_time_changed = true;
> - genpd_recalc_cpu_exit_latency(genpd);
>   pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>genpd->name, "on", elapsed_ns);
>  
> @@ -280,13 +266,6 @@ static int __pm_genpd_poweron(struct generic_pm_domain 
> *genpd)
>   return 0;
>   }
>  
> - if (genpd->cpuidle_data) {
> - cpuidle_pause_and_lock();
> - genpd->cpuidle_data->idle_state->disabled = true;
> - cpuidle_resume_and_unlock();
> - goto out;
> - }
> -
>   /*
>* The list is guaranteed not to change while the loop below is being
>* executed, unless one of the masters' .power_on() callbacks fiddles
> @@ -595,21 +574,6 @@ static int pm_genpd_poweroff(struct generic_pm_domain 
> *genpd)
>   }
>   }
>  
> - if (genpd->cpuidle_data) {
> - /*
> -  * If cpuidle_data is set, cpuidle should turn the domain off
> -  * when the CPU in it is idle.  In that case we don't decrement
> -  * the subdomain counts of the master domains, so that power is
> -  * not removed from the current domain prematurely as a result
> -  * of cutting off the masters' power.
> -  */
> - genpd->status = GPD_STATE_POWER_OFF;
> - cpuidle_pause_and_lock();
> - genpd->cpuidle_data->idle_state->disabled = false;
> - cpuidle_resume_and_unlock();
> - goto out;
> - }
> -
>   if (genpd->power_off) {
>   if (atomic_read(>sd_count) > 0) {
>   ret = -EBUSY;
> @@ -1725,124 +1689,6 @@ int pm_genpd_remove_subdomain(struct 
> generic_pm_domain *genpd,
>   return ret;
>  }
>  
> -/**
> - * pm_genpd_attach_cpuidle - Connect the given PM domain with cpuidle.
> - * @genpd: PM domain to be connected with cpuidle.
> - * @state: cpuidle state this domain can disable/enable.
> - *
> - * Make a PM domain behave as though it contained a CPU core, that is, 
> instead
> - * of calling its power down routine it will enable the given cpuidle state 
> so
> - * that the cpuidle subsystem can power it down (if possible and desirable).
> - */
> -int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
> -{
> - struct cpuidle_driver *cpuidle_drv;
> - struct gpd_cpuidle_data *cpuidle_data;
> - struct cpuidle_state *idle_state;
> - int ret = 0;
> -
> - if (IS_ERR_OR_NULL(genpd) || state < 0)
> - return -EINVAL;
> -
> - genpd_acquire_lock(genpd);
> -
> - if (genpd->cpuidle_data) {
> - ret = -EEXIST;
> - goto out;
> - }
> - cpuidle_data = kzalloc(sizeof(*cpuidle_data), 

Re: [PATCH V2] PM / Domains: Remove cpuidle attach

[Kevin Hilman]

Daniel Lezcano  writes:

> The power domains code allows to tie a cpuidle state with a power domain.
>
> Preventing the cpuidle framework to enter a specific idle state by disabling
> from the power domain framework is a good idea. Unfortunately, the current
> implementation has some gaps with a SMP system and a complex cpuidle
> implementation. Enabling a power domain wakes up all the cpus even if a cpu
> does not belong to the power domain.
>
> There is some work to do a logical representation with the power domains of
> the hardware dependencies (eg. a cpu belongs to a power domains, these power
> domains belong to a higher power domain for a cluster, etc ...). A new code
> relying on the genpd hierarchy to disable the idle states would make more
> sense.
>
> As the unique user of this code has been removed, let's wipe out this code
> to prevent new user and to have a clean place to put a new implementation.
>
> Signed-off-by: Daniel Lezcano 

Acked-by: Kevin Hilman 
--
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 V2] PM / Domains: Remove cpuidle attach

[Daniel Lezcano]

The power domains code allows to tie a cpuidle state with a power domain.

Preventing the cpuidle framework to enter a specific idle state by disabling
from the power domain framework is a good idea. Unfortunately, the current
implementation has some gaps with a SMP system and a complex cpuidle
implementation. Enabling a power domain wakes up all the cpus even if a cpu
does not belong to the power domain.

There is some work to do a logical representation with the power domains of
the hardware dependencies (eg. a cpu belongs to a power domains, these power
domains belong to a higher power domain for a cluster, etc ...). A new code
relying on the genpd hierarchy to disable the idle states would make more
sense.

As the unique user of this code has been removed, let's wipe out this code
to prevent new user and to have a clean place to put a new implementation.

Signed-off-by: Daniel Lezcano 
---
 drivers/base/power/domain.c | 154 
 include/linux/pm_domain.h   |  27 
 2 files changed, 181 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0ee43c1..f468627 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -171,19 +171,6 @@ static void genpd_set_active(struct generic_pm_domain 
*genpd)
genpd->status = GPD_STATE_ACTIVE;
 }
 
-static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
-{
-   s64 usecs64;
-
-   if (!genpd->cpuidle_data)
-   return;
-
-   usecs64 = genpd->power_on_latency_ns;
-   do_div(usecs64, NSEC_PER_USEC);
-   usecs64 += genpd->cpuidle_data->saved_exit_latency;
-   genpd->cpuidle_data->idle_state->exit_latency = usecs64;
-}
-
 static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
ktime_t time_start;
@@ -207,7 +194,6 @@ static int genpd_power_on(struct generic_pm_domain *genpd, 
bool timed)
 
genpd->power_on_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
-   genpd_recalc_cpu_exit_latency(genpd);
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
 genpd->name, "on", elapsed_ns);
 
@@ -280,13 +266,6 @@ static int __pm_genpd_poweron(struct generic_pm_domain 
*genpd)
return 0;
}
 
-   if (genpd->cpuidle_data) {
-   cpuidle_pause_and_lock();
-   genpd->cpuidle_data->idle_state->disabled = true;
-   cpuidle_resume_and_unlock();
-   goto out;
-   }
-
/*
 * The list is guaranteed not to change while the loop below is being
 * executed, unless one of the masters' .power_on() callbacks fiddles
@@ -595,21 +574,6 @@ static int pm_genpd_poweroff(struct generic_pm_domain 
*genpd)
}
}
 
-   if (genpd->cpuidle_data) {
-   /*
-* If cpuidle_data is set, cpuidle should turn the domain off
-* when the CPU in it is idle.  In that case we don't decrement
-* the subdomain counts of the master domains, so that power is
-* not removed from the current domain prematurely as a result
-* of cutting off the masters' power.
-*/
-   genpd->status = GPD_STATE_POWER_OFF;
-   cpuidle_pause_and_lock();
-   genpd->cpuidle_data->idle_state->disabled = false;
-   cpuidle_resume_and_unlock();
-   goto out;
-   }
-
if (genpd->power_off) {
if (atomic_read(>sd_count) > 0) {
ret = -EBUSY;
@@ -1725,124 +1689,6 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain 
*genpd,
return ret;
 }
 
-/**
- * pm_genpd_attach_cpuidle - Connect the given PM domain with cpuidle.
- * @genpd: PM domain to be connected with cpuidle.
- * @state: cpuidle state this domain can disable/enable.
- *
- * Make a PM domain behave as though it contained a CPU core, that is, instead
- * of calling its power down routine it will enable the given cpuidle state so
- * that the cpuidle subsystem can power it down (if possible and desirable).
- */
-int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
-{
-   struct cpuidle_driver *cpuidle_drv;
-   struct gpd_cpuidle_data *cpuidle_data;
-   struct cpuidle_state *idle_state;
-   int ret = 0;
-
-   if (IS_ERR_OR_NULL(genpd) || state < 0)
-   return -EINVAL;
-
-   genpd_acquire_lock(genpd);
-
-   if (genpd->cpuidle_data) {
-   ret = -EEXIST;
-   goto out;
-   }
-   cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-   if (!cpuidle_data) {
-   ret = -ENOMEM;
-   goto out;
-   }
-   cpuidle_drv = cpuidle_driver_ref();
-   if (!cpuidle_drv) {
-   ret = -ENODEV;
-   goto err_drv;
-   }
-   if (cpuidle_drv->state_count <= 

[PATCH V2] PM / Domains: Remove cpuidle attach

[Daniel Lezcano]

The power domains code allows to tie a cpuidle state with a power domain.

Preventing the cpuidle framework to enter a specific idle state by disabling
from the power domain framework is a good idea. Unfortunately, the current
implementation has some gaps with a SMP system and a complex cpuidle
implementation. Enabling a power domain wakes up all the cpus even if a cpu
does not belong to the power domain.

There is some work to do a logical representation with the power domains of
the hardware dependencies (eg. a cpu belongs to a power domains, these power
domains belong to a higher power domain for a cluster, etc ...). A new code
relying on the genpd hierarchy to disable the idle states would make more
sense.

As the unique user of this code has been removed, let's wipe out this code
to prevent new user and to have a clean place to put a new implementation.

Signed-off-by: Daniel Lezcano 
---
 drivers/base/power/domain.c | 154 
 include/linux/pm_domain.h   |  27 
 2 files changed, 181 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0ee43c1..f468627 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -171,19 +171,6 @@ static void genpd_set_active(struct generic_pm_domain 
*genpd)
genpd->status = GPD_STATE_ACTIVE;
 }
 
-static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
-{
-   s64 usecs64;
-
-   if (!genpd->cpuidle_data)
-   return;
-
-   usecs64 = genpd->power_on_latency_ns;
-   do_div(usecs64, NSEC_PER_USEC);
-   usecs64 += genpd->cpuidle_data->saved_exit_latency;
-   genpd->cpuidle_data->idle_state->exit_latency = usecs64;
-}
-
 static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
ktime_t time_start;
@@ -207,7 +194,6 @@ static int genpd_power_on(struct generic_pm_domain *genpd, 
bool timed)
 
genpd->power_on_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
-   genpd_recalc_cpu_exit_latency(genpd);
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
 genpd->name, "on", elapsed_ns);
 
@@ -280,13 +266,6 @@ static int __pm_genpd_poweron(struct generic_pm_domain 
*genpd)
return 0;
}
 
-   if (genpd->cpuidle_data) {
-   cpuidle_pause_and_lock();
-   genpd->cpuidle_data->idle_state->disabled = true;
-   cpuidle_resume_and_unlock();
-   goto out;
-   }
-
/*
 * The list is guaranteed not to change while the loop below is being
 * executed, unless one of the masters' .power_on() callbacks fiddles
@@ -595,21 +574,6 @@ static int pm_genpd_poweroff(struct generic_pm_domain 
*genpd)
}
}
 
-   if (genpd->cpuidle_data) {
-   /*
-* If cpuidle_data is set, cpuidle should turn the domain off
-* when the CPU in it is idle.  In that case we don't decrement
-* the subdomain counts of the master domains, so that power is
-* not removed from the current domain prematurely as a result
-* of cutting off the masters' power.
-*/
-   genpd->status = GPD_STATE_POWER_OFF;
-   cpuidle_pause_and_lock();
-   genpd->cpuidle_data->idle_state->disabled = false;
-   cpuidle_resume_and_unlock();
-   goto out;
-   }
-
if (genpd->power_off) {
if (atomic_read(>sd_count) > 0) {
ret = -EBUSY;
@@ -1725,124 +1689,6 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain 
*genpd,
return ret;
 }
 
-/**
- * pm_genpd_attach_cpuidle - Connect the given PM domain with cpuidle.
- * @genpd: PM domain to be connected with cpuidle.
- * @state: cpuidle state this domain can disable/enable.
- *
- * Make a PM domain behave as though it contained a CPU core, that is, instead
- * of calling its power down routine it will enable the given cpuidle state so
- * that the cpuidle subsystem can power it down (if possible and desirable).
- */
-int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
-{
-   struct cpuidle_driver *cpuidle_drv;
-   struct gpd_cpuidle_data *cpuidle_data;
-   struct cpuidle_state *idle_state;
-   int ret = 0;
-
-   if (IS_ERR_OR_NULL(genpd) || state < 0)
-   return -EINVAL;
-
-   genpd_acquire_lock(genpd);
-
-   if (genpd->cpuidle_data) {
-   ret = -EEXIST;
-   goto out;
-   }
-   cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-   if (!cpuidle_data) {
-   ret = -ENOMEM;
-   goto out;
-   }
-   cpuidle_drv = cpuidle_driver_ref();
-   if (!cpuidle_drv) {
-   ret = -ENODEV;
-   goto err_drv;
-   }
-   if 

Re: [PATCH V2] PM / Domains: Remove cpuidle attach

[Kevin Hilman]

Daniel Lezcano  writes:

> The power domains code allows to tie a cpuidle state with a power domain.
>
> Preventing the cpuidle framework to enter a specific idle state by disabling
> from the power domain framework is a good idea. Unfortunately, the current
> implementation has some gaps with a SMP system and a complex cpuidle
> implementation. Enabling a power domain wakes up all the cpus even if a cpu
> does not belong to the power domain.
>
> There is some work to do a logical representation with the power domains of
> the hardware dependencies (eg. a cpu belongs to a power domains, these power
> domains belong to a higher power domain for a cluster, etc ...). A new code
> relying on the genpd hierarchy to disable the idle states would make more
> sense.
>
> As the unique user of this code has been removed, let's wipe out this code
> to prevent new user and to have a clean place to put a new implementation.
>
> Signed-off-by: Daniel Lezcano 

Acked-by: Kevin Hilman 
--
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/