| Message ID | 2675141.cAMB9masRU@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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 <rafael.j.wysocki@intel.com> > --- > 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? > - > - 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; > + > device_lock(dev); > > dev->power.wakeup_path = device_may_wakeup(dev); > Kind regards Ulf Hansson -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote: > On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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 <rafael.j.wysocki@intel.com> > > --- > > 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(). > > device_lock(dev); > > > > dev->power.wakeup_path = device_may_wakeup(dev); > > Thanks!
On 13 May 2014 12:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote: >> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > >> > 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 <rafael.j.wysocki@intel.com> >> > --- >> > 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? Similar check for pm_wakeup_pending() is done at __device_suspend_noirq, __device_suspend_late - I assumed it was because of the same reasons. Kind regards Ulf Hansson > >> > device_lock(dev); >> > >> > dev->power.wakeup_path = device_may_wakeup(dev); >> > > > Thanks! > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, May 13, 2014 12:59:43 PM Ulf Hansson wrote: > On 13 May 2014 12:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote: > >> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > > >> > 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 <rafael.j.wysocki@intel.com> > >> > --- > >> > 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. Thanks!
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 <rjw@rjwysocki.net> wrote: > > > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote: > > >> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > >> > > > >> > 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 <rafael.j.wysocki@intel.com> > > >> > --- > > >> > 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()?
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; - } - - 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; + device_lock(dev); dev->power.wakeup_path = device_may_wakeup(dev);