On Thu, Feb 12, 2009 at 2:00 PM, mark gross <mgross@xxxxxxxxxxxxxxx> wrote:
>> +A locked wakelock, depending on its type, prevents the system from entering
>> +suspend or other low-power states. When creating a wakelock, you can select
>> +if it prevents suspend or low-power idle states. If the type is set to
>> +WAKE_LOCK_SUSPEND, the wakelock prevents a full system suspend. If the type
>> +is WAKE_LOCK_IDLE, low-power states that cause large interrupt latencies, or
>> +that disable a set of interrupts, will not be entered from idle until the
>> +wakelocks are released. Unless the type is specified, this document refers
>> +to wakelocks with the type set to WAKE_LOCK_SUSPEND.
>
> If there are no wakelocks held, what code path pushes the system to a
> lower state? (wake_unlock?)
wake_unlock, wake_lock_timeout or the expire timer may wake up the
suspend worker which calls pm_suspend.
> Would it make more sense to remove the suspend types of locks? Those
> seem to be pretty contentious.
I can remove the idle wake lock type from the patch, but I prefer to
keep the type so we can easily add types. We need the idle lock on the
g1 for now, and anyone adds emergency or high priority sleep support
to linux, we may want a separate wakelock type for that.
> Do the ARM devices really go into a suspend state or just a low power
> state?
If by suspend state you mean something more than halt or
wait-for-interrupt, then yes. The on the G1 the cpu core is powered
down, and when its wakes up it starts at the reset vector. But, it is
fast enough to also be usable from idle.
>> +Wakelocks can be used to allow user-space to decide which keys should wake the
>> +full system up and turn the screen on. Use set_irq_wake or a platform specific
>> +api to make sure the keypad interrupt wakes up the cpu. Once the keypad driver
>> +has resumed, the sequence of events can look like this:
>> +- The Keypad driver gets an interrupt. It then locks the keypad-scan wakelock
>> + and starts scanning the keypad matrix.
>> +- The keypad-scan code detects a key change and reports it to the input-event
>> + driver.
>> +- The input-event driver sees the key change, enqueues an event, and locks
>> + the input-event-queue wakelock.
>> +- The keypad-scan code detects that no keys are held and unlocks the
>> + keypad-scan wakelock.
>
> Lets assume the system drops to suspend after 0ms of no-locks held.
> Don't we have a race between the keypad driver unlocking the only lock
> and inpu-event-queue lock getting taken?
The input-event-queue lock is locked before the keypad driver lock is unlocked.
>> +- The user-space input-event thread returns from select/poll, locks the
>> + process-input-events wakelock and then calls read in the input-event device.
>> +- The input-event driver dequeues the key-event and, since the queue is now
>> + empty, it unlocks the input-event-queue wakelock.
>> +- The user-space input-event thread returns from read. It determines that the
>> + key should not wake up the full system, releases the process-input-events
>> + wakelock and calls select or poll.
>> +
>> + Key pressed Key released
>> + | |
>> +keypad-scan ++++++++++++++++++
>> +input-event-queue +++ +++
>> +process-input-events +++ +++
>> +
>
> it feels like there needs to be interlocking wakelocks to keep the
> system from unexpectedly falling asleep. Or, some hysteresis slop time
> baked in to hide the races.
These locks overlap. There is no need for any slop time.
>> +static void __wake_lock(struct wake_lock *lock, long timeout, bool has_timeout)
>
> If the timeout == 0 or < 0 can you assume has_timeout?
No, wake_lock(lock) is different from wake_lock_timeout(lock, 0) or
wake_lock_timeout(lock, -1).
>> +/**
>> + * wake_lock() - Lock a wakelock
>> + * @lock: The wakelock to lock.
>> + */
>> +void wake_lock(struct wake_lock *lock)
>
> Naming is a little confusing to me. wake_lock(...) reads as an
> initialization / constructor type of function, but its really a locking
> operation. Perhaps lock_wake_lock would be more clear.
So is spin_lock. Most init functions in the kernel have init in their name.
>> +/**
>> + * wake_lock() - Unlock a wakelock
>> + * @lock: The wakelock to unlock.
>> + */
>> +void wake_unlock(struct wake_lock *lock)
>
> unlock_wake_lock?
That does not seem consistent with other kernel apis.
>> +int wake_lock_active(struct wake_lock *lock)
>> +{
>> + return !!(lock->flags & WAKE_LOCK_ACTIVE);
>
> A double bang? '!!' shouldn't this be syntax error? looks odd.
No.
>
>> +}
>> +EXPORT_SYMBOL(wake_lock_active);
>> +
>> +void request_suspend_state(suspend_state_t state)
>> +{
>> + unsigned long irqflags;
>> + spin_lock_irqsave(&state_lock, irqflags);
> why not grab this lock after the debug_mask if block?
To make sure the debug output is correct. It prints state protected by
the spinlock.
--
Arve Hjønnevåg
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
