| Message ID | 20200910211520.1490626-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device | expand |
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
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
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
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.
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
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*:*");
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