[PATCH v2 1/4] gpiolib: acpi: Correct comment for HP x2 10 honor_wakeup quirk

[PATCH v2 1/4] gpiolib: acpi: Correct comment for HP x2 10 honor_wakeup quirk

[Hans de Goede]

Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
quirk mechanism") added a quirk for some models of the HP x2 10 series.

There are 2 issues with the comment describing the quirk:
1) The comment claims the DMI quirk applies to all Cherry Trail based HP x2
   10 models. In the mean time I have learned that there are at least 3
   models of the HP x2 10 models:

   Bay Trail SoC + AXP288 PMIC
   Cherry Trail SoC + AXP288 PMIC
   Cherry Trail SoC + TI PMIC

   And this quirk's DMI matches only match the Cherry Trail SoC + TI PMIC
   SoC, which is good because we want a slightly different quirk for the
   others. This commit updates the comment to make it clear that the quirk
   is only for the Cherry Trail SoC + TI PMIC models.

2) The comment says that it is ok to disable wakeup on all ACPI GPIO event
   handlers, because there is only the one for the embedded-controller
   events. This is not true, there also is a handler for the special
   INT0002 device which is related to USB wakeups. We need to also disable
   wakeups on that one because the device turns of the USB-keyboard built
   into the dock when closing the lid. The XHCI controller takes a while
   to notice this, so it only notices it when already suspended, causing
   a spurious wakeup because of this. So disabling wakeup on all handlers
   is the right thing to do, but not because there only is the one handler
   for the EC events. This commit updates the comment to correctly reflect
   this.

Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 31fee5e918b7..a77edd31dd60 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1345,12 +1345,14 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 	},
 	{
 		/*
-		 * Various HP X2 10 Cherry Trail models use an external
-		 * embedded-controller connected via I2C + an ACPI GPIO
-		 * event handler. The embedded controller generates various
-		 * spurious wakeup events when suspended. So disable wakeup
-		 * for its handler (it uses the only ACPI GPIO event handler).
-		 * This breaks wakeup when opening the lid, the user needs
+		 * HP X2 10 models with Cherry Trail SoC + TI PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI GPIO
+		 * event handler on INT33FF:01 pin 0, causing spurious wakeups.
+		 * When suspending by closing the LID, the power to the USB
+		 * keyboard is turned off, causing INT0002 ACPI events to
+		 * trigger once the XHCI controller notices the keyboard is
+		 * gone. So INT0002 events cause spurious wakeups too. Ignoring
+		 * EC wakes 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.
 		 */
-- 
2.25.1

[PATCH v2 2/4] gpiolib: acpi: Rework honor_wakeup option into an ignore_wake option

[Hans de Goede]

Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
quirk mechanism") was added to deal with spurious wakeups on one specific
model of the HP x2 10 series.

The approach taken there was to add a bool controlling wakeup support for
all ACPI GPIO events. This was sufficient for the specific HP x2 10 model
the commit was trying to fix, but in the mean time other models have
turned up which need a similar workaround to avoid spurious wakeups from
suspend, but only for one of the pins on which the ACPI tables request
ACPI GPIO events.

Since the honor_wakeup option was added to be able to ignore wake events,
the name was perhaps not the best, this commit renames it to ignore_wake
and changes it to a string with the following format:
gpiolib_acpi.ignore_wake=controller@pin[,controller@pin[,...]]

This allows working around spurious wakeup issues on a per pin basis.

This commit also reworks the existing quirk for the HP x2 10 so that
it functions as before.

Note:
-This removes the honor_wakeup parameter. This has only been upstream for
 a short time and to the best of my knowledge there are no users using
 this module parameter.

-The controller@pin[,controller@pin[,...]] syntax is based on an existing
 kernel module parameter using the same controller@pin format. That version
 uses ';' as separator, but in practice that is problematic because grub2
 cannot handle this without taking special care to escape the ';', so here
 we are using a ',' as separator instead which does not have this issue.

Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Rewrite to use a much more generic string based format to describe
 the controller@pin combination(s) on which to ignore wakeups
---
 drivers/gpio/gpiolib-acpi.c | 96 +++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index a77edd31dd60..87bb0d3c69da 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -21,18 +21,21 @@
 #include "gpiolib.h"
 #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");
+static char *ignore_wake;
+module_param(ignore_wake, charp, 0444);
+MODULE_PARM_DESC(ignore_wake,
+		 "controller@pin combos on which to ignore the ACPI wake flag "
+		 "ignore_wake=controller@pin[,controller@pin[,...]]");
+
+struct acpi_gpiolib_dmi_quirk {
+	bool no_edge_events_on_boot;
+	char *ignore_wake;
+};
 
 /**
  * struct acpi_gpio_event - ACPI GPIO event handler data
@@ -202,6 +205,57 @@ static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
 		acpi_gpiochip_request_irq(acpi_gpio, event);
 }
 
+static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
+{
+	const char *controller, *pin_str;
+	int len, pin;
+	char *endp;
+
+	controller = ignore_wake;
+	while (controller) {
+		pin_str = strchr(controller, '@');
+		if (!pin_str)
+			goto err;
+
+		len = pin_str - controller;
+		if (len == strlen(controller_in) &&
+		    strncmp(controller, controller_in, len) == 0) {
+			pin = simple_strtoul(pin_str + 1, &endp, 10);
+			if (*endp != 0 && *endp != ',')
+				goto err;
+
+			if (pin == pin_in)
+				return true;
+		}
+
+		controller = strchr(controller, ',');
+		if (controller)
+			controller++;
+	}
+
+	return false;
+err:
+	pr_err_once("Error invalid value for gpiolib_acpi.ignore_wake: %s\n",
+		    ignore_wake);
+	return false;
+}
+
+static bool acpi_gpio_irq_is_wake(struct device *parent,
+				  struct acpi_resource_gpio *agpio)
+{
+	int pin = agpio->pin_table[0];
+
+	if (agpio->wake_capable != ACPI_WAKE_CAPABLE)
+		return false;
+
+	if (acpi_gpio_in_ignore_list(dev_name(parent), pin)) {
+		dev_info(parent, "Ignoring wakeup on pin %d\n", pin);
+		return false;
+	}
+
+	return true;
+}
+
 /* Always returns AE_OK so that we keep looping over the resources */
 static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 					     void *context)
@@ -289,7 +343,7 @@ 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 = honor_wakeup && agpio->wake_capable == ACPI_WAKE_CAPABLE;
+	event->irq_is_wake = acpi_gpio_irq_is_wake(chip->parent, agpio);
 	event->pin = pin;
 	event->desc = desc;
 
@@ -1328,7 +1382,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "MINIX"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
 		},
-		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.no_edge_events_on_boot = true,
+		},
 	},
 	{
 		/*
@@ -1341,7 +1397,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Wortmann_AG"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "TERRA_PAD_1061"),
 		},
-		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.no_edge_events_on_boot = true,
+		},
 	},
 	{
 		/*
@@ -1360,33 +1418,31 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "HP x2 Detachable 10-p0XX"),
 		},
-		.driver_data = (void *)QUIRK_NO_WAKEUP,
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.ignore_wake = "INT33FF:01@0,INT0002:00@2",
+		},
 	},
 	{} /* Terminating entry */
 };
 
 static int acpi_gpio_setup_params(void)
 {
+	const struct acpi_gpiolib_dmi_quirk *quirk = NULL;
 	const struct dmi_system_id *id;
-	long quirks = 0;
 
 	id = dmi_first_match(gpiolib_acpi_quirks);
 	if (id)
-		quirks = (long)id->driver_data;
+		quirk = id->driver_data;
 
 	if (run_edge_events_on_boot < 0) {
-		if (quirks & QUIRK_NO_EDGE_EVENTS_ON_BOOT)
+		if (quirk && quirk->no_edge_events_on_boot)
 			run_edge_events_on_boot = 0;
 		else
 			run_edge_events_on_boot = 1;
 	}
 
-	if (honor_wakeup < 0) {
-		if (quirks & QUIRK_NO_WAKEUP)
-			honor_wakeup = 0;
-		else
-			honor_wakeup = 1;
-	}
+	if (ignore_wake == NULL && quirk && quirk->ignore_wake)
+		ignore_wake = quirk->ignore_wake;
 
 	return 0;
 }
-- 
2.25.1

[PATCH v2 3/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 BYT + AXP288 model

[Hans de Goede]

Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
quirk mechanism") was added to deal with spurious wakeups on one specific
model of the HP x2 10 series. In the mean time I have learned that there
are at least 3 different HP x2 10 models:

Bay Trail SoC + AXP288 PMIC
Cherry Trail SoC + AXP288 PMIC
Cherry Trail SoC + TI PMIC

And the original quirk is only correct for (and only matches the)
Cherry Trail SoC + TI PMIC model.

The Bay Trail SoC + AXP288 PMIC model has different DMI strings, has
the external EC interrupt on a different GPIO pin and only needs to ignore
wakeups on the EC interrupt, the INT0002 device works fine on this model.

This commit adds an extra DMI based quirk for the HP x2 10 BYT + AXP288
model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
on this model. This fixes spurious wakeups from suspend on this model.

Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Split out new quirk addition into its own patch
---
 drivers/gpio/gpiolib-acpi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 87bb0d3c69da..d1ef060a5873 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1422,6 +1422,21 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 			.ignore_wake = "INT33FF:01@0,INT0002:00@2",
 		},
 	},
+	{
+		/*
+		 * HP X2 10 models with Bay Trail SoC + AXP288 PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI GPIO
+		 * event handler on INT33FC:02 pin 28, causing spurious wakeups.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion x2 Detachable"),
+			DMI_MATCH(DMI_BOARD_NAME, "815D"),
+		},
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.ignore_wake = "INT33FC:02@28",
+		},
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.25.1

[PATCH v2 4/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 CHT + AXP288 model

[Hans de Goede]

There are at least 3 models of the HP x2 10 models:

Bay Trail SoC + AXP288 PMIC
Cherry Trail SoC + AXP288 PMIC
Cherry Trail SoC + TI PMIC

Like on the other HP x2 10 models we need to ignore wakeup for ACPI GPIO
events on the external embedded-controller pin to avoid spurious wakeups
on the HP x2 10 CHT + AXP288 model too.

This commit adds an extra DMI based quirk for the HP x2 10 CHT + AXP288
model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
on this model. This fixes spurious wakeups from suspend on this model.

Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mech
Reported-and-tested-by: Marc Lehmann <schmorp@schmorp.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d1ef060a5873..0017367e94ee 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1437,6 +1437,21 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 			.ignore_wake = "INT33FC:02@28",
 		},
 	},
+	{
+		/*
+		 * HP X2 10 models with Cherry Trail SoC + AXP288 PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI GPIO
+		 * event handler on INT33FF:01 pin 0, causing spurious wakeups.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion x2 Detachable"),
+			DMI_MATCH(DMI_BOARD_NAME, "813E"),
+		},
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.ignore_wake = "INT33FF:01@0",
+		},
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.25.1

Re: [PATCH v2 1/4] gpiolib: acpi: Correct comment for HP x2 10 honor_wakeup quirk

[Mika Westerberg]

On Mon, Mar 02, 2020 at 12:12:22PM +0100, Hans de Goede wrote:
> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") added a quirk for some models of the HP x2 10 series.
> 
> There are 2 issues with the comment describing the quirk:
> 1) The comment claims the DMI quirk applies to all Cherry Trail based HP x2
>    10 models. In the mean time I have learned that there are at least 3
>    models of the HP x2 10 models:
> 
>    Bay Trail SoC + AXP288 PMIC
>    Cherry Trail SoC + AXP288 PMIC
>    Cherry Trail SoC + TI PMIC
> 
>    And this quirk's DMI matches only match the Cherry Trail SoC + TI PMIC
>    SoC, which is good because we want a slightly different quirk for the
>    others. This commit updates the comment to make it clear that the quirk
>    is only for the Cherry Trail SoC + TI PMIC models.
> 
> 2) The comment says that it is ok to disable wakeup on all ACPI GPIO event
>    handlers, because there is only the one for the embedded-controller
>    events. This is not true, there also is a handler for the special
>    INT0002 device which is related to USB wakeups. We need to also disable
>    wakeups on that one because the device turns of the USB-keyboard built
>    into the dock when closing the lid. The XHCI controller takes a while
>    to notice this, so it only notices it when already suspended, causing
>    a spurious wakeup because of this. So disabling wakeup on all handlers
>    is the right thing to do, but not because there only is the one handler
>    for the EC events. This commit updates the comment to correctly reflect
>    this.
> 
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")

Not sure if Fixes tag is needed if you only correct comment but fine :)

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

Re: [PATCH v2 2/4] gpiolib: acpi: Rework honor_wakeup option into an ignore_wake option

[Mika Westerberg]

On Mon, Mar 02, 2020 at 12:12:23PM +0100, Hans de Goede wrote:
> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series.
> 
> The approach taken there was to add a bool controlling wakeup support for
> all ACPI GPIO events. This was sufficient for the specific HP x2 10 model
> the commit was trying to fix, but in the mean time other models have
> turned up which need a similar workaround to avoid spurious wakeups from
> suspend, but only for one of the pins on which the ACPI tables request
> ACPI GPIO events.
> 
> Since the honor_wakeup option was added to be able to ignore wake events,
> the name was perhaps not the best, this commit renames it to ignore_wake
> and changes it to a string with the following format:
> gpiolib_acpi.ignore_wake=controller@pin[,controller@pin[,...]]
> 
> This allows working around spurious wakeup issues on a per pin basis.
> 
> This commit also reworks the existing quirk for the HP x2 10 so that
> it functions as before.
> 
> Note:
> -This removes the honor_wakeup parameter. This has only been upstream for
>  a short time and to the best of my knowledge there are no users using
>  this module parameter.
> 
> -The controller@pin[,controller@pin[,...]] syntax is based on an existing
>  kernel module parameter using the same controller@pin format. That version
>  uses ';' as separator, but in practice that is problematic because grub2
>  cannot handle this without taking special care to escape the ';', so here
>  we are using a ',' as separator instead which does not have this issue.
> 
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

Re: [PATCH v2 3/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 BYT + AXP288 model

[Mika Westerberg]

On Mon, Mar 02, 2020 at 12:12:24PM +0100, Hans de Goede wrote:
> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series. In the mean time I have learned that there
> are at least 3 different HP x2 10 models:
> 
> Bay Trail SoC + AXP288 PMIC
> Cherry Trail SoC + AXP288 PMIC
> Cherry Trail SoC + TI PMIC
> 
> And the original quirk is only correct for (and only matches the)
> Cherry Trail SoC + TI PMIC model.
> 
> The Bay Trail SoC + AXP288 PMIC model has different DMI strings, has
> the external EC interrupt on a different GPIO pin and only needs to ignore
> wakeups on the EC interrupt, the INT0002 device works fine on this model.
> 
> This commit adds an extra DMI based quirk for the HP x2 10 BYT + AXP288
> model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
> on this model. This fixes spurious wakeups from suspend on this model.
> 
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

Re: [PATCH v2 4/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 CHT + AXP288 model

[Mika Westerberg]

On Mon, Mar 02, 2020 at 12:12:25PM +0100, Hans de Goede wrote:
> There are at least 3 models of the HP x2 10 models:
> 
> Bay Trail SoC + AXP288 PMIC
> Cherry Trail SoC + AXP288 PMIC
> Cherry Trail SoC + TI PMIC
> 
> Like on the other HP x2 10 models we need to ignore wakeup for ACPI GPIO
> events on the external embedded-controller pin to avoid spurious wakeups
> on the HP x2 10 CHT + AXP288 model too.
> 
> This commit adds an extra DMI based quirk for the HP x2 10 CHT + AXP288
> model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
> on this model. This fixes spurious wakeups from suspend on this model.
> 
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mech
> Reported-and-tested-by: Marc Lehmann <schmorp@schmorp.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

Re: [PATCH v2 2/4] gpiolib: acpi: Rework honor_wakeup option into an ignore_wake option

[Andy Shevchenko]

On Mon, Mar 02, 2020 at 12:12:23PM +0100, Hans de Goede wrote:
> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series.
> 
> The approach taken there was to add a bool controlling wakeup support for
> all ACPI GPIO events. This was sufficient for the specific HP x2 10 model
> the commit was trying to fix, but in the mean time other models have
> turned up which need a similar workaround to avoid spurious wakeups from
> suspend, but only for one of the pins on which the ACPI tables request
> ACPI GPIO events.
> 
> Since the honor_wakeup option was added to be able to ignore wake events,
> the name was perhaps not the best, this commit renames it to ignore_wake
> and changes it to a string with the following format:
> gpiolib_acpi.ignore_wake=controller@pin[,controller@pin[,...]]
> 
> This allows working around spurious wakeup issues on a per pin basis.
> 
> This commit also reworks the existing quirk for the HP x2 10 so that
> it functions as before.
> 
> Note:
> -This removes the honor_wakeup parameter. This has only been upstream for
>  a short time and to the best of my knowledge there are no users using
>  this module parameter.
> 
> -The controller@pin[,controller@pin[,...]] syntax is based on an existing
>  kernel module parameter using the same controller@pin format. That version
>  uses ';' as separator, but in practice that is problematic because grub2
>  cannot handle this without taking special care to escape the ';', so here
>  we are using a ',' as separator instead which does not have this issue.
> 

All are fine to me.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Rewrite to use a much more generic string based format to describe
>  the controller@pin combination(s) on which to ignore wakeups
> ---
>  drivers/gpio/gpiolib-acpi.c | 96 +++++++++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index a77edd31dd60..87bb0d3c69da 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -21,18 +21,21 @@
>  #include "gpiolib.h"
>  #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");
> +static char *ignore_wake;
> +module_param(ignore_wake, charp, 0444);
> +MODULE_PARM_DESC(ignore_wake,
> +		 "controller@pin combos on which to ignore the ACPI wake flag "
> +		 "ignore_wake=controller@pin[,controller@pin[,...]]");
> +
> +struct acpi_gpiolib_dmi_quirk {
> +	bool no_edge_events_on_boot;
> +	char *ignore_wake;
> +};
>  
>  /**
>   * struct acpi_gpio_event - ACPI GPIO event handler data
> @@ -202,6 +205,57 @@ static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
>  		acpi_gpiochip_request_irq(acpi_gpio, event);
>  }
>  
> +static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
> +{
> +	const char *controller, *pin_str;
> +	int len, pin;
> +	char *endp;
> +
> +	controller = ignore_wake;
> +	while (controller) {
> +		pin_str = strchr(controller, '@');
> +		if (!pin_str)
> +			goto err;
> +
> +		len = pin_str - controller;
> +		if (len == strlen(controller_in) &&
> +		    strncmp(controller, controller_in, len) == 0) {
> +			pin = simple_strtoul(pin_str + 1, &endp, 10);
> +			if (*endp != 0 && *endp != ',')
> +				goto err;
> +
> +			if (pin == pin_in)
> +				return true;
> +		}
> +
> +		controller = strchr(controller, ',');
> +		if (controller)
> +			controller++;
> +	}
> +
> +	return false;
> +err:
> +	pr_err_once("Error invalid value for gpiolib_acpi.ignore_wake: %s\n",
> +		    ignore_wake);
> +	return false;
> +}
> +
> +static bool acpi_gpio_irq_is_wake(struct device *parent,
> +				  struct acpi_resource_gpio *agpio)
> +{
> +	int pin = agpio->pin_table[0];
> +
> +	if (agpio->wake_capable != ACPI_WAKE_CAPABLE)
> +		return false;
> +
> +	if (acpi_gpio_in_ignore_list(dev_name(parent), pin)) {
> +		dev_info(parent, "Ignoring wakeup on pin %d\n", pin);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /* Always returns AE_OK so that we keep looping over the resources */
>  static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>  					     void *context)
> @@ -289,7 +343,7 @@ 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 = honor_wakeup && agpio->wake_capable == ACPI_WAKE_CAPABLE;
> +	event->irq_is_wake = acpi_gpio_irq_is_wake(chip->parent, agpio);
>  	event->pin = pin;
>  	event->desc = desc;
>  
> @@ -1328,7 +1382,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "MINIX"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.no_edge_events_on_boot = true,
> +		},
>  	},
>  	{
>  		/*
> @@ -1341,7 +1397,9 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Wortmann_AG"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "TERRA_PAD_1061"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_EDGE_EVENTS_ON_BOOT,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.no_edge_events_on_boot = true,
> +		},
>  	},
>  	{
>  		/*
> @@ -1360,33 +1418,31 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "HP x2 Detachable 10-p0XX"),
>  		},
> -		.driver_data = (void *)QUIRK_NO_WAKEUP,
> +		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
> +			.ignore_wake = "INT33FF:01@0,INT0002:00@2",
> +		},
>  	},
>  	{} /* Terminating entry */
>  };
>  
>  static int acpi_gpio_setup_params(void)
>  {
> +	const struct acpi_gpiolib_dmi_quirk *quirk = NULL;
>  	const struct dmi_system_id *id;
> -	long quirks = 0;
>  
>  	id = dmi_first_match(gpiolib_acpi_quirks);
>  	if (id)
> -		quirks = (long)id->driver_data;
> +		quirk = id->driver_data;
>  
>  	if (run_edge_events_on_boot < 0) {
> -		if (quirks & QUIRK_NO_EDGE_EVENTS_ON_BOOT)
> +		if (quirk && quirk->no_edge_events_on_boot)
>  			run_edge_events_on_boot = 0;
>  		else
>  			run_edge_events_on_boot = 1;
>  	}
>  
> -	if (honor_wakeup < 0) {
> -		if (quirks & QUIRK_NO_WAKEUP)
> -			honor_wakeup = 0;
> -		else
> -			honor_wakeup = 1;
> -	}
> +	if (ignore_wake == NULL && quirk && quirk->ignore_wake)
> +		ignore_wake = quirk->ignore_wake;
>  
>  	return 0;
>  }
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] gpiolib: acpi: Correct comment for HP x2 10 honor_wakeup quirk

[Linus Walleij]

On Mon, Mar 2, 2020 at 12:12 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") added a quirk for some models of the HP x2 10 series.
>
> There are 2 issues with the comment describing the quirk:
> 1) The comment claims the DMI quirk applies to all Cherry Trail based HP x2
>    10 models. In the mean time I have learned that there are at least 3
>    models of the HP x2 10 models:
>
>    Bay Trail SoC + AXP288 PMIC
>    Cherry Trail SoC + AXP288 PMIC
>    Cherry Trail SoC + TI PMIC
>
>    And this quirk's DMI matches only match the Cherry Trail SoC + TI PMIC
>    SoC, which is good because we want a slightly different quirk for the
>    others. This commit updates the comment to make it clear that the quirk
>    is only for the Cherry Trail SoC + TI PMIC models.
>
> 2) The comment says that it is ok to disable wakeup on all ACPI GPIO event
>    handlers, because there is only the one for the embedded-controller
>    events. This is not true, there also is a handler for the special
>    INT0002 device which is related to USB wakeups. We need to also disable
>    wakeups on that one because the device turns of the USB-keyboard built
>    into the dock when closing the lid. The XHCI controller takes a while
>    to notice this, so it only notices it when already suspended, causing
>    a spurious wakeup because of this. So disabling wakeup on all handlers
>    is the right thing to do, but not because there only is the one handler
>    for the EC events. This commit updates the comment to correctly reflect
>    this.
>
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v2 2/4] gpiolib: acpi: Rework honor_wakeup option into an ignore_wake option

[Linus Walleij]

On Mon, Mar 2, 2020 at 12:12 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series.
>
> The approach taken there was to add a bool controlling wakeup support for
> all ACPI GPIO events. This was sufficient for the specific HP x2 10 model
> the commit was trying to fix, but in the mean time other models have
> turned up which need a similar workaround to avoid spurious wakeups from
> suspend, but only for one of the pins on which the ACPI tables request
> ACPI GPIO events.
>
> Since the honor_wakeup option was added to be able to ignore wake events,
> the name was perhaps not the best, this commit renames it to ignore_wake
> and changes it to a string with the following format:
> gpiolib_acpi.ignore_wake=controller@pin[,controller@pin[,...]]
>
> This allows working around spurious wakeup issues on a per pin basis.
>
> This commit also reworks the existing quirk for the HP x2 10 so that
> it functions as before.
>
> Note:
> -This removes the honor_wakeup parameter. This has only been upstream for
>  a short time and to the best of my knowledge there are no users using
>  this module parameter.
>
> -The controller@pin[,controller@pin[,...]] syntax is based on an existing
>  kernel module parameter using the same controller@pin format. That version
>  uses ';' as separator, but in practice that is problematic because grub2
>  cannot handle this without taking special care to escape the ';', so here
>  we are using a ',' as separator instead which does not have this issue.
>
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Rewrite to use a much more generic string based format to describe
>  the controller@pin combination(s) on which to ignore wakeups

Patch applied with the ACKs.

Yours,
Linus Walleij

Re: [PATCH v2 3/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 BYT + AXP288 model

[Linus Walleij]

On Mon, Mar 2, 2020 at 12:12 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Commit aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option +
> quirk mechanism") was added to deal with spurious wakeups on one specific
> model of the HP x2 10 series. In the mean time I have learned that there
> are at least 3 different HP x2 10 models:
>
> Bay Trail SoC + AXP288 PMIC
> Cherry Trail SoC + AXP288 PMIC
> Cherry Trail SoC + TI PMIC
>
> And the original quirk is only correct for (and only matches the)
> Cherry Trail SoC + TI PMIC model.
>
> The Bay Trail SoC + AXP288 PMIC model has different DMI strings, has
> the external EC interrupt on a different GPIO pin and only needs to ignore
> wakeups on the EC interrupt, the INT0002 device works fine on this model.
>
> This commit adds an extra DMI based quirk for the HP x2 10 BYT + AXP288
> model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
> on this model. This fixes spurious wakeups from suspend on this model.
>
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mechanism")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Split out new quirk addition into its own patch

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] gpiolib: acpi: Add quirk to ignore EC wakeups on HP x2 10 CHT + AXP288 model

[Linus Walleij]

On Mon, Mar 2, 2020 at 12:12 PM Hans de Goede <hdegoede@redhat.com> wrote:

> There are at least 3 models of the HP x2 10 models:
>
> Bay Trail SoC + AXP288 PMIC
> Cherry Trail SoC + AXP288 PMIC
> Cherry Trail SoC + TI PMIC
>
> Like on the other HP x2 10 models we need to ignore wakeup for ACPI GPIO
> events on the external embedded-controller pin to avoid spurious wakeups
> on the HP x2 10 CHT + AXP288 model too.
>
> This commit adds an extra DMI based quirk for the HP x2 10 CHT + AXP288
> model, ignoring wakeups for ACPI GPIO events on the EC interrupt pin
> on this model. This fixes spurious wakeups from suspend on this model.
>
> Fixes: aa23ca3d98f7 ("gpiolib: acpi: Add honor_wakeup module-option + quirk mech
> Reported-and-tested-by: Marc Lehmann <schmorp@schmorp.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij