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

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

[Lukas Märdian]

This gives the userspace (Replicant) a chance to fully handle the
pm_wakeup_event, before autosleep suspends the system alltogether
again.

This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
the Replicant 4.2.2 userspace, which needs to execute this to stay
awake: 'echo on > /sys/power/state'

Signed-off-by: Lukas Märdian <lukas@goldelico.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/keyboard/gpio_keys.c     | 7 ++-----
 drivers/input/misc/twl4030-pwrbutton.c | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..be8c62e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -345,9 +345,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		container_of(work, struct gpio_button_data, work);
 
 	gpio_keys_gpio_report_event(bdata);
-
-	if (bdata->button->wakeup)
-		pm_relax(bdata->input->dev.parent);
 }
 
 static void gpio_keys_gpio_timer(unsigned long _data)
@@ -364,7 +361,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 	BUG_ON(irq != bdata->irq);
 
 	if (bdata->button->wakeup)
-		pm_stay_awake(bdata->input->dev.parent);
+		pm_wakeup_event(bdata->input->dev.parent, 1000);
 	if (bdata->timer_debounce)
 		mod_timer(&bdata->timer,
 			jiffies + msecs_to_jiffies(bdata->timer_debounce));
@@ -402,7 +399,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 
 	if (!bdata->key_pressed) {
 		if (bdata->button->wakeup)
-			pm_wakeup_event(bdata->input->dev.parent, 0);
+			pm_wakeup_event(bdata->input->dev.parent, 1000);
 
 		input_event(input, EV_KEY, button->code, 1);
 		input_sync(input);
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index fb3b63b..8622e43 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -41,7 +41,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
 
 	err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &value, STS_HW_CONDITIONS);
 	if (!err)  {
-		pm_wakeup_event(pwr->dev.parent, 0);
+		pm_wakeup_event(pwr->dev.parent, 1000);
 		input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
 		input_sync(pwr);
 	} else {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pavel Machek]

Hi!

> This gives the userspace (Replicant) a chance to fully handle the
> pm_wakeup_event, before autosleep suspends the system alltogether
> again.
> 
> This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
> the Replicant 4.2.2 userspace, which needs to execute this to stay
> awake: 'echo on > /sys/power/state'
> 
> Signed-off-by: Lukas Märdian <lukas@goldelico.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

I'm sorry, but we should not be doing this.

You basically put a delay in driver to work around userspace bug. There must be better
solution....

										Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Dr. H. Nikolaus Schaller]

Hi,
Am 28.06.2014 um 19:08 schrieb Pavel Machek:

> Hi!
> 
>> This gives the userspace (Replicant) a chance to fully handle the
>> pm_wakeup_event, before autosleep suspends the system alltogether
>> again.
>> 
>> This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
>> the Replicant 4.2.2 userspace, which needs to execute this to stay
>> awake: 'echo on > /sys/power/state'
>> 
>> Signed-off-by: Lukas Märdian <lukas@goldelico.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> I'm sorry, but we should not be doing this.
> 
> You basically put a delay in driver to work around userspace bug.

Do you think it is a user-space bug if the kernel goes to sleep again
before giving user space any chance to react to an event?

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().

And if I understand the code of __pm_wakeup_event() correctly it does
*not* delay, but just modifies the wakeup_source timer to expire a little
later. I.e. keep the system longer awake. So as long as the system is
not suspended there is no difference to current driver.

> There must be better
> solution....

I am not sure how it could look like.

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

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

[Pavel Machek]

Hi!

> >> This gives the userspace (Replicant) a chance to fully handle the
> >> pm_wakeup_event, before autosleep suspends the system alltogether
> >> again.
> >> 
> >> This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
> >> the Replicant 4.2.2 userspace, which needs to execute this to stay
> >> awake: 'echo on > /sys/power/state'
> >> 
> >> Signed-off-by: Lukas Märdian <lukas@goldelico.com>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> > 
> > I'm sorry, but we should not be doing this.
> > 
> > You basically put a delay in driver to work around userspace bug.
> 
> Do you think it is a user-space bug if the kernel goes to sleep again
> before giving user space any chance to react to an event?

Well, who says 1000msec is enough? Some userspace may need
more. ... for example on PC when you keyboard-handling deamon is
swapped out.

> 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...

> > There must be better
> > solution....
>  
> I am not sure how it could look like.

Rafael, do you have any idea how this is supposed to work?

Original patch is at https://lkml.org/lkml/2014/4/10/156 .
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2570 bytes --]

On Sat, 28 Jun 2014, Pavel Machek wrote:

> Hi!
> 
> > >> This gives the userspace (Replicant) a chance to fully handle the
> > >> pm_wakeup_event, before autosleep suspends the system alltogether
> > >> again.
> > >> 
> > >> This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
> > >> the Replicant 4.2.2 userspace, which needs to execute this to stay
> > >> awake: 'echo on > /sys/power/state'
> > >> 
> > >> Signed-off-by: Lukas Märdian <lukas@goldelico.com>
> > >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> > > 
> > > I'm sorry, but we should not be doing this.
> > > 
> > > You basically put a delay in driver to work around userspace bug.
> > 
> > Do you think it is a user-space bug if the kernel goes to sleep again
> > before giving user space any chance to react to an event?
> 
> Well, who says 1000msec is enough? Some userspace may need
> more. ... for example on PC when you keyboard-handling deamon is
> swapped out.
> 
> > 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...
> 
> > > There must be better
> > > solution....
> >  
> > I am not sure how it could look like.
> 
> Rafael, do you have any idea how this is supposed to work?
> 
> Original patch is at https://lkml.org/lkml/2014/4/10/156 .

One possibility is not to use autosleep at all.  The user program,
instead of writing "on" to /sys/power/state to stay awake, would have
to write "mem" to go to sleep when no more work remained to be handled.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lukas Maerdian]

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

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

On Fri, 04 Jul 2014 12:39:38 +0200 Lukas Maerdian <lukas@goldelico.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]

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

> On Fri, 04 Jul 2014 12:39:38 +0200 Lukas Maerdian <lukas@goldelico.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[Dr. H. Nikolaus Schaller]

This gives the userspace (e.g. Replicant) a chance to fully handle the
pm_wakeup_event, before autosleep suspends the system alltogether
again.

This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
the Replicant 4.2.2 userspace, which needs to execute this to stay
awake: 'echo on > /sys/power/state'

Signed-off-by: Lukas Märdian <lukas@goldelico.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/keyboard/gpio_keys.c     | 7 ++-----
 drivers/input/misc/twl4030-pwrbutton.c | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..be8c62e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -345,9 +345,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		container_of(work, struct gpio_button_data, work);
 
 	gpio_keys_gpio_report_event(bdata);
-
-	if (bdata->button->wakeup)
-		pm_relax(bdata->input->dev.parent);
 }
 
 static void gpio_keys_gpio_timer(unsigned long _data)
@@ -364,7 +361,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 	BUG_ON(irq != bdata->irq);
 
 	if (bdata->button->wakeup)
-		pm_stay_awake(bdata->input->dev.parent);
+		pm_wakeup_event(bdata->input->dev.parent, 1000);
 	if (bdata->timer_debounce)
 		mod_timer(&bdata->timer,
 			jiffies + msecs_to_jiffies(bdata->timer_debounce));
@@ -402,7 +399,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 
 	if (!bdata->key_pressed) {
 		if (bdata->button->wakeup)
-			pm_wakeup_event(bdata->input->dev.parent, 0);
+			pm_wakeup_event(bdata->input->dev.parent, 1000);
 
 		input_event(input, EV_KEY, button->code, 1);
 		input_sync(input);
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index fb3b63b..8622e43 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -41,7 +41,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
 
 	err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &value, STS_HW_CONDITIONS);
 	if (!err)  {
-		pm_wakeup_event(pwr->dev.parent, 0);
+		pm_wakeup_event(pwr->dev.parent, 1000);
 		input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
 		input_sync(pwr);
 	} else {
-- 
1.9.1