On Thu, 5 Dec 2019 at 20:09, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > > On Thu, 5 Dec 2019 at 19:07, Lorenzo Pieralisi > <lorenzo.pieralisi@xxxxxxx> 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@xxxxxxxxxx> > > > --- > > > > > > 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. Just forget about what I said above, it's nonsense. cpuhp_setup_state() and friends works fine even if CONFIG_CPU_HOTPLUG is unset. I did test this, but some reason I forgot about it. However, there is another problem with your proposal above, which is that the CPUHP callback is assigned after the cpuidle drivers (one per CPU) have been registered. Which means, CPUs may enter idle even before the CPUHP callbacks are registered. In other words, we need to keep the call to pm_runtime_get_sync_dev() in psci_dt_attach_cpu() as to properly reflect the state of the CPU. [...] BTW, I am about to post a new version in a couple of hours. Any other thing you want to point out, before the next submission? Kind regards Uffe
