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

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

[Hans de Goede]

Hi all,

On some HP devices _AEI handlers which are marked as WakeCapable are
causing spurious wake-ups.

We may be able to fix this better then in this series, but that
requires significant changes to how we handle s2idle, which I do not
see happening any time soon, as explained in more detail here:
https://lore.kernel.org/linux-acpi/61450f9b-cbc6-0c09-8b3a-aff6bf9a0b3c@redhat.c

This series adds a quirk mechanism which allows disabling wakeups
from _AEI handlers as a workaround for this.

Regards,

Hans

[PATCH 1/2] gpiolib: acpi: Turn dmi_system_id table into a generic quirk table

[Hans de Goede]

Turn the existing run_edge_events_on_boot_blacklist dmi_system_id table
into a generic quirk table, storing the quirks in the driver_data ptr.

This is a preparation patch for adding other types of (DMI based) quirks.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d30e57dc755c..2b47d906d536 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -21,6 +21,8 @@
 #include "gpiolib.h"
 #include "gpiolib-acpi.h"
 
+#define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
+
 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,
@@ -1309,7 +1311,7 @@ static int acpi_gpio_handle_deferred_request_irqs(void)
 /* We must use _sync so that this runs after the first deferred_probe run */
 late_initcall_sync(acpi_gpio_handle_deferred_request_irqs);
 
-static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
+static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 	{
 		/*
 		 * The Minix Neo Z83-4 has a micro-USB-B id-pin handler for
@@ -1319,7 +1321,8 @@ static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "MINIX"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
-		}
+		},
+		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
 	},
 	{
 		/*
@@ -1331,15 +1334,23 @@ static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Wortmann_AG"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "TERRA_PAD_1061"),
-		}
+		},
+		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
 	},
 	{} /* Terminating entry */
 };
 
 static int acpi_gpio_setup_params(void)
 {
+	const struct dmi_system_id *id;
+	long quirks = 0;
+
+	id = dmi_first_match(gpiolib_acpi_quirks);
+	if (id)
+		quirks = (long)id->driver_data;
+
 	if (run_edge_events_on_boot < 0) {
-		if (dmi_check_system(run_edge_events_on_boot_blacklist))
+		if (quirks & QUIRK_NO_EDGE_EVENTS_ON_BOOT)
 			run_edge_events_on_boot = 0;
 		else
 			run_edge_events_on_boot = 1;
-- 
2.23.0

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

[Hans de Goede]

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@redhat.com/

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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;
 	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

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

[Andy Shevchenko]

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@redhat.com/
> 
> 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@linux.intel.com>

One nit below, though.

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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;

?

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpiolib: acpi: Turn dmi_system_id table into a generic quirk table

[Andy Shevchenko]

On Fri, Nov 22, 2019 at 08:23:33PM +0100, Hans de Goede wrote:
> Turn the existing run_edge_events_on_boot_blacklist dmi_system_id table
> into a generic quirk table, storing the quirks in the driver_data ptr.
> 
> This is a preparation patch for adding other types of (DMI based) quirks.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index d30e57dc755c..2b47d906d536 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -21,6 +21,8 @@
>  #include "gpiolib.h"
>  #include "gpiolib-acpi.h"
>  
> +#define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
> +
>  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,
> @@ -1309,7 +1311,7 @@ static int acpi_gpio_handle_deferred_request_irqs(void)
>  /* We must use _sync so that this runs after the first deferred_probe run */
>  late_initcall_sync(acpi_gpio_handle_deferred_request_irqs);
>  
> -static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
> +static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  	{
>  		/*
>  		 * The Minix Neo Z83-4 has a micro-USB-B id-pin handler for
> @@ -1319,7 +1321,8 @@ static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "MINIX"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
> -		}
> +		},
> +		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
>  	},
>  	{
>  		/*
> @@ -1331,15 +1334,23 @@ static const struct dmi_system_id run_edge_events_on_boot_blacklist[] = {
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Wortmann_AG"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "TERRA_PAD_1061"),
> -		}
> +		},
> +		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
>  	},
>  	{} /* Terminating entry */
>  };
>  
>  static int acpi_gpio_setup_params(void)
>  {
> +	const struct dmi_system_id *id;
> +	long quirks = 0;
> +
> +	id = dmi_first_match(gpiolib_acpi_quirks);
> +	if (id)
> +		quirks = (long)id->driver_data;
> +
>  	if (run_edge_events_on_boot < 0) {
> -		if (dmi_check_system(run_edge_events_on_boot_blacklist))
> +		if (quirks & QUIRK_NO_EDGE_EVENTS_ON_BOOT)
>  			run_edge_events_on_boot = 0;
>  		else
>  			run_edge_events_on_boot = 1;
> -- 
> 2.23.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpiolib: acpi: Turn dmi_system_id table into a generic quirk table

[Mika Westerberg]

On Fri, Nov 22, 2019 at 08:23:33PM +0100, Hans de Goede wrote:
> Turn the existing run_edge_events_on_boot_blacklist dmi_system_id table
> into a generic quirk table, storing the quirks in the driver_data ptr.
> 
> This is a preparation patch for adding other types of (DMI based) quirks.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Mika Westerberg]

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@redhat.com/
> 
> 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Hans de Goede]

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@redhat.com/
>>
>> 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@linux.intel.com>

Thanks.

> One nit below, though.
> 
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   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
>>
>