Thread (17 messages) 17 messages, 6 authors, 2017-10-27

Re: [PATCH] PM / QoS: Fix device resume latency PM QoS

From: Ramesh Thomas <hidden>
Date: 2017-10-24 06:00:46
Also in: lkml

On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:
quoted hunk ↗ jump to hunk
From: Rafael J. Wysocki <redacted>

 
 static ssize_t pm_qos_resume_latency_store(struct device *dev,
@@ -228,11 +235,19 @@ static ssize_t pm_qos_resume_latency_sto
 	s32 value;
 	int ret;
 
-	if (kstrtos32(buf, 0, &value))
-		return -EINVAL;
+	if (!kstrtos32(buf, 0, &value)) {
+		/*
+		 * Prevent users from writing negative or "no constraint" values
+		 * directly.
+		 */
+		if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
+			return -EINVAL;
 
-	if (value < 0)
-		return -EINVAL;
+		if (value == 0)
+			value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+	} else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
Can the 2 checks for "n/a" be combined by checking first 3 characters?
+		value = 0;
+	}
Should there be a check for kstrtos32 failure and return -EINVAL?
 
 	ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
 					value);
quoted hunk ↗ jump to hunk
Index: linux-pm/drivers/base/power/domain_governor.c
===================================================================
--- linux-pm.orig/drivers/base/power/domain_governor.c
+++ linux-pm/drivers/base/power/domain_governor.c
@@ -14,23 +14,20 @@
 static int dev_update_qos_constraint(struct device *dev, void *data)
 {
 	s64 *constraint_ns_p = data;
-	s32 constraint_ns = -1;
+	s64 constraint_ns = -1;
 
 	if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
 		constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
 
-	if (constraint_ns < 0) {
+	if (constraint_ns < 0)
 		constraint_ns = dev_pm_qos_read_value(dev);
-		constraint_ns *= NSEC_PER_USEC;
-	}
-	if (constraint_ns == 0)
+
+	if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
 		return 0;
 
-	/*
-	 * constraint_ns cannot be negative here, because the device has been
-	 * suspended.
-	 */
-	if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
+	constraint_ns *= NSEC_PER_USEC;
+
+	if (constraint_ns < *constraint_ns_p || *constraint_ns_p < 0)
 		*constraint_ns_p = constraint_ns;
 
 	return 0;
@@ -63,10 +60,14 @@ static bool default_suspend_ok(struct de
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
-	if (constraint_ns < 0)
+	if (constraint_ns == 0)
 		return false;
 
-	constraint_ns *= NSEC_PER_USEC;
+	if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
+		constraint_ns = -1;
+	else
+		constraint_ns *= NSEC_PER_USEC;
+
 	/*
 	 * We can walk the children without any additional locking, because
 	 * they all have been suspended at this point and their
@@ -76,14 +77,19 @@ static bool default_suspend_ok(struct de
 		device_for_each_child(dev, &constraint_ns,
 				      dev_update_qos_constraint);
 
-	if (constraint_ns > 0) {
-		constraint_ns -= td->suspend_latency_ns +
-				td->resume_latency_ns;
-		if (constraint_ns == 0)
-			return false;
+	if (constraint_ns < 0) {
+		/* The children have no constraints. */
+		td->effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+		td->cached_suspend_ok = true;
+	} else {
+		constraint_ns -= td->suspend_latency_ns + td->resume_latency_ns;
+		if (constraint_ns > 0) {
+			td->effective_constraint_ns = constraint_ns;
+			td->cached_suspend_ok = true;
+		} else {
+			td->effective_constraint_ns = 0;
Previously effective_constraint_ns was left as -1 if constraint_ns becomes 0
Not sure if this change is intentional. I think at dev_update_qos_constraint,
this can cause to skip call to dev_pm_qos_read_value. 
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help