Re: [PATCH] PM: Prevent waiting forever on asynchronous resume after abort

Previous thread: [PATCH] x86: use {push,pop}{l,q}_cfi in more places by Jan Beulich on Thursday, September 2, 2010 - 6:07 am. (3 messages)

Next thread: [PATCH 2/2] Add mremap trace point by Eric B Munson on Thursday, September 2, 2010 - 6:59 am. (1 message)
From: Alan Stern
Date: Thursday, September 2, 2010 - 6:50 am

This would work okay if C->power.completion had been initialized to the 

I think it would be better to change device_pm_init() and add a 
complete_all().

Alan Stern

--

From: Rafael J. Wysocki
Date: Thursday, September 2, 2010 - 12:46 pm

I agree.

Who's writing the patch?

Rafael
--

From: Colin Cross
Date: Thursday, September 2, 2010 - 1:24 pm

That would work, and was my first solution, but it increases the
reliance on the completion variable being left completed between state
transitions, which is undocumented and unnecessary.  It seems more
straightforward to me to only wait on the parent if the parent is
--

From: Rafael J. Wysocki
Date: Thursday, September 2, 2010 - 1:30 pm

Well, I need to think about that a little bit more.

Rafael
--

From: Rafael J. Wysocki
Date: Thursday, September 2, 2010 - 2:05 pm

In fact it is necessary, because dpm_wait() may be called by external code
through device_pm_wait_for_dev() which is exported for a reason.  That may
lead to problems analogous to the one you described if the completion

Yes, please.

Thanks,
Rafael
--

From: Colin Cross
Date: Thursday, September 2, 2010 - 2:31 pm

OK - do you prefer it in dpm_prepare or device_pm_init?
--

From: Rafael J. Wysocki
Date: Thursday, September 2, 2010 - 2:40 pm

device_pm_init(), please.

Rafael
--

From: Colin Cross
Date: Thursday, September 2, 2010 - 3:59 pm

During suspend, the power.completion is expected to be set when a
device has not yet started suspending.  Set it on init to fix a
corner case where a device is resumed when its parent has never
suspended.

Consider three drivers, A, B, and C.  The parent of A is C, and C
has async_suspend set.  On boot, C->power.completion is initialized
to 0.

During the first suspend:
suspend_devices_and_enter(...)
 dpm_resume(...)
  device_suspend(A)
  device_suspend(B) returns error, aborts suspend
 dpm_resume_end(...)
   dpm_resume(...)
    device_resume(A)
     dpm_wait(A->parent == C)
      wait_for_completion(C->power.completion)

The wait_for_completion will never complete, because
complete_all(C->power.completion) will only be called from
device_suspend(C) or device_resume(C), neither of which is called
if suspend is aborted before C.

After a successful suspend->resume cycle, where B doesn't abort
suspend, C->power.completion is left in the completed state by the
call to device_resume(C), and the same call path will work if B
aborts suspend.

Signed-off-by: Colin Cross <ccross@android.com>
---
 drivers/base/power/main.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index cb784a0..b1b4029 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -63,6 +63,7 @@ void device_pm_init(struct device *dev)
 {
 	dev->power.status = DPM_ON;
 	init_completion(&dev->power.completion);
+	complete_all(&dev->power.completion);
 	pm_runtime_init(dev);
 }
 
-- 
1.7.1

--

From: Rafael J. Wysocki
Date: Thursday, September 2, 2010 - 4:25 pm

Thanks, rebased on top of the current Linus' tree and applied to
suspend-2.6/pm-fixes .


--

Previous thread: [PATCH] x86: use {push,pop}{l,q}_cfi in more places by Jan Beulich on Thursday, September 2, 2010 - 6:07 am. (3 messages)

Next thread: [PATCH 2/2] Add mremap trace point by Eric B Munson on Thursday, September 2, 2010 - 6:59 am. (1 message)