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

Paul Walmsley
2010-05-14T06:27:56Z

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3860 bytes --]

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.

The problems with the above example are not isolated:

[paul@twilight msm]$ fgrep -r wake_lock_timeout . | egrep '(^./drivers|^./arch)'
./arch/arm/mach-msm/htc_battery.c: wake_lock_timeout(&vbus_wake_lock, HZ / 2);
./arch/arm/mach-msm/smd_tty.c: wake_lock_timeout(&info->wake_lock, HZ / 2);
./arch/arm/mach-msm/smd_qmi.c: wake_lock_timeout(&ctxt->wake_lock, HZ / 2);
./drivers/rtc/alarm.c: wake_lock_timeout(&alarm_wake_lock, 5 * HZ);
./drivers/rtc/alarm.c: wake_lock_timeout(&alarm_rtc_wake_lock, 1 * HZ);
./drivers/rtc/alarm.c: wake_lock_timeout(&alarm_rtc_wake_lock, 2 * HZ);
./drivers/mmc/core/core.c: wake_lock_timeout(&mmc_delayed_work_wake_lock, HZ / 2);
./drivers/net/msm_rmnet.c: wake_lock_timeout(&p->wake_lock, HZ / 2);
./drivers/video/msm/msm_fb.c: wake_lock_timeout(&msmfb->idle_lock, HZ/4);
./drivers/video/msm/msm_fb.c: wake_lock_timeout(&msmfb->idle_lock, HZ/4);
./drivers/video/msm/msm_fb.c: wake_lock_timeout(&msmfb->idle_lock, HZ);
./drivers/input/evdev.c: wake_lock_timeout(&client->wake_lock, 5 * HZ);
./drivers/serial/msm_serial_hs.c: wake_lock_timeout(&msm_uport->rx.wake_lock, HZ / 2);

It is true that the current patch series does not propose the *_timeout()
interface. But that raises the question of what it will be replaced with,
in situations where the problem can't simply be pushed to userspace, as it
is in Arve's evdev patch 7. Some of these cases seem difficult to handle
without some sort of timer-based approach. Timer-based approaches are
intrinsically problematic for the reasons mentioned above.

- Paul

1. git://android.git.kernel.org/kernel/msm.git - as of commit
c7f8bcecb06b937c45dd0e342450a3218b286b8d, "[ARM] msm: defconfig:
Set mmap_min_addr to 32k"

2. http://android.git.kernel.org/?p=kernel/msm.git;a=blob;f=drivers/mmc/core/core.c;h=22f6ddad85c92f34c55035982049bb03b5abfc74;hb=c7f8bcecb06b937c45dd0e342450a3218b286b8d#l721

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]