[PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Ulf Hansson]

Updates in v2:
	- Rebased and fixed a small issue in genpd, see patch3.
	- Re-tested on v6.9-rt5 (PREEMPT_RT enabled)
	- Re-tested on v6.10-rc1 (for regressions, PREEMPT_RT disabled)

The hierarchical PM domain topology and the corresponding domain-idle-states
are currently disabled on a PREEMPT_RT based configuration. The main reason is
because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
genpd and runtime PM can't be use in the atomic idle-path when
selecting/entering an idle-state.

For s2idle/s2ram this is an unnecessary limitation that this series intends to
address. Note that, the support for cpuhotplug is left to future improvements.
More information about this are available in the commit messages.

I have tested this on a Dragonboard 410c.

Kind regards
Ulf Hansson


Ulf Hansson (7):
  pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT
  pmdomain: core: Don't hold the genpd-lock when calling
    dev_pm_domain_set()
  pmdomain: core: Use dev_name() instead of kobject_get_path() in
    debugfs
  cpuidle: psci-domain: Enable system-wide suspend on PREEMPT_RT
  cpuidle: psci: Drop redundant assignment of CPUIDLE_FLAG_RCU_IDLE
  cpuidle: psci: Enable the hierarchical topology for s2ram on
    PREEMPT_RT
  cpuidle: psci: Enable the hierarchical topology for s2idle on
    PREEMPT_RT

 drivers/cpuidle/cpuidle-psci-domain.c | 10 ++--
 drivers/cpuidle/cpuidle-psci.c        | 26 ++++++----
 drivers/pmdomain/core.c               | 75 +++++++++++++++++++--------
 include/linux/pm_domain.h             |  5 +-
 4 files changed, 80 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Ulf Hansson]

To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
during s2idle on a PREEMPT_RT based configuration, we can't use the regular
spinlock, as they are turned into sleepable locks on PREEMPT_RT.

To address this problem, let's convert into using the raw spinlock, but
only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
this way, the lock can still be acquired/released in atomic context, which
is needed in the idle-path for PREEMPT_RT.

Do note that the genpd power-on/off notifiers may also be fired during
s2idle, but these are already prepared for PREEMPT_RT as they are based on
the raw notifiers. However, consumers of them may need to adopt accordingly
to work properly on PREEMPT_RT.

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

Changes in v2:
	- None.

---
 drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pm_domain.h |  5 ++++-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 623d15b68707..072e6bdb6ee6 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
 	.unlock = genpd_unlock_spin,
 };
 
+static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
+	__acquires(&genpd->raw_slock)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&genpd->raw_slock, flags);
+	genpd->raw_lock_flags = flags;
+}
+
+static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
+					int depth)
+	__acquires(&genpd->raw_slock)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
+	genpd->raw_lock_flags = flags;
+}
+
+static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
+	__acquires(&genpd->raw_slock)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&genpd->raw_slock, flags);
+	genpd->raw_lock_flags = flags;
+	return 0;
+}
+
+static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
+	__releases(&genpd->raw_slock)
+{
+	raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
+}
+
+static const struct genpd_lock_ops genpd_raw_spin_ops = {
+	.lock = genpd_lock_raw_spin,
+	.lock_nested = genpd_lock_nested_raw_spin,
+	.lock_interruptible = genpd_lock_interruptible_raw_spin,
+	.unlock = genpd_unlock_raw_spin,
+};
+
 #define genpd_lock(p)			p->lock_ops->lock(p)
 #define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
 #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
@@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
 
 static void genpd_lock_init(struct generic_pm_domain *genpd)
 {
-	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+	if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
+		raw_spin_lock_init(&genpd->raw_slock);
+		genpd->lock_ops = &genpd_raw_spin_ops;
+	} else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
 		spin_lock_init(&genpd->slock);
 		genpd->lock_ops = &genpd_spin_ops;
 	} else {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index f24546a3d3db..0d7fc47de3bc 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -194,8 +194,11 @@ struct generic_pm_domain {
 			spinlock_t slock;
 			unsigned long lock_flags;
 		};
+		struct {
+			raw_spinlock_t raw_slock;
+			unsigned long raw_lock_flags;
+		};
 	};
-
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-- 
2.34.1

[PATCH v2 2/7] pmdomain: core: Don't hold the genpd-lock when calling dev_pm_domain_set()

[Ulf Hansson]

There is no need to hold the genpd-lock, while assigning the
dev->pm_domain. In fact, it becomes a problem on a PREEMPT_RT based
configuration as the genpd-lock may be a raw spinlock, while the lock
acquired through the call to dev_pm_domain_set() is a regular spinlock.

To fix the problem, let's simply move the calls to dev_pm_domain_set()
outside the genpd-lock.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- None.

---
 drivers/pmdomain/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 072e6bdb6ee6..454fccc38df1 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -1736,7 +1736,6 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	genpd_lock(genpd);
 
 	genpd_set_cpumask(genpd, gpd_data->cpu);
-	dev_pm_domain_set(dev, &genpd->domain);
 
 	genpd->device_count++;
 	if (gd)
@@ -1745,6 +1744,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
 	genpd_unlock(genpd);
+	dev_pm_domain_set(dev, &genpd->domain);
  out:
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
@@ -1801,12 +1801,13 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 		genpd->gd->max_off_time_changed = true;
 
 	genpd_clear_cpumask(genpd, gpd_data->cpu);
-	dev_pm_domain_set(dev, NULL);
 
 	list_del_init(&pdd->list_node);
 
 	genpd_unlock(genpd);
 
+	dev_pm_domain_set(dev, NULL);
+
 	if (genpd->detach_dev)
 		genpd->detach_dev(genpd, dev);
 
-- 
2.34.1

[PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Ulf Hansson]

Using kobject_get_path() means a dynamic memory allocation gets done, which
doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
spinlock.

To fix the problem, let's convert into using the simpler dev_name(). This
means the information about the path doesn't get presented in debugfs, but
hopefully this shouldn't be an issue.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- New patch.
---
 drivers/pmdomain/core.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 454fccc38df1..90a65bd09d52 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3182,7 +3182,6 @@ static int genpd_summary_one(struct seq_file *s,
 		[GENPD_STATE_OFF] = "off"
 	};
 	struct pm_domain_data *pm_data;
-	const char *kobj_path;
 	struct gpd_link *link;
 	char state[16];
 	int ret;
@@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
 	}
 
 	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj,
-				genpd_is_irq_safe(genpd) ?
-				GFP_ATOMIC : GFP_KERNEL);
-		if (kobj_path == NULL)
-			continue;
-
-		seq_printf(s, "\n    %-50s  ", kobj_path);
+		seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));
 		rtpm_status_str(s, pm_data->dev);
 		perf_status_str(s, pm_data->dev);
-		kfree(kobj_path);
 	}
 
 	seq_puts(s, "\n");
@@ -3393,23 +3385,14 @@ static int devices_show(struct seq_file *s, void *data)
 {
 	struct generic_pm_domain *genpd = s->private;
 	struct pm_domain_data *pm_data;
-	const char *kobj_path;
 	int ret = 0;
 
 	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
-	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj,
-				genpd_is_irq_safe(genpd) ?
-				GFP_ATOMIC : GFP_KERNEL);
-		if (kobj_path == NULL)
-			continue;
-
-		seq_printf(s, "%s\n", kobj_path);
-		kfree(kobj_path);
-	}
+	list_for_each_entry(pm_data, &genpd->dev_list, list_node)
+		seq_printf(s, "%s\n", dev_name(pm_data->dev));
 
 	genpd_unlock(genpd);
 	return ret;
-- 
2.34.1

[PATCH v2 4/7] cpuidle: psci-domain: Enable system-wide suspend on PREEMPT_RT

[Ulf Hansson]

The domain-idle-states are currently disabled on a PREEMPT_RT based
configuration for the cpuidle-psci-domain. To enable them to be used for
system-wide suspend and in particular during s2idle, let's set the
GENPD_FLAG_RPM_ALWAYS_ON instead of GENPD_FLAG_ALWAYS_ON for the
corresponding genpd provider.

In this way, the runtime PM path remains disabled in genpd for its attached
devices, while powering-on/off the PM domain during system-wide suspend
becomes allowed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- None.
---
 drivers/cpuidle/cpuidle-psci-domain.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index fae958794339..ea28b73ef3fb 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -67,12 +67,16 @@ static int psci_pd_init(struct device_node *np, bool use_osi)
 
 	/*
 	 * Allow power off when OSI has been successfully enabled.
-	 * PREEMPT_RT is not yet ready to enter domain idle states.
+	 * On a PREEMPT_RT based configuration the domain idle states are
+	 * supported, but only during system-wide suspend.
 	 */
-	if (use_osi && !IS_ENABLED(CONFIG_PREEMPT_RT))
+	if (use_osi) {
 		pd->power_off = psci_pd_power_off;
-	else
+		if (IS_ENABLED(CONFIG_PREEMPT_RT))
+			pd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
+	} else {
 		pd->flags |= GENPD_FLAG_ALWAYS_ON;
+	}
 
 	/* Use governor for CPU PM domains if it has some states to manage. */
 	pd_gov = pd->states ? &pm_domain_cpu_gov : NULL;
-- 
2.34.1

[PATCH v2 5/7] cpuidle: psci: Drop redundant assignment of CPUIDLE_FLAG_RCU_IDLE

[Ulf Hansson]

When using the hierarchical topology and PSCI OSI-mode we may end up
overriding the deepest idle-state's ->enter|enter_s2idle() callbacks, but
there is no point to also re-assign the CPUIDLE_FLAG_RCU_IDLE for the
idle-state in question, as that has already been set when parsing the
states from DT. See init_state_node().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- None.
---
 drivers/cpuidle/cpuidle-psci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 782030a27703..d82a8bc1b194 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -234,7 +234,6 @@ static int psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
 	 * of a shared state for the domain, assumes the domain states are all
 	 * deeper states.
 	 */
-	drv->states[state_count - 1].flags |= CPUIDLE_FLAG_RCU_IDLE;
 	drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
 	drv->states[state_count - 1].enter_s2idle = psci_enter_s2idle_domain_idle_state;
 	psci_cpuidle_use_cpuhp = true;
-- 
2.34.1

[PATCH v2 6/7] cpuidle: psci: Enable the hierarchical topology for s2ram on PREEMPT_RT

[Ulf Hansson]

The hierarchical PM domain topology are currently disabled on a PREEMPT_RT
based configuration. As a first step to enable it to be used, let's try to
attach the CPU devices to their PM domains on PREEMPT_RT. In this way the
syscore ops becomes available, allowing the PM domain topology to be
managed during s2ram.

For the moment let's leave the support for CPU hotplug outside PREEMPT_RT,
as it's depending on using runtime PM. For s2ram, this isn't a problem as
all CPUs are managed via the syscore ops.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- None.
---
 drivers/cpuidle/cpuidle-psci.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index d82a8bc1b194..ad6ce9fe12b4 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -37,6 +37,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_syscore;
 static bool psci_cpuidle_use_cpuhp;
 
 void psci_set_domain_state(u32 state)
@@ -166,6 +167,12 @@ static struct syscore_ops psci_idle_syscore_ops = {
 	.resume = psci_idle_syscore_resume,
 };
 
+static void psci_idle_init_syscore(void)
+{
+	if (psci_cpuidle_use_syscore)
+		register_syscore_ops(&psci_idle_syscore_ops);
+}
+
 static void psci_idle_init_cpuhp(void)
 {
 	int err;
@@ -173,8 +180,6 @@ static void psci_idle_init_cpuhp(void)
 	if (!psci_cpuidle_use_cpuhp)
 		return;
 
-	register_syscore_ops(&psci_idle_syscore_ops);
-
 	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
 					"cpuidle/psci:online",
 					psci_idle_cpuhp_up,
@@ -222,13 +227,16 @@ static int psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
 	if (!psci_has_osi_support())
 		return 0;
 
-	if (IS_ENABLED(CONFIG_PREEMPT_RT))
-		return 0;
-
 	data->dev = dt_idle_attach_cpu(cpu, "psci");
 	if (IS_ERR_OR_NULL(data->dev))
 		return PTR_ERR_OR_ZERO(data->dev);
 
+	psci_cpuidle_use_syscore = true;
+
+	/* The hierarchical topology is limited to s2ram on PREEMPT_RT. */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		return 0;
+
 	/*
 	 * Using the deepest state for the CPU to trigger a potential selection
 	 * of a shared state for the domain, assumes the domain states are all
@@ -312,6 +320,7 @@ static void psci_cpu_deinit_idle(int cpu)
 	struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
 
 	dt_idle_detach_cpu(data->dev);
+	psci_cpuidle_use_syscore = false;
 	psci_cpuidle_use_cpuhp = false;
 }
 
@@ -408,6 +417,7 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
 			goto out_fail;
 	}
 
+	psci_idle_init_syscore();
 	psci_idle_init_cpuhp();
 	return 0;
 
-- 
2.34.1

[PATCH v2 7/7] cpuidle: psci: Enable the hierarchical topology for s2idle on PREEMPT_RT

[Ulf Hansson]

To enable the domain-idle-states to be used during s2idle on a PREEMPT_RT
based configuration, let's allow the re-assignment of the ->enter_s2idle()
callback to psci_enter_s2idle_domain_idle_state().

Similar to s2ram, let's leave the support for CPU hotplug outside
PREEMPT_RT, as it's depending on using runtime PM. For s2idle, this means
that an offline CPU's PM domain will remain powered-on. In practise this
may lead to that a shallower idle-state than necessary gets selected, which
shouldn't be an issue (besides wasting power).

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
	- None.
---
 drivers/cpuidle/cpuidle-psci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index ad6ce9fe12b4..2562dc001fc1 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -233,18 +233,17 @@ static int psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
 
 	psci_cpuidle_use_syscore = true;
 
-	/* The hierarchical topology is limited to s2ram on PREEMPT_RT. */
-	if (IS_ENABLED(CONFIG_PREEMPT_RT))
-		return 0;
-
 	/*
 	 * Using the deepest state for the CPU to trigger a potential selection
 	 * of a shared state for the domain, assumes the domain states are all
-	 * deeper states.
+	 * deeper states. On PREEMPT_RT the hierarchical topology is limited to
+	 * s2ram and s2idle.
 	 */
-	drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
 	drv->states[state_count - 1].enter_s2idle = psci_enter_s2idle_domain_idle_state;
-	psci_cpuidle_use_cpuhp = true;
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
+		psci_cpuidle_use_cpuhp = true;
+	}
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Nikunj Kela]

On 5/27/2024 7:25 AM, Ulf Hansson wrote:
> To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
> during s2idle on a PREEMPT_RT based configuration, we can't use the regular
> spinlock, as they are turned into sleepable locks on PREEMPT_RT.
>
> To address this problem, let's convert into using the raw spinlock, but
> only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
> this way, the lock can still be acquired/released in atomic context, which
> is needed in the idle-path for PREEMPT_RT.
>
> Do note that the genpd power-on/off notifiers may also be fired during
> s2idle, but these are already prepared for PREEMPT_RT as they are based on
> the raw notifiers. However, consumers of them may need to adopt accordingly
> to work properly on PREEMPT_RT.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> 	- None.
>
> ---
>  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_domain.h |  5 ++++-
>  2 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 623d15b68707..072e6bdb6ee6 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>  	.unlock = genpd_unlock_spin,
>  };
>  
> +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
> +	__acquires(&genpd->raw_slock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> +	genpd->raw_lock_flags = flags;
> +}
> +
> +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
> +					int depth)
> +	__acquires(&genpd->raw_slock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
> +	genpd->raw_lock_flags = flags;
> +}
> +
> +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
> +	__acquires(&genpd->raw_slock)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> +	genpd->raw_lock_flags = flags;
> +	return 0;
> +}
> +
> +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
> +	__releases(&genpd->raw_slock)
> +{
> +	raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
> +}
> +
> +static const struct genpd_lock_ops genpd_raw_spin_ops = {
> +	.lock = genpd_lock_raw_spin,
> +	.lock_nested = genpd_lock_nested_raw_spin,
> +	.lock_interruptible = genpd_lock_interruptible_raw_spin,
> +	.unlock = genpd_unlock_raw_spin,
> +};
> +
>  #define genpd_lock(p)			p->lock_ops->lock(p)
>  #define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
>  #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
> @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
>  
>  static void genpd_lock_init(struct generic_pm_domain *genpd)
>  {
> -	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> +	if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
> +		raw_spin_lock_init(&genpd->raw_slock);
> +		genpd->lock_ops = &genpd_raw_spin_ops;
> +	} else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {

Hi Ulf, though you are targeting only CPU domains for now, I wonder if
FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
it is safe for atomic context which won't be the case for PREEMPT_RT? 


>  		spin_lock_init(&genpd->slock);
>  		genpd->lock_ops = &genpd_spin_ops;
>  	} else {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f24546a3d3db..0d7fc47de3bc 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -194,8 +194,11 @@ struct generic_pm_domain {
>  			spinlock_t slock;
>  			unsigned long lock_flags;
>  		};
> +		struct {
> +			raw_spinlock_t raw_slock;
> +			unsigned long raw_lock_flags;
> +		};
>  	};
> -
>  };
>  
>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)

Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Ulf Hansson]

On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>
>
> On 5/27/2024 7:25 AM, Ulf Hansson wrote:
> > To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
> > during s2idle on a PREEMPT_RT based configuration, we can't use the regular
> > spinlock, as they are turned into sleepable locks on PREEMPT_RT.
> >
> > To address this problem, let's convert into using the raw spinlock, but
> > only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
> > this way, the lock can still be acquired/released in atomic context, which
> > is needed in the idle-path for PREEMPT_RT.
> >
> > Do note that the genpd power-on/off notifiers may also be fired during
> > s2idle, but these are already prepared for PREEMPT_RT as they are based on
> > the raw notifiers. However, consumers of them may need to adopt accordingly
> > to work properly on PREEMPT_RT.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v2:
> >       - None.
> >
> > ---
> >  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pm_domain.h |  5 ++++-
> >  2 files changed, 50 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 623d15b68707..072e6bdb6ee6 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> >       .unlock = genpd_unlock_spin,
> >  };
> >
> > +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
> > +     __acquires(&genpd->raw_slock)
> > +{
> > +     unsigned long flags;
> > +
> > +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> > +     genpd->raw_lock_flags = flags;
> > +}
> > +
> > +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
> > +                                     int depth)
> > +     __acquires(&genpd->raw_slock)
> > +{
> > +     unsigned long flags;
> > +
> > +     raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
> > +     genpd->raw_lock_flags = flags;
> > +}
> > +
> > +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
> > +     __acquires(&genpd->raw_slock)
> > +{
> > +     unsigned long flags;
> > +
> > +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> > +     genpd->raw_lock_flags = flags;
> > +     return 0;
> > +}
> > +
> > +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
> > +     __releases(&genpd->raw_slock)
> > +{
> > +     raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
> > +}
> > +
> > +static const struct genpd_lock_ops genpd_raw_spin_ops = {
> > +     .lock = genpd_lock_raw_spin,
> > +     .lock_nested = genpd_lock_nested_raw_spin,
> > +     .lock_interruptible = genpd_lock_interruptible_raw_spin,
> > +     .unlock = genpd_unlock_raw_spin,
> > +};
> > +
> >  #define genpd_lock(p)                        p->lock_ops->lock(p)
> >  #define genpd_lock_nested(p, d)              p->lock_ops->lock_nested(p, d)
> >  #define genpd_lock_interruptible(p)  p->lock_ops->lock_interruptible(p)
> > @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
> >
> >  static void genpd_lock_init(struct generic_pm_domain *genpd)
> >  {
> > -     if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> > +     if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
> > +             raw_spin_lock_init(&genpd->raw_slock);
> > +             genpd->lock_ops = &genpd_raw_spin_ops;
> > +     } else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>
> Hi Ulf, though you are targeting only CPU domains for now, I wonder if
> FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
> it is safe for atomic context which won't be the case for PREEMPT_RT?

You have a point!

However, we also need to limit the use of raw spinlocks, from
PREEMPT_RT point of view. In other words, just because a genpd
provider is capable of executing its callbacks in atomic context,
doesn't always mean that it should use raw spinlocks too.

[...]

Kind regards
Uffe

Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Nikunj Kela]

On 5/30/2024 1:15 AM, Ulf Hansson wrote:
> On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>>
>> On 5/27/2024 7:25 AM, Ulf Hansson wrote:
>>> To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
>>> during s2idle on a PREEMPT_RT based configuration, we can't use the regular
>>> spinlock, as they are turned into sleepable locks on PREEMPT_RT.
>>>
>>> To address this problem, let's convert into using the raw spinlock, but
>>> only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
>>> this way, the lock can still be acquired/released in atomic context, which
>>> is needed in the idle-path for PREEMPT_RT.
>>>
>>> Do note that the genpd power-on/off notifiers may also be fired during
>>> s2idle, but these are already prepared for PREEMPT_RT as they are based on
>>> the raw notifiers. However, consumers of them may need to adopt accordingly
>>> to work properly on PREEMPT_RT.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>
>>> Changes in v2:
>>>       - None.
>>>
>>> ---
>>>  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
>>>  include/linux/pm_domain.h |  5 ++++-
>>>  2 files changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>> index 623d15b68707..072e6bdb6ee6 100644
>>> --- a/drivers/pmdomain/core.c
>>> +++ b/drivers/pmdomain/core.c
>>> @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>>>       .unlock = genpd_unlock_spin,
>>>  };
>>>
>>> +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
>>> +     __acquires(&genpd->raw_slock)
>>> +{
>>> +     unsigned long flags;
>>> +
>>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
>>> +     genpd->raw_lock_flags = flags;
>>> +}
>>> +
>>> +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
>>> +                                     int depth)
>>> +     __acquires(&genpd->raw_slock)
>>> +{
>>> +     unsigned long flags;
>>> +
>>> +     raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
>>> +     genpd->raw_lock_flags = flags;
>>> +}
>>> +
>>> +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
>>> +     __acquires(&genpd->raw_slock)
>>> +{
>>> +     unsigned long flags;
>>> +
>>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
>>> +     genpd->raw_lock_flags = flags;
>>> +     return 0;
>>> +}
>>> +
>>> +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
>>> +     __releases(&genpd->raw_slock)
>>> +{
>>> +     raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
>>> +}
>>> +
>>> +static const struct genpd_lock_ops genpd_raw_spin_ops = {
>>> +     .lock = genpd_lock_raw_spin,
>>> +     .lock_nested = genpd_lock_nested_raw_spin,
>>> +     .lock_interruptible = genpd_lock_interruptible_raw_spin,
>>> +     .unlock = genpd_unlock_raw_spin,
>>> +};
>>> +
>>>  #define genpd_lock(p)                        p->lock_ops->lock(p)
>>>  #define genpd_lock_nested(p, d)              p->lock_ops->lock_nested(p, d)
>>>  #define genpd_lock_interruptible(p)  p->lock_ops->lock_interruptible(p)
>>> @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
>>>
>>>  static void genpd_lock_init(struct generic_pm_domain *genpd)
>>>  {
>>> -     if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>>> +     if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
>>> +             raw_spin_lock_init(&genpd->raw_slock);
>>> +             genpd->lock_ops = &genpd_raw_spin_ops;
>>> +     } else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>> Hi Ulf, though you are targeting only CPU domains for now, I wonder if
>> FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
>> it is safe for atomic context which won't be the case for PREEMPT_RT?
> You have a point!
>
> However, we also need to limit the use of raw spinlocks, from
> PREEMPT_RT point of view. In other words, just because a genpd
> provider is capable of executing its callbacks in atomic context,
> doesn't always mean that it should use raw spinlocks too.

Got it! Thanks. Maybe in future, if there is a need, a new GENPD FLAG
for RT, something like GENPD_FLAG_IRQ_SAFE_RT, can be added to address this.


>
> [...]
>
> Kind regards
> Uffe

Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Ulf Hansson]

On Thu, 30 May 2024 at 16:23, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>
>
> On 5/30/2024 1:15 AM, Ulf Hansson wrote:
> > On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@quicinc.com> wrote:
> >>
> >> On 5/27/2024 7:25 AM, Ulf Hansson wrote:
> >>> To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
> >>> during s2idle on a PREEMPT_RT based configuration, we can't use the regular
> >>> spinlock, as they are turned into sleepable locks on PREEMPT_RT.
> >>>
> >>> To address this problem, let's convert into using the raw spinlock, but
> >>> only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
> >>> this way, the lock can still be acquired/released in atomic context, which
> >>> is needed in the idle-path for PREEMPT_RT.
> >>>
> >>> Do note that the genpd power-on/off notifiers may also be fired during
> >>> s2idle, but these are already prepared for PREEMPT_RT as they are based on
> >>> the raw notifiers. However, consumers of them may need to adopt accordingly
> >>> to work properly on PREEMPT_RT.
> >>>
> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>       - None.
> >>>
> >>> ---
> >>>  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
> >>>  include/linux/pm_domain.h |  5 ++++-
> >>>  2 files changed, 50 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> >>> index 623d15b68707..072e6bdb6ee6 100644
> >>> --- a/drivers/pmdomain/core.c
> >>> +++ b/drivers/pmdomain/core.c
> >>> @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> >>>       .unlock = genpd_unlock_spin,
> >>>  };
> >>>
> >>> +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +}
> >>> +
> >>> +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
> >>> +                                     int depth)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +}
> >>> +
> >>> +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __acquires(&genpd->raw_slock)
> >>> +{
> >>> +     unsigned long flags;
> >>> +
> >>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
> >>> +     genpd->raw_lock_flags = flags;
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
> >>> +     __releases(&genpd->raw_slock)
> >>> +{
> >>> +     raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
> >>> +}
> >>> +
> >>> +static const struct genpd_lock_ops genpd_raw_spin_ops = {
> >>> +     .lock = genpd_lock_raw_spin,
> >>> +     .lock_nested = genpd_lock_nested_raw_spin,
> >>> +     .lock_interruptible = genpd_lock_interruptible_raw_spin,
> >>> +     .unlock = genpd_unlock_raw_spin,
> >>> +};
> >>> +
> >>>  #define genpd_lock(p)                        p->lock_ops->lock(p)
> >>>  #define genpd_lock_nested(p, d)              p->lock_ops->lock_nested(p, d)
> >>>  #define genpd_lock_interruptible(p)  p->lock_ops->lock_interruptible(p)
> >>> @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
> >>>
> >>>  static void genpd_lock_init(struct generic_pm_domain *genpd)
> >>>  {
> >>> -     if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> >>> +     if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
> >>> +             raw_spin_lock_init(&genpd->raw_slock);
> >>> +             genpd->lock_ops = &genpd_raw_spin_ops;
> >>> +     } else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> >> Hi Ulf, though you are targeting only CPU domains for now, I wonder if
> >> FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
> >> it is safe for atomic context which won't be the case for PREEMPT_RT?
> > You have a point!
> >
> > However, we also need to limit the use of raw spinlocks, from
> > PREEMPT_RT point of view. In other words, just because a genpd
> > provider is capable of executing its callbacks in atomic context,
> > doesn't always mean that it should use raw spinlocks too.
>
> Got it! Thanks. Maybe in future, if there is a need, a new GENPD FLAG
> for RT, something like GENPD_FLAG_IRQ_SAFE_RT, can be added to address this.

Yes, I agree, something along those lines would make sense.

BTW, did you manage to get some time to test the series on your end?

Kind regards
Uffe

Re: [PATCH v2 1/7] pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT

[Nikunj Kela]

On 6/3/2024 6:58 AM, Ulf Hansson wrote:
> On Thu, 30 May 2024 at 16:23, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>>
>> On 5/30/2024 1:15 AM, Ulf Hansson wrote:
>>> On Tue, 28 May 2024 at 21:56, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>>>> On 5/27/2024 7:25 AM, Ulf Hansson wrote:
>>>>> To allow a genpd provider for a CPU PM domain to enter a domain-idle-state
>>>>> during s2idle on a PREEMPT_RT based configuration, we can't use the regular
>>>>> spinlock, as they are turned into sleepable locks on PREEMPT_RT.
>>>>>
>>>>> To address this problem, let's convert into using the raw spinlock, but
>>>>> only for genpd providers that have the GENPD_FLAG_CPU_DOMAIN bit set. In
>>>>> this way, the lock can still be acquired/released in atomic context, which
>>>>> is needed in the idle-path for PREEMPT_RT.
>>>>>
>>>>> Do note that the genpd power-on/off notifiers may also be fired during
>>>>> s2idle, but these are already prepared for PREEMPT_RT as they are based on
>>>>> the raw notifiers. However, consumers of them may need to adopt accordingly
>>>>> to work properly on PREEMPT_RT.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>>       - None.
>>>>>
>>>>> ---
>>>>>  drivers/pmdomain/core.c   | 47 ++++++++++++++++++++++++++++++++++++++-
>>>>>  include/linux/pm_domain.h |  5 ++++-
>>>>>  2 files changed, 50 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>>>>> index 623d15b68707..072e6bdb6ee6 100644
>>>>> --- a/drivers/pmdomain/core.c
>>>>> +++ b/drivers/pmdomain/core.c
>>>>> @@ -117,6 +117,48 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>>>>>       .unlock = genpd_unlock_spin,
>>>>>  };
>>>>>
>>>>> +static void genpd_lock_raw_spin(struct generic_pm_domain *genpd)
>>>>> +     __acquires(&genpd->raw_slock)
>>>>> +{
>>>>> +     unsigned long flags;
>>>>> +
>>>>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
>>>>> +     genpd->raw_lock_flags = flags;
>>>>> +}
>>>>> +
>>>>> +static void genpd_lock_nested_raw_spin(struct generic_pm_domain *genpd,
>>>>> +                                     int depth)
>>>>> +     __acquires(&genpd->raw_slock)
>>>>> +{
>>>>> +     unsigned long flags;
>>>>> +
>>>>> +     raw_spin_lock_irqsave_nested(&genpd->raw_slock, flags, depth);
>>>>> +     genpd->raw_lock_flags = flags;
>>>>> +}
>>>>> +
>>>>> +static int genpd_lock_interruptible_raw_spin(struct generic_pm_domain *genpd)
>>>>> +     __acquires(&genpd->raw_slock)
>>>>> +{
>>>>> +     unsigned long flags;
>>>>> +
>>>>> +     raw_spin_lock_irqsave(&genpd->raw_slock, flags);
>>>>> +     genpd->raw_lock_flags = flags;
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static void genpd_unlock_raw_spin(struct generic_pm_domain *genpd)
>>>>> +     __releases(&genpd->raw_slock)
>>>>> +{
>>>>> +     raw_spin_unlock_irqrestore(&genpd->raw_slock, genpd->raw_lock_flags);
>>>>> +}
>>>>> +
>>>>> +static const struct genpd_lock_ops genpd_raw_spin_ops = {
>>>>> +     .lock = genpd_lock_raw_spin,
>>>>> +     .lock_nested = genpd_lock_nested_raw_spin,
>>>>> +     .lock_interruptible = genpd_lock_interruptible_raw_spin,
>>>>> +     .unlock = genpd_unlock_raw_spin,
>>>>> +};
>>>>> +
>>>>>  #define genpd_lock(p)                        p->lock_ops->lock(p)
>>>>>  #define genpd_lock_nested(p, d)              p->lock_ops->lock_nested(p, d)
>>>>>  #define genpd_lock_interruptible(p)  p->lock_ops->lock_interruptible(p)
>>>>> @@ -2079,7 +2121,10 @@ static void genpd_free_data(struct generic_pm_domain *genpd)
>>>>>
>>>>>  static void genpd_lock_init(struct generic_pm_domain *genpd)
>>>>>  {
>>>>> -     if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>>>>> +     if (genpd->flags & GENPD_FLAG_CPU_DOMAIN) {
>>>>> +             raw_spin_lock_init(&genpd->raw_slock);
>>>>> +             genpd->lock_ops = &genpd_raw_spin_ops;
>>>>> +     } else if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>>>> Hi Ulf, though you are targeting only CPU domains for now, I wonder if
>>>> FLAG_IRQ_SAFE will be a better choice?  The description of the flag says
>>>> it is safe for atomic context which won't be the case for PREEMPT_RT?
>>> You have a point!
>>>
>>> However, we also need to limit the use of raw spinlocks, from
>>> PREEMPT_RT point of view. In other words, just because a genpd
>>> provider is capable of executing its callbacks in atomic context,
>>> doesn't always mean that it should use raw spinlocks too.
>> Got it! Thanks. Maybe in future, if there is a need, a new GENPD FLAG
>> for RT, something like GENPD_FLAG_IRQ_SAFE_RT, can be added to address this.
> Yes, I agree, something along those lines would make sense.
>
> BTW, did you manage to get some time to test the series on your end?

I haven't been able spend time testing it but I have requested Maulik to
test it and update you. Thanks


> Kind regards
> Uffe

Re: [PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2024-05-27 16:25:50 [+0200], Ulf Hansson wrote:
> Updates in v2:
> 	- Rebased and fixed a small issue in genpd, see patch3.
> 	- Re-tested on v6.9-rt5 (PREEMPT_RT enabled)
> 	- Re-tested on v6.10-rc1 (for regressions, PREEMPT_RT disabled)
> 
> The hierarchical PM domain topology and the corresponding domain-idle-states
> are currently disabled on a PREEMPT_RT based configuration. The main reason is
> because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
> genpd and runtime PM can't be use in the atomic idle-path when
> selecting/entering an idle-state.
> 
> For s2idle/s2ram this is an unnecessary limitation that this series intends to
> address. Note that, the support for cpuhotplug is left to future improvements.
> More information about this are available in the commit messages.

I looked at it and it seems limited to pmdomain/core.c, also I don't
know if there is a ->set_performance_state callback set since the one I
checked have mutex_t locking ;)
So if this is needed, then be it. s2ram wouldn't be used in "production"
but in "safe state" so I wouldn't worry too much about latency spikes.
Not sure what it means for the other modes.
I am not to worried for now, please don't let spread more than needed ;)

> Kind regards
> Ulf Hansson

Sebastian

Re: [PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Ulf Hansson]

On Mon, 8 Jul 2024 at 15:53, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2024-05-27 16:25:50 [+0200], Ulf Hansson wrote:
> > Updates in v2:
> >       - Rebased and fixed a small issue in genpd, see patch3.
> >       - Re-tested on v6.9-rt5 (PREEMPT_RT enabled)
> >       - Re-tested on v6.10-rc1 (for regressions, PREEMPT_RT disabled)
> >
> > The hierarchical PM domain topology and the corresponding domain-idle-states
> > are currently disabled on a PREEMPT_RT based configuration. The main reason is
> > because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
> > genpd and runtime PM can't be use in the atomic idle-path when
> > selecting/entering an idle-state.
> >
> > For s2idle/s2ram this is an unnecessary limitation that this series intends to
> > address. Note that, the support for cpuhotplug is left to future improvements.
> > More information about this are available in the commit messages.
>
> I looked at it and it seems limited to pmdomain/core.c, also I don't
> know if there is a ->set_performance_state callback set since the one I
> checked have mutex_t locking ;)
> So if this is needed, then be it. s2ram wouldn't be used in "production"
> but in "safe state" so I wouldn't worry too much about latency spikes.
> Not sure what it means for the other modes.
> I am not to worried for now, please don't let spread more than needed ;)

Thanks for taking a look and for providing your thoughts. Can I
consider that as an "ack" for the whole series?

Before I decide to apply this I am awaiting some additional
confirmation from Qcom guys. It's getting late for v6.11, so I may
need to make another re-spin, but let's see.

Kind regards
Uffe

Re: [PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Raghavendra Kakarla]

On 5/27/2024 7:55 PM, Ulf Hansson wrote:
> Updates in v2:
> 	- Rebased and fixed a small issue in genpd, see patch3.
> 	- Re-tested on v6.9-rt5 (PREEMPT_RT enabled)
> 	- Re-tested on v6.10-rc1 (for regressions, PREEMPT_RT disabled)
> 
> The hierarchical PM domain topology and the corresponding domain-idle-states
> are currently disabled on a PREEMPT_RT based configuration. The main reason is
> because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
> genpd and runtime PM can't be use in the atomic idle-path when
> selecting/entering an idle-state.
> 
> For s2idle/s2ram this is an unnecessary limitation that this series intends to
> address. Note that, the support for cpuhotplug is left to future improvements.
> More information about this are available in the commit messages.
> 
> I have tested this on a Dragonboard 410c.
This series is tested on qcm6490 with PREEMPT_RT enabled. For the series,

Tested-by: Raghavendra Kakarla <quic_rkakarla@quicinc.com>  # qcm6490 
with PREEMPT_RT enabled

Tested APSS suspend and then SoC power collapse through s2idle and s2ram 
paths.
APSS and soc power collapse stats are incremented.
/sys/kernel/debug/pm_genpd/power-domain-cluster/idle_states
/sys/kernel/debug/qcom_stats/cxsd

Without this series, they are not incremented.

Thanks,
Raghavendra K.
> 
> Kind regards
> Ulf Hansson
> 
> 
> Ulf Hansson (7):
>    pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT
>    pmdomain: core: Don't hold the genpd-lock when calling
>      dev_pm_domain_set()
>    pmdomain: core: Use dev_name() instead of kobject_get_path() in
>      debugfs
>    cpuidle: psci-domain: Enable system-wide suspend on PREEMPT_RT
>    cpuidle: psci: Drop redundant assignment of CPUIDLE_FLAG_RCU_IDLE
>    cpuidle: psci: Enable the hierarchical topology for s2ram on
>      PREEMPT_RT
>    cpuidle: psci: Enable the hierarchical topology for s2idle on
>      PREEMPT_RT
> 
>   drivers/cpuidle/cpuidle-psci-domain.c | 10 ++--
>   drivers/cpuidle/cpuidle-psci.c        | 26 ++++++----
>   drivers/pmdomain/core.c               | 75 +++++++++++++++++++--------
>   include/linux/pm_domain.h             |  5 +-
>   4 files changed, 80 insertions(+), 36 deletions(-)
> 

Re: [PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2024-07-09 17:31:12 [+0200], Ulf Hansson wrote:
> 
> Thanks for taking a look and for providing your thoughts. Can I
> consider that as an "ack" for the whole series?

Yes.

Sebastian

Re: [PATCH v2 0/7] pmdomain/cpuidle-psci: Support s2idle/s2ram on PREEMPT_RT

[Ulf Hansson]

+ Raghavendra Kakarla, Sebastian Andrzej Siewior

On Mon, 27 May 2024 at 16:26, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Updates in v2:
>         - Rebased and fixed a small issue in genpd, see patch3.
>         - Re-tested on v6.9-rt5 (PREEMPT_RT enabled)
>         - Re-tested on v6.10-rc1 (for regressions, PREEMPT_RT disabled)
>
> The hierarchical PM domain topology and the corresponding domain-idle-states
> are currently disabled on a PREEMPT_RT based configuration. The main reason is
> because spinlocks are turned into sleepable locks on PREEMPT_RT, which means
> genpd and runtime PM can't be use in the atomic idle-path when
> selecting/entering an idle-state.
>
> For s2idle/s2ram this is an unnecessary limitation that this series intends to
> address. Note that, the support for cpuhotplug is left to future improvements.
> More information about this are available in the commit messages.
>
> I have tested this on a Dragonboard 410c.
>
> Kind regards
> Ulf Hansson
>
>
> Ulf Hansson (7):
>   pmdomain: core: Enable s2idle for CPU PM domains on PREEMPT_RT
>   pmdomain: core: Don't hold the genpd-lock when calling
>     dev_pm_domain_set()
>   pmdomain: core: Use dev_name() instead of kobject_get_path() in
>     debugfs
>   cpuidle: psci-domain: Enable system-wide suspend on PREEMPT_RT
>   cpuidle: psci: Drop redundant assignment of CPUIDLE_FLAG_RCU_IDLE
>   cpuidle: psci: Enable the hierarchical topology for s2ram on
>     PREEMPT_RT
>   cpuidle: psci: Enable the hierarchical topology for s2idle on
>     PREEMPT_RT
>
>  drivers/cpuidle/cpuidle-psci-domain.c | 10 ++--
>  drivers/cpuidle/cpuidle-psci.c        | 26 ++++++----
>  drivers/pmdomain/core.c               | 75 +++++++++++++++++++--------
>  include/linux/pm_domain.h             |  5 +-
>  4 files changed, 80 insertions(+), 36 deletions(-)
>

Just wanted to let you all know that I have queued up this series via
my pmdomain tree. Please let me know if you have any further
comments/suggestions.

Kind regards
Uffe

Re: [PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Geert Uytterhoeven]

Hi Ulf,

On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Using kobject_get_path() means a dynamic memory allocation gets done, which
> doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
> spinlock.
>
> To fix the problem, let's convert into using the simpler dev_name(). This
> means the information about the path doesn't get presented in debugfs, but
> hopefully this shouldn't be an issue.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> Changes in v2:
>         - New patch.

Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain:
core: Use dev_name() instead of kobject_get_path() in debugfs")
in pmdomain/next.

> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
>         }
>
>         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> -               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> -                               genpd_is_irq_safe(genpd) ?
> -                               GFP_ATOMIC : GFP_KERNEL);
> -               if (kobj_path == NULL)
> -                       continue;
> -
> -               seq_printf(s, "\n    %-50s  ", kobj_path);
> +               seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));

While some of the old names didn't even fit in 50 characters, the new
names need much less space, so perhaps this is a good opportunity to
decrease the table width?

>                 rtpm_status_str(s, pm_data->dev);
>                 perf_status_str(s, pm_data->dev);
> -               kfree(kobj_path);
>         }
>
>         seq_puts(s, "\n");

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Ulf Hansson]

On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > Using kobject_get_path() means a dynamic memory allocation gets done, which
> > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
> > spinlock.
> >
> > To fix the problem, let's convert into using the simpler dev_name(). This
> > means the information about the path doesn't get presented in debugfs, but
> > hopefully this shouldn't be an issue.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > Changes in v2:
> >         - New patch.
>
> Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain:
> core: Use dev_name() instead of kobject_get_path() in debugfs")
> in pmdomain/next.
>
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
> >         }
> >
> >         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> > -               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> > -                               genpd_is_irq_safe(genpd) ?
> > -                               GFP_ATOMIC : GFP_KERNEL);
> > -               if (kobj_path == NULL)
> > -                       continue;
> > -
> > -               seq_printf(s, "\n    %-50s  ", kobj_path);
> > +               seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));
>
> While some of the old names didn't even fit in 50 characters, the new
> names need much less space, so perhaps this is a good opportunity to
> decrease the table width?

Sure, it seems reasonable! Do you want to send a patch?

>
> >                 rtpm_status_str(s, pm_data->dev);
> >                 perf_status_str(s, pm_data->dev);
> > -               kfree(kobj_path);
> >         }
> >
> >         seq_puts(s, "\n");

Kind regards
Uffe

Re: [PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Geert Uytterhoeven]

Hi Ulf,

On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > Using kobject_get_path() means a dynamic memory allocation gets done, which
> > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
> > > spinlock.
> > >
> > > To fix the problem, let's convert into using the simpler dev_name(). This
> > > means the information about the path doesn't get presented in debugfs, but
> > > hopefully this shouldn't be an issue.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > > Changes in v2:
> > >         - New patch.
> >
> > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain:
> > core: Use dev_name() instead of kobject_get_path() in debugfs")
> > in pmdomain/next.
> >
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
> > >         }
> > >
> > >         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> > > -               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> > > -                               genpd_is_irq_safe(genpd) ?
> > > -                               GFP_ATOMIC : GFP_KERNEL);
> > > -               if (kobj_path == NULL)
> > > -                       continue;
> > > -
> > > -               seq_printf(s, "\n    %-50s  ", kobj_path);
> > > +               seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));
> >
> > While some of the old names didn't even fit in 50 characters, the new
> > names need much less space, so perhaps this is a good opportunity to
> > decrease the table width?
>
> Sure, it seems reasonable! Do you want to send a patch?

I started looking into it.  Then I noticed that on some systems
(e.g. TI am335x) the device names may have a longer format than
the typical <unit-address>.<nodename>. So I wanted to verify on
BeagleBone Black, but recent kernels crash during early boot.
Apparently that platform was broken between v6.8 and v6.9-rc1.
And during bisection, I encountered 3 different failure modes...

To be continued...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Aug 30, 2024 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > Using kobject_get_path() means a dynamic memory allocation gets done, which
> > > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
> > > > spinlock.
> > > >
> > > > To fix the problem, let's convert into using the simpler dev_name(). This
> > > > means the information about the path doesn't get presented in debugfs, but
> > > > hopefully this shouldn't be an issue.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > > Changes in v2:
> > > >         - New patch.
> > >
> > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain:
> > > core: Use dev_name() instead of kobject_get_path() in debugfs")
> > > in pmdomain/next.
> > >
> > > > --- a/drivers/pmdomain/core.c
> > > > +++ b/drivers/pmdomain/core.c
> > > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
> > > >         }
> > > >
> > > >         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> > > > -               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> > > > -                               genpd_is_irq_safe(genpd) ?
> > > > -                               GFP_ATOMIC : GFP_KERNEL);
> > > > -               if (kobj_path == NULL)
> > > > -                       continue;
> > > > -
> > > > -               seq_printf(s, "\n    %-50s  ", kobj_path);
> > > > +               seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));
> > >
> > > While some of the old names didn't even fit in 50 characters, the new
> > > names need much less space, so perhaps this is a good opportunity to
> > > decrease the table width?
> >
> > Sure, it seems reasonable! Do you want to send a patch?
>
> I started looking into it.  Then I noticed that on some systems
> (e.g. TI am335x) the device names may have a longer format than
> the typical <unit-address>.<nodename>. So I wanted to verify on
> BeagleBone Black, but recent kernels crash during early boot.
> Apparently that platform was broken between v6.8 and v6.9-rc1.
> And during bisection, I encountered 3 different failure modes...
>
> To be continued...

The longest generic node names documented in the Devicetree
Specification are "air-pollution-sensor" and "interrupt-controller"
(both counting 20 characters), so a typical device name needs 8
(32-bit unit address) + 1 (dot) + 20 = 29 characters.
However, I assume some devices lie outside the 32-bit address space,
and thus need more space?

With the BeagleBone Black boot issue fixed:
"/devices/platform/ocp/5600fe00.target-module"
resp. "/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@200000/44e3e074.target-module"
are now shortened to "5600fe00.target-module" resp. "44e3e074.target-module".
However, "/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@200000/48000000.interconnect:segment@200000:target-module@0"
is shortened to "48000000.interconnect:segment@200000:target-module@0",
which is still longer than the old column width...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/7] pmdomain: core: Use dev_name() instead of kobject_get_path() in debugfs

[Ulf Hansson]

On Mon, 2 Sept 2024 at 16:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, Aug 30, 2024 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Aug 20, 2024 at 10:58 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Tue, 20 Aug 2024 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Mon, May 27, 2024 at 4:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > Using kobject_get_path() means a dynamic memory allocation gets done, which
> > > > > doesn't work on a PREEMPT_RT based configuration while holding genpd's raw
> > > > > spinlock.
> > > > >
> > > > > To fix the problem, let's convert into using the simpler dev_name(). This
> > > > > means the information about the path doesn't get presented in debugfs, but
> > > > > hopefully this shouldn't be an issue.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > > Changes in v2:
> > > > >         - New patch.
> > > >
> > > > Thanks for your patch, which is now commit 9094e53ff5c86ebe ("pmdomain:
> > > > core: Use dev_name() instead of kobject_get_path() in debugfs")
> > > > in pmdomain/next.
> > > >
> > > > > --- a/drivers/pmdomain/core.c
> > > > > +++ b/drivers/pmdomain/core.c
> > > > > @@ -3215,16 +3214,9 @@ static int genpd_summary_one(struct seq_file *s,
> > > > >         }
> > > > >
> > > > >         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> > > > > -               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> > > > > -                               genpd_is_irq_safe(genpd) ?
> > > > > -                               GFP_ATOMIC : GFP_KERNEL);
> > > > > -               if (kobj_path == NULL)
> > > > > -                       continue;
> > > > > -
> > > > > -               seq_printf(s, "\n    %-50s  ", kobj_path);
> > > > > +               seq_printf(s, "\n    %-50s  ", dev_name(pm_data->dev));
> > > >
> > > > While some of the old names didn't even fit in 50 characters, the new
> > > > names need much less space, so perhaps this is a good opportunity to
> > > > decrease the table width?
> > >
> > > Sure, it seems reasonable! Do you want to send a patch?
> >
> > I started looking into it.  Then I noticed that on some systems
> > (e.g. TI am335x) the device names may have a longer format than
> > the typical <unit-address>.<nodename>. So I wanted to verify on
> > BeagleBone Black, but recent kernels crash during early boot.
> > Apparently that platform was broken between v6.8 and v6.9-rc1.
> > And during bisection, I encountered 3 different failure modes...
> >
> > To be continued...
>
> The longest generic node names documented in the Devicetree
> Specification are "air-pollution-sensor" and "interrupt-controller"
> (both counting 20 characters), so a typical device name needs 8
> (32-bit unit address) + 1 (dot) + 20 = 29 characters.
> However, I assume some devices lie outside the 32-bit address space,
> and thus need more space?
>
> With the BeagleBone Black boot issue fixed:
> "/devices/platform/ocp/5600fe00.target-module"
> resp. "/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@200000/44e3e074.target-module"
> are now shortened to "5600fe00.target-module" resp. "44e3e074.target-module".
> However, "/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@200000/48000000.interconnect:segment@200000:target-module@0"
> is shortened to "48000000.interconnect:segment@200000:target-module@0",
> which is still longer than the old column width...

Should we really care about those silly long names? And are those
really a problem from genpd debugfs point of view?

That said, I don't have a suggestion for a new value of the table
width, but I am certainly open to adjusting it to whatever you
propose.

Kind regards
Uffe