diff mbox

[RFC] PM / Runtime: Rework the "runtime idle" helper routine

Message ID Pine.LNX.4.44L0.1305301247370.1137-100000@iolanthe.rowland.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Alan Stern May 30, 2013, 5:08 p.m. UTC
On Thu, 30 May 2013, Rafael J. Wysocki wrote:

> > Since you're making this change, wouldn't it be a good idea to adopt
> > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > when the use_autosuspend flag is set?
> 
> I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> change in any case.

That makes sense.

> > What about cases where the runtime-idle callback does
> > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > that it returns -EBUSY in such cases.  Did you audit for this?
> 
> As far as I could.
> 
> I'm not worried about the subsystems modified by this patch, because the
> functionality there won't change (except for PCI, that is).

Right.  The subsystems that _aren't_ modified are the ones to worry 
about -- like the USB callback.  They are the ones where the behavior 
might change.

> > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > ===================================================================
> > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > >  management callbacks provided by the PM core, defined in
> > >  driver/base/power/generic_ops.c:
> > >  
> > > -  int pm_generic_runtime_idle(struct device *dev);
> > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > -      return value is 0 or the callback is not defined
> > > -
> > 
> > The documentation for the runtime-idle callback needs to be updated too.
> 
> Well, I actually couldn't find the part of it that would need to be updated. :-)

How about this?




Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael Wysocki May 30, 2013, 7:55 p.m. UTC | #1
On Thursday, May 30, 2013 01:08:08 PM Alan Stern wrote:
> On Thu, 30 May 2013, Rafael J. Wysocki wrote:
> 
> > > Since you're making this change, wouldn't it be a good idea to adopt
> > > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > > when the use_autosuspend flag is set?
> > 
> > I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> > change in any case.
> 
> That makes sense.
> 
> > > What about cases where the runtime-idle callback does
> > > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > > that it returns -EBUSY in such cases.  Did you audit for this?
> > 
> > As far as I could.
> > 
> > I'm not worried about the subsystems modified by this patch, because the
> > functionality there won't change (except for PCI, that is).
> 
> Right.  The subsystems that _aren't_ modified are the ones to worry 
> about -- like the USB callback.  They are the ones where the behavior 
> might change.

Right.

> > > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > > ===================================================================
> > > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > > >  management callbacks provided by the PM core, defined in
> > > >  driver/base/power/generic_ops.c:
> > > >  
> > > > -  int pm_generic_runtime_idle(struct device *dev);
> > > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > > -      return value is 0 or the callback is not defined
> > > > -
> > > 
> > > The documentation for the runtime-idle callback needs to be updated too.
> > 
> > Well, I actually couldn't find the part of it that would need to be updated. :-)
> 
> How about this?

Looks good! :-)

May I add your sign-off to it?

Rafael


> Index: usb-3.10/Documentation/power/runtime_pm.txt
> ===================================================================
> --- usb-3.10.orig/Documentation/power/runtime_pm.txt
> +++ usb-3.10/Documentation/power/runtime_pm.txt
> @@ -144,8 +144,12 @@ The action performed by the idle callbac
>  (or driver) in question, but the expected and recommended action is to check
>  if the device can be suspended (i.e. if all of the conditions necessary for
>  suspending the device are satisfied) and to queue up a suspend request for the
> -device in that case.  The value returned by this callback is ignored by the PM
> -core.
> +device in that case.  If there is no idle callback, or if the callback returns
> +0, then the PM core will attempt to carry out a runtime suspend of the device;
> +in essence, it will call pm_runtime_suspend() directly.  To prevent this (for
> +example, if the callback routine has started a delayed suspend), the routine
> +should return a non-zero value.  Negative error return codes are ignored by the
> +PM core.
>  
>  The helper functions provided by the PM core, described in Section 4, guarantee
>  that the following constraints are met with respect to runtime PM callbacks for
> @@ -301,9 +305,10 @@ drivers/base/power/runtime.c and include
>        removing the device from device hierarchy
>  
>    int pm_runtime_idle(struct device *dev);
> -    - execute the subsystem-level idle callback for the device; returns 0 on
> -      success or error code on failure, where -EINPROGRESS means that
> -      ->runtime_idle() is already being executed
> +    - execute the subsystem-level idle callback for the device; returns an
> +      error code on failure, where -EINPROGRESS means that ->runtime_idle() is
> +      already being executed; if there is no callback or the callback returns 0
> +      then run pm_runtime_suspend(dev) and return its result
>  
>    int pm_runtime_suspend(struct device *dev);
>      - execute the subsystem-level suspend callback for the device; returns 0 on
> 
> 
> Alan Stern
>
Alan Stern May 30, 2013, 8:13 p.m. UTC | #2
On Thu, 30 May 2013, Rafael J. Wysocki wrote:

> > > > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > > > ===================================================================
> > > > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > > > >  management callbacks provided by the PM core, defined in
> > > > >  driver/base/power/generic_ops.c:
> > > > >  
> > > > > -  int pm_generic_runtime_idle(struct device *dev);
> > > > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > > > -      return value is 0 or the callback is not defined
> > > > > -
> > > > 
> > > > The documentation for the runtime-idle callback needs to be updated too.
> > > 
> > > Well, I actually couldn't find the part of it that would need to be updated. :-)
> > 
> > How about this?
> 
> Looks good! :-)
> 
> May I add your sign-off to it?

Go ahead.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki June 2, 2013, 9:50 p.m. UTC | #3
On Thursday, May 30, 2013 01:08:08 PM Alan Stern wrote:
> On Thu, 30 May 2013, Rafael J. Wysocki wrote:
> 
> > > Since you're making this change, wouldn't it be a good idea to adopt
> > > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > > when the use_autosuspend flag is set?
> > 
> > I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> > change in any case.
> 
> That makes sense.
> 
> > > What about cases where the runtime-idle callback does
> > > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > > that it returns -EBUSY in such cases.  Did you audit for this?
> > 
> > As far as I could.
> > 
> > I'm not worried about the subsystems modified by this patch, because the
> > functionality there won't change (except for PCI, that is).
> 
> Right.  The subsystems that _aren't_ modified are the ones to worry 
> about -- like the USB callback.  They are the ones where the behavior 
> might change.

OK, this time I think I've caught all of them. :-)

I've retained the ACKs and Reviewed-by tags in [1/2], because it only makes
more changes in addition to the previously ACKed ones.  The PCI changeset
has been updated and [2/2] is the documentation update.

Thanks,
Rafael
diff mbox

Patch

Index: usb-3.10/Documentation/power/runtime_pm.txt
===================================================================
--- usb-3.10.orig/Documentation/power/runtime_pm.txt
+++ usb-3.10/Documentation/power/runtime_pm.txt
@@ -144,8 +144,12 @@  The action performed by the idle callbac
 (or driver) in question, but the expected and recommended action is to check
 if the device can be suspended (i.e. if all of the conditions necessary for
 suspending the device are satisfied) and to queue up a suspend request for the
-device in that case.  The value returned by this callback is ignored by the PM
-core.
+device in that case.  If there is no idle callback, or if the callback returns
+0, then the PM core will attempt to carry out a runtime suspend of the device;
+in essence, it will call pm_runtime_suspend() directly.  To prevent this (for
+example, if the callback routine has started a delayed suspend), the routine
+should return a non-zero value.  Negative error return codes are ignored by the
+PM core.
 
 The helper functions provided by the PM core, described in Section 4, guarantee
 that the following constraints are met with respect to runtime PM callbacks for
@@ -301,9 +305,10 @@  drivers/base/power/runtime.c and include
       removing the device from device hierarchy
 
   int pm_runtime_idle(struct device *dev);
-    - execute the subsystem-level idle callback for the device; returns 0 on
-      success or error code on failure, where -EINPROGRESS means that
-      ->runtime_idle() is already being executed
+    - execute the subsystem-level idle callback for the device; returns an
+      error code on failure, where -EINPROGRESS means that ->runtime_idle() is
+      already being executed; if there is no callback or the callback returns 0
+      then run pm_runtime_suspend(dev) and return its result
 
   int pm_runtime_suspend(struct device *dev);
     - execute the subsystem-level suspend callback for the device; returns 0 on