On Thu, Feb 05, 2009 at 04:38:04PM -0800, Arve Hjønnevåg wrote:
> 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.
ok, I was just looking for it and didn't notice it in the calling code.
>
> >> +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.
ok.
>
> >> +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.
its just a latency enhanced api call I wouldn't expect to see called.
>
> >
> >> + 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.
>
why not under sysfs?
--mgross
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
