On Fri, 4 Jul 2014 22:03:36 +1000 NeilBrown [off-list ref] wrote:

On Fri, 04 Jul 2014 12:39:38 +0200 Lukas Maerdian [off-list ref] wrote:

quoted

Hi all!

On 28.06.2014 22:04 UTC+0200, Pavel Machek wrote:

quoted

quoted

And the msec parameter is described as:

@msec: Anticipated event processing time (in milliseconds).

Isn't calling pm_wakeup_event() with a non-zero msec the standard
method to handle this situation? And it is used in other drivers. E.g. in
_mmc_detect_change() or hub_suspend().

 * Notify the PM core of a wakeup event whose source is @ws that will
   take                    
 * approximately @msec milliseconds to be processed by the kernel.  If
   @ws is                 
 * not active, activate it.  If @msec is nonzero, set up the @ws'
   timer to                    
 * execute pm_wakeup_timer_fn() in future.                                                    

Will take @msec milliseconds to be processed by the _kernel_. Yes, USB
probing takes a lot of time in kernel. But you are using this
parameter to wait for userspace...

Well, it's not exactly waiting for userspace: The kernel goes to
suspend, before even being fully resumed.

In any case, 0 msec (i.e. nothing) seems to be insufficient, even for
just the in kernel processing. And I think that's exactly the root cause
of this race condition between the device drivers and the autosleep
module. Of course this only materializes if CONFIG_PM_AUTOSLEEP and
CONFIG_PM_WAKELOCKS are enabled, which is rarely used up to now, I guess.

I think we either need some way of signaling that the kernel has fully
finished resuming, before autosleep sets the system to suspend state
again, or we need to set appropriate delays in the individual device
drivers, to give them enough time to process the resume event.

As the pm_wakeup_event() call is already in place, I assume, that
setting appropriate processing times for each individual driver was the
intended way to go...

I've CCed Neil Brown, who inserted the pm_wakeup_even() call with a
0msec argument in both of the discussed drivers, so maybe he can shed
some light in this discussion?

Best regards,
  Lukas

You definitely shouldn't need a timeout.

I know I understood the whole "autosleep" design once, but I never really
liked it and memory fades.....

I think that a key part of using autosleep was that userspace needs to use
epoll with the EPOLLWAKEUP flag to listen for any events which can wake from
suspend.

If user-space is doing that properly, then the simple pm_wakeup_event(dev,0)
is enough to ensure that the event gets through epoll and into user-space.
I think userspace needs to take a wakelock before reading the event, though I
don't recall the exact details.

So: if Android is trying to use autosleep and finding that an event wakes the
device but it goes back to sleep again before it can handle the event, then
either the driver isn't doing the basic pm_wakeup_event, or android
user-space isn't using epoll and EPOLLWAKEUP as required.

The man pages for epoll-ctl(2) and epoll(7) now document the use of
EPOLLWAKEUP.
I don't think a formatted version is available yet but the raw version can be
seen at
  http://git.kernel.org/cgit/docs/man-pages/man-pages.git/tree/man2/epoll_ctl.2
and 
  http://git.kernel.org/cgit/docs/man-pages/man-pages.git/tree/man7/epoll.7

and you can right-click the "plain" link to down load, then
   man --local-file epoll_ctl.2
to read it nicely.

NeilBrown