Message-ID: <16404618.nBqGQGUtrE@vostro.rjw.lan>
Date:	Thu, 03 Sep 2015 03:24:39 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:	zhaoyang.huang@...aro.org, Kevin Hilman <khilman@...nel.org>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"open list:GENERIC PM DOMAINS" <linux-pm@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] PM / Domains: Remove cpuidle attach

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 <daniel.lezcano@...aro.org>

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(&genpd->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 <= state) {
> -		ret = -EINVAL;
> -		goto err;
> -	}
> -	idle_state = &cpuidle_drv->states[state];
> -	if (!idle_state->disabled) {
> -		ret = -EAGAIN;
> -		goto err;
> -	}
> -	cpuidle_data->idle_state = idle_state;
> -	cpuidle_data->saved_exit_latency = idle_state->exit_latency;
> -	genpd->cpuidle_data = cpuidle_data;
> -	genpd_recalc_cpu_exit_latency(genpd);
> -
> - out:
> -	genpd_release_lock(genpd);
> -	return ret;
> -
> - err:
> -	cpuidle_driver_unref();
> -
> - err_drv:
> -	kfree(cpuidle_data);
> -	goto out;
> -}
> -
> -/**
> - * pm_genpd_name_attach_cpuidle - Find PM domain and connect cpuidle to it.
> - * @name: Name of the domain to connect to cpuidle.
> - * @state: cpuidle state this domain can manipulate.
> - */
> -int pm_genpd_name_attach_cpuidle(const char *name, int state)
> -{
> -	return pm_genpd_attach_cpuidle(pm_genpd_lookup_name(name), state);
> -}
> -
> -/**
> - * pm_genpd_detach_cpuidle - Remove the cpuidle connection from a PM domain.
> - * @genpd: PM domain to remove the cpuidle connection from.
> - *
> - * Remove the cpuidle connection set up by pm_genpd_attach_cpuidle() from the
> - * given PM domain.
> - */
> -int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
> -{
> -	struct gpd_cpuidle_data *cpuidle_data;
> -	struct cpuidle_state *idle_state;
> -	int ret = 0;
> -
> -	if (IS_ERR_OR_NULL(genpd))
> -		return -EINVAL;
> -
> -	genpd_acquire_lock(genpd);
> -
> -	cpuidle_data = genpd->cpuidle_data;
> -	if (!cpuidle_data) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -	idle_state = cpuidle_data->idle_state;
> -	if (!idle_state->disabled) {
> -		ret = -EAGAIN;
> -		goto out;
> -	}
> -	idle_state->exit_latency = cpuidle_data->saved_exit_latency;
> -	cpuidle_driver_unref();
> -	genpd->cpuidle_data = NULL;
> -	kfree(cpuidle_data);
> -
> - out:
> -	genpd_release_lock(genpd);
> -	return ret;
> -}
> -
> -/**
> - * pm_genpd_name_detach_cpuidle - Find PM domain and disconnect cpuidle from it.
> - * @name: Name of the domain to disconnect cpuidle from.
> - */
> -int pm_genpd_name_detach_cpuidle(const char *name)
> -{
> -	return pm_genpd_detach_cpuidle(pm_genpd_lookup_name(name));
> -}
> -
>  /* Default device callbacks for generic PM domains. */
>  
>  /**
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 681ccb0..85c5862 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -15,7 +15,6 @@
>  #include <linux/err.h>
>  #include <linux/of.h>
>  #include <linux/notifier.h>
> -#include <linux/cpuidle.h>
>  
>  /* Defines used for the flags field in the struct generic_pm_domain */
>  #define GENPD_FLAG_PM_CLK	(1U << 0) /* PM domain uses PM clk */
> @@ -41,11 +40,6 @@ struct gpd_dev_ops {
>  	bool (*active_wakeup)(struct device *dev);
>  };
>  
> -struct gpd_cpuidle_data {
> -	unsigned int saved_exit_latency;
> -	struct cpuidle_state *idle_state;
> -};
> -
>  struct generic_pm_domain {
>  	struct dev_pm_domain domain;	/* PM domain operations */
>  	struct list_head gpd_list_node;	/* Node in the global PM domains list */
> @@ -74,7 +68,6 @@ struct generic_pm_domain {
>  	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
>  	bool max_off_time_changed;
>  	bool cached_power_down_ok;
> -	struct gpd_cpuidle_data *cpuidle_data;
>  	int (*attach_dev)(struct generic_pm_domain *domain,
>  			  struct device *dev);
>  	void (*detach_dev)(struct generic_pm_domain *domain,
> @@ -144,10 +137,6 @@ extern int pm_genpd_add_subdomain_names(const char *master_name,
>  					const char *subdomain_name);
>  extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>  				     struct generic_pm_domain *target);
> -extern int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state);
> -extern int pm_genpd_name_attach_cpuidle(const char *name, int state);
> -extern int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd);
> -extern int pm_genpd_name_detach_cpuidle(const char *name);
>  extern void pm_genpd_init(struct generic_pm_domain *genpd,
>  			  struct dev_power_governor *gov, bool is_off);
>  
> @@ -199,22 +188,6 @@ static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>  {
>  	return -ENOSYS;
>  }
> -static inline int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int st)
> -{
> -	return -ENOSYS;
> -}
> -static inline int pm_genpd_name_attach_cpuidle(const char *name, int state)
> -{
> -	return -ENOSYS;
> -}
> -static inline int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
> -{
> -	return -ENOSYS;
> -}
> -static inline int pm_genpd_name_detach_cpuidle(const char *name)
> -{
> -	return -ENOSYS;
> -}
>  static inline void pm_genpd_init(struct generic_pm_domain *genpd,
>  				 struct dev_power_governor *gov, bool is_off)
>  {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists