On Thu, 5 Dec 2019 at 20:09, Ulf Hansson [off-list ref] wrote:

On Thu, 5 Dec 2019 at 19:07, Lorenzo Pieralisi
[off-list ref] wrote:

quoted

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

quoted

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 <redacted>
---

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel