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

[NeilBrown]

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.

NeilBrown

Attachment: signature.asc
Description: PGP signature