Received: by 2002:a05:6a10:a0d1:0:0:0:0 with SMTP id j17csp494039pxa; Fri, 21 Aug 2020 12:35:46 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwXgE49ivMdtEnFcCvLSfCMxUXIE8QyWTsEwWwSkB27EzvDoJpTHr2rcLp0ZL+Kg3O5F2DS X-Received: by 2002:a17:906:393:: with SMTP id b19mr3446960eja.268.1598038545781; Fri, 21 Aug 2020 12:35:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1598038545; cv=none; d=google.com; s=arc-20160816; b=V5SpcNeb9ORbdGux5pyKJrg3nonvtGYlsqMfOCuJFjGOeNfdL9xAqYhGGAjAdcIlw0 jv3PU+TbjjLRo57suYgVPZTAfdFffnwJYSvU7fw0jWjKLMHq2OfEIv9EWTOYjWym+6+4 FOYRqlRL6W+e4+VClt963sQ435Gly0v0utjITECNoaz1jtcnz00LtGFHYvzTJtFlYy4W nUxUPOK+sfhFhCjbfyjjB6lxnzlgI4aUY14h3p7fyfmzN8/yk6fLcuC1Kp71uCS2KTfM 5bWRIj+cXdgv28UvFcXW86kT/yPs2HpAbtyeiEjQr72Cg7yzMT66MLqnX3S5gnHS2Ngv Ts3w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=DOTFIyKar/raFad0rjEcLwk+Dl8xGAMiKLNXHI6Sojg=; b=Ybb1gqY++PvHttEq+TfNkH9oYakBAdGHDq7T50731SBDfzUsfLKgaT/rP1sI3x2g9f vS+G2ydERGU+3+P6gsxLJQ6F+7YmdVf/TAQzBIAJDdM5fPmyHk61gzwZXpUec2Y+5Zt+ ktNwk4/ViLGwvGy2+iuzV+6eLHnwpDZAD8IDsPLR1jtYQwZA2ojQLQ9oolFfyS/RBtPo Y13wmIFY52FW+PXOiVYkF0Y4TYpPZKSeoPKjEB/Dk8JrC48TLie6+oneH3zxGxUB6y2o M/xg3XwwLltd+96vNlsIm/lsVGg9u8C97lkAYk/tHxiF3hc5ZYsNlLMlzt7yk1t8ayUY IXPw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id nv2si1896713ejb.154.2020.08.21.12.35.21; Fri, 21 Aug 2020 12:35:45 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726358AbgHUTeo (ORCPT + 99 others); Fri, 21 Aug 2020 15:34:44 -0400 Received: from netrider.rowland.org ([192.131.102.5]:58621 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1725831AbgHUTeo (ORCPT ); Fri, 21 Aug 2020 15:34:44 -0400 Received: (qmail 266418 invoked by uid 1000); 21 Aug 2020 15:34:42 -0400 Date: Fri, 21 Aug 2020 15:34:42 -0400 From: Alan Stern To: "Rafael J. Wysocki" Cc: Linux PM , LKML , Linux ACPI , Greg Kroah-Hartman , "Rafael J. Wysocki" , Mika Westerberg Subject: Re: [PATCH] PM: sleep: core: Fix the handling of pending runtime resume requests Message-ID: <20200821193442.GA264863@rowland.harvard.edu> References: <7969920.MVx1BpXlEM@kreacher> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7969920.MVx1BpXlEM@kreacher> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Aug 21, 2020 at 07:41:02PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > It has been reported that system-wide suspend may be aborted in the > absence of any wakeup events due to unforseen interactions of it with > the runtume PM framework. > > One failing scenario is when there are multiple devices sharing an > ACPI power resource and runtime-resume needs to be carried out for > one of them during system-wide suspend (for example, because it needs > to be reconfigured before the whole system goes to sleep). In that > case, the runtime-resume of that device involves turning the ACPI > power resource "on" which in turn causes runtime resume requests > to be queued up for all of the other devices sharing it. Those > requests go to the runtime PM workqueue which is frozen during > system-wide suspend, so they are not actually taken care of until > the resume of the whole system, but the pm_runtime_barrier() > call in __device_suspend() sees them and triggers system wakeup > events for them which then cause the system-wide suspend to be > aborted if wakeup source objects are in active use. > > Of course, the logic that leads to triggering those wakeup events is > questionable in the first place, because clearly there are cases in > which a pending runtime resume request for a device is not connected > to any real wakeup events in any way (like the one above). Moreover, > if there is a pending runtime resume request for a device while > __device_suspend() is running for it, the physical state of the > device may not be in agreement with the "suspended" runtime PM status > of it (which may be the very reason for queuing up the runtime resume > request for it). > > For these reasons, rework __device_suspend() to carry out synchronous > runtime-resume for devices with pending runtime resume requests before > attempting to invoke system-wide suspend callbacks for them with the > expectation that their drivers will trigger system-wide wakeup events > in the process of handling the runtime resume, if necessary. > > Fixes: 1e2ef05bb8cf8 ("PM: Limit race conditions between runtime PM and system sleep (v2)") > Reported-by: Mika Westerberg > Signed-off-by: Rafael J. Wysocki > --- > drivers/base/power/main.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1606,13 +1606,13 @@ static int __device_suspend(struct devic > } > > /* > - * If a device configured to wake up the system from sleep states > - * has been suspended at run time and there's a resume request pending > - * for it, this is equivalent to the device signaling wakeup, so the > - * system suspend operation should be aborted. > + * If there's a runtime resume request pending for the device, resume > + * it before proceeding with invoking the system-wide suspend callbacks > + * for it, because the physical state of the device may not reflect the > + * "suspended" runtime PM status already in that case. > */ > - if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > - pm_wakeup_event(dev, 0); > + if (pm_runtime_barrier(dev)) > + pm_runtime_resume(dev); Is this really right? Note that whenever pm_runtime_barrier() returns a nonzero value, it already calls rpm_resume(dev, 0). So the pm_runtime_resume() call added here is redundant. Furthermore, by the logic used in this patch, the call to pm_wakeup_event() in the original code is also redundant: Any required wakeup event should have been generated when the runtime resume inside pm_runtime_barrer() was carried out. Removing a redundant function call can't fix a bug! This means that the code could be simplified to just: pm_runtime_barrier(dev); Will this fix the reported bug? It seems likely to me that the actual problem with the failure scenario in the patch description was that turning on an ACPI power resource causes runtime-resume requests to be queued for all devices sharing that resource. Wouldn't it make more sense to resume only the device that requested it and leave the others in runtime suspend? Alan Stern