Re: [PATCH 03/13] PM: Implement wakelock api.

[mark gross <mgross@linux.intel.com>]

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@linux.intel.com> 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