Re: [PATCH 01/13] PM: Add wake lock api.

Arve Hjønnevåg
2009-02-06T00:13:27Z

On Thu, Feb 5, 2009 at 2:51 PM, mark gross <mgross@linux.intel.com> 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.
>
> How much of this can be implemented using a pm-qos approach?

WAKE_LOCK_IDLE overlaps with pm-qos, if you use it to prevent latency.

>
>> +
>> +If the suspend operation has already started when locking a wakelock, it will
>> +abort the suspend operation as long it has not already reached the suspend_late
>> +stage. This means that locking a wakelock from an interrupt handler or a
>> +freezeable thread always works, but if you lock a wakelock from a suspend_late
>> +handler you must also return an error from that handler to abort suspend.
>> +
>> +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.
>> +- 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 +++ +++
>> +
>
> This feels a lot like operating points implemented as a collection of
> product specific binary locks. How could this be generalized to be less
> application specific?

Are you complaining about the example being product specific, or the
api? I don't think the api is product specific.

>> +When the driver determines that it needs to run (usually in an interrupt
>> +handler) it calls wake_lock:
>> + wake_lock(&state->wakelock);
>
> um, if a driver grabs the wakelock for another device that is sleeping
> how will it know when that sleeping device is ready to be used? Is

A driver does not grab another driver's wakelock. If your driver
depends on another device it should wait for resume to be called
before accessing it.

> wake_lock going to block until the dives its locking is in a good power
> state? (I'm guessing yes) Is doing this in the context of and IRQ a
> good idea?

wake_lock never blocks.

>> +It can also call wake_lock_timeout to release the wakelock after a delay:
>> + wake_lock_timeout(&state->wakelock, HZ);
>
> Timer?

What is your question?

>> +struct wake_lock {
>> +#ifdef CONFIG_HAS_WAKELOCK
>
> Shouldn't this #ifdef be in the files using wake_lock.h? If
> !CONFIG_HAS_WAKELOCK then should all the definitions here not be parsed
> at compile time?

I think the general policy is to avoid #ifdefs in the .c files.

>
>> + struct list_head link;
>> + int flags;
>> + const char *name;
>> + unsigned long expires;
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + struct {
>> + int count;
>> + int expire_count;
>> + int wakeup_count;
>> + ktime_t total_time;
>> + ktime_t prevent_suspend_time;
>> + ktime_t max_time;
>> + ktime_t last_time;
>> + } stat;
>> +#endif
>> +#endif
>> +};
>> +
>> +#ifdef CONFIG_HAS_WAKELOCK
>> +
>> +void wake_lock_init(struct wake_lock *lock, int type, const char *name);
>> +void wake_lock_destroy(struct wake_lock *lock);
>> +void wake_lock(struct wake_lock *lock);
>> +void wake_lock_timeout(struct wake_lock *lock, long timeout);
>> +void wake_unlock(struct wake_lock *lock);
>> +
>> +/* wake_lock_active returns a non-zero value if the wake_lock is currently
>> + * locked. If the wake_lock has a timeout, it does not check the timeout,
>> + * but if the timeout had already expired when it was checked elsewhere
>> + * this function will return 0.
>> + */
>> +int wake_lock_active(struct wake_lock *lock);
>> +
>> +/* has_wake_lock returns 0 if no wake locks of the specified type are active,
>> + * and non-zero if one or more wake locks are held. Specifically it returns
>> + * -1 if one or more wake locks with no timeout are active or the
>> + * number of jiffies until all active wake locks time out.
>> + */
>> +long has_wake_lock(int type);
>
> Type? or class? What is this type concept? Should we have some strong
> typing to the enum list above or at least a comment on what are the
> types that are valid to call this API with?

+ WAKE_LOCK_SUSPEND, /* Prevent suspend */
+ WAKE_LOCK_IDLE, /* Prevent low power idle */

--
Arve Hjønnevåg