Hi -
Yesterday I studied suspend status on 3.0 kernel for Panda, for mem suspend it's working pretty well in Ubuntu case, desktop is coming back up woken by USB keyboard action, WLAN is workable after reassociation.
However in Android case, the same tree merged with common-android-3.0 to get Androidization is blowing chunks in suspend / resume, entering a loop where it aborts suspend and then tries to suspend again all the time.
Increasing the debug level in wakelock code shows at least two guys that can make trouble, locks "mmc_delayed_work" and "alarm_rtc".
"mmc_delayed_work" casues wakelock stuff to return -EAGAIN, and "alarm_rtc" seems to timeout as a wakelock, but leave the alarm device in a state where it will abort suspend on -EBUSY.
I took a look in drivers/mmc/core/core.c to see what the wakelock support patches had done there and was a bit surprised.
They have a single wakelock to cover delayed work in there, however there are multiple delayed works possible to be queued, eg delayed disable and delayed detect actions, and although they wrap scheduling the delayed work to also lock the wakelock, they don't wrap cancelling it, eg -->
void mmc_stop_host(struct mmc_host *host) { ... if (host->caps & MMC_CAP_DISABLE) cancel_delayed_work(&host->disable); cancel_delayed_work_sync(&host->detect); mmc_flush_scheduled_work();
Nor do wakelocks seem to maintain counters, they're either locked or unlocked.
So the following code looks highly suspicious:
static int mmc_schedule_delayed_work(struct delayed_work *work, unsigned long delay) { wake_lock(&mmc_delayed_work_wake_lock); return queue_delayed_work(workqueue, work, delay); }
static int mmc_host_do_disable(struct mmc_host *host, int lazy) { .... (conditionally) mmc_schedule_delayed_work(&host->disable, delay); ... }
void mmc_host_deeper_disable(struct work_struct *work) { ... mmc_host_do_disable(host, 1); ... wake_unlock(&mmc_delayed_work_wake_lock); }
Ie mmc_host_deeper_disable() can provoke delayed work which it proceeds to unlock wakelock coverage for.
To the point of the symptoms:
int mmc_pm_notify(struct notifier_block *notify_block, unsigned long mode, void *unused) { ... switch (mode) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: ... cancel_delayed_work_sync(&host->detect);
So here when preparing for suspend we can cancel the delayed work we presumably arranged wakelock coverage for, without unlocking the wakelock.
Am I missing the point or is some work needed to improve wakelock application for mmc stuff in common-android-3.0?
-Andy
Hi Andy,
Andy Green andy.green@linaro.org writes:
Hi -
Yesterday I studied suspend status on 3.0 kernel for Panda, for mem suspend it's working pretty well in Ubuntu case, desktop is coming back up woken by USB keyboard action, WLAN is workable after reassociation.
However in Android case, the same tree merged with common-android-3.0 to get Androidization is blowing chunks in suspend / resume, entering a loop where it aborts suspend and then tries to suspend again all the time.
I've been hit by the same problem when I was trying to implement hibernation on Panda.
Increasing the debug level in wakelock code shows at least two guys that can make trouble, locks "mmc_delayed_work" and "alarm_rtc".
"mmc_delayed_work" casues wakelock stuff to return -EAGAIN, and "alarm_rtc" seems to timeout as a wakelock, but leave the alarm device in a state where it will abort suspend on -EBUSY.
I took a look in drivers/mmc/core/core.c to see what the wakelock support patches had done there and was a bit surprised.
They have a single wakelock to cover delayed work in there, however there are multiple delayed works possible to be queued, eg delayed disable and delayed detect actions, and although they wrap scheduling the delayed work to also lock the wakelock, they don't wrap cancelling it, eg -->
Yes, they unlock the wakelock when the delayed work is done, in the handler.
So here when preparing for suspend we can cancel the delayed work we presumably arranged wakelock coverage for, without unlocking the wakelock.
When the delayed work was canceled and handled immediately the wakelock should have been unlocked.
The problem is here:
int mmc_pm_notify(struct notifier_block *notify_block, unsigned long mode, void *unused) { ... switch (mode) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: ... mmc_release_host(host);
Which calls
void mmc_release_host(struct mmc_host *host) { ... mmc_host_lazy_disable(host); ... }
Since omap_hsmmc has non-zero host->disable_delay by default, this will schedule a new delayed_work thus acquire new wakelock.
I tried to fix this by ignore the host->disable_delay if host->rescan_disable is non-zero, which means we are in suspend phase, and it worked.
On 07/07/2011 09:37 AM, Somebody in the thread at some point said:
Hi Kan-Ru -
They have a single wakelock to cover delayed work in there, however there are multiple delayed works possible to be queued, eg delayed disable and delayed detect actions, and although they wrap scheduling the delayed work to also lock the wakelock, they don't wrap cancelling it, eg -->
Yes, they unlock the wakelock when the delayed work is done, in the handler.
Right so cancelling the delayed work means the wakelock is left locked / asserted and we will never do the work to unlock it; the code for this case is broken.
So here when preparing for suspend we can cancel the delayed work we presumably arranged wakelock coverage for, without unlocking the wakelock.
When the delayed work was canceled and handled immediately the wakelock should have been unlocked.
Not certain you meant this by "handled immediately" but to be clear cancel_delayed_work() deletes the timer that creates the delay, and stops the work ever from being executed. In that case the wakelock will be left locked with nothing coming to unlock him except any private wakelock timeout. And I am guessing private wakelock timeouts are there to hide a whole bunch of immortal wakelocks due to broken code that would otherwise kill suspend from ever happening.
The problem is here:
int mmc_pm_notify(struct notifier_block *notify_block, unsigned long mode, void *unused) { ... switch (mode) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: ... mmc_release_host(host);
Which calls
void mmc_release_host(struct mmc_host *host) { ... mmc_host_lazy_disable(host); ... }
Since omap_hsmmc has non-zero host->disable_delay by default, this will schedule a new delayed_work thus acquire new wakelock.
Okay, I see: thanks for sharing. I will look at the code.
I tried to fix this by ignore the host->disable_delay if host->rescan_disable is non-zero, which means we are in suspend phase, and it worked.
I got the idea from looking at core.c it's full of bad corner cases and potential problems wrt wakelock. I just wanted to confirm it really does suck and it's not just me missing the point somewhere ^^
-Andy
Andy Green andy.green@linaro.org writes:
They have a single wakelock to cover delayed work in there, however there are multiple delayed works possible to be queued, eg delayed disable and delayed detect actions, and although they wrap scheduling the delayed work to also lock the wakelock, they don't wrap cancelling it, eg -->
Yes, they unlock the wakelock when the delayed work is done, in the handler.
Right so cancelling the delayed work means the wakelock is left locked / asserted and we will never do the work to unlock it; the code for this case is broken.
Hum..
So here when preparing for suspend we can cancel the delayed work we presumably arranged wakelock coverage for, without unlocking the wakelock.
When the delayed work was canceled and handled immediately the wakelock should have been unlocked.
Not certain you meant this by "handled immediately" but to be clear cancel_delayed_work() deletes the timer that creates the delay, and stops the work ever from being executed. In that case the wakelock will be left locked with nothing coming to unlock him except any private wakelock timeout. And I am guessing private wakelock timeouts are there to hide a whole bunch of immortal wakelocks due to broken code that would otherwise kill suspend from ever happening.
The name cancel_delayed_work might be misleading but I learned that although it deletes the timer, it still waits the work to be finished. So in this case the wakelock is handled, no?
The problem is here:
int mmc_pm_notify(struct notifier_block *notify_block, unsigned long mode, void *unused) { ... switch (mode) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: ... mmc_release_host(host);
Which calls
void mmc_release_host(struct mmc_host *host) { ... mmc_host_lazy_disable(host); ... }
Since omap_hsmmc has non-zero host->disable_delay by default, this will schedule a new delayed_work thus acquire new wakelock.
Okay, I see: thanks for sharing. I will look at the code.
I tried to fix this by ignore the host->disable_delay if host->rescan_disable is non-zero, which means we are in suspend phase, and it worked.
I got the idea from looking at core.c it's full of bad corner cases and potential problems wrt wakelock. I just wanted to confirm it really does suck and it's not just me missing the point somewhere ^^
It does suck and it's not only you ;-)
On 07/07/2011 10:43 AM, Somebody in the thread at some point said:
Not certain you meant this by "handled immediately" but to be clear cancel_delayed_work() deletes the timer that creates the delay, and stops the work ever from being executed. In that case the wakelock will be left locked with nothing coming to unlock him except any private wakelock timeout. And I am guessing private wakelock timeouts are there to hide a whole bunch of immortal wakelocks due to broken code that would otherwise kill suspend from ever happening.
The name cancel_delayed_work might be misleading but I learned that although it deletes the timer, it still waits the work to be finished. So in this case the wakelock is handled, no?
I think you're thinking about the flush... apis which do act like that.
But cancel... cancels.
2662 /** 2663 * cancel_delayed_work_sync - cancel a delayed work and wait for it to finish 2664 * @dwork: the delayed work cancel 2665 * 2666 * This is cancel_work_sync() for delayed works. 2667 * 2668 * RETURNS: 2669 * %true if @dwork was pending, %false otherwise. 2670 */ 2671 bool cancel_delayed_work_sync(struct delayed_work *dwork) 2672 { 2673 return __cancel_work_timer(&dwork->work, &dwork->timer); 2674 } 2675 EXPORT_SYMBOL(cancel_delayed_work_sync);
http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/kernel/workqueue.c#L2671
2580 static bool __cancel_work_timer(struct work_struct *work, 2581 struct timer_list* timer) 2582 { 2583 int ret; 2584 2585 do { 2586 ret = (timer && likely(del_timer(timer))); 2587 if (!ret) 2588 ret = try_to_grab_pending(work); 2589 wait_on_work(work); <--- 2590 } while (unlikely(ret < 0)); 2591 2592 clear_work_data(work); 2593 return ret; 2594 }
http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/kernel/workqueue.c#L2580
wait_on_work() -- another misleading name -- only waits in the case the work is already running. If it hasn't run yet, it just returns.
I got the idea from looking at core.c it's full of bad corner cases and potential problems wrt wakelock. I just wanted to confirm it really does suck and it's not just me missing the point somewhere ^^
It does suck and it's not only you ;-)
I feel much better now ^^
-Andy
-
Andy Green -
Kan-Ru Chen