Message-ID: <AANLkTinS_UyhRNMnViK_4aA4ve3thHhLthC+OM7cUMZc@mail.gmail.com>
Date:	Fri, 6 Aug 2010 20:19:26 -0700
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	Matthew Garrett <mjg59@...f.ucam.org>, david@...g.hm,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Arjan van de Ven <arjan@...radead.org>,
	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	pavel@....cz, florian@...kler.org, swetland@...gle.com,
	peterz@...radead.org, tglx@...utronix.de, alan@...rguk.ukuu.org.uk
Subject: Re: Attempted summary of suspend-blockers LKML thread

2010/8/6 Alan Stern <stern@...land.harvard.edu>:
> On Thu, 5 Aug 2010, Arve Hjønnevåg wrote:
>
>> count, tells you how many times the wakelock was activated. If a
>> wakelock prevented suspend for a long time a large count tells you it
>> handled a lot of events while a small count tells you it took a long
>> time to process the events, or the wakelock was not released properly.
>
> As noted, we already have this.
>

Almost. We have it when a device is passed in.

>> expire_count, tells you how many times the timeout expired. For the
>> input event wakelock in the android kernel (which has a timeout) an
>> expire count that matches the count tells you that someone opened an
>> input device but is not reading from it (this has happened several
>> times).
>
> This is a little tricky.  Rafael's model currently does not allow
> wakeup events started by pm_wakeup_event() to be cancelled any way
> other than by having their timer expire.  This essentially means that
> for some devices, expire_count will always be the same as count and for
> others it will always be 0.  To change this would require adding an
> extra timer struct, which could be done (in fact, an earlier version of
> the code included it).  It would be nice if we could avoid the need.
>
> Does Android use any kernel-internal wakelocks both with a timer and
> with active cancellation?
>

I don't know if they are all kernel-internal but these drivers appear
to use timeouts and active cancellation on the same wakelock:
wifi driver, mmc core, alarm driver, evdev (suspend blocker version
removes the timeout).

>> wake_count, tells you that this is the first wakelock that was
>> acquired in the resume path. This is currently less useful than I
>> would like on the Nexus One since it is usually "SMD_RPCCALL" which
>> does not tell me a lot.
>
> This could be done easily enough, but if it's not very useful then
> there's no point.
>
It is useful there is no other way to tell what triggered a wakeup,
but it would probably be better to just track wakeup interrupts/events
elsewhere.

>> active_since, tells you how long a a still active wakelock has been
>> active. If someone activated a wakelock and never released it, it will
>> be obvious here.
>
> Easily added.  But you didn't mention any field saying whether the
> wakelock is currently active.  That could be added too (although it
> would be racy -- but for detecting unreleased wakelocks you wouldn't
> care).
>

These are the reported stats, not the fields in the stats structure.
The wakelock code has an active flag. If we want to keep the
pm_stay_wake nesting (which I would argue against), we would need an
active count. It would also require a handle, which is a change Rafael
said would not fly.

>> total_time, total time the wake lock has been active. This one should
>> be obvious.
>
> Also easily added.
>
Only with a handle passed to all the calls.

>> sleep_time, total time the wake lock has been active when the screen was off.
>
> Not applicable to general systems.  Is there anything like it that
> _would_ apply in general?
>

The screen off is how it is used on android, the stats is keyed of
what user space wrote to /sys/power/state. If "on" was written the
sleep time is not updated.

>> max_time, longest time the wakelock was active uninterrupted. This
>> used less often, but the battery on a device was draining fast, but
>> the problem went away before looking at the stats this will show if a
>> wakelock was active for a long time.
>
> Again, easily added.  The only drawback is that all these additions
> will bloat the size of struct device.  Of course, that's why you used
> separately-allocated structures for your wakelocks.  Maybe we can
> change to do the same; it seems likely that the majority of device
> structures won't ever be used for wakeup events.
>

Since many wakelocks are not associated with s struct device we need a
separate object for this anyway.

>> >> and I would prefer that the kernel interfaces would
>> >> encourage drivers to block suspend until user space has consumed the
>> >> event, which works for the android user space, instead of just long
>> >> enough to work with a hypothetical user space power manager.
>
> Rafael doesn't _discourage_ drivers from doing this.  However you have
> to keep in mind that many kernel developers are accustomed to working
> on systems (mostly PCs) with a different range of hardware devices from
> embedded systems like your phones.  With PCI devices(*), for example,
> there's no clear point where a wakeup event gets handed off to
> userspace.
>
> On the other hand, there's no reason the input layer shouldn't use
> pm_stay_awake and pm_relax.  It simply hasn't been implemented yet.
...

The merged user space interface makes this unclear to me. When I first
used suspend on android I had a power manager process that opened all
the input devices and reset a screen off timeout every time there was
an input event. If the input layer uses pm_stay_awake to block suspend
when the queue is not empty, this will deadlock with the current
interface since reading the wake count will block forever if an input
event occurred right after the power manager decides to suspend.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists