[PATCH] ACPI button: remove hotkey suspend acpi proc event

[PATCH] ACPI button: remove hotkey suspend acpi proc event

[AceLan Kao]

If user press the suspend hotkey many times quickly, system will keep
entering S3 after wake up as many times as user pressed.
We do not expect the system enter S3 again after wake up, even if we
press the hotkey many times.

This issue comes from the acpi proc event will queue the events that
didn't process yet, and then report the events one by one when available,
so that system will enter S3 after wake up.

I think it's safe not to generate the proc event while pressing the
suspend hotkey, since "/proc/acpi/event" is deprecated. And system can
enter S3 correctly without it.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/acpi/button.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index d27d072..e35bd92 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -298,8 +298,6 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 
 			pm_wakeup_event(&device->dev, 0);
 		}
-
-		acpi_bus_generate_proc_event(device, event, ++button->pushed);
 		break;
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-- 
1.7.9.5

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[Matthew Garrett]

On Fri, Jul 13, 2012 at 02:13:22PM +0800, AceLan Kao wrote:

> I think it's safe not to generate the proc event while pressing the
> suspend hotkey, since "/proc/acpi/event" is deprecated. And system can
> enter S3 correctly without it.

Why does this happen via /proc, but not via evdev?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[AceLan Kao]

2012/7/13 Matthew Garrett <mjg59@srcf.ucam.org>:
> On Fri, Jul 13, 2012 at 02:13:22PM +0800, AceLan Kao wrote:
>
>> I think it's safe not to generate the proc event while pressing the
>> suspend hotkey, since "/proc/acpi/event" is deprecated. And system can
>> enter S3 correctly without it.
>
> Why does this happen via /proc, but not via evdev?
There are many different events to inform the system concurrently,
evdev is just one of them.
I'm not sure which one trigger the system to suspend while pressing
the suspend hotkey, but
the following suspends are triggered by acpi proc event.


-- 
Chia-Lin Kao(AceLan)
http://blog.acelan.idv.tw/
E-Mail: acelan.kaoATcanonical.com (s/AT/@/)

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[Matthew Garrett]

On Fri, Jul 13, 2012 at 03:07:38PM +0800, AceLan Kao wrote:

> There are many different events to inform the system concurrently,
> evdev is just one of them.
> I'm not sure which one trigger the system to suspend while pressing
> the suspend hotkey, but
> the following suspends are triggered by acpi proc event.

I'm fine with killing the procfs interface (it's deprecated anyway, I 
think?) but I'm a little worried about this only handling one case. Is 
userspace guaranteed to ignore any further events that were delivered 
over the input layer.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[Henrique de Moraes Holschuh]

On Fri, 13 Jul 2012, Matthew Garrett wrote:
> On Fri, Jul 13, 2012 at 03:07:38PM +0800, AceLan Kao wrote:
> > There are many different events to inform the system concurrently,
> > evdev is just one of them.
> > I'm not sure which one trigger the system to suspend while pressing
> > the suspend hotkey, but
> > the following suspends are triggered by acpi proc event.
> 
> I'm fine with killing the procfs interface (it's deprecated anyway, I 
> think?) but I'm a little worried about this only handling one case. Is 
> userspace guaranteed to ignore any further events that were delivered 
> over the input layer.

No.  Some firmware (thinkpads) even used to latch state and refuse to
reissue sleep buttom events for a while...

Is there any reason why we cannot do the same?  Entering S3/S4 is a
really good time to flush the contents of any input device and acpi
[userspace] event queues at first glance...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[AceLan Kao]

2012/7/13 Henrique de Moraes Holschuh <hmh@hmh.eng.br>:
> On Fri, 13 Jul 2012, Matthew Garrett wrote:
>> On Fri, Jul 13, 2012 at 03:07:38PM +0800, AceLan Kao wrote:
>> > There are many different events to inform the system concurrently,
>> > evdev is just one of them.
>> > I'm not sure which one trigger the system to suspend while pressing
>> > the suspend hotkey, but
>> > the following suspends are triggered by acpi proc event.
>>
>> I'm fine with killing the procfs interface (it's deprecated anyway, I
>> think?) but I'm a little worried about this only handling one case. Is
>> userspace guaranteed to ignore any further events that were delivered
>> over the input layer.
>
> No.  Some firmware (thinkpads) even used to latch state and refuse to
> reissue sleep buttom events for a while...
>
> Is there any reason why we cannot do the same?  Entering S3/S4 is a
> really good time to flush the contents of any input device and acpi
> [userspace] event queues at first glance...
thinkpads use its own thinkpad_acpi driver to handle this, it will submit
events to proc acpi event by itself through acpi_bus_generate_proc_event().

And BTW, I found a ThinkPad X220 and do the test, it won't enter S3 multiple
times if I press the suspend keys many times. And I try to remove thinkpad_acpi
module and stop the acpid and press the button again, the system enter S3
correctly, too. So, it won't damage this function from removing proc acpi event.

So, I didn't get the point why we can just remove the event, we don't rely on it
to enter S3 now, and to take into account that it's deprecated since
2007(fb804714),
I don't think we should do more workaround for this.

-- 
Chia-Lin Kao(AceLan)
http://blog.acelan.idv.tw/
E-Mail: acelan.kaoATcanonical.com (s/AT/@/)

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[Matthew Garrett]

On Mon, Jul 16, 2012 at 10:27:57AM +0800, AceLan Kao wrote:

> So, I didn't get the point why we can just remove the event, we don't rely on it
> to enter S3 now, and to take into account that it's deprecated since
> 2007(fb804714),
> I don't think we should do more workaround for this.

I've no objection to removing the event, I just want to know why we 
don't also need a solution in the evdev path. Why is this not a problem 
there?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[Henrique de Moraes Holschuh]

On Mon, 23 Jul 2012, Matthew Garrett wrote:
> On Mon, Jul 16, 2012 at 10:27:57AM +0800, AceLan Kao wrote:
> > So, I didn't get the point why we can just remove the event, we don't rely on it
> > to enter S3 now, and to take into account that it's deprecated since
> > 2007(fb804714),
> > I don't think we should do more workaround for this.
> 
> I've no objection to removing the event, I just want to know why we 
> don't also need a solution in the evdev path. Why is this not a problem 
> there?

Indeed.

And if it is related _directly_ to thinkpad-acpi, please state so.  Because
thinkpad-acpi does have legacy compatibility crap that can issue duplicated
events (same event over different paths: evdev, acpi procfs, acpi netlink)
but it is supposed to never do that unless actively configured to do so.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[AceLan Kao]

Hi,

2012/7/23 Henrique de Moraes Holschuh <hmh@hmh.eng.br>:
> On Mon, 23 Jul 2012, Matthew Garrett wrote:
>> On Mon, Jul 16, 2012 at 10:27:57AM +0800, AceLan Kao wrote:
>> > So, I didn't get the point why we can just remove the event, we don't rely on it
>> > to enter S3 now, and to take into account that it's deprecated since
>> > 2007(fb804714),
>> > I don't think we should do more workaround for this.
>>
>> I've no objection to removing the event, I just want to know why we
>> don't also need a solution in the evdev path. Why is this not a problem
>> there?
Actually, it will enter S3 at most 2 times through evdev path.
The keyevents will be emitted at once while pressing the hotkey,
the GUI(gnome-power-manager under gnome, for instance) will do the reaction.
So, I think GUI will just ignore the following sleep events.
And it will enter another S3 after doing input_dev_release_keys() in
drivers/input/input.c +2174 during resuming if the key is not thought
to be released.

The big difference between evdev and proc acpi event is that
proc acpi event won't send out the next event until the current one is done.
We can observe its behavior by "acpi_listen"
It sends out the next event after the system wakeup, so that the
system will enter S3 again.
I didn't dig into the code where it queues the events, I just observed it.

>
> Indeed.
>
> And if it is related _directly_ to thinkpad-acpi, please state so.  Because
> thinkpad-acpi does have legacy compatibility crap that can issue duplicated
> events (same event over different paths: evdev, acpi procfs, acpi netlink)
> but it is supposed to never do that unless actively configured to do so.
This patch do nothing to thinkpad-acpi, and I have already verified
that it won't
damage the current behavior of thinkpad-acpi driver.

>
> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh



-- 
Chia-Lin Kao(AceLan)
http://blog.acelan.idv.tw/
E-Mail: acelan.kaoATcanonical.com (s/AT/@/)

Re: [PATCH] ACPI button: remove hotkey suspend acpi proc event

[AceLan Kao]

Hi,

Do we have any conclusion about this patch?

Taking account of the proc acpi event is deprecated, it should be okay
to remove it.
I also verified this patch works well with ThinkPad X220, and once if
there is a problem,
then we should come out another fix for it, instead of preventing this
patch from being merged,
since we face an re-entering S3 issue now.
Thanks.

Best regards,
AceLan Kao.

2012/7/24 AceLan Kao <acelan.kao@canonical.com>:
> Hi,
>
> 2012/7/23 Henrique de Moraes Holschuh <hmh@hmh.eng.br>:
>> On Mon, 23 Jul 2012, Matthew Garrett wrote:
>>> On Mon, Jul 16, 2012 at 10:27:57AM +0800, AceLan Kao wrote:
>>> > So, I didn't get the point why we can just remove the event, we don't rely on it
>>> > to enter S3 now, and to take into account that it's deprecated since
>>> > 2007(fb804714),
>>> > I don't think we should do more workaround for this.
>>>
>>> I've no objection to removing the event, I just want to know why we
>>> don't also need a solution in the evdev path. Why is this not a problem
>>> there?
> Actually, it will enter S3 at most 2 times through evdev path.
> The keyevents will be emitted at once while pressing the hotkey,
> the GUI(gnome-power-manager under gnome, for instance) will do the reaction.
> So, I think GUI will just ignore the following sleep events.
> And it will enter another S3 after doing input_dev_release_keys() in
> drivers/input/input.c +2174 during resuming if the key is not thought
> to be released.
>
> The big difference between evdev and proc acpi event is that
> proc acpi event won't send out the next event until the current one is done.
> We can observe its behavior by "acpi_listen"
> It sends out the next event after the system wakeup, so that the
> system will enter S3 again.
> I didn't dig into the code where it queues the events, I just observed it.
>
>>
>> Indeed.
>>
>> And if it is related _directly_ to thinkpad-acpi, please state so.  Because
>> thinkpad-acpi does have legacy compatibility crap that can issue duplicated
>> events (same event over different paths: evdev, acpi procfs, acpi netlink)
>> but it is supposed to never do that unless actively configured to do so.
> This patch do nothing to thinkpad-acpi, and I have already verified
> that it won't
> damage the current behavior of thinkpad-acpi driver.
>
>>
>> --
>>   "One disk to rule them all, One disk to find them. One disk to bring
>>   them all and in the darkness grind them. In the Land of Redmond
>>   where the shadows lie." -- The Silicon Valley Tarot
>>   Henrique Holschuh
>
>
>
> --
> Chia-Lin Kao(AceLan)
> http://blog.acelan.idv.tw/
> E-Mail: acelan.kaoATcanonical.com (s/AT/@/)



-- 
Chia-Lin Kao(AceLan)
http://blog.acelan.idv.tw/
E-Mail: acelan.kaoATcanonical.com (s/AT/@/)