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
