| Message ID | Pine.LNX.4.44L0.1305301247370.1137-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 >
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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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