| Message ID | 20160818125731.27256-11-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)