On Thu, Feb 5, 2009 at 4:10 PM, mark gross <mgross@xxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 04, 2009 at 06:50:16PM -0800, Arve Hjønnevåg wrote:
>> +static void update_sleep_wait_stats_locked(int done)
>> +{
>> + struct wake_lock *lock;
>> + ktime_t now, etime, elapsed, add;
>> + int expired;
>> +
>> + now = ktime_get();
>> + elapsed = ktime_sub(now, last_sleep_time_update);
>> + list_for_each_entry(lock, &active_wake_locks[WAKE_LOCK_SUSPEND], link) {
>
> Is this list opperation SMP safe? what if some on on the other CPU
> removes a lock form user mode while walking this guy? this goes for all
> your list walking.
I have not tested the code on an SMP system, but the list is protected
by a spinlock.
>> +static void expire_wake_lock(struct wake_lock *lock)
>> +{
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + wake_unlock_stat_locked(lock, 1);
>> +#endif
>> + lock->flags &= ~(WAKE_LOCK_ACTIVE | WAKE_LOCK_AUTO_EXPIRE);
>> + list_del(&lock->link);
>> + list_add(&lock->link, &inactive_locks);
>> + if (debug_mask & (DEBUG_WAKE_LOCK | DEBUG_EXPIRE))
>> + pr_info("expired wake lock %s\n", lock->name);
>> +}
>
> Are we missing locking for this list opperation?
No.
>> +static void suspend(struct work_struct *work)
>
> there are a lot of "suspend" functions in the kernel already that have
> different calling semantics, can we change this name so my TAG file is
> more sane?
OK.
>
>> +{
>> + int ret;
>> + int entry_event_num;
>> +
>> + if (has_wake_lock(WAKE_LOCK_SUSPEND)) {
>> + if (debug_mask & DEBUG_SUSPEND)
>> + pr_info("suspend: abort suspend\n");
>> + return;
>> + }
>> +
>> + entry_event_num = current_event_num;
>> + sys_sync();
>
> um, this feel wrong to me.
Wrong or redundant? I can remove sys_sync here since pm_suspend calls it anyway.
>
>> + if (debug_mask & DEBUG_SUSPEND)
>> + pr_info("suspend: enter suspend\n");
>> + ret = pm_suspend(requested_suspend_state);
>
> oh, you are adding yet another path to getting the system into a
> suspended state. is this necessary?
Yes.
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + create_proc_read_entry("wakelocks", S_IRUGO, NULL,
>> + wakelocks_read_proc, NULL);
>
> Shouldn't we *not* be using /proc? I think this should be under sysfs.
It is not allowed under sysfs. Debugfs has been suggested, but we
don't have debugfs mounted, and we include the wakelock stats in debug
reports.
--
Arve Hjønnevåg
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
