On Wed, Oct 25, 2017 at 9:27 AM, Ramesh Thomas [off-list ref] wrote:

On 2017-10-24 at 13:23:23 +0200, Rafael J. Wysocki wrote:

quoted

On Tue, Oct 24, 2017 at 10:49 AM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Tuesday, October 24, 2017 7:54:09 AM CEST Ramesh Thomas wrote:

quoted

On 2017-10-20 at 13:27:34 +0200, Rafael J. Wysocki wrote:

quoted

From: Rafael J. Wysocki <redacted>

[cut]

quoted

quoted

quoted

@@ -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.

Yes, it is.

quoted

I think at dev_update_qos_constraint, this can cause to skip call to
dev_pm_qos_read_value.

I need to double check that.

If constraint_ns becomes 0 (or less) here, power cannot be removed
from the device, because it would add an unacceptable latency.

Thus effective_constraint_ns has to be 0 for it to indicate that
situation.  If it was left at -1, it would mean "no requirement", but
that wouldn't be correct.

A negative value in effective_constraint_ns is used as trigger to read new
resume latency constraints.

I guess you mean in __default_power_down_ok(), right?

That doesn't matter, because it covers the case when the device has
never been runtime-suspended: it started in the "suspended" state and
has never been made "active".

The case we are talking about is when default_suspend_ok() *was* run
and it returned "true", or the device would not have been suspended,
so __default_power_down_ok() would not have run for that domain at
all.  In that case effective_constraint has to be positive anyway,
because that is the only case when default_suspend_ok() returns
"true".

It matters in default_suspend_ok() itself, however, where the
constraints for the children are checked and -1 means "no
restriction".  So it still looks like the patch needs to be improved,
but that's because effective_constraint should not remain -1 if
constraint_ns is 0 (which it still does in one case).

Thanks,
Rafael