New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 836279 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Kevin, Bob, Scarlet/Dru: with Dark Resume, power button press does not fully wake

Project Member Reported by ravisadineni@chromium.org, Apr 24 2018

Issue description

On 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.  
 
Cc: derat@chromium.org brainnorris@google.com snanda@chromium.org
So 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.

Cc: -brainnorris@google.com briannorris@chromium.org
Components: OS>Kernel>Power
Labels: OS-Chrome
I'm flattered
Summary: Kevin, Bob, Scarlet/Dru: with Dark Resume, power button press does not fully wake (was: When Dark Resume is enabled, dru does not cause a full wake on power button press.)
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.
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.)
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. 
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by amstan@chromium.org, 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.
Cc: bleung@chromium.org
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.

Comment 9 by amstan@chromium.org, 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).
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. 

Comment 11 by derat@chromium.org, 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 .
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 16 2018

Labels: merge-merged-chromeos-4.4
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

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 16 2018

Labels: merge-merged-chromeos-4.14
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