All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
@ 2020-09-08 17:19 ` Maximilian Luz
  0 siblings, 0 replies; 7+ messages in thread
From: Maximilian Luz @ 2020-09-08 17:19 UTC (permalink / raw)
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown,
	platform-driver-x86, linux-acpi, linux-kernel, Maximilian Luz

Conventionally, wake-up events for a specific device, in our case the
lid device, are managed via the ACPI _PRW field. While this does not
seem strictly necessary based on ACPI spec, the kernel disables GPE
wakeups to avoid non-wakeup interrupts preventing suspend by default and
only enables GPEs associated via the _PRW field with a wake-up capable
device. This behavior has been introduced in commit f941d3e41da7 ("ACPI:
EC / PM: Disable non-wakeup GPEs for suspend-to-idle") and is described
in more detail in its commit message.

Unfortunately, on MS Surface devices, there is no _PRW field present on
the lid device, thus no GPE is associated with it, and therefore the GPE
responsible for sending the status-change notification to the lid gets
disabled during suspend, making it impossible to wake the device via the
lid.

This patch introduces a pseudo-device and respective driver which, based
on some DMI matching, marks the corresponding GPE of the lid device for
wake and enables it during suspend. The behavior of this driver models
the behavior of the ACPI/PM core for normal wakeup GPEs, properly
declared via the _PRW field.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

This driver has been tested (and is currently in use) on various Surface
models via the linux-surface project [1].

[1]: https://github.com/linux-surface/linux-surface

---
 MAINTAINERS                        |   6 +
 drivers/platform/x86/Kconfig       |  10 +
 drivers/platform/x86/Makefile      |   1 +
 drivers/platform/x86/surface_gpe.c | 298 +++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/platform/x86/surface_gpe.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b5cfab015bd61..a9f8400096e16 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11549,6 +11549,12 @@ F:	drivers/scsi/smartpqi/smartpqi*.[ch]
 F:	include/linux/cciss*.h
 F:	include/uapi/linux/cciss*.h
 
+MICROSOFT SURFACE GPE LID SUPPORT DRIVER
+M:	Maximilian Luz <luzmaximilian@gmail.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/surface_gpe.c
+
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
 M:	Chen Yu <yu.c.chen@intel.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba68011..cd29ab65f8b15 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -894,6 +894,16 @@ config SURFACE_3_POWER_OPREGION
 	  This driver provides support for ACPI operation
 	  region of the Surface 3 battery platform driver.
 
+config SURFACE_GPE
+	tristate "Surface GPE/Lid Support Driver"
+	depends on ACPI
+	depends on DMI
+	help
+	  This driver marks the GPEs related to the ACPI lid device found on
+	  Microsoft Surface devices as wakeup sources and prepares them
+	  accordingly. It is required on those devices to allow wake-ups from
+	  suspend by opening the lid.
+
 config SURFACE_PRO3_BUTTON
 	tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
 	depends on ACPI && INPUT
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff452..58c2a6f52e394 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
 obj-$(CONFIG_SURFACE3_WMI)		+= surface3-wmi.o
 obj-$(CONFIG_SURFACE_3_BUTTON)		+= surface3_button.o
 obj-$(CONFIG_SURFACE_3_POWER_OPREGION)	+= surface3_power.o
+obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
 obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
 
 # MSI
diff --git a/drivers/platform/x86/surface_gpe.c b/drivers/platform/x86/surface_gpe.c
new file mode 100644
index 0000000000000..b08fa66b948cb
--- /dev/null
+++ b/drivers/platform/x86/surface_gpe.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Surface GPE/Lid driver to enable wakeup from suspend via the lid by
+ * properly configuring the respective GPEs.
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+
+struct surface_lid_device {
+	u32 gpe_number;
+};
+
+static const struct surface_lid_device lid_device_l17 = {
+	.gpe_number = 0x17,
+};
+
+static const struct surface_lid_device lid_device_l4D = {
+	.gpe_number = 0x4D,
+};
+
+static const struct surface_lid_device lid_device_l4F = {
+	.gpe_number = 0x4F,
+};
+
+static const struct surface_lid_device lid_device_l57 = {
+	.gpe_number = 0x57,
+};
+
+// Note: When changing this don't forget to change the MODULE_ALIAS below.
+static const struct dmi_system_id dmi_lid_device_table[] = {
+	{
+		.ident = "Surface Pro 4",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Pro 5",
+		.matches = {
+			/*
+			 * We match for SKU here due to generic product name
+			 * "Surface Pro".
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 5 (LTE)",
+		.matches = {
+			/*
+			 * We match for SKU here due to generic product name
+			 * "Surface Pro"
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 6",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 7",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 7"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{
+		.ident = "Surface Book 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Book 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Book 3",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 3"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{
+		.ident = "Surface Laptop 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
+		},
+		.driver_data = (void *)&lid_device_l57,
+	},
+	{
+		.ident = "Surface Laptop 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
+		},
+		.driver_data = (void *)&lid_device_l57,
+	},
+	{
+		.ident = "Surface Laptop 3 (Intel 13\")",
+		.matches = {
+			/*
+			 * We match for SKU here due to different vairants: The
+			 * AMD (15") version does not rely on GPEs.
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Laptop_3_1867:1868"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{ }
+};
+
+
+static int surface_lid_enable_wakeup(struct device *dev,
+				     const struct surface_lid_device *lid,
+				     bool enable)
+{
+	int action = enable ? ACPI_GPE_ENABLE : ACPI_GPE_DISABLE;
+	acpi_status status;
+
+	status = acpi_set_gpe_wake_mask(NULL, lid->gpe_number, action);
+	if (status) {
+		dev_err(dev, "failed to set GPE wake mask: %d\n", status);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+static int surface_gpe_suspend(struct device *dev)
+{
+	const struct surface_lid_device *lid;
+
+	lid = dev_get_platdata(dev);
+	return surface_lid_enable_wakeup(dev, lid, true);
+}
+
+static int surface_gpe_resume(struct device *dev)
+{
+	const struct surface_lid_device *lid;
+
+	lid = dev_get_platdata(dev);
+	return surface_lid_enable_wakeup(dev, lid, false);
+}
+
+static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume);
+
+
+static int surface_gpe_probe(struct platform_device *pdev)
+{
+	const struct surface_lid_device *lid;
+	int status;
+
+	lid = dev_get_platdata(&pdev->dev);
+	if (!lid)
+		return -ENODEV;
+
+	status = acpi_mark_gpe_for_wake(NULL, lid->gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
+		return -EINVAL;
+	}
+
+	status = acpi_enable_gpe(NULL, lid->gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
+		return -EINVAL;
+	}
+
+	status = surface_lid_enable_wakeup(&pdev->dev, lid, false);
+	if (status) {
+		acpi_disable_gpe(NULL, lid->gpe_number);
+		return status;
+	}
+
+	return 0;
+}
+
+static int surface_gpe_remove(struct platform_device *pdev)
+{
+	struct surface_lid_device *lid = dev_get_platdata(&pdev->dev);
+
+	/* restore default behavior without this module */
+	surface_lid_enable_wakeup(&pdev->dev, lid, false);
+	acpi_disable_gpe(NULL, lid->gpe_number);
+
+	return 0;
+}
+
+static struct platform_driver surface_gpe_driver = {
+	.probe = surface_gpe_probe,
+	.remove = surface_gpe_remove,
+	.driver = {
+		.name = "surface_gpe",
+		.pm = &surface_gpe_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+
+
+static struct platform_device *surface_gpe_device;
+
+static int __init surface_gpe_init(void)
+{
+	const struct dmi_system_id *match;
+	const struct surface_lid_device *lid;
+	struct platform_device *pdev;
+	int status;
+
+	match = dmi_first_match(dmi_lid_device_table);
+	if (!match) {
+		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
+		return 0;
+	}
+
+	lid = match->driver_data;
+
+	status = platform_driver_register(&surface_gpe_driver);
+	if (status)
+		return status;
+
+	pdev = platform_device_alloc("surface_gpe", PLATFORM_DEVID_NONE);
+	if (!pdev) {
+		platform_driver_unregister(&surface_gpe_driver);
+		return -ENOMEM;
+	}
+
+	status = platform_device_add_data(pdev, lid, sizeof(*lid));
+	if (status) {
+		platform_device_put(pdev);
+		platform_driver_unregister(&surface_gpe_driver);
+		return status;
+	}
+
+	status = platform_device_add(pdev);
+	if (status) {
+		platform_device_put(pdev);
+		platform_driver_unregister(&surface_gpe_driver);
+		return status;
+	}
+
+	surface_gpe_device = pdev;
+	return 0;
+}
+
+static void __exit surface_gpe_exit(void)
+{
+	if (!surface_gpe_device)
+		return;
+
+	platform_device_unregister(surface_gpe_device);
+	platform_driver_unregister(&surface_gpe_driver);
+}
+
+module_init(surface_gpe_init);
+module_exit(surface_gpe_exit);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
+MODULE_DESCRIPTION("Surface GPE/Lid Driver");
+MODULE_LICENSE("GPL");
+
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro4:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro6:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro7:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook2:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook3:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop2:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop3:*");
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
@ 2020-09-08 17:19 ` Maximilian Luz
  0 siblings, 0 replies; 7+ messages in thread
From: Maximilian Luz @ 2020-09-08 17:19 UTC (permalink / raw)
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown,
	platform-driver-x86, linux-acpi, linux-kernel, Maximilian Luz

Conventionally, wake-up events for a specific device, in our case the
lid device, are managed via the ACPI _PRW field. While this does not
seem strictly necessary based on ACPI spec, the kernel disables GPE
wakeups to avoid non-wakeup interrupts preventing suspend by default and
only enables GPEs associated via the _PRW field with a wake-up capable
device. This behavior has been introduced in commit f941d3e41da7 ("ACPI:
EC / PM: Disable non-wakeup GPEs for suspend-to-idle") and is described
in more detail in its commit message.

Unfortunately, on MS Surface devices, there is no _PRW field present on
the lid device, thus no GPE is associated with it, and therefore the GPE
responsible for sending the status-change notification to the lid gets
disabled during suspend, making it impossible to wake the device via the
lid.

This patch introduces a pseudo-device and respective driver which, based
on some DMI matching, marks the corresponding GPE of the lid device for
wake and enables it during suspend. The behavior of this driver models
the behavior of the ACPI/PM core for normal wakeup GPEs, properly
declared via the _PRW field.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

This driver has been tested (and is currently in use) on various Surface
models via the linux-surface project [1].

[1]: https://github.com/linux-surface/linux-surface

---
 MAINTAINERS                        |   6 +
 drivers/platform/x86/Kconfig       |  10 +
 drivers/platform/x86/Makefile      |   1 +
 drivers/platform/x86/surface_gpe.c | 298 +++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/platform/x86/surface_gpe.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b5cfab015bd61..a9f8400096e16 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11549,6 +11549,12 @@ F:	drivers/scsi/smartpqi/smartpqi*.[ch]
 F:	include/linux/cciss*.h
 F:	include/uapi/linux/cciss*.h
 
+MICROSOFT SURFACE GPE LID SUPPORT DRIVER
+M:	Maximilian Luz <luzmaximilian@gmail.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/surface_gpe.c
+
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
 M:	Chen Yu <yu.c.chen@intel.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba68011..cd29ab65f8b15 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -894,6 +894,16 @@ config SURFACE_3_POWER_OPREGION
 	  This driver provides support for ACPI operation
 	  region of the Surface 3 battery platform driver.
 
+config SURFACE_GPE
+	tristate "Surface GPE/Lid Support Driver"
+	depends on ACPI
+	depends on DMI
+	help
+	  This driver marks the GPEs related to the ACPI lid device found on
+	  Microsoft Surface devices as wakeup sources and prepares them
+	  accordingly. It is required on those devices to allow wake-ups from
+	  suspend by opening the lid.
+
 config SURFACE_PRO3_BUTTON
 	tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet"
 	depends on ACPI && INPUT
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff452..58c2a6f52e394 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
 obj-$(CONFIG_SURFACE3_WMI)		+= surface3-wmi.o
 obj-$(CONFIG_SURFACE_3_BUTTON)		+= surface3_button.o
 obj-$(CONFIG_SURFACE_3_POWER_OPREGION)	+= surface3_power.o
+obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
 obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
 
 # MSI
diff --git a/drivers/platform/x86/surface_gpe.c b/drivers/platform/x86/surface_gpe.c
new file mode 100644
index 0000000000000..b08fa66b948cb
--- /dev/null
+++ b/drivers/platform/x86/surface_gpe.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Surface GPE/Lid driver to enable wakeup from suspend via the lid by
+ * properly configuring the respective GPEs.
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+
+struct surface_lid_device {
+	u32 gpe_number;
+};
+
+static const struct surface_lid_device lid_device_l17 = {
+	.gpe_number = 0x17,
+};
+
+static const struct surface_lid_device lid_device_l4D = {
+	.gpe_number = 0x4D,
+};
+
+static const struct surface_lid_device lid_device_l4F = {
+	.gpe_number = 0x4F,
+};
+
+static const struct surface_lid_device lid_device_l57 = {
+	.gpe_number = 0x57,
+};
+
+// Note: When changing this don't forget to change the MODULE_ALIAS below.
+static const struct dmi_system_id dmi_lid_device_table[] = {
+	{
+		.ident = "Surface Pro 4",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Pro 5",
+		.matches = {
+			/*
+			 * We match for SKU here due to generic product name
+			 * "Surface Pro".
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 5 (LTE)",
+		.matches = {
+			/*
+			 * We match for SKU here due to generic product name
+			 * "Surface Pro"
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 6",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
+		},
+		.driver_data = (void *)&lid_device_l4F,
+	},
+	{
+		.ident = "Surface Pro 7",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 7"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{
+		.ident = "Surface Book 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Book 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
+		},
+		.driver_data = (void *)&lid_device_l17,
+	},
+	{
+		.ident = "Surface Book 3",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 3"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{
+		.ident = "Surface Laptop 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
+		},
+		.driver_data = (void *)&lid_device_l57,
+	},
+	{
+		.ident = "Surface Laptop 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
+		},
+		.driver_data = (void *)&lid_device_l57,
+	},
+	{
+		.ident = "Surface Laptop 3 (Intel 13\")",
+		.matches = {
+			/*
+			 * We match for SKU here due to different vairants: The
+			 * AMD (15") version does not rely on GPEs.
+			 */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Laptop_3_1867:1868"),
+		},
+		.driver_data = (void *)&lid_device_l4D,
+	},
+	{ }
+};
+
+
+static int surface_lid_enable_wakeup(struct device *dev,
+				     const struct surface_lid_device *lid,
+				     bool enable)
+{
+	int action = enable ? ACPI_GPE_ENABLE : ACPI_GPE_DISABLE;
+	acpi_status status;
+
+	status = acpi_set_gpe_wake_mask(NULL, lid->gpe_number, action);
+	if (status) {
+		dev_err(dev, "failed to set GPE wake mask: %d\n", status);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+static int surface_gpe_suspend(struct device *dev)
+{
+	const struct surface_lid_device *lid;
+
+	lid = dev_get_platdata(dev);
+	return surface_lid_enable_wakeup(dev, lid, true);
+}
+
+static int surface_gpe_resume(struct device *dev)
+{
+	const struct surface_lid_device *lid;
+
+	lid = dev_get_platdata(dev);
+	return surface_lid_enable_wakeup(dev, lid, false);
+}
+
+static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume);
+
+
+static int surface_gpe_probe(struct platform_device *pdev)
+{
+	const struct surface_lid_device *lid;
+	int status;
+
+	lid = dev_get_platdata(&pdev->dev);
+	if (!lid)
+		return -ENODEV;
+
+	status = acpi_mark_gpe_for_wake(NULL, lid->gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
+		return -EINVAL;
+	}
+
+	status = acpi_enable_gpe(NULL, lid->gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
+		return -EINVAL;
+	}
+
+	status = surface_lid_enable_wakeup(&pdev->dev, lid, false);
+	if (status) {
+		acpi_disable_gpe(NULL, lid->gpe_number);
+		return status;
+	}
+
+	return 0;
+}
+
+static int surface_gpe_remove(struct platform_device *pdev)
+{
+	struct surface_lid_device *lid = dev_get_platdata(&pdev->dev);
+
+	/* restore default behavior without this module */
+	surface_lid_enable_wakeup(&pdev->dev, lid, false);
+	acpi_disable_gpe(NULL, lid->gpe_number);
+
+	return 0;
+}
+
+static struct platform_driver surface_gpe_driver = {
+	.probe = surface_gpe_probe,
+	.remove = surface_gpe_remove,
+	.driver = {
+		.name = "surface_gpe",
+		.pm = &surface_gpe_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+
+
+static struct platform_device *surface_gpe_device;
+
+static int __init surface_gpe_init(void)
+{
+	const struct dmi_system_id *match;
+	const struct surface_lid_device *lid;
+	struct platform_device *pdev;
+	int status;
+
+	match = dmi_first_match(dmi_lid_device_table);
+	if (!match) {
+		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
+		return 0;
+	}
+
+	lid = match->driver_data;
+
+	status = platform_driver_register(&surface_gpe_driver);
+	if (status)
+		return status;
+
+	pdev = platform_device_alloc("surface_gpe", PLATFORM_DEVID_NONE);
+	if (!pdev) {
+		platform_driver_unregister(&surface_gpe_driver);
+		return -ENOMEM;
+	}
+
+	status = platform_device_add_data(pdev, lid, sizeof(*lid));
+	if (status) {
+		platform_device_put(pdev);
+		platform_driver_unregister(&surface_gpe_driver);
+		return status;
+	}
+
+	status = platform_device_add(pdev);
+	if (status) {
+		platform_device_put(pdev);
+		platform_driver_unregister(&surface_gpe_driver);
+		return status;
+	}
+
+	surface_gpe_device = pdev;
+	return 0;
+}
+
+static void __exit surface_gpe_exit(void)
+{
+	if (!surface_gpe_device)
+		return;
+
+	platform_device_unregister(surface_gpe_device);
+	platform_driver_unregister(&surface_gpe_driver);
+}
+
+module_init(surface_gpe_init);
+module_exit(surface_gpe_exit);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
+MODULE_DESCRIPTION("Surface GPE/Lid Driver");
+MODULE_LICENSE("GPL");
+
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro4:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro6:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro7:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook2:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceBook3:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop2:*");
+MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfaceLaptop3:*");
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
  2020-09-08 17:19 ` Maximilian Luz
  (?)
@ 2020-09-08 18:40 ` Andy Shevchenko
  2020-09-08 20:19   ` Maximilian Luz
  -1 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2020-09-08 18:40 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown, Platform Driver,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Tue, Sep 8, 2020 at 8:20 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Conventionally, wake-up events for a specific device, in our case the
> lid device, are managed via the ACPI _PRW field. While this does not
> seem strictly necessary based on ACPI spec, the kernel disables GPE
> wakeups to avoid non-wakeup interrupts preventing suspend by default and
> only enables GPEs associated via the _PRW field with a wake-up capable
> device. This behavior has been introduced in commit f941d3e41da7 ("ACPI:
> EC / PM: Disable non-wakeup GPEs for suspend-to-idle") and is described
> in more detail in its commit message.
>
> Unfortunately, on MS Surface devices, there is no _PRW field present on
> the lid device, thus no GPE is associated with it, and therefore the GPE
> responsible for sending the status-change notification to the lid gets
> disabled during suspend, making it impossible to wake the device via the
> lid.
>
> This patch introduces a pseudo-device and respective driver which, based
> on some DMI matching, marks the corresponding GPE of the lid device for
> wake and enables it during suspend. The behavior of this driver models
> the behavior of the ACPI/PM core for normal wakeup GPEs, properly
> declared via the _PRW field.

...

> +#include <linux/platform_device.h>
> +
> +

One blank line is enough.

...

> +       .gpe_number = 0x17,
> +       .gpe_number = 0x4D,
> +       .gpe_number = 0x4F,
> +       .gpe_number = 0x57,

From where these numbers come from? Can we get them from firmware (ACPI)?

...

> +       { }
> +};
> +
> +

One is enough. Same for other places.

...

> +static int surface_gpe_suspend(struct device *dev)
> +{
> +       const struct surface_lid_device *lid;
> +
> +       lid = dev_get_platdata(dev);

There is enough room to put this assignment directly into definition.

> +       return surface_lid_enable_wakeup(dev, lid, true);
> +}
> +
> +static int surface_gpe_resume(struct device *dev)
> +{
> +       const struct surface_lid_device *lid;
> +
> +       lid = dev_get_platdata(dev);

Ditto.

> +       return surface_lid_enable_wakeup(dev, lid, false);
> +}

...

> +static int surface_gpe_probe(struct platform_device *pdev)
> +{
> +       const struct surface_lid_device *lid;
> +       int status;
> +

> +       lid = dev_get_platdata(&pdev->dev);
> +       if (!lid)
> +               return -ENODEV;

Can we use software nodes?

> +       status = acpi_mark_gpe_for_wake(NULL, lid->gpe_number);
> +       if (status) {
> +               dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
> +               return -EINVAL;
> +       }
> +

> +       status = acpi_enable_gpe(NULL, lid->gpe_number);

Did I miss anything or all calls of enable / disable GPE are using
NULL as a first parameter? What the point in such case?

> +       if (status) {
> +               dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
> +               return -EINVAL;
> +       }
> +
> +       status = surface_lid_enable_wakeup(&pdev->dev, lid, false);
> +       if (status) {
> +               acpi_disable_gpe(NULL, lid->gpe_number);
> +               return status;
> +       }
> +
> +       return 0;
> +}

...

> +static void __exit surface_gpe_exit(void)
> +{

> +       if (!surface_gpe_device)
> +               return;

This is redundant check.

> +       platform_device_unregister(surface_gpe_device);
> +       platform_driver_unregister(&surface_gpe_driver);
> +}
> +

> +module_init(surface_gpe_init);
> +module_exit(surface_gpe_exit);

Attach each to the corresponding method w/o blank line in between.

...

> +MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro:*");
> +MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro4:*");

Can simply

MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurface*:*");

work?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
  2020-09-08 18:40 ` Andy Shevchenko
@ 2020-09-08 20:19   ` Maximilian Luz
  2020-09-10 21:20     ` Maximilian Luz
  0 siblings, 1 reply; 7+ messages in thread
From: Maximilian Luz @ 2020-09-08 20:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown, Platform Driver,
	ACPI Devel Maling List, Linux Kernel Mailing List

On 9/8/20 8:40 PM, Andy Shevchenko wrote:
> On Tue, Sep 8, 2020 at 8:20 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:

...

>> +       .gpe_number = 0x17,
>> +       .gpe_number = 0x4D,
>> +       .gpe_number = 0x4F,
>> +       .gpe_number = 0x57,
> 
>  From where these numbers come from? Can we get them from firmware (ACPI)?

Yes, they are obtained from ACPI/the DSDT. Specifically from the name of
the GPE handler notifying the lid device. See [1] for a repo full of
Surface ACPI dumps (source for this). I'll add a comment pointing this out
in v2.

[1]: https://github.com/linux-surface/acpidumps

...

>> +static int surface_gpe_probe(struct platform_device *pdev)
>> +{
>> +       const struct surface_lid_device *lid;
>> +       int status;
>> +
> 
>> +       lid = dev_get_platdata(&pdev->dev);
>> +       if (!lid)
>> +               return -ENODEV;
> 
> Can we use software nodes?

As far as I can tell this would work via fwnode_create_software_node /
fwnode_remove_software_node and device properties? I don't seem to find
much documentation on this (there doesn't seem to be an entry for
software nodes in the official docs?), but I think I should be able to
make this work.

>> +       status = acpi_mark_gpe_for_wake(NULL, lid->gpe_number);
>> +       if (status) {
>> +               dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
>> +               return -EINVAL;
>> +       }
>> +
> 
>> +       status = acpi_enable_gpe(NULL, lid->gpe_number);
> 
> Did I miss anything or all calls of enable / disable GPE are using
> NULL as a first parameter? What the point in such case?

As far as I can tell, some of the more generic uses have a non-NULL
gpe_device parameter (acpi/device_pm.c, acpi/wakeup.c) and NULL just
means index-0/main device? Not an expert on that though, so probably
just ignore me here and let the ACPI guys answer this.

...

>> +MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro:*");
>> +MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurfacePro4:*");
> 
> Can simply
> 
> MODULE_ALIAS("dmi:*:svnMicrosoftCorporation:pnSurface*:*");
> 
> work?

Depends on your preference, really. That would also auto-load the module
on Surface Pro 3 and earlier devices (just won't do anything on those).
So it's a trade-off between unnecessary loading of the module and
maintainability/readability. Let me know what you prefer and I'll switch
to that.

Style and other issues are noted, I'll fix them for v2.

Regards,
Max

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
  2020-09-08 20:19   ` Maximilian Luz
@ 2020-09-10 21:20     ` Maximilian Luz
  0 siblings, 0 replies; 7+ messages in thread
From: Maximilian Luz @ 2020-09-10 21:20 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown, Platform Driver,
	ACPI Devel Maling List, Linux Kernel Mailing List


I've just sent v2.

Thank you for your comments,
Max

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
       [not found] ` <20200911221053.GF103884@mtg-dev.jf.intel.com>
@ 2020-09-11 22:46   ` Maximilian Luz
  2020-09-15 21:19     ` mark gross
  0 siblings, 1 reply; 7+ messages in thread
From: Maximilian Luz @ 2020-09-11 22:46 UTC (permalink / raw)
  To: mgross
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown,
	platform-driver-x86, linux-acpi, linux-kernel

On 9/12/20 12:10 AM, mark gross wrote:
> Surface devices are tablets with detachable keyboards.  they don't really
> have a "lid" as the tablet is the "lid".

The Surface Laptop series doesn't have a detachable keyboard, yet still
requires this. Arguably, the Surface Books are also more laptop than
tablet (at least that's the way I use mine...). Finally, on the actual
tablets (Surface Pro series) the lid switch detects when the keyboard
cover is opened (or at least that's what I have been told, I don't
own/have access to a Pro series device).

Regardless of that, this patch is intended to provide the same behavior
as found on Windows, for all devices included in this patch, which is:
When you open the lid, or in case of the Pro series fold away the
keyboard cover, the device wakes from suspend/s2idle. Without this
patch, that doesn't work.

> I'm just questioning if the creator of the device designed it the way they did
> maybe we should think twice about doing this.

As far as I can tell, the intended behavior is to wake the device when
the lid is opened, which on the Laptops and Books is a more conventional
lid and on the Pros constitutes opening the cover.

I'm open for any alternative though.

Also please note that I've already sent a v2 of this patch with Andy's
comments addressed: https://lore.kernel.org/patchwork/patch/1303997/

--
Regards,
Max

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device
  2020-09-11 22:46   ` Maximilian Luz
@ 2020-09-15 21:19     ` mark gross
  0 siblings, 0 replies; 7+ messages in thread
From: mark gross @ 2020-09-15 21:19 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: Darren Hart, Andy Shevchenko, Hans de Goede, Mika Westerberg,
	Gayatri Kammela, Rafael J. Wysocki, Len Brown,
	platform-driver-x86, linux-acpi, linux-kernel

On Sat, Sep 12, 2020 at 12:46:30AM +0200, Maximilian Luz wrote:
> On 9/12/20 12:10 AM, mark gross wrote:
> > Surface devices are tablets with detachable keyboards.  they don't really
> > have a "lid" as the tablet is the "lid".
> 
> The Surface Laptop series doesn't have a detachable keyboard, yet still
> requires this. Arguably, the Surface Books are also more laptop than
> tablet (at least that's the way I use mine...). Finally, on the actual
> tablets (Surface Pro series) the lid switch detects when the keyboard
> cover is opened (or at least that's what I have been told, I don't
> own/have access to a Pro series device).
> 
> Regardless of that, this patch is intended to provide the same behavior
> as found on Windows, for all devices included in this patch, which is:
> When you open the lid, or in case of the Pro series fold away the
> keyboard cover, the device wakes from suspend/s2idle. Without this
> patch, that doesn't work.
> 
> > I'm just questioning if the creator of the device designed it the way they did
> > maybe we should think twice about doing this.
> 
> As far as I can tell, the intended behavior is to wake the device when
> the lid is opened, which on the Laptops and Books is a more conventional
> lid and on the Pros constitutes opening the cover.
> 
> I'm open for any alternative though.
> 
> Also please note that I've already sent a v2 of this patch with Andy's
> comments addressed: https://lore.kernel.org/patchwork/patch/1303997/
never mind then.
--mark


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-15 21:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 17:19 [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device Maximilian Luz
2020-09-08 17:19 ` Maximilian Luz
2020-09-08 18:40 ` Andy Shevchenko
2020-09-08 20:19   ` Maximilian Luz
2020-09-10 21:20     ` Maximilian Luz
     [not found] ` <20200911221053.GF103884@mtg-dev.jf.intel.com>
2020-09-11 22:46   ` Maximilian Luz
2020-09-15 21:19     ` mark gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.