[PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[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>
---

Changes in v2:
 - Use software nodes and device properties instead of platform data.
 - Simplify module alias.
 - Add comment regarding origin of GPE numbers.
 - Fix style issues.

---
 MAINTAINERS                        |   6 +
 drivers/platform/x86/Kconfig       |  10 +
 drivers/platform/x86/Makefile      |   1 +
 drivers/platform/x86/surface_gpe.c | 303 +++++++++++++++++++++++++++++
 4 files changed, 320 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..10e563f253b9e
--- /dev/null
+++ b/drivers/platform/x86/surface_gpe.c
@@ -0,0 +1,303 @@
+// 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>
+
+/*
+ * Note: The GPE numbers for the lid devices found below have been obtained
+ *       from ACPI/the DSDT table, specifically from the GPE handler for the
+ *       lid.
+ */
+
+static const struct property_entry lid_device_props_l17[] = {
+	PROPERTY_ENTRY_U32("gpe", 0x17),
+	{},
+};
+
+static const struct property_entry lid_device_props_l4D[] = {
+	PROPERTY_ENTRY_U32("gpe", 0x4D),
+	{},
+};
+
+static const struct property_entry lid_device_props_l4F[] = {
+	PROPERTY_ENTRY_U32("gpe", 0x4F),
+	{},
+};
+
+static const struct property_entry lid_device_props_l57[] = {
+	PROPERTY_ENTRY_U32("gpe", 0x57),
+	{},
+};
+
+/*
+ * Note: When changing this, don't forget to check that the MODULE_ALIAS below
+ *       still fits.
+ */
+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_props_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_props_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_props_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_props_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_props_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_props_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_props_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_props_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_props_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_props_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_props_l4D,
+	},
+	{ }
+};
+
+struct surface_lid_device {
+	u32 gpe_number;
+};
+
+static int surface_lid_enable_wakeup(struct device *dev, bool enable)
+{
+	const struct surface_lid_device *lid = dev_get_drvdata(dev);
+	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)
+{
+	return surface_lid_enable_wakeup(dev, true);
+}
+
+static int surface_gpe_resume(struct device *dev)
+{
+	return surface_lid_enable_wakeup(dev, false);
+}
+
+static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume);
+
+static int surface_gpe_probe(struct platform_device *pdev)
+{
+	struct surface_lid_device *lid;
+	u32 gpe_number;
+	int status;
+
+	status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
+	if (status)
+		return -ENODEV;
+
+	status = acpi_mark_gpe_for_wake(NULL, gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
+		return -EINVAL;
+	}
+
+	status = acpi_enable_gpe(NULL, gpe_number);
+	if (status) {
+		dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
+		return -EINVAL;
+	}
+
+	lid = devm_kzalloc(&pdev->dev, sizeof(struct surface_lid_device),
+			   GFP_KERNEL);
+	if (!lid)
+		return -ENOMEM;
+
+	lid->gpe_number = gpe_number;
+	platform_set_drvdata(pdev, lid);
+
+	status = surface_lid_enable_wakeup(&pdev->dev, false);
+	if (status) {
+		acpi_disable_gpe(NULL, gpe_number);
+		platform_set_drvdata(pdev, NULL);
+		return status;
+	}
+
+	return 0;
+}
+
+static int surface_gpe_remove(struct platform_device *pdev)
+{
+	struct surface_lid_device *lid = dev_get_drvdata(&pdev->dev);
+
+	/* restore default behavior without this module */
+	surface_lid_enable_wakeup(&pdev->dev, false);
+	acpi_disable_gpe(NULL, lid->gpe_number);
+
+	platform_set_drvdata(pdev, NULL);
+	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 property_entry *props;
+	struct platform_device *pdev;
+	struct fwnode_handle *fwnode;
+	int status;
+
+	match = dmi_first_match(dmi_lid_device_table);
+	if (!match) {
+		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
+		return 0;
+	}
+
+	props = 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;
+	}
+
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode)) {
+		platform_device_put(pdev);
+		platform_driver_unregister(&surface_gpe_driver);
+		return PTR_ERR(fwnode);
+	}
+
+	pdev->dev.fwnode = fwnode;
+
+	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;
+}
+module_init(surface_gpe_init);
+
+static void __exit surface_gpe_exit(void)
+{
+	if (!surface_gpe_device)
+		return;
+
+	fwnode_remove_software_node(surface_gpe_device->dev.fwnode);
+	platform_device_unregister(surface_gpe_device);
+	platform_driver_unregister(&surface_gpe_driver);
+}
+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:pnSurface*:*");
-- 
2.28.0

Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[Barnabás Pőcze]

Hi


> [...]
> +static int surface_lid_enable_wakeup(struct device *dev, bool enable)
> +{
> +	const struct surface_lid_device *lid = dev_get_drvdata(dev);
> +	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) {

I think 'if (ACPI_FAILURE(status))' would be better.


> +		dev_err(dev, "failed to set GPE wake mask: %d\n", status);

I'm not sure if it's technically safe to print acpi_status with the %d format
specifier since 'acpi_status' is defined as 'u32' at the moment.
 func("%lu", (unsigned long) status)
would be safer. You could also use 'acpi_format_exception()', which is possibly
the most correct approach since it assumes nothing about what 'acpi_status'
actually is.


> +		return -EINVAL;

I'm not sure if -EINVAL is the best error to return here.


> +	}
> +
> +	return 0;
> +}
> [...]
> +static int surface_gpe_probe(struct platform_device *pdev)
> +{
> +	struct surface_lid_device *lid;
> +	u32 gpe_number;
> +	int status;
> +
> +	status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
> +	if (status)
> +		return -ENODEV;

'device_property_read_u32()' returns an error code, you could simply return that
instead of hiding it.


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

My previous comments about ACPI and the returned value apply here as well.
Furthermore, 'acpi_mark_gpe_for_wake()' and 'acpi_enable_gpe()' both return
a value of type 'acpi_status', not 'int'.


> +
> +	lid = devm_kzalloc(&pdev->dev, sizeof(struct surface_lid_device),
> +			   GFP_KERNEL);

 lid = devm_kzalloc(..., sizeof(*lid), ...)
is preferred.


> +	if (!lid)
> +		return -ENOMEM;

Isn't that problematic that the side effects of the previous two ACPI calls are
not undone when returning here with -ENOMEM? Allocating this struct right after
querying 'gpe_number' could prevent it.


> +
> +	lid->gpe_number = gpe_number;
> +	platform_set_drvdata(pdev, lid);
> +
> +	status = surface_lid_enable_wakeup(&pdev->dev, false);
> +	if (status) {
> +		acpi_disable_gpe(NULL, gpe_number);
> +		platform_set_drvdata(pdev, NULL);

Why is 'platform_set_drvdata(pdev, NULL)' needed?


> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int surface_gpe_remove(struct platform_device *pdev)
> +{
> +	struct surface_lid_device *lid = dev_get_drvdata(&pdev->dev);
> +
> +	/* restore default behavior without this module */
> +	surface_lid_enable_wakeup(&pdev->dev, false);
> +	acpi_disable_gpe(NULL, lid->gpe_number);
> +
> +	platform_set_drvdata(pdev, NULL);

I'm wondering why this is needed?


> +	return 0;
> +}
> [...]
> +static int __init surface_gpe_init(void)
> +{
> +	const struct dmi_system_id *match;
> +	const struct property_entry *props;
> +	struct platform_device *pdev;
> +	struct fwnode_handle *fwnode;
> +	int status;
> +
> +	match = dmi_first_match(dmi_lid_device_table);
> +	if (!match) {
> +		pr_info(KBUILD_MODNAME": no device detected, exiting\n");

If you put
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before including any headers, you can simply write 'pr_info("no device...")' and it'll
be prefixed by the module name. This is the "usual" way of achieving what you want.


> +		return 0;

Shouldn't it return -ENODEV?


> +	}
> +
> +	props = 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;
> +	}
> +
> +	fwnode = fwnode_create_software_node(props, NULL);
> +	if (IS_ERR(fwnode)) {
> +		platform_device_put(pdev);
> +		platform_driver_unregister(&surface_gpe_driver);
> +		return PTR_ERR(fwnode);
> +	}
> +
> +	pdev->dev.fwnode = fwnode;
> +
> +	status = platform_device_add(pdev);
> +	if (status) {
> +		platform_device_put(pdev);
> +		platform_driver_unregister(&surface_gpe_driver);
> +		return status;
> +	}
> +

It may be a matter of preference, but I think the 'if (err) goto X' pattern would
be better in this function (at least for the last 3 or so error paths).


> +	surface_gpe_device = pdev;
> +	return 0;
> +}
> +module_init(surface_gpe_init);
> +
> +static void __exit surface_gpe_exit(void)
> +{
> +	if (!surface_gpe_device)
> +		return;

If you returned -ENODEV in init when no DMI match is found,
then this check would be redundant.


> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[Maximilian Luz]

Hi,

On 9/16/20 1:58 AM, Barnabás Pőcze wrote:
> Hi
> 
> 
>> [...]
>> +static int surface_lid_enable_wakeup(struct device *dev, bool enable)
>> +{
>> +	const struct surface_lid_device *lid = dev_get_drvdata(dev);
>> +	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) {
> 
> I think 'if (ACPI_FAILURE(status))' would be better.

Okay, I'll change that (here and below).

>> +		dev_err(dev, "failed to set GPE wake mask: %d\n", status);
> 
> I'm not sure if it's technically safe to print acpi_status with the %d format
> specifier since 'acpi_status' is defined as 'u32' at the moment.
>   func("%lu", (unsigned long) status)
> would be safer. You could also use 'acpi_format_exception()', which is possibly
> the most correct approach since it assumes nothing about what 'acpi_status'
> actually is.

I wasn't aware of acpi_format_exception(). That looks like a good thing
to do here, thanks!
  
> 
>> +		return -EINVAL;
> 
> I'm not sure if -EINVAL is the best error to return here.

I'd argue that if this fails, it's most likely due to the GPE number
being invalid (which I'd argue is an input), although I'm open for
suggestions. Same reasoning for the -EINVALs below.

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> [...]
>> +static int surface_gpe_probe(struct platform_device *pdev)
>> +{
>> +	struct surface_lid_device *lid;
>> +	u32 gpe_number;
>> +	int status;
>> +
>> +	status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
>> +	if (status)
>> +		return -ENODEV;
> 
> 'device_property_read_u32()' returns an error code, you could simply return that
> instead of hiding it.

My thought there was that if the "gpe" property isn't present or of a
different type, this is not a device that we want to/can handle. Thus
the -ENODEV. Although I think a debug print statement may be useful
here.

>> +
>> +	status = acpi_mark_gpe_for_wake(NULL, gpe_number);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
>> +		return -EINVAL;
>> +	}
>> +
>> +	status = acpi_enable_gpe(NULL, gpe_number);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
>> +		return -EINVAL;
>> +	}
> 
> My previous comments about ACPI and the returned value apply here as well.
> Furthermore, 'acpi_mark_gpe_for_wake()' and 'acpi_enable_gpe()' both return
> a value of type 'acpi_status', not 'int'.

Noted, I'll use acpi_status and acpi_format_exception(). See my
reasoning above for the -EINVAL. Again, I'm open for suggestions on
better values here.

>> +
>> +	lid = devm_kzalloc(&pdev->dev, sizeof(struct surface_lid_device),
>> +			   GFP_KERNEL);
> 
>   lid = devm_kzalloc(..., sizeof(*lid), ...)
> is preferred.

Okay, I'll change that.

>> +	if (!lid)
>> +		return -ENOMEM;
> 
> Isn't that problematic that the side effects of the previous two ACPI calls are
> not undone when returning here with -ENOMEM? Allocating this struct right after
> querying 'gpe_number' could prevent it.

Yes, I think switching those would be cleaner. Also yes, the
acpi_enable_gpe() call is missing a acpi_disable_gpe() here, thanks! I'm
not aware of any counter-part to acpi_mark_gpe_for_wake(), but I think
the side-effects of that are fairly negligible when the GPE is disabled.

>> +
>> +	lid->gpe_number = gpe_number;
>> +	platform_set_drvdata(pdev, lid);
>> +
>> +	status = surface_lid_enable_wakeup(&pdev->dev, false);
>> +	if (status) {
>> +		acpi_disable_gpe(NULL, gpe_number);
>> +		platform_set_drvdata(pdev, NULL);
> 
> Why is 'platform_set_drvdata(pdev, NULL)' needed?

Is this not required for clean-up once the driver data has been set? Or
does the driver-base take care of that for us when the driver is
removed/fails to probe? My reasoning was that I don't want to leave
stuff around for any other driver to trip on (and rather have that
driver oops on a NULL-pointer). If the driver-core already takes care of
NULL-ing that, that line is not needed. Unfortunately that behavior
doesn't seem to be explained in the documentation.

>> +		return status;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int surface_gpe_remove(struct platform_device *pdev)
>> +{
>> +	struct surface_lid_device *lid = dev_get_drvdata(&pdev->dev);
>> +
>> +	/* restore default behavior without this module */
>> +	surface_lid_enable_wakeup(&pdev->dev, false);
>> +	acpi_disable_gpe(NULL, lid->gpe_number);
>> +
>> +	platform_set_drvdata(pdev, NULL);
> 
> I'm wondering why this is needed?

See my question above.

>> +	return 0;
>> +}
>> [...]
>> +static int __init surface_gpe_init(void)
>> +{
>> +	const struct dmi_system_id *match;
>> +	const struct property_entry *props;
>> +	struct platform_device *pdev;
>> +	struct fwnode_handle *fwnode;
>> +	int status;
>> +
>> +	match = dmi_first_match(dmi_lid_device_table);
>> +	if (!match) {
>> +		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
> 
> If you put
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before including any headers, you can simply write 'pr_info("no device...")' and it'll
> be prefixed by the module name. This is the "usual" way of achieving what you want.

Right, thanks!

>> +		return 0;
> 
> Shouldn't it return -ENODEV?

How does module auto-loading behave with a -ENODEV return value in init?
I know that in the driver's probe callback it signals that the driver
isn't intended for the device. Is this the same for modules or would a
user get an error message in the kernel log? As I couldn't find any
documentation on this, I assumed it didn't behave the same and would
emit an error message.

The reason I don't want to emit an error message here is that the module
can be loaded for devices that it's not intended (and that's not
something we can fix with a better MODULE_ALIAS as Microsoft cleverly
named their 5th generation Surface Pro "Surface Pro", without any
version number). Mainly, I don't want users to get a random error
message that doesn't indicate an actual error.

>> +	}
>> +
>> +	props = 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;
>> +	}
>> +
>> +	fwnode = fwnode_create_software_node(props, NULL);
>> +	if (IS_ERR(fwnode)) {
>> +		platform_device_put(pdev);
>> +		platform_driver_unregister(&surface_gpe_driver);
>> +		return PTR_ERR(fwnode);
>> +	}
>> +
>> +	pdev->dev.fwnode = fwnode;
>> +
>> +	status = platform_device_add(pdev);
>> +	if (status) {
>> +		platform_device_put(pdev);
>> +		platform_driver_unregister(&surface_gpe_driver);
>> +		return status;
>> +	}
>> +
> 
> It may be a matter of preference, but I think the 'if (err) goto X' pattern would
> be better in this function (at least for the last 3 or so error paths).

Noted, I'll change that.

>> +	surface_gpe_device = pdev;
>> +	return 0;
>> +}
>> +module_init(surface_gpe_init);
>> +
>> +static void __exit surface_gpe_exit(void)
>> +{
>> +	if (!surface_gpe_device)
>> +		return;
> 
> If you returned -ENODEV in init when no DMI match is found,
> then this check would be redundant.

Yes. See my question regarding -ENODEV above.

Thank you for your time,
Max

Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[Barnabás Pőcze]

2020. szeptember 16., szerda 3:22 keltezéssel, Maximilian Luz írta:
> [...]
> >> +static int surface_lid_enable_wakeup(struct device *dev, bool enable)
> >> +{
> >> +	const struct surface_lid_device *lid = dev_get_drvdata(dev);
> >> +	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) {
> >
> > I think 'if (ACPI_FAILURE(status))' would be better.
>
> Okay, I'll change that (here and below).
>
> >> +		dev_err(dev, "failed to set GPE wake mask: %d\n", status);
> >
> > I'm not sure if it's technically safe to print acpi_status with the %d format
> > specifier since 'acpi_status' is defined as 'u32' at the moment.
> >   func("%lu", (unsigned long) status)
> > would be safer. You could also use 'acpi_format_exception()', which is possibly
> > the most correct approach since it assumes nothing about what 'acpi_status'
> > actually is.
>
> I wasn't aware of acpi_format_exception(). That looks like a good thing
> to do here, thanks!
>
> >
> >> +		return -EINVAL;
> >
> > I'm not sure if -EINVAL is the best error to return here.
>
> I'd argue that if this fails, it's most likely due to the GPE number
> being invalid (which I'd argue is an input), although I'm open for
> suggestions. Same reasoning for the -EINVALs below.
>

I see, I guess that makes sense, I didn't think of looking at it this way.


> >
> >> +	}s
> >> +
> >> +	return 0;
> >> +}
> >> [...]
> >> +static int surface_gpe_probe(struct platform_device *pdev)
> >> +{
> >> +	struct surface_lid_device *lid;
> >> +	u32 gpe_number;
> >> +	int status;
> >> +
> >> +	status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
> >> +	if (status)
> >> +		return -ENODEV;
> >
> > 'device_property_read_u32()' returns an error code, you could simply return that
> > instead of hiding it.
>
> My thought there was that if the "gpe" property isn't present or of a
> different type, this is not a device that we want to/can handle. Thus
> the -ENODEV. Although I think a debug print statement may be useful
> here.
>

I see, I just wanted to bring to your attention that 'device_property_read_u32()'
returns various standard error codes and you could simply return those.


> [...]
> >> +
> >> +	lid->gpe_number = gpe_number;
> >> +	platform_set_drvdata(pdev, lid);
> >> +
> >> +	status = surface_lid_enable_wakeup(&pdev->dev, false);
> >> +	if (status) {
> >> +		acpi_disable_gpe(NULL, gpe_number);
> >> +		platform_set_drvdata(pdev, NULL);
> >
> > Why is 'platform_set_drvdata(pdev, NULL)' needed?
>
> Is this not required for clean-up once the driver data has been set? Or
> does the driver-base take care of that for us when the driver is
> removed/fails to probe? My reasoning was that I don't want to leave
> stuff around for any other driver to trip on (and rather have that
> driver oops on a NULL-pointer). If the driver-core already takes care of
> NULL-ing that, that line is not needed. Unfortunately that behavior
> doesn't seem to be explained in the documentation.
>

I'm not aware that it would be required. As a matter of fact, only two x86
platform drivers (intel_pmc_core, ideapad-laptop) do any cleanup of driver data.
There are much more hits (536) for "set_drvdata(.* NULL" when scanning all drivers.
There are 4864 hits for "set_drvdata(.*" that have no 'NULL' in them.

There is drivers/base/dd.c:really_probe(), which seems to be the place where driver
probes are actually called. And it calls 'dev_set_drvdata(dev, NULL)' if the probe
fails. And it also sets the driver data to NULL in '__device_release_driver()',
so I'm pretty sure the driver core takes care of it.


> >> +		return status;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> [...]
> >> +static int __init surface_gpe_init(void)
> >> +{
> >> +	const struct dmi_system_id *match;
> >> +	const struct property_entry *props;
> >> +	struct platform_device *pdev;
> >> +	struct fwnode_handle *fwnode;
> >> +	int status;
> >> +
> >> +	match = dmi_first_match(dmi_lid_device_table);
> >> +	if (!match) {
> >> +		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
> >
> > If you put
> >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > before including any headers, you can simply write 'pr_info("no device...")' and it'll
> > be prefixed by the module name. This is the "usual" way of achieving what you want.
>
> Right, thanks!
>
> >> +		return 0;
> >
> > Shouldn't it return -ENODEV?
>
> How does module auto-loading behave with a -ENODEV return value in init?
> I know that in the driver's probe callback it signals that the driver
> isn't intended for the device. Is this the same for modules or would a
> user get an error message in the kernel log? As I couldn't find any
> documentation on this, I assumed it didn't behave the same and would
> emit an error message.
>
> The reason I don't want to emit an error message here is that the module
> can be loaded for devices that it's not intended (and that's not
> something we can fix with a better MODULE_ALIAS as Microsoft cleverly
> named their 5th generation Surface Pro "Surface Pro", without any
> version number). Mainly, I don't want users to get a random error
> message that doesn't indicate an actual error.
>

I wasn't sure, so I tested it. It prints the "no device" message, but that's it,
no more indication of the -ENODEV error in the kernel log during automatic
module loading at boot.

You could print "no compatible Microsoft Surface device found, exitig" (or something
similar). I think this provides enough information for any user to decide if
they should be concerned or not.


> >> +	}
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[Andy Shevchenko]

On Wed, Sep 16, 2020 at 2:58 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:

...

> > +             dev_err(dev, "failed to set GPE wake mask: %d\n", status);
>
> I'm not sure if it's technically safe to print acpi_status with the %d format
> specifier since 'acpi_status' is defined as 'u32' at the moment.
>  func("%lu", (unsigned long) status)
> would be safer.

Please, no explicit castings for printf(). In 99% cases it means that
something is not being used correctly.





-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

[Maximilian Luz]

On 9/16/20 7:13 PM, Barnabás Pőcze wrote:
...
>>>> +	}s
>>>> +
>>>> +	return 0;
>>>> +}
>>>> [...]
>>>> +static int surface_gpe_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct surface_lid_device *lid;
>>>> +	u32 gpe_number;
>>>> +	int status;
>>>> +
>>>> +	status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
>>>> +	if (status)
>>>> +		return -ENODEV;
>>>
>>> 'device_property_read_u32()' returns an error code, you could simply return that
>>> instead of hiding it.
>>
>> My thought there was that if the "gpe" property isn't present or of a
>> different type, this is not a device that we want to/can handle. Thus
>> the -ENODEV. Although I think a debug print statement may be useful
>> here.
>>
> 
> I see, I just wanted to bring to your attention that 'device_property_read_u32()'
> returns various standard error codes and you could simply return those.

I think one could also argue that module-loading should have taken care
of filtering out devices that we don't load on, so -ENODEV would be
redundant here. At least if one neglects that a user could try to
manually bind the driver to a device. Following that thought, I guess it
makes more sense to return the actual value here.

>> [...]
>>>> +
>>>> +	lid->gpe_number = gpe_number;
>>>> +	platform_set_drvdata(pdev, lid);
>>>> +
>>>> +	status = surface_lid_enable_wakeup(&pdev->dev, false);
>>>> +	if (status) {
>>>> +		acpi_disable_gpe(NULL, gpe_number);
>>>> +		platform_set_drvdata(pdev, NULL);
>>>
>>> Why is 'platform_set_drvdata(pdev, NULL)' needed?
>>
>> Is this not required for clean-up once the driver data has been set? Or
>> does the driver-base take care of that for us when the driver is
>> removed/fails to probe? My reasoning was that I don't want to leave
>> stuff around for any other driver to trip on (and rather have that
>> driver oops on a NULL-pointer). If the driver-core already takes care of
>> NULL-ing that, that line is not needed. Unfortunately that behavior
>> doesn't seem to be explained in the documentation.
>>
> 
> I'm not aware that it would be required. As a matter of fact, only two x86
> platform drivers (intel_pmc_core, ideapad-laptop) do any cleanup of driver data.
> There are much more hits (536) for "set_drvdata(.* NULL" when scanning all drivers.
> There are 4864 hits for "set_drvdata(.*" that have no 'NULL' in them.
> 
> There is drivers/base/dd.c:really_probe(), which seems to be the place where driver
> probes are actually called. And it calls 'dev_set_drvdata(dev, NULL)' if the probe
> fails. And it also sets the driver data to NULL in '__device_release_driver()',
> so I'm pretty sure the driver core takes care of it.

I see, thanks! Would make sense that the core takes care of that.

>>>> +		return status;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>> [...]
>>>> +static int __init surface_gpe_init(void)
>>>> +{
>>>> +	const struct dmi_system_id *match;
>>>> +	const struct property_entry *props;
>>>> +	struct platform_device *pdev;
>>>> +	struct fwnode_handle *fwnode;
>>>> +	int status;
>>>> +
>>>> +	match = dmi_first_match(dmi_lid_device_table);
>>>> +	if (!match) {
>>>> +		pr_info(KBUILD_MODNAME": no device detected, exiting\n");
>>>
>>> If you put
>>>    #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> before including any headers, you can simply write 'pr_info("no device...")' and it'll
>>> be prefixed by the module name. This is the "usual" way of achieving what you want.
>>
>> Right, thanks!
>>
>>>> +		return 0;
>>>
>>> Shouldn't it return -ENODEV?
>>
>> How does module auto-loading behave with a -ENODEV return value in init?
>> I know that in the driver's probe callback it signals that the driver
>> isn't intended for the device. Is this the same for modules or would a
>> user get an error message in the kernel log? As I couldn't find any
>> documentation on this, I assumed it didn't behave the same and would
>> emit an error message.
>>
>> The reason I don't want to emit an error message here is that the module
>> can be loaded for devices that it's not intended (and that's not
>> something we can fix with a better MODULE_ALIAS as Microsoft cleverly
>> named their 5th generation Surface Pro "Surface Pro", without any
>> version number). Mainly, I don't want users to get a random error
>> message that doesn't indicate an actual error.
>>
> 
> I wasn't sure, so I tested it. It prints the "no device" message, but that's it,
> no more indication of the -ENODEV error in the kernel log during automatic
> module loading at boot.
> 
> You could print "no compatible Microsoft Surface device found, exitig" (or something
> similar). I think this provides enough information for any user to decide if
> they should be concerned or not.

I ran the same test (with same results) earlier today and also did some
digging: From what I can tell, udev is responsible for auto-loading and
the code doing that can be found at [1]. This code seems to, by default,
log any errors as debug output. Only in verbose mode it logs them as
error, with the exception of -ENODEV, which then is specifically logged
only as notice.

It also seems to be used by a couple of other modules this way. So I
guess that's the expected use-case for -ENODEV in module-init and pretty
much guarantees the behavior I've wanted.

[1]: https://github.com/systemd/systemd/blob/6d95e7d9b263c94e94704e3125cb790840b76ca2/src/shared/module-util.c#L58-L64

Thanks again. If there are no other comments, I'll likely submit a v3
addressing the issues tomorrow.

Regards,
Max