| Message ID | 1342160002-13427-1-git-send-email-acelan.kao@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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.
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...
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.
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?
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.
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
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/@/)
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,
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(-)