On Sat, Feb 7, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> +{
>> + struct timespec ts;
>> + struct timespec kt;
>> + struct timespec tomono;
>> + struct timespec delta;
>> + unsigned long seq;
>> + long timeout;
>> +
>> + if (!(lock->flags & WAKE_LOCK_AUTO_EXPIRE))
>> + return 0;
>> + do {
>> + seq = read_seqbegin(&xtime_lock);
>> + timeout = lock->expires - jiffies;
>
> Can it overflow?
Yes, but I have not fixed this since you have to hold a wakelock for
several months to hit it. I could fix it may expiring wakelocks at
least every MAX_JIFFY_OFFSET or by moving to jiffies_64.
> Anyway, you are using struct timespec in computations, so why don't you
> store it in the lock structure?
I only use struct timespec for the stats.
>> +
>> +static int print_lock_stat(char *buf, struct wake_lock *lock)
>> +{
>> + int lock_count = lock->stat.count;
>> + int expire_count = lock->stat.expire_count;
>> + ktime_t active_time = ktime_set(0, 0);
>> + ktime_t total_time = lock->stat.total_time;
>> + ktime_t max_time = lock->stat.max_time;
>> + ktime_t prevent_suspend_time = lock->stat.prevent_suspend_time;
>> + if (lock->flags & WAKE_LOCK_ACTIVE) {
>> + ktime_t now, add_time;
>> + int expired = get_expired_time(lock, &now);
>> + if (!expired)
>> + now = ktime_get();
>> + add_time = ktime_sub(now, lock->stat.last_time);
>> + lock_count++;
>> + if (!expired)
>> + active_time = add_time;
>> + else
>> + expire_count++;
>> + total_time = ktime_add(total_time, add_time);
>> + if (lock->flags & WAKE_LOCK_PREVENTING_SUSPEND)
>> + prevent_suspend_time = ktime_add(prevent_suspend_time,
>> + ktime_sub(now, last_sleep_time_update));
>> + if (add_time.tv64 > max_time.tv64)
>> + max_time = add_time;
>> + }
>> +
>> + return sprintf(buf, "\"%s\"\t%d\t%d\t%d\t%lld\t%lld\t%lld\t%lld\t"
>> + "%lld\n", lock->name, lock_count, expire_count,
>> + lock->stat.wakeup_count, ktime_to_ns(active_time),
>> + ktime_to_ns(total_time),
>> + ktime_to_ns(prevent_suspend_time), ktime_to_ns(max_time),
>> + ktime_to_ns(lock->stat.last_time));
>> +}
>> +
>
> Why did you decide to put that in /proc ?
That's where most other stats are reported.
>> +static long has_wake_lock_locked(int type)
>
> I'd change the name, it suggests something different from what the function
> does.
It does what the name suggest, and more. The code that just cares if
there are lock held or not is easier to read this way.
>> +{
>> + struct wake_lock *lock, *n;
>> + long max_timeout = 0;
>> +
>> + BUG_ON(type >= WAKE_LOCK_TYPE_COUNT);
>> + list_for_each_entry_safe(lock, n, &active_wake_locks[type], link) {
>> + if (lock->flags & WAKE_LOCK_AUTO_EXPIRE) {
>> + long timeout = lock->expires - jiffies;
>
> I think time_after() is for things like this.
time_after does not return the timeout, which I need here. I changed
another instance though.
>> +static void suspend(struct work_struct *work)
>> +{
>> + 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();
>
> pm_suspend() will do it, you don't need to.
Removed.
>
>> + if (debug_mask & DEBUG_SUSPEND)
>> + pr_info("suspend: enter suspend\n");
>
> Shouldn't that check if someone has taken a wakelock in the meantime?
It checks at the top of the function. With sys_sync gone, another
check would not be useful.
>> + ret = pm_suspend(requested_suspend_state);
>> +static int power_suspend_late(struct platform_device *pdev, pm_message_t state)
>> +{
>> + int ret = has_wake_lock(WAKE_LOCK_SUSPEND) ? -EAGAIN : 0;
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + wait_for_wakeup = 1;
>> +#endif
>> + if (debug_mask & DEBUG_SUSPEND)
>> + pr_info("power_suspend_late return %d\n", ret);
>> + return ret;
>> +}
>> +
>> +static struct platform_driver power_driver = {
>> + .driver.name = "power",
>> + .suspend_late = power_suspend_late,
>> +};
>> +static struct platform_device power_device = {
>> + .name = "power",
>> +};
>
> What's this and what's it for?
It makes sure suspend fails if a wakelock is locked while suspending.
Is there a simpler way to register a suspend_late handler?
>> +void wake_lock_init(struct wake_lock *lock, int type, const char *name)
>> +{
>> + unsigned long irqflags = 0;
>> +
>> + if (name)
>> + lock->name = name;
>
> Hm. I'd rather reserve memory for the name and copy it from the address
> provided by the caller.
I'd rather not. With the current interface, wake_lock_init can never fail.
>> + BUG_ON(!lock->name);
>
> Isn't it a bit too drastic? Perhaps make it return a value so that the caller
> can check if it has the lock?
I think a BUG_ON is appropriate. Calling wake_lock_init with an
invalid argument is a driver bug.
>> +static void wake_lock_internal(
>
> What about __wake_lock() ?
OK.
>
>> + struct wake_lock *lock, long timeout, int has_timeout)
>
> What's the point of the last argument? Wouldn't timeout == 0 be sufficient?
No, timeout == 0 means expire immediately.
> Also, does it make sense to pass negative timeout to it?
A negative timeout is fine, as long as it does not underflow.
>
>> +{
>> + int type;
>> + unsigned long irqflags;
>> + long expire_in;
>> +
>> + spin_lock_irqsave(&list_lock, irqflags);
>> + type = lock->flags & WAKE_LOCK_TYPE_MASK;
>> + BUG_ON(type >= WAKE_LOCK_TYPE_COUNT);
>> + BUG_ON(!(lock->flags & WAKE_LOCK_INITIALIZED));
>
> I don't like these BUG_ON()s. Please do something less drastic instead.
>
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + if (type == WAKE_LOCK_SUSPEND && wait_for_wakeup) {
>> + if (debug_mask & DEBUG_WAKEUP)
>> + pr_info("wakeup wake lock: %s\n", lock->name);
>> + wait_for_wakeup = 0;
>> + lock->stat.wakeup_count++;
>> + }
>> + if ((lock->flags & WAKE_LOCK_AUTO_EXPIRE) &&
>> + (long)(lock->expires - jiffies) <= 0) {
>> + wake_unlock_stat_locked(lock, 0);
>> + lock->stat.last_time = ktime_get();
>> + }
>> +#endif
>> + if (!(lock->flags & WAKE_LOCK_ACTIVE)) {
>> + lock->flags |= WAKE_LOCK_ACTIVE;
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + lock->stat.last_time = ktime_get();
>> +#endif
>> + }
>> + list_del(&lock->link);
>> + if (has_timeout) {
>> + if (debug_mask & DEBUG_WAKE_LOCK)
>> + pr_info("wake_lock: %s, type %d, timeout %ld.%03lu\n",
>> + lock->name, type, timeout / HZ,
>> + (timeout % HZ) * MSEC_PER_SEC / HZ);
>> + lock->expires = jiffies + timeout;
>> + lock->flags |= WAKE_LOCK_AUTO_EXPIRE;
>> + list_add_tail(&lock->link, &active_wake_locks[type]);
>> + } else {
>> + if (debug_mask & DEBUG_WAKE_LOCK)
>> + pr_info("wake_lock: %s, type %d\n", lock->name, type);
>> + lock->expires = LONG_MAX;
>> + lock->flags &= ~WAKE_LOCK_AUTO_EXPIRE;
>> + list_add(&lock->link, &active_wake_locks[type]);
>> + }
>
> Why not to use list_add_tail() in both cases?
If you add a wakelock with no timeout, there is no reason for
has_wake_lock_locked to look at more than one wakelock.
>
>> + if (type == WAKE_LOCK_SUSPEND) {
>> + current_event_num++;
>> +#ifdef CONFIG_WAKELOCK_STAT
>> + if (lock == &main_wake_lock)
>> + update_sleep_wait_stats_locked(1);
>> + else if (!wake_lock_active(&main_wake_lock))
>> + update_sleep_wait_stats_locked(0);
>> +#endif
>> + if (has_timeout)
>> + expire_in = has_wake_lock_locked(type);
>> + else
>> + expire_in = -1;
>
> expire_i = has_timeout ? has_wake_lock_locked(type) : -1;
>
> (there is something like this above too).
>
>> + if (expire_in > 0) {
>> + if (debug_mask & DEBUG_EXPIRE)
>> + pr_info("wake_lock: %s, start expire timer, "
>> + "%ld\n", lock->name, expire_in);
>> + mod_timer(&expire_timer, jiffies + expire_in);
>
> What exactly are you trying to achieve here?
Unlock wakelocks with timeouts when doing so can allow the system to
go to sleep.
>
>> + } else {
>> + if (del_timer(&expire_timer))
>> + if (debug_mask & DEBUG_EXPIRE)
>> + pr_info("wake_lock: %s, stop expire "
>> + "timer\n", lock->name);
>> + if (expire_in == 0)
>> + queue_work(suspend_work_queue, &suspend_work);
>
> This appears to only happen if timeout = 0 is passed to this function while
> has_timeout == true. Is it correct?
Unless you are on a system where jiffies can change while this
function is running, then yes.
>> + ret = platform_device_register(&power_device);
>> + if (ret) {
>> + pr_err("wakelocks_init: platform_device_register failed\n");
>> + goto err_platform_device_register;
>> + }
>> + ret = platform_driver_register(&power_driver);
>> + if (ret) {
>> + pr_err("wakelocks_init: platform_driver_register failed\n");
>> + goto err_platform_driver_register;
>> + }
>
> Is this really a platform thing?
>
> What exactly is the benefit of having the 'power_device' and 'power_driver'
> registered?.
I need both for suspend_late to be called.
> Also, I'd really prefer to see the patch without the CONFIG_WAKELOCK_STAT
> thing included, first. It would have been much easier to read.
I moved more of the stats code to the CONFIG_WAKELOCK_STAT section
which should help.
--
Arve Hjønnevåg
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
