Re: [PATCH 2/2] gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 25-11-2019 10:25, Andy Shevchenko wrote:
On Fri, Nov 22, 2019 at 08:23:34PM +0100, Hans de Goede wrote:
On some laptops enabling wakeup on the GPIO interrupts used for ACPI _AEI
event handling causes spurious wakeups.

This commit adds a new honor_wakeup option, defaulting to true (our current
behavior), which can be used to disable wakeup on troublesome hardware
to avoid these spurious wakeups.

This is a workaround for an architectural problem with s2idle under Linux
where we do not have any mechanism to immediately go back to sleep after
wakeup events, other then for embedded-controller events using the standard
ACPI EC interface, for details see:
https://lore.kernel.org/linux-acpi/61450f9b-cbc6-0c09-8b3a-aff6bf9a0b3c@xxxxxxxxxx/

One series of laptops which is not able to suspend without this workaround
is the HP x2 10 Cherry Trail models, this commit adds a DMI based quirk
which makes sets honor_wakeup to false on these models.

I'm not against this approach (yeah, it seems we will always have a stream of
quirks for BIOS enabled platforms, especially cheapest ones), though last word
is by Rafael.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Thanks.

One nit below, though.


Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
  drivers/gpio/gpiolib-acpi.c | 33 ++++++++++++++++++++++++++++++++-
  1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 2b47d906d536..9ce9b449ac4b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -22,12 +22,18 @@
  #include "gpiolib-acpi.h"
#define QUIRK_NO_EDGE_EVENTS_ON_BOOT 0x01l
+#define QUIRK_NO_WAKEUP				0x02l
static int run_edge_events_on_boot = -1;
  module_param(run_edge_events_on_boot, int, 0444);
  MODULE_PARM_DESC(run_edge_events_on_boot,
  		 "Run edge _AEI event-handlers at boot: 0=no, 1=yes, -1=auto");
+static int honor_wakeup = -1;
+module_param(honor_wakeup, int, 0444);
+MODULE_PARM_DESC(honor_wakeup,
+		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");
+
  /**
   * struct acpi_gpio_event - ACPI GPIO event handler data
   *
@@ -283,7 +289,8 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
  	event->handle = evt_handle;
  	event->handler = handler;
  	event->irq = irq;
-	event->irq_is_wake = agpio->wake_capable == ACPI_WAKE_CAPABLE;

+	if (honor_wakeup)
+		event->irq_is_wake = agpio->wake_capable == ACPI_WAKE_CAPABLE;

Perhaps:

	event->irq_is_wake = honor_wakeup && agpio->wake_capable == ACPI_WAKE_CAPABLE;

Yes that is better, I also noticed some typos in the comment explaining why the
quirk is necessary, I will submit a v2 fixing both.

Regards,

Hans



(I don't care about 80 limit here)

  	event->pin = pin;
  	event->desc = desc;
@@ -1337,6 +1344,23 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
  		},
  		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
  	},
+	{
+		/*
+		 * Various HP X2 10 Cherry Trail models use external
+		 * embedded-controller connected via I2C + a ACPI GPIO
+		 * event handler. The embedded controller generates various
+		 * spurious wakeup events when suspended. So disable wakeup
+		 * for its handler (it used the only ACPI GPIO event handler).
+		 * This breaks wakeup when opening the lid, the user needs
+		 * to press the power-button to wakeup the system. The
+		 * alternative is suspend simply not working, which is worse.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP x2 Detachable 10-p0XX"),
+		},
+		.driver_data = (void *)QUIRK_NO_WAKEUP,
+	},
  	{} /* Terminating entry */
  };
@@ -1356,6 +1380,13 @@ static int acpi_gpio_setup_params(void)
  			run_edge_events_on_boot = 1;
  	}
+ if (honor_wakeup < 0) {
+		if (quirks & QUIRK_NO_WAKEUP)
+			honor_wakeup = 0;
+		else
+			honor_wakeup = 1;
+	}
+
  	return 0;
  }
--
2.23.0






[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux