Kevin, Bob, Scarlet/Dru: with Dark Resume, power button press does not fully wake |
||||||
Issue descriptionOn DRU, when power button is pressed, the device does not cause a full wake. Powerbutton input events are reported by the cros_ec_keyb.c on DRU. But the driver currently drops events on resume. Also the driver does not increment wakeup count. This is the tracking bug to fix the same so that we can identify if the wakeup source is power button on resume.
,
Apr 28 2018
I'm flattered
,
Apr 28 2018
Rewording the subject a bit to be more concise. Also, this problem likely applies to Kevin and Bob too, no? (Remove that if I'm wrong.) They use the same combined keyboard/buttons driver. So you can't totally fix this just by removing the dummy keyboard, because you definitely can't remove the keyboard on Kevin and Bob.
,
May 2 2018
Is something like this enough to give you separate "wakeup" knobs for each device? diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c index 0d1cfc0e48b9..3db195065393 100644 --- a/drivers/input/keyboard/cros_ec_keyb.c +++ b/drivers/input/keyboard/cros_ec_keyb.c @@ -545,6 +545,9 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb *ckdev) dev_err(dev, "cannot query switches\n"); return ret; } + + device_init_wakeup(&idev->dev, 1); + return 0; } @@ -621,6 +624,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) return err; } + device_init_wakeup(&idev->dev, 1); + return 0; } @@ -668,6 +673,8 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) return err; } + device_init_wakeup(dev, 1); + return 0; } Then powerd will have something to disable on the keyboard side, instead of just walking the tree up to the parent EC device. So instead of this: [0502/102900:INFO:input_device_controller.cc(203)] Disabling wakeup for /sys/devices/platform/ff200000.spi/spi_master/spi5/spi5.0/ff200000.spi:ec@0:keyboard-controller/input/input0 through /sys/devices/platform/ff200000.spi/spi_master/spi5/spi5.0 I see: [0502/103312:INFO:input_device_controller.cc(203)] Disabling wakeup for /sys/devices/platform/ff200000.spi/spi_master/spi5/spi5.0/ff200000.spi:ec@0:keyboard-controller/input/input0 through /sys/devices/platform/ff200000.spi/spi_master/spi5/spi5.0/ff200000.spi:ec@0:keyboard-controller/input/input0 Unfortunately, that means that the 'wakeup' properties probably won't actually do anything to inhibit the actual keyboard/buttons wakeup any more. (They don't quite do that right today either when there's more than one EC wakeup method (e.g., RTC) either.)
,
May 2 2018
Ya I am planning to do something like this. I will send a host command on the suspend path to mask wake interrupts from the devices for which wake is disabled. Gimme a week. I am coming up with a holistic approach for both x86/arm devices. Also It is just not about disabling the wakeups, but also about mapping the wakeup _count to the right device. Right now the way we distribute Irq's to the devices is via blocking notifiers. I need to understand if they are scheduled before powerd unfreezes after resume. The design doc is located at go/crosecwakes.
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/db55f6866bc33f740761f642c2b0138f4dd6fa2d commit db55f6866bc33f740761f642c2b0138f4dd6fa2d Author: Brian Norris <briannorris@chromium.org> Date: Thu May 03 05:21:11 2018 ectool: add 'kbinfo' command We might use this in the kernel, so it's nice to have a diagnostic command for it too. BRANCH=none BUG=chromium:836279 TEST=`ectool kbinfo` on kevin and scarlet Change-Id: I746badf0d2be53d471592a2ca0d7b8ff8070f7a1 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1038729 Reviewed-by: Aseda Aboagye <aaboagye@chromium.org> [modify] https://crrev.com/db55f6866bc33f740761f642c2b0138f4dd6fa2d/util/ectool.c
,
May 16 2018
Re #1: > 6. Powerd tries to inhibit input events and wakeup events from the internal keyboard (powerd/policy/input_device_controller.cc:185). The EC is already supposed to inhibit wakeup events in the convertible case. If you're on something like bob, the AP shouldn't even get woken up from S3 when there's a keyboard press. The EC will know it's not supposed to. There shouldn't be another check on the AP for this since that case shouldn't happen. If you remove that check, it sounds like this bug would be fixed. In the cases where the power button get pressed, the EC will wake up the AP, then send the keyboard event, and the AP will take it seriously.
,
May 17 2018
There is a special effort that went in to add this code in powerd. probably benson knows if this was due to spurious wakes that we observed on particular intel/arm devices. Here is the design doc for the same https://docs.google.com/document/d/1yIzhiiXHUI0dhTUm18A8drNLudiGTip0iqOAd6wjpDA/edit#heading=h.xgjl2srtytjt I can't go ahead and remove the check on all boards. I am afraid that might result in spurious wakes on boards that I am not aware of. Also we cannot claim that it is broken on all arm devices either. Take veyron minnie for example. The only wake capable device that ec enumerates on veyron minnie is the actual keyboard. So removing wake capability from EC here makes sense. One alternative fix if we really don't want to add new code is to have a board specific udev rule on all new boards to ask power_manager to not control wakeup capability. I am not sure how happy we are to do this on every new board.
,
May 22 2018
Re our in person conversation. There should be no need to mask the EC wakeup. At the least it's redundant (EC already masks things like keyboard wakeups when in tablet mode), but it could also do bad things (AP ignoring the EC when going to sleep in tablet mode, but if the user switched to normal mode during sleep, the keyboard should now be able to wake us up). The EC already has "masking" for the tablet mode to enable and disable the keyboard: https://cs.corp.google.com/chromeos_public/src/platform/ec/common/lid_angle.c?q=lid_angle.c&sq=package:(%5Echromeos_public$%7C%5Echromeos_internal$)&dr&l=154 It relies on every board.c declaring a lid_angle_peripheral_enable function to eventually call keyboard_scan_enable. Older version: https://chromium.git.corp.google.com/chromiumos/platform/ec/+/fd6a6900f786d47fc5364f9013356a741da5c113/common/lid_angle.c#155 That older version applies to at least minnie, firmware-glimmer-5216.198.B and firmware-clapper-5216.199.B (though, not the firmware branches for glimmer/clapper without the ".199."). So based on that, we should remove the powerd code that does the same masking, since it's redundant. On the "bad things" argument. I tried to reproduce it on minnie. * switch to tablet mode * powerd_dbus_suspend * pressing buttons doesn't wake us up * switch to normal mode, still suspended * press keys * expect it to still not wake us up (to demonstrate the "bad things"), but for some reason it does wake us up We should figure out why minnie works, because according to powerd it shouldn't. There might be something else at play there (yet another thing that makes the powerd code not used).
,
May 22 2018
Ok this is what happens on minnie. minnie does not have a tablet mode switch input device. So powerd assumes the device is always in the 'laptop mode' and does not disable wakes from the keyboard.
,
May 22 2018
I don't think you need to change any code in powerd. It's probably just a matter of updating the udev rules for these devices to tell powerd to not manage wakeup and/or inhibit or to keep them usable while in tablet mode, right? See http://go/powerd/docs/udev.md .
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/d71e086d993f7564a12418bdc430f94640fdba28 commit d71e086d993f7564a12418bdc430f94640fdba28 Author: Ravi Chandra Sadineni <ravisadineni@chromium.org> Date: Wed May 23 01:45:45 2018 power: Do not change wakeup capability for internal kb. This code was primarily added to supress spurious wakes from touchpad and touchscreen( crbug.com/221348 , crbug.com/391046 ). Internal keyboard never seemed to be a problamatic device. Powerd trying to control wake cpabilities on devices that EC enumerates creates complex bugs. So remove the check. Please look at the bug for more info. BUG=chromium:836279 TEST=Tested that EC is still a wake enabled device before sleep. Change-Id: I510c31bf0acaa121b434e8eb25e7f353be9ab23c Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1069220 Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/d71e086d993f7564a12418bdc430f94640fdba28/power_manager/udev/92-powerd-tags.rules
,
Jun 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/374f11942be510bdfd73169ce76105bd849a8372 commit 374f11942be510bdfd73169ce76105bd849a8372 Author: Ravi Chandra Sadineni <ravisadineni@chromium.org> Date: Sat Jun 16 20:53:57 2018 BACKPORT: Input: cros_ec_keyb - mark cros_ec_keyb driver as wake enabled device. Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event related to keyboard, call pm_wakeup_event() to make sure wakeup triggers are accounted to keyb during suspend resume path. Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> (cherry picked from commit 38ba34a43dbc00c87d13d1c4b6d0719a2ac87b2e) Conflicts: drivers/mfd/cros_ec 29d99b966d60("cros_ec: Don't signal wake event for non-wake host events") missing. Change cros_ec_get_next_event to take only one argument. BUG=chromium:836279 TEST=Wakeup count for the keyboard input device increments on wakeup if the device woke up due to a button press. Does not increase otherwise. Change-Id: If8a6d3769331cfba46c8fc06b77f9b323ecd9f26 Reviewed-on: https://chromium-review.googlesource.com/1101698 Commit-Ready: Ravi Chandra Sadineni <ravisadineni@chromium.org> Tested-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/374f11942be510bdfd73169ce76105bd849a8372/drivers/mfd/cros_ec.c [modify] https://crrev.com/374f11942be510bdfd73169ce76105bd849a8372/drivers/input/keyboard/cros_ec_keyb.c
,
Jun 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/fca0ae5a7f2e4ec9fe5d747fbc6131c646cfe709 commit fca0ae5a7f2e4ec9fe5d747fbc6131c646cfe709 Author: Ravi Chandra Sadineni <ravisadineni@chromium.org> Date: Sat Jun 16 20:53:59 2018 UPSTREAM: Input: cros_ec_keyb - mark cros_ec_keyb driver as wake enabled device. Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event related to keyboard, call pm_wakeup_event() to make sure wakeup triggers are accounted to keyb during suspend resume path. Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> (cherry picked from commit 38ba34a43dbc00c87d13d1c4b6d0719a2ac87b2e) BUG=chromium:836279 TEST=Wakeup count for the keyboard input device increments on wakeup if the device woke up due to a button press. Does not increase otherwise. Change-Id: I43f169f2c4b18b136f48b7a940a613d91cd2688a Reviewed-on: https://chromium-review.googlesource.com/1101959 Commit-Ready: Ravi Chandra Sadineni <ravisadineni@chromium.org> Tested-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/fca0ae5a7f2e4ec9fe5d747fbc6131c646cfe709/drivers/mfd/cros_ec.c [modify] https://crrev.com/fca0ae5a7f2e4ec9fe5d747fbc6131c646cfe709/drivers/input/keyboard/cros_ec_keyb.c |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by ravisadineni@chromium.org
, Apr 26 2018So this is what is happening. 1. Dru DTS says that dru has an internal keyboard. (v4.4/arch/arm/boot/dts/cros-ec-keyboard.dtsi) 2. cros_ec_spi.c marks EC device as wake capable and wake enabled by default during probe (v4.4/drivers/mfd/cros_ec_spi.c: 680). 3. cros_ec-keyb.c enumerates 2 devices 1. internal keyboard based on the key matrix in the DTS (v4.4/drivers/input/keyboard/cros_ec_keyb.c:650) 2. cros_ec_buttons by querying the EC whether it has any special buttons (v4.4/drivers/input/keyboard/cros_ec_keyb.c:657) 4. system boots to userland. Powerd sees 2 input devices (keyboard and cros_ec_buttons). 5. Powerd asks the device mode. Device is in tablet mode by default. 6. Powerd tries to inhibit input events and wakeup events from the internal keyboard (powerd/policy/input_device_controller.cc:185). 7. internal keyboard by itself is not wake capable. So powerd traverses up the path to find the first wake capable parent to inhibit wakeup events. So ends up inhibiting events on cros-ec-spi.c (powerd/policy/input_device_controller.cc: 233) Side effects: 1. Since EC (cros_ec_spi device) is not wake capable anymore, Cros ec driver drops events that are queued up during resume. (drivers/mfd/cros_ec.c :388). Thus userland on resume does not see Input events from power button incase of a wakeup from a power button press. 3. Also since the device is not a wake capable device the wakeup count is not incremented by EC anymore. ((drivers/mfd/cros_ec.c :198) Interesting Observations : Disabling wakeup capability on EC should prevent EC irq from waking up the device. This should prevent power button from waking up the device. The only reason power button can still wakes up the device is, the irq is shared between ec and RTC and RTC is still wake enabled. This results in enable_irq_wake being called for the IRQ. Fix: 1. Remove keyboard matrix from the DTS of DRU.