[PATCH 1/4] platform/x86: intel-vbtn: Rework wakeup handling in notify_handler()

[PATCH 1/4] platform/x86: intel-vbtn: Rework wakeup handling in notify_handler()

[Hans de Goede]

Rework the wakeup path inside notify_handler() to special case the
buttons (KE_KEY) case instead of the switches case.

In case of a button wake event we want to skip reporting this,
mirroring how the drivers/acpi/button.c code skips the reporting
in the wakeup case (suspended flag set) too.

The reason to skip reporting in this case is that some Linux
desktop-environments will immediately resuspend if we report an
evdev event for the power-button press on wakeup.

Before this commit the skipping of the button-press was done
in a round-about way: In case of a wakeup the regular
sparse_keymap_report_event() would always be skipped by
an early return, and then to avoid not reporting switch changes
on wakeup there was a special KE_SW path with a duplicate
sparse_keymap_report_event() call.

This commit refactors the wakeup handling to explicitly skip the
reporting for button wake events, while using the regular
reporting path for non button (switches) wakeup events.

No intentional functional impact.

Cc: Elia Devito <eliadevito@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 30a9062d2b4b..e1bb37a03ba3 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -131,22 +131,17 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 	if (priv->wakeup_mode) {
 		ke = sparse_keymap_entry_from_scancode(priv->input_dev, event);
-		if (ke) {
-			pm_wakeup_hard_event(&device->dev);
-
-			/*
-			 * Switch events like tablet mode will wake the device
-			 * and report the new switch position to the input
-			 * subsystem.
-			 */
-			if (ke->type == KE_SW)
-				sparse_keymap_report_event(priv->input_dev,
-							   event,
-							   val,
-							   0);
+		if (!ke)
+			goto out_unknown;
+
+		pm_wakeup_hard_event(&device->dev);
+
+		/*
+		 * Skip reporting an evdev event for button wake events,
+		 * mirroring how the drivers/acpi/button.c code skips this too.
+		 */
+		if (ke->type == KE_KEY)
 			return;
-		}
-		goto out_unknown;
 	}
 
 	/*
-- 
2.28.0

[PATCH 2/4] platform/x86: intel-vbtn: Create 2 separate input-devs for buttons and switches

[Hans de Goede]

Create 2 separate input-devs for buttons and switches, this is a
preparation for dynamically registering the switches-input device
for devices which are not on the switches allow-list, but do make
Notify() calls with an event value from the switches sparse-keymap.

This also brings the intel-vbtn driver inline with the intel-hid
driver which is doing the same thing.

Cc: Elia Devito <eliadevito@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note that checkpatch will complain about the 2 assignments of ke
inside if-s. I know those are generally something to be avoided
but in this case using them leads to much cleaner code.
---
 drivers/platform/x86/intel-vbtn.c | 98 +++++++++++++++++++------------
 1 file changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index e1bb37a03ba3..04725173d087 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -44,6 +44,7 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
 	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key press */
 	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key release */
+	{ KE_END }
 };
 
 static const struct key_entry intel_vbtn_switchmap[] = {
@@ -51,14 +52,15 @@ static const struct key_entry intel_vbtn_switchmap[] = {
 	{ KE_SW,     0xCB, { .sw = { SW_DOCK, 0 } } },		/* Undocked */
 	{ KE_SW,     0xCC, { .sw = { SW_TABLET_MODE, 1 } } },	/* Tablet */
 	{ KE_SW,     0xCD, { .sw = { SW_TABLET_MODE, 0 } } },	/* Laptop */
+	{ KE_END }
 };
 
 #define KEYMAP_LEN \
 	(ARRAY_SIZE(intel_vbtn_keymap) + ARRAY_SIZE(intel_vbtn_switchmap) + 1)
 
 struct intel_vbtn_priv {
-	struct key_entry keymap[KEYMAP_LEN];
-	struct input_dev *input_dev;
+	struct input_dev *buttons_dev;
+	struct input_dev *switches_dev;
 	bool has_buttons;
 	bool has_switches;
 	bool wakeup_mode;
@@ -77,48 +79,62 @@ static void detect_tablet_mode(struct platform_device *device)
 		return;
 
 	m = !(vgbs & VGBS_TABLET_MODE_FLAGS);
-	input_report_switch(priv->input_dev, SW_TABLET_MODE, m);
+	input_report_switch(priv->switches_dev, SW_TABLET_MODE, m);
 	m = (vgbs & VGBS_DOCK_MODE_FLAG) ? 1 : 0;
-	input_report_switch(priv->input_dev, SW_DOCK, m);
+	input_report_switch(priv->switches_dev, SW_DOCK, m);
 }
 
+/*
+ * Note this unconditionally creates the 2 input_dev-s and sets up
+ * the sparse-keymaps. Only the registration is conditional on
+ * have_buttons / have_switches. This is done so that the notify
+ * handler can always call sparse_keymap_entry_from_scancode()
+ * on the input_dev-s do determine the event type.
+ */
 static int intel_vbtn_input_setup(struct platform_device *device)
 {
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
-	int ret, keymap_len = 0;
+	int ret;
 
-	if (priv->has_buttons) {
-		memcpy(&priv->keymap[keymap_len], intel_vbtn_keymap,
-		       ARRAY_SIZE(intel_vbtn_keymap) *
-		       sizeof(struct key_entry));
-		keymap_len += ARRAY_SIZE(intel_vbtn_keymap);
-	}
+	priv->buttons_dev = devm_input_allocate_device(&device->dev);
+	if (!priv->buttons_dev)
+		return -ENOMEM;
 
-	if (priv->has_switches) {
-		memcpy(&priv->keymap[keymap_len], intel_vbtn_switchmap,
-		       ARRAY_SIZE(intel_vbtn_switchmap) *
-		       sizeof(struct key_entry));
-		keymap_len += ARRAY_SIZE(intel_vbtn_switchmap);
-	}
+	ret = sparse_keymap_setup(priv->buttons_dev, intel_vbtn_keymap, NULL);
+	if (ret)
+		return ret;
 
-	priv->keymap[keymap_len].type = KE_END;
+	priv->buttons_dev->dev.parent = &device->dev;
+	priv->buttons_dev->name = "Intel Virtual Buttons";
+	priv->buttons_dev->id.bustype = BUS_HOST;
+
+	if (priv->has_buttons) {
+		ret = input_register_device(priv->buttons_dev);
+		if (ret)
+			return ret;
+	}
 
-	priv->input_dev = devm_input_allocate_device(&device->dev);
-	if (!priv->input_dev)
+	priv->switches_dev = devm_input_allocate_device(&device->dev);
+	if (!priv->switches_dev)
 		return -ENOMEM;
 
-	ret = sparse_keymap_setup(priv->input_dev, priv->keymap, NULL);
+	ret = sparse_keymap_setup(priv->switches_dev, intel_vbtn_switchmap, NULL);
 	if (ret)
 		return ret;
 
-	priv->input_dev->dev.parent = &device->dev;
-	priv->input_dev->name = "Intel Virtual Button driver";
-	priv->input_dev->id.bustype = BUS_HOST;
+	priv->switches_dev->dev.parent = &device->dev;
+	priv->switches_dev->name = "Intel Virtual Switches";
+	priv->switches_dev->id.bustype = BUS_HOST;
 
-	if (priv->has_switches)
+	if (priv->has_switches) {
 		detect_tablet_mode(device);
 
-	return input_register_device(priv->input_dev);
+		ret = input_register_device(priv->switches_dev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static void notify_handler(acpi_handle handle, u32 event, void *context)
@@ -127,13 +143,27 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned int val = !(event & 1); /* Even=press, Odd=release */
 	const struct key_entry *ke, *ke_rel;
+	struct input_dev *input_dev;
 	bool autorelease;
 
-	if (priv->wakeup_mode) {
-		ke = sparse_keymap_entry_from_scancode(priv->input_dev, event);
-		if (!ke)
-			goto out_unknown;
+	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
+		if (!priv->has_buttons) {
+			dev_warn(&device->dev, "Warning: received a button event on a device without buttons, please report this.\n");
+			return;
+		}
+		input_dev = priv->buttons_dev;
+	} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
+		if (!priv->has_switches) {
+			dev_warn(&device->dev, "Warning: received a switches event on a device without switchess, please report this.\n");
+			return;
+		}
+		input_dev = priv->switches_dev;
+	} else {
+		dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
+		return;
+	}
 
+	if (priv->wakeup_mode) {
 		pm_wakeup_hard_event(&device->dev);
 
 		/*
@@ -148,14 +178,10 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	 * Even press events are autorelease if there is no corresponding odd
 	 * release event, or if the odd event is KE_IGNORE.
 	 */
-	ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev, event | 1);
+	ke_rel = sparse_keymap_entry_from_scancode(input_dev, event | 1);
 	autorelease = val && (!ke_rel || ke_rel->type == KE_IGNORE);
 
-	if (sparse_keymap_report_event(priv->input_dev, event, val, autorelease))
-		return;
-
-out_unknown:
-	dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
+	sparse_keymap_report_event(input_dev, event, val, autorelease);
 }
 
 static bool intel_vbtn_has_buttons(acpi_handle handle)
-- 
2.28.0

[PATCH 3/4] platform/x86: intel-vbtn: Add alternative method to enable switches

[Hans de Goede]

Some 2-in-1s have a broken VGBS method, so we cannot get an initial
state for the switches from them. Reporting the wrong initial state for
SW_TABLET_MODE causes serious problems (touchpad and/or keyboard events
being ignored by userspace when reporting SW_TABLET_MODE=1), so on these
devices we cannot register an input-dev for the switches at probe time.

We can however register an input-dev for the switches as soon as we
receive the first switches event, because then we will know the state.

Note this mirrors the behavior of recent changs to the intel-hid driver
which also registers a separate switches input-dev on receiving the
first event on machines with a broken VGBS method.

Cc: Elia Devito <eliadevito@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 04725173d087..852cb07c3dfd 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -145,6 +145,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	const struct key_entry *ke, *ke_rel;
 	struct input_dev *input_dev;
 	bool autorelease;
+	int ret;
 
 	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
 		if (!priv->has_buttons) {
@@ -154,8 +155,12 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		input_dev = priv->buttons_dev;
 	} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
 		if (!priv->has_switches) {
-			dev_warn(&device->dev, "Warning: received a switches event on a device without switchess, please report this.\n");
-			return;
+			dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
+			ret = input_register_device(priv->switches_dev);
+			if (ret)
+				return;
+
+			priv->has_switches = true;
 		}
 		input_dev = priv->switches_dev;
 	} else {
-- 
2.28.0

[PATCH 4/4] platform/x86: intel-vbtn: Eval VBDL after registering our notifier

[Hans de Goede]

The VBDL ACPI method enables button/switch reporting through the
intel-vbtn device. In some cases the embedded-controller (EC) might
call Notify() on the intel-vbtn device immediately after the
the VBDL call to make sure that the OS is synced with the EC's
button and switch state.

If we register our notify_handler after evaluating VBDL this means
that we might miss the Notify() calls made by the EC to sync the
state.

E.g. the HP Stream x360 Convertible PC 11 has a VGBS method which
always returns 0, independent of the actual SW_TABLET_MODE state
of the device; and immediately after the VBDL call it calls
Notify(0xCD) or Notify(0xCC) to report the actual state.

Move the evaluation of VBDL to after registering our notify_handler
so that we don't miss any events.

Cc: Elia Devito <eliadevito@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 852cb07c3dfd..8a8017f9ca91 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -189,14 +189,6 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	sparse_keymap_report_event(input_dev, event, val, autorelease);
 }
 
-static bool intel_vbtn_has_buttons(acpi_handle handle)
-{
-	acpi_status status;
-
-	status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
-	return ACPI_SUCCESS(status);
-}
-
 /*
  * There are several laptops (non 2-in-1) models out there which support VGBS,
  * but simply always return 0, which we translate to SW_TABLET_MODE=1. This in
@@ -271,7 +263,7 @@ static int intel_vbtn_probe(struct platform_device *device)
 	acpi_status status;
 	int err;
 
-	has_buttons = intel_vbtn_has_buttons(handle);
+	has_buttons = acpi_has_method(handle, "VBDL");
 	has_switches = intel_vbtn_has_switches(handle);
 
 	if (!has_buttons && !has_switches) {
@@ -300,6 +292,12 @@ static int intel_vbtn_probe(struct platform_device *device)
 	if (ACPI_FAILURE(status))
 		return -EBUSY;
 
+	if (has_buttons) {
+		status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
+		if (ACPI_FAILURE(status))
+			dev_err(&device->dev, "Error VBDL failed with ACPI status %d\n", status);
+	}
+
 	device_init_wakeup(&device->dev, true);
 	/*
 	 * In order for system wakeup to work, the EC GPE has to be marked as
-- 
2.28.0

Re: [PATCH 1/4] platform/x86: intel-vbtn: Rework wakeup handling in notify_handler()

[Hans de Goede]

Hi,

On 1/15/21 5:18 PM, Hans de Goede wrote:
> Rework the wakeup path inside notify_handler() to special case the
> buttons (KE_KEY) case instead of the switches case.
> 
> In case of a button wake event we want to skip reporting this,
> mirroring how the drivers/acpi/button.c code skips the reporting
> in the wakeup case (suspended flag set) too.
> 
> The reason to skip reporting in this case is that some Linux
> desktop-environments will immediately resuspend if we report an
> evdev event for the power-button press on wakeup.
> 
> Before this commit the skipping of the button-press was done
> in a round-about way: In case of a wakeup the regular
> sparse_keymap_report_event() would always be skipped by
> an early return, and then to avoid not reporting switch changes
> on wakeup there was a special KE_SW path with a duplicate
> sparse_keymap_report_event() call.
> 
> This commit refactors the wakeup handling to explicitly skip the
> reporting for button wake events, while using the regular
> reporting path for non button (switches) wakeup events.
> 
> No intentional functional impact.
> 
> Cc: Elia Devito <eliadevito@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've applied this series to my review-hans branch now, so this will
show up in for-next soon.

Regards,

Hans

> ---
>  drivers/platform/x86/intel-vbtn.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> index 30a9062d2b4b..e1bb37a03ba3 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -131,22 +131,17 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  
>  	if (priv->wakeup_mode) {
>  		ke = sparse_keymap_entry_from_scancode(priv->input_dev, event);
> -		if (ke) {
> -			pm_wakeup_hard_event(&device->dev);
> -
> -			/*
> -			 * Switch events like tablet mode will wake the device
> -			 * and report the new switch position to the input
> -			 * subsystem.
> -			 */
> -			if (ke->type == KE_SW)
> -				sparse_keymap_report_event(priv->input_dev,
> -							   event,
> -							   val,
> -							   0);
> +		if (!ke)
> +			goto out_unknown;
> +
> +		pm_wakeup_hard_event(&device->dev);
> +
> +		/*
> +		 * Skip reporting an evdev event for button wake events,
> +		 * mirroring how the drivers/acpi/button.c code skips this too.
> +		 */
> +		if (ke->type == KE_KEY)
>  			return;
> -		}
> -		goto out_unknown;
>  	}
>  
>  	/*
>