On Thursday 06 August 2009, Alan Stern wrote:
On Wed, 5 Aug 2009, Rafael J. Wysocki wrote:
quoted
Hi,
The patch below should address all of your recent comments.
Additionally I changed a few bits that I thought could turn out to be
problematic at one point.
Looking good. I've got a few more suggestions.
It occurred to me that there's no need for a separate
"runtime_failure" flag. A nonzero value of "last_error" will do just
as well.
Yes, good catch. I don't quite remember why I wanted the flag and a separate
error field.
If you make this change, note that it affects the documentation as well as
the code.
Sure.
If we defer a resume request while a suspend is in progress, then when
the suspend finishes should the resume be carried out immediately
rather than queued? I don't see any reason why not.
Well, it's not very clear what to return to the caller in such a case. I guess
we can return -EAGAIN.
quoted
+/**
+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be suspended and run the ->runtime_suspend() callback
+ * provided by its bus type. If another suspend has been started earlier, wait
+ * for it to finish. If there's an idle notification pending, cancel it. If
+ * there's a suspend request scheduled while this function is running and @sync
+ * is 'true', cancel that request.
Change the last two sentences as follows: If an idle notification or suspend
request is pending or scheduled, cancel it.
OK
quoted
+ *
+ * This function must be called under dev->power.lock with interrupts disabled.
+ */
+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+ __releases(&dev->power.lock) __acquires(&dev->power.lock)
+{
...
quoted
+ pm_runtime_deactivate_timer(dev);
+
+ if (dev->power.request_pending) {
+ /* Pending resume requests take precedence over us. */
+ if (dev->power.request == RPM_REQ_RESUME)
+ return -EAGAIN;
+ /* Other pending requests need to be canceled. */
+ dev->power.request = RPM_REQ_NONE;
+ }
Might as well use pm_runtime_cancel_pending since we have it:
/* Pending resume requests take precedence over us. */
if (dev->power.request_pending && dev->power.request == RPM_REQ_RESUME)
return -EAGAIN;
/* Other pending requests need to be canceled. */
pm_runtime_cancel_pending(dev);
OK
...
quoted
+ if (dev->power.deferred_resume) {
+ __pm_request_resume(dev);
__pm_runtime_resume instead?
In which case we shouldn't execute the code below, IMO, but return immediately
instead.
quoted
+/**
+ * __pm_runtime_resume - Carry out run-time resume of given device.
+ * @dev: Device to resume.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be woken up and run the ->runtime_resume() callback
+ * provided by its bus type. If another resume has been started earlier, wait
+ * for it to finish. If there's a suspend running in parallel with this
+ * function, wait for it to finish and resume the device. If there's a suspend
+ * request or idle notification pending, cancel it. If there's a resume request
+ * scheduled while this function is running, cancel that request.
Change the last two sentences as follows: Cancel any pending requests.
OK
quoted
+ *
+ * This function must be called under dev->power.lock with interrupts disabled.
+ */
+int __pm_runtime_resume(struct device *dev, bool from_wq)
+ __releases(&dev->power.lock) __acquires(&dev->power.lock)
+{
+ struct device *parent = NULL;
+ int retval = 0;
+
+ repeat:
+ if (dev->power.runtime_failure)
+ return -EINVAL;
Here and in two places below, goto out_parent instead of returning
directly.
Ah, that was a real bug. Thanks for catching it!
...
quoted
+ if (!parent && dev->parent) {
+ /*
+ * Increment the parent's resume counter and resume it if
+ * necessary.
+ */
+ parent = dev->parent;
+ spin_unlock_irq(&dev->power.lock);
+
+ retval = pm_runtime_get_sync(parent);
+
+ spin_lock_irq(&dev->power.lock);
+ /* We can resume if the parent's run-time PM is disabled. */
+ if (retval < 0 && retval != -EAGAIN)
+ goto out_parent;
Instead of checking retval, how about checking the parent's PM status?
Should work.
Also, this isn't needed if the parent is set to ignore children.
OK, I'll change that.
quoted
+static int __pm_request_idle(struct device *dev)
+{
+ int retval = 0;
+
+ if (dev->power.runtime_failure)
+ retval = -EINVAL;
+ else if (atomic_read(&dev->power.usage_count) > 0
+ || dev->power.disable_depth > 0
+ || dev->power.timer_expires > 0
This line should be removed.
Yeah, thanks!
...
quoted
+ if (dev->power.request_pending && dev->power.request != RPM_REQ_NONE) {
+ /* Any requests other then RPM_REQ_IDLE take precedence. */
+ if (dev->power.request != RPM_REQ_IDLE)
+ retval = -EAGAIN;
+ return retval;
+ }
+
+ dev->power.request = RPM_REQ_IDLE;
+ if (dev->power.request_pending)
+ return retval;
+
+ dev->power.request_pending = true;
+ queue_work(pm_wq, &dev->power.work);
This should be done consistently with the other routines. Thus:
if (dev->power.request_pending) {
/* All other requests take precedence. */
if (dev->power.request == RPM_REQ_NONE)
dev->power.request = RPM_REQ_IDLE;
else if (dev->power.request != RPM_REQ_IDLE)
retval = -EAGAIN;
return retval;
}
dev->power.request = RPM_REQ_IDLE;
dev->power.request_pending = true;
queue_work(pm_wq, &dev->power.work);
OK
quoted
+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+ struct device *parent = dev->parent;
+ unsigned long flags;
+ bool notify_parent = false;
+ int error = 0;
+
+ if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+ return -EINVAL;
+
+ spin_lock_irqsave(&dev->power.lock, flags);
+
+ if (!dev->power.runtime_failure && !dev->power.disable_depth)
+ goto out;
Set "error" to a negative code?
OK
quoted
@@ -757,11 +770,16 @@ static int dpm_prepare(pm_message_t stat
dev->power.status = DPM_PREPARING;
mutex_unlock(&dpm_list_mtx);
- error = device_prepare(dev, state);
+ if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+ /* Wake-up during suspend. */
+ error = -EBUSY;
Or maybe "Wakeup was requested during sleep transition."
Sounds better.
quoted
+ unsigned int deferred_resume;
+ - set if ->runtime_resume() is about to be run while ->runtime_suspend() is
+ being executed for that device and it is not practical to wait for the
+ suspend to complete; means "queue up a resume request as soon as you've
+ suspended"
"start a resume" instead of "queue up a resume request"?
OK
quoted
+5. Run-time PM Initialization
...
quoted
+If the defaul initial run-time PM status of the device (i.e. 'suspended')
Fix spelling of "default".
OK
quoted
+reflects the actual state of the device, its bus type's or its driver's
+->probe() callback will likely need to wake it up using one of the PM core's
+helper functions described in Section 4. In that case, pm_runtime_resume()
+should be used. Of course, for this purpose the device's run-time PM has to be
+enabled earlier by calling pm_runtime_enable().
+
+If ->probe() calls pm_runtime_suspend() or pm_runtime_idle(), or their
+asynchronous counterparts, they will fail returning -EAGAIN, because the
+device's usage counter is incremented by the core before executing ->probe().
+Still, it may be desirable to suspend the device as soon as ->probe() has
+finished, so the core uses pm_runtime_idle() to invoke the device bus type's
+->runtime_idle() callback at that time, which only happens even if ->probe()
s/which only happens even/but only/
quoted
+is successful.
OK
Thanks for the comments! In fact I've already updated the patch to address
them, so I'll send it in a little while.
Best,
Rafael