Andy Green <andy.gr...@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 ;-)
--
Kanru
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
