Re: [PATCH] gpio_keys, twl4030-pwrbutton: stay awake for 1sec on resume

[NeilBrown]

On Fri, 4 Jul 2014 22:03:36 +1000 NeilBrown <neilb@xxxxxxx> wrote:

> On Fri, 04 Jul 2014 12:39:38 +0200 Lukas Maerdian <lukas@xxxxxxxxxxxxx> wrote:
> 
> > Hi all!
> > 
> > On 28.06.2014 22:04 UTC+0200, Pavel Machek wrote:
> > >> 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

Attachment: signature.asc
Description: PGP signature