From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964861AbaEMPDA (ORCPT ); Tue, 13 May 2014 11:03:00 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:57452 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S933327AbaEMPC5 (ORCPT ); Tue, 13 May 2014 11:02:57 -0400 From: "Rafael J. Wysocki" To: Ulf Hansson Cc: Alan Stern , Linux PM list , ACPI Devel Maling List , Aaron Lu , Mika Westerberg , Linux Kernel Mailing List , Kevin Hilman Subject: Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare() Date: Tue, 13 May 2014 17:19:43 +0200 Message-ID: <3628909.qRlHUQe4cS@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.14.0-rc7+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <2132869.H2tFu0p7Mg@vostro.rjw.lan> References: <4649019.is79p6EySp@vostro.rjw.lan> <2132869.H2tFu0p7Mg@vostro.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday, May 13, 2014 05:07:12 PM Rafael J. Wysocki wrote: > On Tuesday, May 13, 2014 12:59:43 PM Ulf Hansson wrote: > > On 13 May 2014 12:35, Rafael J. Wysocki wrote: > > > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote: > > >> On 13 May 2014 03:03, Rafael J. Wysocki wrote: > > >> > From: Rafael J. Wysocki > > >> > > > >> > Move the invocation of the runtime PM barrier during system suspend > > >> > (or hibernation) from __device_suspend() to device_prepare() to make > > >> > all runtime PM transitions in progress complete before executing > > >> > ->prepare() callbacks for devices. > > >> > > > >> > That will allow those callbacks to check if devices are runtime > > >> > suspended in a non-racy way. > > >> > > > >> > Signed-off-by: Rafael J. Wysocki > > >> > --- > > >> > drivers/base/power/main.c | 31 +++++++++++++------------------ > > >> > 1 file changed, 13 insertions(+), 18 deletions(-) > > >> > > > >> > Index: linux-pm/drivers/base/power/main.c > > >> > =================================================================== > > >> > --- linux-pm.orig/drivers/base/power/main.c > > >> > +++ linux-pm/drivers/base/power/main.c > > >> > @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic > > >> > > > >> > dpm_wait_for_children(dev, async); > > >> > > > >> > - if (async_error) > > >> > - goto Complete; > > >> > - > > >> > - /* > > >> > - * 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 (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > > >> > - pm_wakeup_event(dev, 0); > > >> > - > > >> > - if (pm_wakeup_pending()) { > > >> > - async_error = -EBUSY; > > >> > - goto Complete; > > >> > - } > > >> > > >> I suppose you went a bit too far here!? > > >> > > >> We can still have wakeup pending at this point, and thus we should > > >> bail out, right? > > > > > > That pm_wakeup_pending() is part of the barrier handling, so -> > > > > > >> > - > > >> > - if (dev->power.syscore) > > >> > + if (async_error || dev->power.syscore) > > >> > goto Complete; > > >> > > > >> > dpm_watchdog_set(&wd, dev); > > >> > @@ -1500,6 +1483,18 @@ static int device_prepare(struct device > > >> > */ > > >> > pm_runtime_get_noresume(dev); > > >> > > > >> > + /* > > >> > + * 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 (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > > >> > + pm_wakeup_event(dev, 0); > > >> > + > > >> > + if (pm_wakeup_pending()) > > >> > + return -EBUSY; > > >> > + > > > > > > -> it is done here now. > > > > > > I don't see why it would be still necessary in __device_suspend(). > > > > Can't we have wakeup configured for !CONFIG_PM_RUNTIME case? > > pm_runtime_barrier() won't handle those scenarios, right? > > The pm_wakeup_pending() is in effect for CONFIG_PM_RUNTIME unset too. > > > Similar check for pm_wakeup_pending() is done at > > __device_suspend_noirq, __device_suspend_late - I assumed it was > > because of the same reasons. > > Hmm, OK. I'll leave it in __device_suspend() too, then. Well, actually, that wouldn't make much sense in my opinion. Why would the device status change between device_prepare() and __device_suspend() if we do the barrier in device_prepare()? -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.