From patchwork Mon Jan 13 09:40:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: You-Sheng Yang X-Patchwork-Id: 1222033 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47x7rB5r96z9sQp; Mon, 13 Jan 2020 20:40:58 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1iqwDO-0003iB-OZ; Mon, 13 Jan 2020 09:40:54 +0000 Received: from mail-pg1-f193.google.com ([209.85.215.193]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1iqwDL-0003fD-UW for kernel-team@lists.ubuntu.com; Mon, 13 Jan 2020 09:40:52 +0000 Received: by mail-pg1-f193.google.com with SMTP id s64so4432823pgb.9 for ; Mon, 13 Jan 2020 01:40:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=WRU9on4o6ufpoKEZ4IOkLYt/5JNU0RwF2h9Y62BPAmQ=; b=EXwn6fRLXKVz/sgVmScpwovjHK+1U5ocfNBFvob1aS7YY9/wf1vKILd7CxukiS3xTm sH14Wix8F0sB2A0s3u0GixNco4pRIpFeiChWb6fPmOEqIUTID+VixHRvGbAx5NIxMqc7 c6qtljUVdhWPf4kEWDEYKbD4+/Q9Prf3f/vxpKvblH+onH0jdHBeke3nCCLD4oVNyk81 HWq6g+N2P3rS0SfJ8XJZpT/m9RN42ryDUkgqySUWvaWd+JNbnR3wdTlGWRe+9v8sLxIN ZFkHEj30vZ3z2pn86cebmuEgwc+fkGVR8kwPtLDoZNqkcs7mPSAAs1GkWTNZiwAESe38 ovcQ== X-Gm-Message-State: APjAAAVkohNWvK/lDpVaMoA0nolDPKEfeJ/d9GGh4uEgCN552UZjDTuW a9B1J42Q1UY5/+GMbEoKwwFR+7tSkAQ= X-Google-Smtp-Source: APXvYqz8XoQhXm/pwqbtRWjgcqqfow3/n5C9zozWPm1rlya4f/5RjDXVwEqECZJhILropfADawTXcw== X-Received: by 2002:a65:6815:: with SMTP id l21mr19067699pgt.283.1578908449648; Mon, 13 Jan 2020 01:40:49 -0800 (PST) Received: from localhost (61-220-137-37.HINET-IP.hinet.net. [61.220.137.37]) by smtp.gmail.com with ESMTPSA id b4sm13664846pfd.18.2020.01.13.01.40.48 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jan 2020 01:40:48 -0800 (PST) From: You-Sheng Yang To: kernel-team@lists.ubuntu.com Subject: [SRU][OEM-OSP1-B/E][PATCH 5/6] PM: sleep: Simplify suspend-to-idle control flow Date: Mon, 13 Jan 2020 17:40:33 +0800 Message-Id: <20200113094034.510111-6-vicamo.yang@canonical.com> X-Mailer: git-send-email 2.24.0 In-Reply-To: <20200113094034.510111-1-vicamo.yang@canonical.com> References: <20200113094034.510111-1-vicamo.yang@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: "Rafael J. Wysocki" BugLink: https://bugs.launchpad.net/bugs/1859407 After commit 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle") the "noirq" phases of device suspend and resume may run for multiple times during suspend-to-idle, if there are spurious system wakeup events while suspended. However, this is complicated and fragile and actually unnecessary. The main reason for doing this is that on some systems the EC may signal system wakeup events (power button events, for example) as well as events that should not cause the system to resume (spurious system wakeup events). Thus, in order to determine whether or not a given event signaled by the EC while suspended is a proper system wakeup one, the EC GPE needs to be dispatched and to start with that was achieved by allowing the ACPI SCI action handler to run, which was only possible after calling resume_device_irqs(). However, dispatching the EC GPE this way turned out to take too much time in some cases and some EC events might be missed due to that, so commit 68e22011856f ("ACPI: EC: Dispatch the EC GPE directly on s2idle wake") started to dispatch the EC GPE right after a wakeup event has been detected, so in fact the full ACPI SCI action handler doesn't need to run any more to deal with the wakeups coming from the EC. Use this observation to simplify the suspend-to-idle control flow so that the "noirq" phases of device suspend and resume are each run only once in every suspend-to-idle cycle, which is reported to significantly reduce power drawn by some systems when suspended to idle (by allowing them to reach a deep platform-wide low-power state through the suspend-to-idle flow). [What appears to happen is that the "noirq" resume of devices after a spurious EC wakeup brings some devices into a state in which they prevent the platform from reaching the deep low-power state going forward, even after a subsequent "noirq" suspend phase, and on some systems the EC triggers such wakeups already when the "noirq" suspend of devices is running for the first time in the given suspend/resume cycle, so the platform cannot reach the deep low-power state at all.] First, make acpi_s2idle_wake() use the acpi_ec_dispatch_gpe() return value to determine whether or not the wakeup may have been triggered by the EC (in which case the system wakeup is canceled and ACPI events are processed in order to determine whether or not the event is a proper system wakeup one) and use rearm_wake_irq() (introduced by a previous change) in it to rearm the ACPI SCI for system wakeup detection in case the system will remain suspended. Second, drop acpi_s2idle_sync(), which is not needed any more, and the corresponding global platform suspend-to-idle callback. Next, drop the pm_wakeup_pending() check (which is an optimization only) from __device_suspend_noirq() to prevent it from returning errors on system wakeups occurring before the "noirq" phase of device suspend is complete (as in the case of suspend-to-idle it is not known whether or not these wakeups are suprious at that point), in order to avoid having to carry out a "noirq" resume of devices on a spurious system wakeup. Finally, change the code flow in s2idle_loop() to (1) run the "noirq" suspend of devices once before starting the loop, (2) check for spurious EC wakeups (via the platform ->wake callback) for the first time before calling s2idle_enter(), and (3) run the "noirq" resume of devices once after leaving the loop. Signed-off-by: Rafael J. Wysocki Acked-by: Thomas Gleixner (cherry picked from commit 56b991849009f5def0443bfb2f48c8321d888e15) Signed-off-by: You-Sheng Yang --- drivers/acpi/sleep.c | 47 ++++++++++++++++++---------------- drivers/base/power/main.c | 5 ---- include/linux/suspend.h | 1 - kernel/power/suspend.c | 53 +++++++++++++++++---------------------- 4 files changed, 48 insertions(+), 58 deletions(-) diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index ac428405e318b..9428f94fb4dfa 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -996,33 +996,37 @@ static void acpi_s2idle_wake(void) lpi_check_constraints(); /* - * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means - * that the SCI has triggered while suspended, so cancel the wakeup in - * case it has not been a wakeup event (the GPEs will be checked later). + * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has + * not triggered while suspended, so bail out. */ - if (acpi_sci_irq_valid() && - !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) { + if (!acpi_sci_irq_valid() || + irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) + return; + + /* + * If there are EC events to process, the wakeup may be a spurious one + * coming from the EC. + */ + if (acpi_ec_dispatch_gpe()) { + /* + * Cancel the wakeup and process all pending events in case + * there are any wakeup ones in there. + * + * Note that if any non-EC GPEs are active at this point, the + * SCI will retrigger after the rearming below, so no events + * should be missed by canceling the wakeup here. + */ pm_system_cancel_wakeup(); /* - * On some platforms with the LPS0 _DSM device noirq resume - * takes too much time for EC wakeup events to survive, so look - * for them now. + * The EC driver uses the system workqueue and an additional + * special one, so those need to be flushed too. */ - acpi_ec_dispatch_gpe(); + acpi_os_wait_events_complete(); /* synchronize EC GPE processing */ + acpi_ec_flush_work(); + acpi_os_wait_events_complete(); /* synchronize Notify handling */ } -} -static void acpi_s2idle_sync(void) -{ - /* - * Process all pending events in case there are any wakeup ones. - * - * The EC driver uses the system workqueue and an additional special - * one, so those need to be flushed too. - */ - acpi_os_wait_events_complete(); /* synchronize SCI IRQ handling */ - acpi_ec_flush_work(); - acpi_os_wait_events_complete(); /* synchronize Notify handling */ + rearm_wake_irq(acpi_sci_irq); } static void acpi_s2idle_restore(void) @@ -1054,7 +1058,6 @@ static const struct platform_s2idle_ops acpi_s2idle_ops = { .begin = acpi_s2idle_begin, .prepare = acpi_s2idle_prepare, .wake = acpi_s2idle_wake, - .sync = acpi_s2idle_sync, .restore = acpi_s2idle_restore, .end = acpi_s2idle_end, }; diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 7900debc5ce44..ca3c8700c1f96 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1299,11 +1299,6 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a if (async_error) goto Complete; - if (pm_wakeup_pending()) { - async_error = -EBUSY; - goto Complete; - } - if (dev->power.syscore || dev->power.direct_complete) goto Complete; diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 50e81bff37df5..7d0dfb1ea34e6 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -191,7 +191,6 @@ struct platform_s2idle_ops { int (*begin)(void); int (*prepare)(void); void (*wake)(void); - void (*sync)(void); void (*restore)(void); void (*end)(void); }; diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 07a32e99aa644..09ac6da691ae2 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -115,48 +115,41 @@ static void s2idle_enter(void) static void s2idle_loop(void) { + int error; + + dpm_noirq_begin(); + error = dpm_noirq_suspend_devices(PMSG_SUSPEND); + if (error) + goto resume; + pm_pr_dbg("suspend-to-idle\n"); + /* + * Suspend-to-idle equals: + * frozen processes + suspended devices + idle processors. + * Thus s2idle_enter() should be called right after all devices have + * been suspended. + * + * Wakeups during the noirq suspend of devices may be spurious, so try + * to avoid them upfront. + */ for (;;) { - int error; - - dpm_noirq_begin(); - - /* - * Suspend-to-idle equals - * frozen processes + suspended devices + idle processors. - * Thus s2idle_enter() should be called right after - * all devices have been suspended. - * - * Wakeups during the noirq suspend of devices may be spurious, - * so prevent them from terminating the loop right away. - */ - error = dpm_noirq_suspend_devices(PMSG_SUSPEND); - if (!error) - s2idle_enter(); - else if (error == -EBUSY && pm_wakeup_pending()) - error = 0; - - if (!error && s2idle_ops && s2idle_ops->wake) + if (s2idle_ops && s2idle_ops->wake) s2idle_ops->wake(); - dpm_noirq_resume_devices(PMSG_RESUME); - - dpm_noirq_end(); - - if (error) - break; - - if (s2idle_ops && s2idle_ops->sync) - s2idle_ops->sync(); - if (pm_wakeup_pending()) break; pm_wakeup_clear(false); + + s2idle_enter(); } pm_pr_dbg("resume from suspend-to-idle\n"); + +resume: + dpm_noirq_resume_devices(PMSG_RESUME); + dpm_noirq_end(); } void s2idle_wake(void)