[PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-pseries.c | 50 +++++++++++++++++----------------------
 include/linux/cpuhotplug.h        |  1 +
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 07135e009d8b..10c4c51cd840 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -171,39 +171,29 @@ static struct cpuidle_state shared_states[] = {
 		.enter = &shared_cede_loop },
 };
 
-static int pseries_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int pseries_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = pseries_cpuidle_add_cpu_notifier,
-};
+static int pseries_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * pseries_cpuidle_driver_init()
@@ -273,7 +263,11 @@ static int __init pseries_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "CPUIDLE_PSERIES_ONLINE",
+				  pseries_cpuidle_cpu_online, NULL);
+	cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_PSERIES_DEAD,
+				  "CPUIDLE_PSERIES_DEAD", NULL,
+				  pseries_cpuidle_cpu_dead);
 	printk(KERN_DEBUG "pseries_idle_driver registered\n");
 	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 5811954809af..baecc4faf028 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -19,6 +19,7 @@ enum cpuhp_state {
 	CPUHP_MM_WRITEBACK_DEAD,
 	CPUHP_SOFTIRQ_DEAD,
 	CPUHP_NET_MVNETA_DEAD,
+	CPUHP_CPUIDLE_PSERIES_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
-- 
2.9.3

Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[Daniel Lezcano]

On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 5811954809af..baecc4faf028 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -19,6 +19,7 @@ enum cpuhp_state {
>  	CPUHP_MM_WRITEBACK_DEAD,
>  	CPUHP_SOFTIRQ_DEAD,
>  	CPUHP_NET_MVNETA_DEAD,
> +	CPUHP_CPUIDLE_PSERIES_DEAD,

Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
planned to be done separately ?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

On 2016-08-22 18:09:47 [+0200], Daniel Lezcano wrote:
> On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
> > Install the callbacks via the state machine.
> > 
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index 5811954809af..baecc4faf028 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -19,6 +19,7 @@ enum cpuhp_state {
> >  	CPUHP_MM_WRITEBACK_DEAD,
> >  	CPUHP_SOFTIRQ_DEAD,
> >  	CPUHP_NET_MVNETA_DEAD,
> > +	CPUHP_CPUIDLE_PSERIES_DEAD,
> 
> Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
> planned to be done separately ?

You mean CPUHP_CPUIDLE_DEAD instead of _PSERIES_DEAD and _POWERNV_DEAD?
We could do that but you would have to ensure that only one CPUIDLE
driver registers itself at a time and for those powerpc drivers it looks
like you could have two registered (not sure about ARM's little/big (if
you could have two of those later at run-time)).
For the ONLINE state we have dynamic allocation of IDs. If it is
possible to rework the code to use only ONLINE & PRE_DOWN instead of
DEAD then we wouldn't have this. I can't say at this point if we do
dynamic allocation of the DEAD IDs.

Sebastian

Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[Daniel Lezcano]

On 08/22/2016 09:04 PM, Sebastian Andrzej Siewior wrote:
> On 2016-08-22 18:09:47 [+0200], Daniel Lezcano wrote:
>> On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
>>> Install the callbacks via the state machine.
>>>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm@vger.kernel.org
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>> ---
>>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>>> index 5811954809af..baecc4faf028 100644
>>> --- a/include/linux/cpuhotplug.h
>>> +++ b/include/linux/cpuhotplug.h
>>> @@ -19,6 +19,7 @@ enum cpuhp_state {
>>>  	CPUHP_MM_WRITEBACK_DEAD,
>>>  	CPUHP_SOFTIRQ_DEAD,
>>>  	CPUHP_NET_MVNETA_DEAD,
>>> +	CPUHP_CPUIDLE_PSERIES_DEAD,
>>
>> Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
>> planned to be done separately ?
> 
> You mean CPUHP_CPUIDLE_DEAD instead of _PSERIES_DEAD and _POWERNV_DEAD?

Yes. If we can limit the number of duplicating enum for the same purpose
right now, it would be nice.

> We could do that but you would have to ensure that only one CPUIDLE
> driver registers itself at a time and for those powerpc drivers it looks
> like you could have two registered (not sure about ARM's little/big (if
> you could have two of those later at run-time)).

At the first glance, I don't think it is possible to register the cpu
hotplug callback twice because the cpuidle drivers are doing:

...
        retval = cpuidle_register(&pseries_idle_driver, NULL);
        if (retval) {
                printk(KERN_DEBUG "Registration of pseries driver
failed.\n");
                return retval;
        }

        register_cpu_notifier(&setup_hotplug_notifier);

So if a previous driver was already registered, cpuidle_register will
fail and register_cpu_notifier won't be hit.

There is the same scenario for intel_idle and processor_idle (acpi).

> For the ONLINE state we have dynamic allocation of IDs. If it is
> possible to rework the code to use only ONLINE & PRE_DOWN instead of
> DEAD then we wouldn't have this. I can't say at this point if we do
> dynamic allocation of the DEAD IDs.
> 
> Sebastian
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

On 2016-08-23 16:16:12 [+0200], Daniel Lezcano wrote:
> Yes. If we can limit the number of duplicating enum for the same purpose
> right now, it would be nice.
Okay.

> > We could do that but you would have to ensure that only one CPUIDLE
> > driver registers itself at a time and for those powerpc drivers it looks
> > like you could have two registered (not sure about ARM's little/big (if
> > you could have two of those later at run-time)).
> 
> At the first glance, I don't think it is possible to register the cpu
> hotplug callback twice because the cpuidle drivers are doing:
…
> So if a previous driver was already registered, cpuidle_register will
> fail and register_cpu_notifier won't be hit.
> 
> There is the same scenario for intel_idle and processor_idle (acpi).

I tried to preserve everything as-is during the conversation. However if
you are explicitly asking for this and you are sure that it will work
then you can get it. No problem :)

Sebastian

[PATCH 10/16 v2] cpuidle: pseries: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

v1…v2: Use only CPUHP_CPUIDLE_DEAD (requested by Daniel Lezcano)

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-pseries.c | 53 ++++++++++++++++++---------------------
 include/linux/cpuhotplug.h        |  1 +
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 07135e009d8b..abbce8f96d93 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -171,39 +171,29 @@ static struct cpuidle_state shared_states[] = {
 		.enter = &shared_cede_loop },
 };
 
-static int pseries_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int pseries_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = pseries_cpuidle_add_cpu_notifier,
-};
+static int pseries_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * pseries_cpuidle_driver_init()
@@ -273,7 +263,14 @@ static int __init pseries_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	retval = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					   "CPUIDLE_PSERIES_ONLINE",
+					   pseries_cpuidle_cpu_online, NULL);
+	WARN_ON(retval < 0);
+	retval = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_DEAD,
+					   "CPUIDLE_PSERIES_DEAD", NULL,
+					   pseries_cpuidle_cpu_dead);
+	WARN_ON(retval < 0);
 	printk(KERN_DEBUG "pseries_idle_driver registered\n");
 	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 5811954809af..14b58a9d1450 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -19,6 +19,7 @@ enum cpuhp_state {
 	CPUHP_MM_WRITEBACK_DEAD,
 	CPUHP_SOFTIRQ_DEAD,
 	CPUHP_NET_MVNETA_DEAD,
+	CPUHP_CPUIDLE_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
-- 
2.9.3

[PATCH 11/16] cpuidle: powernv: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-powernv.c | 50 +++++++++++++++++----------------------
 include/linux/cpuhotplug.h        |  1 +
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index f7ca891b5b59..081e688102a7 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -119,39 +119,29 @@ static struct cpuidle_state powernv_states[CPUIDLE_STATE_MAX] = {
 		.enter = snooze_loop },
 };
 
-static int powernv_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int powernv_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = powernv_cpuidle_add_cpu_notifier,
-};
+static int powernv_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * powernv_cpuidle_driver_init()
@@ -355,7 +345,11 @@ static int __init powernv_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "CPUIDLE_POWERNV_ONLINE",
+				  powernv_cpuidle_cpu_online, NULL);
+	cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_POWERNV_DEAD,
+				  "CPUIDLE_POWERNV_DEAD", NULL,
+				  powernv_cpuidle_cpu_dead);
 	printk(KERN_DEBUG "powernv_idle_driver registered\n");
 	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index baecc4faf028..6a08fd142b8c 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -20,6 +20,7 @@ enum cpuhp_state {
 	CPUHP_SOFTIRQ_DEAD,
 	CPUHP_NET_MVNETA_DEAD,
 	CPUHP_CPUIDLE_PSERIES_DEAD,
+	CPUHP_CPUIDLE_POWERNV_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
-- 
2.9.3

[PATCH 11/16] cpuidle: powernv: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

v1…v2: - Use only CPUHP_CPUIDLE_DEAD (requested by Daniel Lezcano)

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-powernv.c | 53 ++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index f7ca891b5b59..5eb1ee70a5f8 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -119,39 +119,29 @@ static struct cpuidle_state powernv_states[CPUIDLE_STATE_MAX] = {
 		.enter = snooze_loop },
 };
 
-static int powernv_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int powernv_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = powernv_cpuidle_add_cpu_notifier,
-};
+static int powernv_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * powernv_cpuidle_driver_init()
@@ -355,7 +345,14 @@ static int __init powernv_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	retval = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					   "CPUIDLE_POWERNV_ONLINE",
+					   powernv_cpuidle_cpu_online, NULL);
+	WARN_ON(retval < 0);
+	retval = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_DEAD,
+					   "CPUIDLE_POWERNV_DEAD", NULL,
+					   powernv_cpuidle_cpu_dead);
+	WARN_ON(retval < 0);
 	printk(KERN_DEBUG "powernv_idle_driver registered\n");
 	return 0;
 }
-- 
2.9.3

[PATCH 12/16] cpuidle: coupled: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/coupled.c  | 81 +++++++++++++++++++++-------------------------
 include/linux/cpuhotplug.h |  1 +
 2 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index d5657d50ac40..11efd9743e88 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -749,65 +749,58 @@ static void cpuidle_coupled_allow_idle(struct cpuidle_coupled *coupled)
 	put_cpu();
 }
 
-/**
- * cpuidle_coupled_cpu_notify - notifier called during hotplug transitions
- * @nb: notifier block
- * @action: hotplug transition
- * @hcpu: target cpu number
- *
- * Called when a cpu is brought on or offline using hotplug.  Updates the
- * coupled cpu set appropriately
- */
-static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
-		unsigned long action, void *hcpu)
+static int coupled_cpu_online(unsigned int cpu)
 {
-	int cpu = (unsigned long)hcpu;
 	struct cpuidle_device *dev;
 
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_UP_PREPARE:
-	case CPU_DOWN_PREPARE:
-	case CPU_ONLINE:
-	case CPU_DEAD:
-	case CPU_UP_CANCELED:
-	case CPU_DOWN_FAILED:
-		break;
-	default:
-		return NOTIFY_OK;
-	}
-
 	mutex_lock(&cpuidle_lock);
 
 	dev = per_cpu(cpuidle_devices, cpu);
 	if (!dev || !dev->coupled)
 		goto out;
 
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_UP_PREPARE:
-	case CPU_DOWN_PREPARE:
-		cpuidle_coupled_prevent_idle(dev->coupled);
-		break;
-	case CPU_ONLINE:
-	case CPU_DEAD:
-		cpuidle_coupled_update_online_cpus(dev->coupled);
-		/* Fall through */
-	case CPU_UP_CANCELED:
-	case CPU_DOWN_FAILED:
-		cpuidle_coupled_allow_idle(dev->coupled);
-		break;
-	}
-
+	cpuidle_coupled_update_online_cpus(dev->coupled);
+	cpuidle_coupled_allow_idle(dev->coupled);
 out:
 	mutex_unlock(&cpuidle_lock);
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block cpuidle_coupled_cpu_notifier = {
-	.notifier_call = cpuidle_coupled_cpu_notify,
-};
+static int coupled_cpu_up_prepare(unsigned int cpu)
+{
+	struct cpuidle_device *dev;
+
+	mutex_lock(&cpuidle_lock);
+
+	dev = per_cpu(cpuidle_devices, cpu);
+	if (!dev || !dev->coupled)
+		goto out;
+
+	cpuidle_coupled_prevent_idle(dev->coupled);
+out:
+	mutex_unlock(&cpuidle_lock);
+	return 0;
+}
 
 static int __init cpuidle_coupled_init(void)
 {
-	return register_cpu_notifier(&cpuidle_coupled_cpu_notifier);
+	int ret;
+
+	ret = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE,
+					"CPUIDLE_COUPLED_PREPARE",
+					coupled_cpu_up_prepare,
+					coupled_cpu_online);
+	if (ret)
+		goto err;
+	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					"CPUIDLE_COUPLED_ONLINE",
+					coupled_cpu_online,
+					coupled_cpu_up_prepare);
+	if (ret < 0)
+		goto err;
+	return 0;
+err:
+	cpuhp_remove_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE);
+	return ret;
 }
 core_initcall(cpuidle_coupled_init);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 6a08fd142b8c..9e50a7b3bbcd 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -31,6 +31,7 @@ enum cpuhp_state {
 	CPUHP_SLAB_PREPARE,
 	CPUHP_RCUTREE_PREP,
 	CPUHP_MD_RAID5_PREPARE,
+	CPUHP_CPUIDLE_COUPLED_PREPARE,
 	CPUHP_NOTIFY_PREPARE,
 	CPUHP_TIMERS_DEAD,
 	CPUHP_BRINGUP_CPU,
-- 
2.9.3

Re: [PATCH 12/16] cpuidle: coupled: Convert to hotplug state machine

[Daniel Lezcano]

On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---

[ ... ]

> -
>  	mutex_lock(&cpuidle_lock);
>  
>  	dev = per_cpu(cpuidle_devices, cpu);
>  	if (!dev || !dev->coupled)
>  		goto out;
>  
> -	switch (action & ~CPU_TASKS_FROZEN) {
> -	case CPU_UP_PREPARE:
> -	case CPU_DOWN_PREPARE:
> -		cpuidle_coupled_prevent_idle(dev->coupled);
> -		break;
> -	case CPU_ONLINE:
> -	case CPU_DEAD:
> -		cpuidle_coupled_update_online_cpus(dev->coupled);
> -		/* Fall through */
> -	case CPU_UP_CANCELED:
> -	case CPU_DOWN_FAILED:
> -		cpuidle_coupled_allow_idle(dev->coupled);
> -		break;
> -	}
> -
> +	cpuidle_coupled_update_online_cpus(dev->coupled);
> +	cpuidle_coupled_allow_idle(dev->coupled);
>  out:
>  	mutex_unlock(&cpuidle_lock);
> -	return NOTIFY_OK;
> +	return 0;

You can remove the useless label 'out':

if (dev && dev->coupled) {
	cpuidle_coupled_update_online_cpus(dev->coupled);
	cpuidle_coupled_allow_idle(dev->coupled);
}

>  }
>  
> -static struct notifier_block cpuidle_coupled_cpu_notifier = {
> -	.notifier_call = cpuidle_coupled_cpu_notify,
> -};
> +static int coupled_cpu_up_prepare(unsigned int cpu)
> +{
> +	struct cpuidle_device *dev;
> +
> +	mutex_lock(&cpuidle_lock);
> +
> +	dev = per_cpu(cpuidle_devices, cpu);
> +	if (!dev || !dev->coupled)
> +		goto out;
> +
> +	cpuidle_coupled_prevent_idle(dev->coupled);
> +out:
> +	mutex_unlock(&cpuidle_lock);
> +	return 0;

Same comment here.

> +}
>  
>  static int __init cpuidle_coupled_init(void)
>  {
> -	return register_cpu_notifier(&cpuidle_coupled_cpu_notifier);
> +	int ret;
> +
> +	ret = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE,
> +					"CPUIDLE_COUPLED_PREPARE",
> +					coupled_cpu_up_prepare,
> +					coupled_cpu_online);
> +	if (ret)
> +		goto err;

There is nothing to undo here.

	if (ret)
		return ret;

> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +					"CPUIDLE_COUPLED_ONLINE",
> +					coupled_cpu_online,
> +					coupled_cpu_up_prepare);
> +	if (ret < 0)
> +		goto err;

	if (ret)
		cpuhp_remove_state_nocalls(
			CPUHP_CPUIDLE_COUPLED_PREPARE);
		
	return ret;

> +	return 0;
> +err:
> +	cpuhp_remove_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE);
> +	return ret;

  ^^^^^ zaaap !



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 12/16 v2] cpuidle: coupled: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: - refactor the code to drop the out label.
       - make error path in cpuidle_coupled_init() simpler

 drivers/cpuidle/coupled.c  | 75 +++++++++++++++++++---------------------------
 include/linux/cpuhotplug.h |  1 +
 2 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index d5657d50ac40..5ba1a8913421 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -749,65 +749,52 @@ static void cpuidle_coupled_allow_idle(struct cpuidle_coupled *coupled)
 	put_cpu();
 }
 
-/**
- * cpuidle_coupled_cpu_notify - notifier called during hotplug transitions
- * @nb: notifier block
- * @action: hotplug transition
- * @hcpu: target cpu number
- *
- * Called when a cpu is brought on or offline using hotplug.  Updates the
- * coupled cpu set appropriately
- */
-static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
-		unsigned long action, void *hcpu)
+static int coupled_cpu_online(unsigned int cpu)
 {
-	int cpu = (unsigned long)hcpu;
 	struct cpuidle_device *dev;
 
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_UP_PREPARE:
-	case CPU_DOWN_PREPARE:
-	case CPU_ONLINE:
-	case CPU_DEAD:
-	case CPU_UP_CANCELED:
-	case CPU_DOWN_FAILED:
-		break;
-	default:
-		return NOTIFY_OK;
-	}
-
 	mutex_lock(&cpuidle_lock);
 
 	dev = per_cpu(cpuidle_devices, cpu);
-	if (!dev || !dev->coupled)
-		goto out;
-
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_UP_PREPARE:
-	case CPU_DOWN_PREPARE:
-		cpuidle_coupled_prevent_idle(dev->coupled);
-		break;
-	case CPU_ONLINE:
-	case CPU_DEAD:
+	if (dev && dev->coupled) {
 		cpuidle_coupled_update_online_cpus(dev->coupled);
-		/* Fall through */
-	case CPU_UP_CANCELED:
-	case CPU_DOWN_FAILED:
 		cpuidle_coupled_allow_idle(dev->coupled);
-		break;
 	}
 
-out:
 	mutex_unlock(&cpuidle_lock);
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block cpuidle_coupled_cpu_notifier = {
-	.notifier_call = cpuidle_coupled_cpu_notify,
-};
+static int coupled_cpu_up_prepare(unsigned int cpu)
+{
+	struct cpuidle_device *dev;
+
+	mutex_lock(&cpuidle_lock);
+
+	dev = per_cpu(cpuidle_devices, cpu);
+	if (dev && dev->coupled)
+		cpuidle_coupled_prevent_idle(dev->coupled);
+
+	mutex_unlock(&cpuidle_lock);
+	return 0;
+}
 
 static int __init cpuidle_coupled_init(void)
 {
-	return register_cpu_notifier(&cpuidle_coupled_cpu_notifier);
+	int ret;
+
+	ret = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE,
+					"CPUIDLE_COUPLED_PREPARE",
+					coupled_cpu_up_prepare,
+					coupled_cpu_online);
+	if (ret)
+		return ret;
+	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					"CPUIDLE_COUPLED_ONLINE",
+					coupled_cpu_online,
+					coupled_cpu_up_prepare);
+	if (ret < 0)
+		cpuhp_remove_state_nocalls(CPUHP_CPUIDLE_COUPLED_PREPARE);
+	return ret;
 }
 core_initcall(cpuidle_coupled_init);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 14b58a9d1450..965acec06d4e 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -30,6 +30,7 @@ enum cpuhp_state {
 	CPUHP_SLAB_PREPARE,
 	CPUHP_RCUTREE_PREP,
 	CPUHP_MD_RAID5_PREPARE,
+	CPUHP_CPUIDLE_COUPLED_PREPARE,
 	CPUHP_NOTIFY_PREPARE,
 	CPUHP_TIMERS_DEAD,
 	CPUHP_BRINGUP_CPU,
-- 
2.9.3