From patchwork Mon Jan 13 09:40:34 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: 1222034 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 47x7rF0wgfz9sQp; Mon, 13 Jan 2020 20:41:01 +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 1iqwDR-0003jt-6w; Mon, 13 Jan 2020 09:40:57 +0000 Received: from mail-pf1-f194.google.com ([209.85.210.194]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1iqwDO-0003hH-59 for kernel-team@lists.ubuntu.com; Mon, 13 Jan 2020 09:40:54 +0000 Received: by mail-pf1-f194.google.com with SMTP id z16so4625258pfk.0 for ; Mon, 13 Jan 2020 01:40:54 -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=wj4YdDSqKlVjjm02j3PydpyO6EM/rfH69P195foZXZs=; b=dXG19FTXeVkaR33DFMfcYeK8CjARfrt9w844jDg3h3WXO6hMrOeY4OxdL+f2sW0w1W xgSMVUiihvBa1g24l0U43MS1AMZREAwZVXdC65xYA4LdVImdYIq1wUsOe8G6obSASKL5 lbcCpOQMsh2YW1un2iLq7fUAx+nscuzJdZZIK+SNmazqAx3GOeoy+PATYpCvzWkIJ0hp ovvF2KU8K2v0Lzls2gjaD/XzMrKEJ4+7nuCTqF7ss6u/zmNQIXRrCIQccbdpkQCCheIm mVE+ACo41IC3YDymyYy1pi57yKI0nZGBng7nda4IArUv53tJlqjIvSUy2P+OOakFM1gr FudQ== X-Gm-Message-State: APjAAAVm8pewW63Hz9mxTDEiSyh13gMQnxj555M4kupXsXdbrBGim5dE g+SNSbd/aWnRewa+a8ghDQy2i/AqXNc= X-Google-Smtp-Source: APXvYqw7GFLIdtScFd4BXFvMMIr7XetQXTkcsZmSGxvyb6tWkRoUytteGCELfWEcBAXfNoJGW0nvsQ== X-Received: by 2002:a63:504f:: with SMTP id q15mr19706076pgl.8.1578908451974; Mon, 13 Jan 2020 01:40:51 -0800 (PST) Received: from localhost (61-220-137-37.HINET-IP.hinet.net. [61.220.137.37]) by smtp.gmail.com with ESMTPSA id u10sm12796273pgg.41.2020.01.13.01.40.50 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jan 2020 01:40:51 -0800 (PST) From: You-Sheng Yang To: kernel-team@lists.ubuntu.com Subject: [SRU][OEM-OSP1-B/E][PATCH 6/6] ACPI: EC: Rework flushing of pending work Date: Mon, 13 Jan 2020 17:40:34 +0800 Message-Id: <20200113094034.510111-7-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 There is a race condition in the ACPI EC driver, between __acpi_ec_flush_event() and acpi_ec_event_handler(), that may cause systems to stay in suspended-to-idle forever after a wakeup event coming from the EC. Namely, acpi_s2idle_wake() calls acpi_ec_flush_work() to wait until the delayed work resulting from the handling of the EC GPE in acpi_ec_dispatch_gpe() is processed, and that function invokes __acpi_ec_flush_event() which uses wait_event() to wait for ec->nr_pending_queries to become zero on ec->wait, and that wait queue may be woken up too early. Suppose that acpi_ec_dispatch_gpe() has caused acpi_ec_gpe_handler() to run, so advance_transaction() has been called and it has invoked acpi_ec_submit_query() to queue up an event work item, so ec->nr_pending_queries has been incremented (under ec->lock). The work function of that work item, acpi_ec_event_handler() runs later and calls acpi_ec_query() to process the event. That function calls acpi_ec_transaction() which invokes acpi_ec_transaction_unlocked() and the latter wakes up ec->wait under ec->lock, but it drops that lock before returning. When acpi_ec_query() returns, acpi_ec_event_handler() acquires ec->lock and decrements ec->nr_pending_queries, but at that point __acpi_ec_flush_event() (woken up previously) may already have acquired ec->lock, checked the value of ec->nr_pending_queries (and it would not have been zero then) and decided to go back to sleep. Next, if ec->nr_pending_queries is equal to zero now, the loop in acpi_ec_event_handler() terminates, ec->lock is released and acpi_ec_check_event() is called, but it does nothing unless ec_event_clearing is equal to ACPI_EC_EVT_TIMING_EVENT (which is not the case by default). In the end, if no more event work items have been queued up while executing acpi_ec_transaction_unlocked(), there is nothing to wake up __acpi_ec_flush_event() again and it sleeps forever, so the suspend-to-idle loop cannot make progress and the system is permanently suspended. To avoid this issue, notice that it actually is not necessary to wait for ec->nr_pending_queries to become zero in every case in which __acpi_ec_flush_event() is used. First, during platform-based system suspend (not suspend-to-idle), __acpi_ec_flush_event() is called by acpi_ec_disable_event() after clearing the EC_FLAGS_QUERY_ENABLED flag, which prevents acpi_ec_submit_query() from submitting any new event work items, so calling flush_scheduled_work() and flushing ec_query_wq subsequently (in order to wait until all of the queries in that queue have been processed) would be sufficient to flush all of the pending EC work in that case. Second, the purpose of the flushing of pending EC work while suspended-to-idle described above really is to wait until the first event work item coming from acpi_ec_dispatch_gpe() is complete, because it should produce system wakeup events if that is a valid EC-based system wakeup, so calling flush_scheduled_work() followed by flushing ec_query_wq is also sufficient for that purpose. Rework the code to follow the above observations. Fixes: 56b9918490 ("PM: sleep: Simplify suspend-to-idle control flow") Reported-by: Kenneth R. Crudup Tested-by: Kenneth R. Crudup Cc: 5.4+ # 5.4+ Signed-off-by: Rafael J. Wysocki (cherry picked from commit 016b87ca5c8c6e9e87db442f04dc99609b11ed36) Signed-off-by: You-Sheng Yang --- drivers/acpi/ec.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 17c416aa46a68..9b193a3193380 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -535,26 +535,10 @@ static void acpi_ec_enable_event(struct acpi_ec *ec) } #ifdef CONFIG_PM_SLEEP -static bool acpi_ec_query_flushed(struct acpi_ec *ec) +static void __acpi_ec_flush_work(void) { - bool flushed; - unsigned long flags; - - spin_lock_irqsave(&ec->lock, flags); - flushed = !ec->nr_pending_queries; - spin_unlock_irqrestore(&ec->lock, flags); - return flushed; -} - -static void __acpi_ec_flush_event(struct acpi_ec *ec) -{ - /* - * When ec_freeze_events is true, we need to flush events in - * the proper position before entering the noirq stage. - */ - wait_event(ec->wait, acpi_ec_query_flushed(ec)); - if (ec_query_wq) - flush_workqueue(ec_query_wq); + flush_scheduled_work(); /* flush ec->work */ + flush_workqueue(ec_query_wq); /* flush queries */ } static void acpi_ec_disable_event(struct acpi_ec *ec) @@ -564,15 +548,21 @@ static void acpi_ec_disable_event(struct acpi_ec *ec) spin_lock_irqsave(&ec->lock, flags); __acpi_ec_disable_event(ec); spin_unlock_irqrestore(&ec->lock, flags); - __acpi_ec_flush_event(ec); + + /* + * When ec_freeze_events is true, we need to flush events in + * the proper position before entering the noirq stage. + */ + __acpi_ec_flush_work(); } void acpi_ec_flush_work(void) { - if (first_ec) - __acpi_ec_flush_event(first_ec); + /* Without ec_query_wq there is nothing to flush. */ + if (!ec_query_wq) + return; - flush_scheduled_work(); + __acpi_ec_flush_work(); } #endif /* CONFIG_PM_SLEEP */