powerd shouldn't disable keyboards that also include power buttons |
|||||||||||||
Issue descriptionFor issue 661368 , powerd started configuring input devices while in tablet mode. We have the following udev rules (in 90-powerd-id.rules) to mark devices as internal keyboards: SUBSYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1", DRIVERS=="atkbd", ENV{POWERD_ROLE}="internal_keyboard" SUBSYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1", DRIVERS=="cros-ec-keyb", ENV{POWERD_ROLE}="internal_keyboard" and the following (in 91-powerd-tags.rules) to make internal keyboards usable (for both input and wakeup) only while in laptop and display-off modes (i.e. not tablet or docked): ENV{POWERD_ROLE}=="internal_keyboard", ENV{POWERD_TAGS_USABLE}="usable_when_laptop usable_when_display_off" ENV{POWERD_ROLE}=="internal_keyboard", KERNEL=="input*", ENV{POWERD_TAGS_WAKEUP}="wakeup wakeup_only_when_usable" We have some partner bugs (http://b/36403242, http://b/35872420) about power buttons not working while in tablet mode on some devices. It sounds like this may be due to power button events coming in through the keyboard driver (atkbd, I think) and thus being suppressed in some modes. This arrangement seems non-ideal but I suspect there's nothing we can do about it (besides hopefully avoiding it in the future). :-/ We need to make powerd stop inhibiting keyboards that include the power button, I assume. (I think that Chrome will still ignore key events while in tablet mode.) What's the right way of doing this? Should we introduce a new "internal_keyboard_with_power_button" tag that's tagged as being usable in all modes? Do we also need to enable wakeup for these keyboards while in tablet mode, or will the power button still work for exiting S3 regardless of what powerd does? It would be unfortunate if regular keys woke convertible Chromebooks that are in tablet mode. I'm not sure that powerd is able to disable wakeup in a mode while still leaving the device usable while awake. Is that something that we need here?
,
Mar 23 2017
I was thinking about this some yesterday, and right now since we don't really use the ACPI power button event (BIOS never enables it for S0 SCI) we could re-purpose its runtime behavior to be a "tablet mode power button" and send it only when we are in tablet mode. MKBP could be another option, but it would also suffer from the same issue that when we aren't in tablet mode the power button event would be seen twice, one from the 8042 and one from MKBP.
,
Mar 23 2017
I am planning to work on moving all the button support from 8042 to MKBP driver for future x86 devices. This will require some re-factoring on the EC side. Kernel side looks good, but depthcharge side might require some work. I have not got a chance to look at this in detail yet. But I should have more updates by next week. If this work looks feasible, then we should have all button events coming only from MKBP.
,
Mar 24 2017
Should I just label all internal keyboard devices as usable while in tablet mode? I think I'll need to change things in powerd so I can disable keyboard wakeups while we're in tablet mode. Probably we need to have individual wakeup_when_laptop, wakeup_when_tablet, etc. tags instead of wakeup_only_when_usable. I believe that Chrome will still ignore keyboard events while it's in tablet mode; see e.g. the X11-era https://codereview.chromium.org/991933002. Jonathan, are you the right person to confirm this? Are there other things that will break if I do this?
,
Mar 24 2017
+igo -- talking with derat, this affects the power button on Caroline. We're sometimes ignoring it when the device is in tablet mode because we think it's part of the physical keyboard.
,
Mar 24 2017
#4 While in tablet mode Chrome will ignore all keyboard events aside from volume keys and power: https://cs.chromium.org/chromium/src/ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc?rcl=82a6458a23cf1be17524e2d329e8f1c5668acb22&l=17 We did this in the past for devices which showed all buttons as coming from one keyboard. Conversely some devices (I think Minnie and Cyan?) would report the power/volume keys are coming from a different keyboard source. Allowing for the complete disabling of the main keyboard.
,
Mar 24 2017
The powerd change to at least make it possible to configure this is low-risk, so I'm going ahead with it.
,
Mar 24 2017
I've uploaded https://chromium-review.googlesource.com/c/459439/ to add support for new "wakeup_when_laptop", "wakeup_when_tablet", etc. to powerd.
,
Mar 24 2017
,
Mar 28 2017
,
Mar 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/8ce1a8c90bf2616c376820b15e1a8fc567234afe commit 8ce1a8c90bf2616c376820b15e1a8fc567234afe Author: Daniel Erat <derat@chromium.org> Date: Tue Mar 28 10:47:45 2017 power: Add support for "wakeup_when_[mode]" udev tags. Add support for new mode-specific udev wakeup tags to powerd. Formerly, only a "wakeup_only_when_usable" tag was supported. This doesn't permit cases where we want a device to be usable but not to produce wakeups (imagine the tablet-mode behavior of a keyboard device that also reports events from a side power button). Now, if the "wakeup" tag is set and at least one of "wakeup_when_laptop", "wakeup_when_docked", "wakeup_when_tablet", and "wakeup_when_display_off" is set, the device will be configured to only produce wakeups while the system is in one of the requested modes. Also add a short doc describing how powerd interprets input devices' udev tags. BUG= chromium:703691 TEST=updated unit tests Change-Id: I1138782052f0370b8df7f0133b79ed1e8494f21a Reviewed-on: https://chromium-review.googlesource.com/459439 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [add] https://crrev.com/8ce1a8c90bf2616c376820b15e1a8fc567234afe/power_manager/docs/udev.md [modify] https://crrev.com/8ce1a8c90bf2616c376820b15e1a8fc567234afe/power_manager/powerd/policy/input_device_controller.cc [modify] https://crrev.com/8ce1a8c90bf2616c376820b15e1a8fc567234afe/power_manager/powerd/policy/input_device_controller.h [modify] https://crrev.com/8ce1a8c90bf2616c376820b15e1a8fc567234afe/power_manager/powerd/policy/input_device_controller_unittest.cc
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/dbc1058b3531a466aef3162da894af2e0f37588a commit dbc1058b3531a466aef3162da894af2e0f37588a Author: Daniel Erat <derat@chromium.org> Date: Thu Mar 30 22:48:18 2017 power: Log when tagged udev devices are added or removed. Make powerd log changes to tagged udev devices. Our tagging is about to get more complicated, and this will hopefully make it easier to figure out what's going on when looking at logs. BUG= chromium:703691 TEST=ran it and saw messages at startup Change-Id: Id512f181531459aa216e4ae0b556165e8272cbe1 Reviewed-on: https://chromium-review.googlesource.com/462378 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/dbc1058b3531a466aef3162da894af2e0f37588a/power_manager/powerd/system/tagged_device.h [modify] https://crrev.com/dbc1058b3531a466aef3162da894af2e0f37588a/power_manager/powerd/system/udev.cc
,
Mar 31 2017
Requesting a merge of the commits in #11 and #12. We'll need to merge udev changes for individual boards too.
,
Mar 31 2017
Bernie, mind approving this one too? I'd like to merge these before issue 707048 to avoid conflicts.
,
Mar 31 2017
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/d546979c52e024085ce3e63445c914728d51800c commit d546979c52e024085ce3e63445c914728d51800c Author: Daniel Erat <derat@chromium.org> Date: Fri Mar 31 18:25:25 2017 power: Add support for "wakeup_when_[mode]" udev tags. Add support for new mode-specific udev wakeup tags to powerd. Formerly, only a "wakeup_only_when_usable" tag was supported. This doesn't permit cases where we want a device to be usable but not to produce wakeups (imagine the tablet-mode behavior of a keyboard device that also reports events from a side power button). Now, if the "wakeup" tag is set and at least one of "wakeup_when_laptop", "wakeup_when_docked", "wakeup_when_tablet", and "wakeup_when_display_off" is set, the device will be configured to only produce wakeups while the system is in one of the requested modes. Also add a short doc describing how powerd interprets input devices' udev tags. BUG= chromium:703691 TEST=updated unit tests Change-Id: I1138782052f0370b8df7f0133b79ed1e8494f21a Reviewed-on: https://chromium-review.googlesource.com/459439 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> (cherry picked from commit 8ce1a8c90bf2616c376820b15e1a8fc567234afe) Reviewed-on: https://chromium-review.googlesource.com/465406 Reviewed-by: Dan Erat <derat@chromium.org> [add] https://crrev.com/d546979c52e024085ce3e63445c914728d51800c/power_manager/docs/udev.md [modify] https://crrev.com/d546979c52e024085ce3e63445c914728d51800c/power_manager/powerd/policy/input_device_controller.cc [modify] https://crrev.com/d546979c52e024085ce3e63445c914728d51800c/power_manager/powerd/policy/input_device_controller.h [modify] https://crrev.com/d546979c52e024085ce3e63445c914728d51800c/power_manager/powerd/policy/input_device_controller_unittest.cc
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/75b52a5dfba5ddb2823a2dba69383a69be8f7bf1 commit 75b52a5dfba5ddb2823a2dba69383a69be8f7bf1 Author: Daniel Erat <derat@chromium.org> Date: Fri Mar 31 18:27:02 2017 power: Log when tagged udev devices are added or removed. Make powerd log changes to tagged udev devices. Our tagging is about to get more complicated, and this will hopefully make it easier to figure out what's going on when looking at logs. BUG= chromium:703691 TEST=ran it and saw messages at startup Change-Id: Id512f181531459aa216e4ae0b556165e8272cbe1 Reviewed-on: https://chromium-review.googlesource.com/462378 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Eric Caruso <ejcaruso@chromium.org> (cherry picked from commit dbc1058b3531a466aef3162da894af2e0f37588a) Reviewed-on: https://chromium-review.googlesource.com/465407 Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/75b52a5dfba5ddb2823a2dba69383a69be8f7bf1/power_manager/powerd/system/tagged_device.h [modify] https://crrev.com/75b52a5dfba5ddb2823a2dba69383a69be8f7bf1/power_manager/powerd/system/udev.cc
,
Apr 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/0fd1a99aeea82ec08c5e08f575c69d602bbfa974 commit 0fd1a99aeea82ec08c5e08f575c69d602bbfa974 Author: Daniel Erat <derat@chromium.org> Date: Mon Apr 03 22:34:32 2017 caroline: Don't inhibit keyboard in tablet mode. Power button events are reported via the keyboard driver on caroline. Install udev rules instructing powerd not to inhibit the keyboard so the power button can be used while in tablet mode. BUG= chromium:703691 ,b:36403242 TEST=none Change-Id: Ic80be045d7ac653a070b9947307f1a1c2be24949 Reviewed-on: https://chromium-review.googlesource.com/462380 Tested-by: Dan Erat <derat@chromium.org> Trybot-Ready: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> [rename] https://crrev.com/0fd1a99aeea82ec08c5e08f575c69d602bbfa974/overlay-caroline/chromeos-base/chromeos-bsp-caroline/chromeos-bsp-caroline-0.0.1-r11.ebuild [add] https://crrev.com/0fd1a99aeea82ec08c5e08f575c69d602bbfa974/overlay-caroline/chromeos-base/chromeos-bsp-caroline/files/92-powerd-overrides.rules [modify] https://crrev.com/0fd1a99aeea82ec08c5e08f575c69d602bbfa974/overlay-caroline/chromeos-base/chromeos-bsp-caroline/chromeos-bsp-caroline-0.0.1.ebuild
,
Apr 4 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 4 2017
Let's use partner bugs to track updating udev configs for various boards.
,
Apr 4 2017
,
Apr 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/bae11debc4a181f6a8c6cad1f064458cee4f1ea6 commit bae11debc4a181f6a8c6cad1f064458cee4f1ea6 Author: Daniel Erat <derat@chromium.org> Date: Wed Apr 05 20:10:09 2017 cave: Don't inhibit keyboard in tablet mode. Power button events are reported via the keyboard driver on cave. Install udev rules instructing powerd not to inhibit the keyboard so the power button can be used while in tablet mode. BUG= chromium:703691 ,b:35872420 TEST=none Change-Id: I0d41827af41196d40875d5fab4e3bfd10512a172 Reviewed-on: https://chromium-review.googlesource.com/465589 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> [rename] https://crrev.com/bae11debc4a181f6a8c6cad1f064458cee4f1ea6/overlay-cave/chromeos-base/chromeos-bsp-cave/chromeos-bsp-cave-0.0.1-r17.ebuild [modify] https://crrev.com/bae11debc4a181f6a8c6cad1f064458cee4f1ea6/overlay-cave/chromeos-base/chromeos-bsp-cave/files/92-powerd-overrides.rules
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/156511c9c6893200783d439ff7b1ebf1a27dc9d5 commit 156511c9c6893200783d439ff7b1ebf1a27dc9d5 Author: Daniel Erat <derat@chromium.org> Date: Thu Apr 06 08:42:18 2017 power: Add optional keyboard-side-buttons udev rules file. Add a new udev/optional directory containing a 92-powerd-tags-keyboard-side-buttons.rules file that can be selectively installed by the power_manager ebuild for convertible devices that need to avoid inhibiting the internal keyboard while in tablet mode due to side power/volume button events being routed through it. BUG= chromium:703691 , chromium:708180 TEST=none Change-Id: I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Reviewed-on: https://chromium-review.googlesource.com/469251 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [add] https://crrev.com/156511c9c6893200783d439ff7b1ebf1a27dc9d5/power_manager/udev/optional/92-powerd-tags-keyboard-side-buttons.rules
,
Apr 6 2017
I'm going to use issue 708180 (and various partner bugs) to track setting the keyboard_includes_side_buttons USE flag in board overlays to make use of this.
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5825e3c875ed14ae54c06a777bbb0f065c0edba1 commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1 Author: Daniel Erat <derat@chromium.org> Date: Thu Apr 06 23:11:15 2017 power_manager: Check keyboard_includes_side_buttons USE flag Make the power_manager package include udev rules files preventing the keyboard from being inhibited in tablet mode if the keyboard_includes_side_buttons USE flag is set. This USE flag indicates that events from the power/volume buttons on the side of a convertible device are reported via the internal keyboard. BUG= chromium:703691 , chromium:708180 TEST=built with and without the USE flag CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19 Reviewed-on: https://chromium-review.googlesource.com/469232 Tested-by: Dan Erat <derat@chromium.org> Trybot-Ready: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> [modify] https://crrev.com/5825e3c875ed14ae54c06a777bbb0f065c0edba1/chromeos-base/power_manager/power_manager-9999.ebuild
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5825e3c875ed14ae54c06a777bbb0f065c0edba1 commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1 Author: Daniel Erat <derat@chromium.org> Date: Thu Apr 06 23:11:15 2017 power_manager: Check keyboard_includes_side_buttons USE flag Make the power_manager package include udev rules files preventing the keyboard from being inhibited in tablet mode if the keyboard_includes_side_buttons USE flag is set. This USE flag indicates that events from the power/volume buttons on the side of a convertible device are reported via the internal keyboard. BUG= chromium:703691 , chromium:708180 TEST=built with and without the USE flag CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19 Reviewed-on: https://chromium-review.googlesource.com/469232 Tested-by: Dan Erat <derat@chromium.org> Trybot-Ready: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> [modify] https://crrev.com/5825e3c875ed14ae54c06a777bbb0f065c0edba1/chromeos-base/power_manager/power_manager-9999.ebuild
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5825e3c875ed14ae54c06a777bbb0f065c0edba1 commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1 Author: Daniel Erat <derat@chromium.org> Date: Thu Apr 06 23:11:15 2017 power_manager: Check keyboard_includes_side_buttons USE flag Make the power_manager package include udev rules files preventing the keyboard from being inhibited in tablet mode if the keyboard_includes_side_buttons USE flag is set. This USE flag indicates that events from the power/volume buttons on the side of a convertible device are reported via the internal keyboard. BUG= chromium:703691 , chromium:708180 TEST=built with and without the USE flag CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19 Reviewed-on: https://chromium-review.googlesource.com/469232 Tested-by: Dan Erat <derat@chromium.org> Trybot-Ready: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> [modify] https://crrev.com/5825e3c875ed14ae54c06a777bbb0f065c0edba1/chromeos-base/power_manager/power_manager-9999.ebuild
,
Apr 7 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4 commit 52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4 Author: Daniel Erat <derat@chromium.org> Date: Sun Apr 09 15:38:01 2017 power: Add optional keyboard-side-buttons udev rules file. Add a new udev/optional directory containing a 92-powerd-tags-keyboard-side-buttons.rules file that can be selectively installed by the power_manager ebuild for convertible devices that need to avoid inhibiting the internal keyboard while in tablet mode due to side power/volume button events being routed through it. BUG= chromium:703691 , chromium:708180 TEST=none Change-Id: I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Reviewed-on: https://chromium-review.googlesource.com/469251 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> (cherry picked from commit 156511c9c6893200783d439ff7b1ebf1a27dc9d5) Reviewed-on: https://chromium-review.googlesource.com/472530 [add] https://crrev.com/52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4/power_manager/udev/optional/92-powerd-tags-keyboard-side-buttons.rules
,
Apr 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/bdc171ef15c36a48aae6093e460e0310a60b8405 commit bdc171ef15c36a48aae6093e460e0310a60b8405 Author: Daniel Erat <derat@chromium.org> Date: Sun Apr 09 15:48:32 2017 power_manager: Check keyboard_includes_side_buttons USE flag Make the power_manager package include udev rules files preventing the keyboard from being inhibited in tablet mode if the keyboard_includes_side_buttons USE flag is set. This USE flag indicates that events from the power/volume buttons on the side of a convertible device are reported via the internal keyboard. BUG= chromium:703691 , chromium:708180 TEST=built with and without the USE flag CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19 Reviewed-on: https://chromium-review.googlesource.com/469232 Tested-by: Dan Erat <derat@chromium.org> Trybot-Ready: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> (cherry picked from commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1) Reviewed-on: https://chromium-review.googlesource.com/472531 Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/bdc171ef15c36a48aae6093e460e0310a60b8405/chromeos-base/power_manager/power_manager-9999.ebuild
,
Apr 10 2017
Verified with version 58.0.3029.66/9334.40.0 beta-channel caroline (1. Power button to wake up in tablet mode 2. Volume up and down button in tablet mode)
,
Apr 24 2017
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by adurbin@chromium.org
, Mar 23 2017