Failure to hibernate on Dell Latitude 7430

If, for example, the power button is configured to suspend, holding it and releasing it after the machine has suspended, will wake the machine.

Also on some machines, power button release events are sent during hibernation, even if the button wasn't used to hibernate the machine. This causes hibernation to be aborted.

Signed-off-by David McFarland <[email protected]>

Quoted 1 line

---

 drivers/platform/x86/intel/hid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/platform/x86/intel/hid.c +7 -1

index 7457ca2b27a6..707de9895965 100644
@@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	struct key_entry *ke;
 	int err;
 
 	/*

@@ -545,11 +546,16 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xc0 || !priv->array)
 			return;
 
-		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
+		ke = sparse_keymap_entry_from_scancode(priv->array, event);
+
+		if (!ke) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
 			return;
 		}
 
+		if (ke->type == KE_IGNORE)
+			return;
+
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
 

David, from here is looks like this is stalled for ten days now. Or was there some progress and I just missed it?

From the cover letter[1] is sounds a lot like a "Fixes: 0c4cae1bc00d31 ("PM: hibernate: Avoid missing wakeup events during hibernation")" would be appropriate here.

"Linux regression tracking (Thorsten Leemhuis)"

Quoted 2 lines · <[email protected]> writes:

David, from here is looks like this is stalled for ten days now. Or was there some progress and I just missed it?

No, I've not seen any emails since your last.

Quoted 3 lines

From the cover letter[1] is sounds a lot like a "Fixes: 0c4cae1bc00d31 ("PM: hibernate: Avoid missing wakeup events during hibernation")" would be appropriate here.

The specific behaviour I encountered (failure to hibernate) started with that commit, but I think it just exposed the underlying behaviour (wake on button release), which probably dates to when the driver was introduced.

[CCing Chris, who authored the culprit; also CCing the platform folks and a few lists]

Regarding the patch itself: hopefully this mail will get things moving.

[1]
https://lore.kernel.org/linux-pm/[email protected]/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

Quoted 1 line

Signed-off-by David McFarland <[email protected]>

Quoted 28 lines

Quoted 1 line

---

 drivers/platform/x86/intel/hid.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/platform/x86/intel/hid.c +0 -0

index 7457ca2b27a6..707de9895965 100644
@@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle,
 u32 event, void *context)
  	struct platform_device *device = context;

  struct intel_hid_priv *priv = dev_get_drvdata(&device-

Quoted 1 line

dev);

  unsigned long long ev_index;

drivers/platform/x86/intel/hid.c +1 -0

+	struct key_entry *ke;

  int err;

  /*

drivers/platform/x86/intel/hid.c +0 -0

@@ -545,11 +546,16 @@ static void notify_handler(acpi_handle
 handle, u32 event, void *context)
  		if (event == 0xc0 || !priv->array)
  			return;

drivers/platform/x86/intel/hid.c +0 -1

-		if (!sparse_keymap_entry_from_scancode(priv-

Quoted 1 line

array, event)) {

drivers/platform/x86/intel/hid.c +1 -0

+		ke = sparse_keymap_entry_from_scancode(priv-

Quoted 1 line

array, event);

Quoted 10 lines

drivers/platform/x86/intel/hid.c +2 -0

+
+		if (!ke) {

  dev_info(&device->dev, "unknown event 0x%x\n", event);
  return;
  }

drivers/platform/x86/intel/hid.c +3 -0

+		if (ke->type == KE_IGNORE)
+			return;
+

Quoted 3 lines

 wakeup:
  pm_wakeup_hard_event(&device->dev);

If, for example, the power button is configured to suspend, holding it and releasing it after the machine has suspended, will wake the machine.

Also on some machines, power button release events are sent during hibernation, even if the button wasn't used to hibernate the machine. This causes hibernation to be aborted.

Signed-off-by David McFarland <[email protected]>

---
 drivers/platform/x86/intel/hid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/platform/x86/intel/hid.c +1 -0

index 7457ca2b27a6..707de9895965 100644
@@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	struct key_entry *ke;
 	int err;
 
 	/*

drivers/platform/x86/intel/hid.c +4 -0

+		if (!ke) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
 			return;
 		}
 
+		if (ke->type == KE_IGNORE)
+			return;
+
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
 

If, for example, the power button is configured to suspend, holding it and releasing it after the machine has suspended, will wake the machine.

Also on some machines, power button release events are sent during hibernation, even if the button wasn't used to hibernate the machine. This causes hibernation to be aborted.

Fixes 0c4cae1bc00d ("PM: hibernate: Avoid missing wakeup events during hibernation")

Signed-off-by David McFarland <[email protected]>

Tested-by Enrik Berkhan <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

---
v2: Added tags and fixed whitespace, as requested by Hans.

 drivers/platform/x86/intel/hid.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/platform/x86/intel/hid.c +6 -1

index 7457ca2b27a6..9ffbdc988fe5 100644
@@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	struct key_entry *ke;
 	int err;
 
 	/*

@@ -545,11 +546,15 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xc0 || !priv->array)
 			return;
 
-		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
+		ke = sparse_keymap_entry_from_scancode(priv->array, event);
+		if (!ke) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
 			return;
 		}
 
+		if (ke->type == KE_IGNORE)
+			return;
+
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
 

If, for example, the power button is configured to suspend, holding it and releasing it after the machine has suspended, will wake the machine.

Also on some machines, power button release events are sent during hibernation, even if the button wasn't used to hibernate the machine. This causes hibernation to be aborted.

[...]

I have a Dell Latitude 7430, and recently whenever I hibernate with systemctl hibernate, the machine would immediately wake.

I bisected it to:

0c4cae1bc00d PM: hibernate: Avoid missing wakeup events during hibernation

However, the underlying problem seems to be that during hibernation, my system gets a 0xcf (power button release) event, and the above change causes it to abort hibernation correctly.

I also noticed that holding the power button down (when it's configured to suspend) causes the system to suspend and then wake upon release, if it's held long enough.

I'm attaching a patch which fixes the problem for me, by skipping the wake on any of the release events. These events are all marked KEY_IGNORE, so think this is a reasonable thing to do.

I'm a little worried about the consequences of doing this unconditionally in intel-hid. Perhaps it should be a quirk?

David McFarland (1):
platform/x86/intel/hid: Don't wake on 5-button releases

 drivers/platform/x86/intel/hid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)