From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933766AbdKAXEG (ORCPT ); Wed, 1 Nov 2017 19:04:06 -0400 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:50826 "EHLO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752129AbdKAXED (ORCPT ); Wed, 1 Nov 2017 19:04:03 -0400 From: "Rafael J. Wysocki" To: Linux PM Cc: LKML , Ulf Hansson , Geert Uytterhoeven , Tero Kristo , Reinette Chatre , Ramesh Thomas , Alex Shi Subject: [RFT][PATCH 1/2] PM / domains: Rework governor code to be more consistent Date: Thu, 02 Nov 2017 00:01:50 +0100 Message-ID: <2099930.xGFd9X30dN@aspire.rjw.lan> In-Reply-To: <5770848.Kdi5IjVKeE@aspire.rjw.lan> References: <5770848.Kdi5IjVKeE@aspire.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Rafael J. Wysocki The genpd governor currently uses negative PM QoS values to indicate the "no suspend" condition and 0 as "no restriction", but it doesn't use them consistently. Moreover, it tries to refresh QoS values for already suspended devices in a quite questionable way. For the above reasons, rework it to be a bit more consistent. First off, note that dev_pm_qos_read_value() in dev_update_qos_constraint() and __default_power_down_ok() is evaluated for devices in suspend. Moreover, that only happens if the effective_constraint_ns value for them is negative (meaning "no suspend"). It is not evaluated in any other cases, so effectively the QoS values are only updated for devices in suspend that should not have been suspended in the first place. In all of the other cases, the QoS values taken into account are the effective ones from the time before the device has been suspended, so generally devices need to be resumed and suspended again for new QoS values to take effect anyway. Thus evaluating dev_update_qos_constraint() in those two places doesn't make sense at all, so drop it. Second, initialize effective_constraint_ns to 0 ("no constraint") rather than to (-1) ("no suspend"), which makes more sense in general and in case effective_constraint_ns is never updated (the device is in suspend all the time or it is never suspended) it doesn't affect the device's parent and so on. Finally, rework default_suspend_ok() to explicitly handle the "no restriction" and "no suspend" special cases. Also add WARN_ON() around checks that should never trigger. Signed-off-by: Rafael J. Wysocki --- drivers/base/power/domain.c | 2 - drivers/base/power/domain_governor.c | 56 ++++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 25 deletions(-) Index: linux-pm/drivers/base/power/domain.c =================================================================== --- linux-pm.orig/drivers/base/power/domain.c +++ linux-pm/drivers/base/power/domain.c @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge gpd_data->base.dev = dev; gpd_data->td.constraint_changed = true; - gpd_data->td.effective_constraint_ns = -1; + gpd_data->td.effective_constraint_ns = 0; gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; spin_lock_irq(&dev->power.lock); 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,22 +14,22 @@ static int dev_update_qos_constraint(struct device *dev, void *data) { s64 *constraint_ns_p = data; - s32 constraint_ns = -1; - - if (dev->power.subsys_data && dev->power.subsys_data->domain_data) - constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns; + s64 constraint_ns; - if (constraint_ns < 0) { - constraint_ns = dev_pm_qos_read_value(dev); - constraint_ns *= NSEC_PER_USEC; - } - if (constraint_ns == 0) + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) return 0; /* - * constraint_ns cannot be negative here, because the device has been - * suspended. + * Only take suspend-time QoS constraints of devices into account, + * because constraints updated after the device has been suspended are + * not guaranteed to be taken into account anyway. In order for them + * to take effect, the device has to be resumed and suspended again. */ + constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns; + /* 0 means "no constraint" */ + if (constraint_ns == 0) + return 0; + if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0) *constraint_ns_p = constraint_ns; @@ -76,14 +76,26 @@ static bool default_suspend_ok(struct de device_for_each_child(dev, &constraint_ns, dev_update_qos_constraint); - if (constraint_ns > 0) { + if (constraint_ns == 0) { + /* "No restriction", so the device is allowed to suspend. */ + td->effective_constraint_ns = 0; + td->cached_suspend_ok = true; + } else if (WARN_ON(constraint_ns < 0)) { + /* + * effective_constraint_ns is negative already and + * cached_suspend_ok is false, so return right away. + */ + return false; + } else { constraint_ns -= td->suspend_latency_ns + td->resume_latency_ns; - if (constraint_ns == 0) + /* Again, if negative or zero, returning right away is fine. */ + if (constraint_ns <= 0) return false; + + td->effective_constraint_ns = constraint_ns; + td->cached_suspend_ok = true; } - td->effective_constraint_ns = constraint_ns; - td->cached_suspend_ok = constraint_ns >= 0; /* * The children have been suspended already, so we don't need to take @@ -144,18 +156,14 @@ static bool __default_power_down_ok(stru */ td = &to_gpd_data(pdd)->td; constraint_ns = td->effective_constraint_ns; - /* default_suspend_ok() need not be called before us. */ - if (constraint_ns < 0) { - constraint_ns = dev_pm_qos_read_value(pdd->dev); - constraint_ns *= NSEC_PER_USEC; - } + /* Negative values mean "no suspend at all" */ + if (WARN_ON(constraint_ns < 0)) + return false; + + /* 0 means "no constraint" */ if (constraint_ns == 0) continue; - /* - * constraint_ns cannot be negative here, because the device has - * been suspended. - */ if (constraint_ns <= off_on_time_ns) return false;