Re: [PATCH 1/8] PM: Add suspend block api.

Arve Hjønnevåg
2010-05-14T07:14:47Z

On Thu, May 13, 2010 at 11:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
>
> Hello,
>
> Another comment on the kernel suspend-blocker API.
>
> On Thu, 13 May 2010, Arve Hjønnevåg wrote:
>
>> Adds /sys/power/policy that selects the behaviour of /sys/power/state.
>> After setting the policy to opportunistic, writes to /sys/power/state
>> become non-blocking requests that specify which suspend state to enter
>> when no suspend blockers are active. A special state, "on", stops the
>> process by activating the "main" suspend blocker.
>>
>> Signed-off-by: Arve Hjønnevåg <arve@android.com>
>
> Looking at the way that suspend-blocks are used in the current Android
> 'msm' kernel tree[1], they seem likely to cause layering violations,
> fragile kernel code with userspace dependencies, and code that consumes
> power unnecessarily.
>
> For example, in drivers/mmc/core/core.c:721, in mmc_rescan() [2], we find
> the following code:
>
>        /* give userspace some time to react */
>        wake_lock_timeout(&mmc_delayed_work_wake_lock, HZ / 2);
>
> This is a layering violation.  The MMC subsystem should have nothing to do
> with "giving userspace time to react."  That is the responsibility of the
> Linux scheduler.
>
> This code is also intrinsically fragile and use-case dependent.  What if
> userspace occasionally needs more than (HZ / 2) to react?  If the
> distributor is lucky enough to catch this before the product ships, then
> the distributor can patch in a new magic number.  But if the device makes
> it to the consumer, the result is an unstable device that unpredictably
> suspends.
>
> The above code will also waste power.  Even if userspace takes less than
> (HZ / 2) to react, the system will still be prevented from entering a
> system-wide low power state for the duration of the remaining time.  This
> is in contrast to an approach that uses the idle loop to enter a
> system-wide low power state.  The moment that the system has no further
> work to do, it can start saving power.
>

Yes, preventing suspend for 1/2 second wastes some power, but are you
seriously suggesting that it wastes more power then never suspending?
Opportunitic suspend does not prevent entering low power modes from
idle.

--
Arve Hjønnevåg