Re: [PATCH v2 2/4] pmdomain: Respect the CPU system-wakeup QoS limit during s2idle

[Kevin Hilman <khilman@baylibre.com>]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Sat, 1 Nov 2025 at 01:11, Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> > A CPU system-wakeup QoS limit may have been requested by user-space. To
>> > avoid breaking this constraint when entering a low-power state during
>> > s2idle through genpd, let's extend the corresponding genpd governor for
>> > CPUs. More precisely, during s2idle let the genpd governor select a
>> > suitable low-power state, by taking into account the QoS limit.
>>
>> In addition to a QoS limit requested by userspace, shouldn't any
>> per-device QoS limits from devices in the PM domain with CPUs/clusters
>> also be considered?
>>
>> Right now, if a device is in a PM domain that also contains CPUs, any
>> per-device QoS constraints for those devices should also impact the
>> state chosen by s2idle.
>
> I am not so sure about that. The existing dev PM QoS latency is
> targeted towards runtime suspend of a device and the genpd governor
> also takes it into account for this use case.
>
> If we would start to take the same dev PM QoS latency constraint into
> account for system suspend (s2idle), it may not be what the user
> really intended. Instead, I think we should consider extending the dev
> PM QoS interface, to allow the user to set a specific latency limit
> for system wakeup. Then the genpd governor should take that into
> account for s2idle.
>>
>> I just tried a quick hack of extending you cpu_system_power_down_ok()
>> function to look for any per-device QoS constraints set all devices in
>> the PM domain (and subdomains).  It takes the min of all the per-device
>> QoS constratins, compares it to the one from userspace, and uses the min
>> of those to decide the genpd state.
>>
>> That has the effect I'm after, but curious what you think about the
>> usecase and the idea?
>
> It makes sense, but as stated above, I think we need a new QoS limit
> specific for system suspend.

OK, got it.  I understand the need to distinguish between QoS
constraints set for runtime PM and system-wide suspend/resume.

So, instead of adding a completely separate entry for system-wide
suspend, and duplicating a bunch of code/API, what about using the QoS
flags, and adding a new flag that indicates if the resume_latency
constraint applies only to runtime PM (the default) or if it *also*
applies to system-wide suspend? (RFC patch below[1])

With this, I was able to extend your new cpu_system_power_down_ok()
function to check for any devices in the domain with a resume_latency
*and* this new flag before applying it to s2idle, and that's working
great locally.

Kevin

[1]
From d25b871c2044ee0e993fd75f5f1df36a35633d1f Mon Sep 17 00:00:00 2001
From: "Kevin Hilman (TI.com)" <khilman@baylibre.com>
Date: Thu, 13 Nov 2025 17:02:38 -0800
Subject: [RFC/PATCH] PM / QoS: add flag to indicate latency applies
 system-wide

Add new PM_QOS_FLAG_LATENCY_SYS flag to indicate that the
resume_latency QoS constraint applies not just to runtime PM
transitions but also to system-wide suspend/s2idle.

Signed-off-by: Kevin Hilman (TI.com) <khilman@baylibre.com>
---
 drivers/base/power/sysfs.c | 27 +++++++++++++++++++++++++++
 include/linux/pm_qos.h     |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 13b31a3adc77..9136c13c17e4 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -316,6 +316,32 @@ static ssize_t pm_qos_no_power_off_store(struct device *dev,
 
 static DEVICE_ATTR_RW(pm_qos_no_power_off);
 
+static ssize_t pm_qos_latency_sys_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return sysfs_emit(buf, "%d\n", !!(dev_pm_qos_requested_flags(dev)
+					  & PM_QOS_FLAG_LATENCY_SYS));
+}
+
+static ssize_t pm_qos_latency_sys_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t n)
+{
+	int ret;
+
+	if (kstrtoint(buf, 0, &ret))
+		return -EINVAL;
+
+	if (ret != 0 && ret != 1)
+		return -EINVAL;
+
+	ret = dev_pm_qos_update_flags(dev, PM_QOS_FLAG_LATENCY_SYS, ret);
+	return ret < 0 ? ret : n;
+}
+
+static DEVICE_ATTR_RW(pm_qos_latency_sys);
+
 #ifdef CONFIG_PM_SLEEP
 static const char _enabled[] = "enabled";
 static const char _disabled[] = "disabled";
@@ -681,6 +707,7 @@ static const struct attribute_group pm_qos_latency_tolerance_attr_group = {
 
 static struct attribute *pm_qos_flags_attrs[] = {
 	&dev_attr_pm_qos_no_power_off.attr,
+	&dev_attr_pm_qos_latency_sys.attr,
 	NULL,
 };
 static const struct attribute_group pm_qos_flags_attr_group = {
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 4a69d4af3ff8..b95f52775dfb 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -37,6 +37,8 @@ enum pm_qos_flags_status {
 #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT	(-1)
 
 #define PM_QOS_FLAG_NO_POWER_OFF	(1 << 0)
+/* latency value applies to system-wide suspend/s2idle */
+#define PM_QOS_FLAG_LATENCY_SYS		(2 << 0)
 
 enum pm_qos_type {
 	PM_QOS_UNITIALIZED,
-- 
2.51.0