cpuidle: psci: Support CPU hotplug for the hierarchical model

Message ID 20191205103330.27166-1-ulf.hansson@linaro.org
State Superseded
Headers show
Series
  • cpuidle: psci: Support CPU hotplug for the hierarchical model
Related show

Commit Message

Ulf Hansson Dec. 5, 2019, 10:33 a.m.
When the hierarchical CPU topology is used and when a CPU is put offline,
that CPU prevents its PM domain from being powered off, which is because
genpd observes the corresponding attached device as being active from a
runtime PM point of view. Furthermore, any potential master PM domains are
also prevented from being powered off.

To address this limitation, let's add add a new CPU hotplug state
(CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
allows us to deal with runtime PM accordingly.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---

Lorenzo, Sudeep, Rafael, Daniel,

Note that, this patch is based upon a recently posted series [1] and the intent
is to queue it on top. I can fold it into the series and resend it all, if that
makes it easier for people. Just tell me what you prefer.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/cover/11263735/

---
 drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
 include/linux/cpuhotplug.h     |  1 +
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Lorenzo Pieralisi Dec. 5, 2019, 6:06 p.m. | #1
On Thu, Dec 05, 2019 at 11:33:30AM +0100, Ulf Hansson wrote:
> When the hierarchical CPU topology is used and when a CPU is put offline,

> that CPU prevents its PM domain from being powered off, which is because

> genpd observes the corresponding attached device as being active from a

> runtime PM point of view. Furthermore, any potential master PM domains are

> also prevented from being powered off.

> 

> To address this limitation, let's add add a new CPU hotplug state

> (CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which

> allows us to deal with runtime PM accordingly.

> 

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

> 

> Lorenzo, Sudeep, Rafael, Daniel,

> 

> Note that, this patch is based upon a recently posted series [1] and the intent

> is to queue it on top. I can fold it into the series and resend it all, if that

> makes it easier for people. Just tell me what you prefer.

> 

> Kind regards

> Uffe

> 

> [1]

> https://patchwork.kernel.org/cover/11263735/

> 

> ---

>  drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-

>  include/linux/cpuhotplug.h     |  1 +

>  2 files changed, 45 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c

> index 835c7c3aa118..46b481c524cc 100644

> --- a/drivers/cpuidle/cpuidle-psci.c

> +++ b/drivers/cpuidle/cpuidle-psci.c

> @@ -8,6 +8,7 @@

>  

>  #define pr_fmt(fmt) "CPUidle PSCI: " fmt

>  

> +#include <linux/cpuhotplug.h>

>  #include <linux/cpuidle.h>

>  #include <linux/cpumask.h>

>  #include <linux/cpu_pm.h>

> @@ -31,6 +32,7 @@ struct psci_cpuidle_data {

>  

>  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);

>  static DEFINE_PER_CPU(u32, domain_state);

> +static bool psci_cpuidle_use_cpuhp;

>  

>  void psci_set_domain_state(u32 state)

>  {

> @@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,

>  	return ret;

>  }

>  

> +static int psci_idle_cpuhp_up(unsigned int cpu)

> +{

> +	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);

> +

> +	if (pd_dev)

> +		pm_runtime_get_sync(pd_dev);

> +

> +	return 0;

> +}

> +

> +static int psci_idle_cpuhp_down(unsigned int cpu)

> +{

> +	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);

> +

> +	if (pd_dev) {

> +		pm_runtime_put_sync(pd_dev);

> +		/* Clear domain state to start fresh at next online. */

> +		psci_set_domain_state(0);

> +	}

> +

> +	return 0;

> +}

> +

> +static void psci_idle_init_cpuhp(void)

> +{

> +	int err;

> +

> +	if (!psci_cpuidle_use_cpuhp)

> +		return;

> +

> +	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,

> +					"cpuidle/psci:online",

> +					psci_idle_cpuhp_up,

> +					psci_idle_cpuhp_down);


I would make it a cpuhp_setup_state() call and remove

if (cpu_online(cpu))
	pm_runtime_get_sync_dev(dev);

in check in psci_dt_attach_cpu().

Lorenzo

> +	if (err)

> +		pr_warn("Failed %d while setup cpuhp state\n", err);

> +}

> +

>  static int psci_enter_idle_state(struct cpuidle_device *dev,

>  				struct cpuidle_driver *drv, int idx)

>  {

> @@ -161,9 +201,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,

>  	}

>  

>  	/* Manage the deepest state via a dedicated enter-function. */

> -	if (dev)

> +	if (dev) {

>  		drv->states[state_count - 1].enter =

>  			psci_enter_domain_idle_state;

> +		psci_cpuidle_use_cpuhp = true;

> +	}

>  

>  	data->dev = dev;

>  

> @@ -285,6 +327,7 @@ static int __init psci_idle_init(void)

>  			goto out_fail;

>  	}

>  

> +	psci_idle_init_cpuhp();

>  	return 0;

>  

>  out_fail:

> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h

> index e51ee772b9f5..01f04ed6ad92 100644

> --- a/include/linux/cpuhotplug.h

> +++ b/include/linux/cpuhotplug.h

> @@ -95,6 +95,7 @@ enum cpuhp_state {

>  	CPUHP_AP_OFFLINE,

>  	CPUHP_AP_SCHED_STARTING,

>  	CPUHP_AP_RCUTREE_DYING,

> +	CPUHP_AP_CPU_PM_STARTING,

>  	CPUHP_AP_IRQ_GIC_STARTING,

>  	CPUHP_AP_IRQ_HIP04_STARTING,

>  	CPUHP_AP_IRQ_ARMADA_XP_STARTING,

> -- 

> 2.17.1

>
Ulf Hansson Dec. 5, 2019, 7:09 p.m. | #2
On Thu, 5 Dec 2019 at 19:07, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>

> On Thu, Dec 05, 2019 at 11:33:30AM +0100, Ulf Hansson wrote:

> > When the hierarchical CPU topology is used and when a CPU is put offline,

> > that CPU prevents its PM domain from being powered off, which is because

> > genpd observes the corresponding attached device as being active from a

> > runtime PM point of view. Furthermore, any potential master PM domains are

> > also prevented from being powered off.

> >

> > To address this limitation, let's add add a new CPU hotplug state

> > (CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which

> > allows us to deal with runtime PM accordingly.

> >

> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> > ---

> >

> > Lorenzo, Sudeep, Rafael, Daniel,

> >

> > Note that, this patch is based upon a recently posted series [1] and the intent

> > is to queue it on top. I can fold it into the series and resend it all, if that

> > makes it easier for people. Just tell me what you prefer.

> >

> > Kind regards

> > Uffe

> >

> > [1]

> > https://patchwork.kernel.org/cover/11263735/

> >

> > ---

> >  drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-

> >  include/linux/cpuhotplug.h     |  1 +

> >  2 files changed, 45 insertions(+), 1 deletion(-)

> >

> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c

> > index 835c7c3aa118..46b481c524cc 100644

> > --- a/drivers/cpuidle/cpuidle-psci.c

> > +++ b/drivers/cpuidle/cpuidle-psci.c

> > @@ -8,6 +8,7 @@

> >

> >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt

> >

> > +#include <linux/cpuhotplug.h>

> >  #include <linux/cpuidle.h>

> >  #include <linux/cpumask.h>

> >  #include <linux/cpu_pm.h>

> > @@ -31,6 +32,7 @@ struct psci_cpuidle_data {

> >

> >  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);

> >  static DEFINE_PER_CPU(u32, domain_state);

> > +static bool psci_cpuidle_use_cpuhp;

> >

> >  void psci_set_domain_state(u32 state)

> >  {

> > @@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,

> >       return ret;

> >  }

> >

> > +static int psci_idle_cpuhp_up(unsigned int cpu)

> > +{

> > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);

> > +

> > +     if (pd_dev)

> > +             pm_runtime_get_sync(pd_dev);

> > +

> > +     return 0;

> > +}

> > +

> > +static int psci_idle_cpuhp_down(unsigned int cpu)

> > +{

> > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);

> > +

> > +     if (pd_dev) {

> > +             pm_runtime_put_sync(pd_dev);

> > +             /* Clear domain state to start fresh at next online. */

> > +             psci_set_domain_state(0);

> > +     }

> > +

> > +     return 0;

> > +}

> > +

> > +static void psci_idle_init_cpuhp(void)

> > +{

> > +     int err;

> > +

> > +     if (!psci_cpuidle_use_cpuhp)

> > +             return;

> > +

> > +     err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,

> > +                                     "cpuidle/psci:online",

> > +                                     psci_idle_cpuhp_up,

> > +                                     psci_idle_cpuhp_down);

>

> I would make it a cpuhp_setup_state() call and remove

>

> if (cpu_online(cpu))

>         pm_runtime_get_sync_dev(dev);

>

> in check in psci_dt_attach_cpu().

>

> Lorenzo


Thanks for the suggestion, but that doesn't work when
CONFIG_CPU_HOTPLUG is unset.

As a matter of fact, I just realized that I haven't tried to compile
without that Kconfig option. I should probably add a stub for
psci_idle_init_cpuhp() to address that.

>

> > +     if (err)

> > +             pr_warn("Failed %d while setup cpuhp state\n", err);

> > +}

> > +

> >  static int psci_enter_idle_state(struct cpuidle_device *dev,

> >                               struct cpuidle_driver *drv, int idx)

> >  {

> > @@ -161,9 +201,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,

> >       }

> >

> >       /* Manage the deepest state via a dedicated enter-function. */

> > -     if (dev)

> > +     if (dev) {

> >               drv->states[state_count - 1].enter =

> >                       psci_enter_domain_idle_state;

> > +             psci_cpuidle_use_cpuhp = true;

> > +     }

> >

> >       data->dev = dev;

> >

> > @@ -285,6 +327,7 @@ static int __init psci_idle_init(void)

> >                       goto out_fail;

> >       }

> >

> > +     psci_idle_init_cpuhp();

> >       return 0;

> >

> >  out_fail:

> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h

> > index e51ee772b9f5..01f04ed6ad92 100644

> > --- a/include/linux/cpuhotplug.h

> > +++ b/include/linux/cpuhotplug.h

> > @@ -95,6 +95,7 @@ enum cpuhp_state {

> >       CPUHP_AP_OFFLINE,

> >       CPUHP_AP_SCHED_STARTING,

> >       CPUHP_AP_RCUTREE_DYING,

> > +     CPUHP_AP_CPU_PM_STARTING,

> >       CPUHP_AP_IRQ_GIC_STARTING,

> >       CPUHP_AP_IRQ_HIP04_STARTING,

> >       CPUHP_AP_IRQ_ARMADA_XP_STARTING,

> > --

> > 2.17.1

> >


Kind regards
Uffe

Patch

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 835c7c3aa118..46b481c524cc 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -8,6 +8,7 @@ 
 
 #define pr_fmt(fmt) "CPUidle PSCI: " fmt
 
+#include <linux/cpuhotplug.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
@@ -31,6 +32,7 @@  struct psci_cpuidle_data {
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 static DEFINE_PER_CPU(u32, domain_state);
+static bool psci_cpuidle_use_cpuhp;
 
 void psci_set_domain_state(u32 state)
 {
@@ -72,6 +74,44 @@  static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	return ret;
 }
 
+static int psci_idle_cpuhp_up(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev)
+		pm_runtime_get_sync(pd_dev);
+
+	return 0;
+}
+
+static int psci_idle_cpuhp_down(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev) {
+		pm_runtime_put_sync(pd_dev);
+		/* Clear domain state to start fresh at next online. */
+		psci_set_domain_state(0);
+	}
+
+	return 0;
+}
+
+static void psci_idle_init_cpuhp(void)
+{
+	int err;
+
+	if (!psci_cpuidle_use_cpuhp)
+		return;
+
+	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
+					"cpuidle/psci:online",
+					psci_idle_cpuhp_up,
+					psci_idle_cpuhp_down);
+	if (err)
+		pr_warn("Failed %d while setup cpuhp state\n", err);
+}
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
@@ -161,9 +201,11 @@  static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 	}
 
 	/* Manage the deepest state via a dedicated enter-function. */
-	if (dev)
+	if (dev) {
 		drv->states[state_count - 1].enter =
 			psci_enter_domain_idle_state;
+		psci_cpuidle_use_cpuhp = true;
+	}
 
 	data->dev = dev;
 
@@ -285,6 +327,7 @@  static int __init psci_idle_init(void)
 			goto out_fail;
 	}
 
+	psci_idle_init_cpuhp();
 	return 0;
 
 out_fail:
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index e51ee772b9f5..01f04ed6ad92 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -95,6 +95,7 @@  enum cpuhp_state {
 	CPUHP_AP_OFFLINE,
 	CPUHP_AP_SCHED_STARTING,
 	CPUHP_AP_RCUTREE_DYING,
+	CPUHP_AP_CPU_PM_STARTING,
 	CPUHP_AP_IRQ_GIC_STARTING,
 	CPUHP_AP_IRQ_HIP04_STARTING,
 	CPUHP_AP_IRQ_ARMADA_XP_STARTING,