[PATCH resend 1/3] gpiolib: acpi: ignore-wakeup handling rework

[PATCH resend 1/3] gpiolib: acpi: ignore-wakeup handling rework

[Hans de Goede]

Hi All,

The first patch just updates the comment describing why we are ignoring
GPIO ACPI event wakeups on HP x2 10 models.

The second patch is more interesting, in the mean time I've learned their
are actually at least 3 variants of the HP x2 10, and the original quirk
only applies to the Cherry Trail with TI PMIC variant (and the original
DMI match only matches that model). We need a similar quirk for the
Bay Trail with AXP288 model, but there we only want to ignore the wakeups
for the GPIO ACPI event which is (ab)used for embedded-controller events
on this model while still honoring the wakeup flags on other pins.

I'm not 100% happy with the solution I've come up with to allow ignoring
events on a single pin. But this was the best KISS thing I could come up
with. Alternatives would involve string parsing (*), which I would rather
avoid. I'm very much open to alternatives for the current approach in the
second patch.

Since sending out the first 2 patches of this series I've received
positive testing feedback for the quirk for the HP X2 10 Cherry Trail +
AXP288 PMIC variant, so here is a resend of the first 2 patches with
a third patch adding a quirk for the third variant of HP X2 10 added.

Regards,

Hans


*) And more complex DMI quirk handling since now we would need to store
a string + some other flags in the DMI driver_data

[PATCH resend 1/3] 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
   variants 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 variants.

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 31fee5e918b7..bc96f28d4807 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1345,11 +1345,16 @@ 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).
+		 * HP X2 10 models with Cherry Trail SoC + TI PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI
+		 * GPIO event handler. The embedded controller generates
+		 * various spurious wakeup events when suspended.
+		 * 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.
+		 * These are the only 2 ACPI event handlers, so we disable
+		 * wakeups for all event handlers to fix the spurious wakeups.
 		 * 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.
-- 
2.25.1

[PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[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 variants of the HP x2 10 models:

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

It turns out that the need to ignore wakeup on *all* ACPI GPIO event
handlers is unique to the Cherry Trail SoC + TI PMIC variant for which
the first quirk was added.

The 2 variants with the AXP288 PMIC only need to have wakeup disabled on
the embedded-controller event handler. We want to e.g. keep wakeup on the
event handler connected to the GPIO for the lid open/closed sensor.

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,
this version of the option has te following possible values:

values >= 0: a pin number on which to ignore wakeups, the ACPI wake flag
will still be honored on all other pins
value -1: auto: check for DMI quirk, otherwise honor the flag on all pins
value -2: all:  ignore the flag on all pins
value -3: none: honor wakeups on all pins

Note that it is possible for an ACPI table to request events on the same
pin-number on multiple GPIO controllers, in that case if such a pin-number
is used as ignore_wake value then wakeups will be ignored for that pin on
all GPIO controllers.

The existing quirk for the Cherry Trail + TI PMIC models is changed to
IGNORE_WAKE_ALL, keeping the current behavior; and a new quirk is added
for the Bay Trail + AXP288 model, ignoring wakeups on the EC GPIO pin only.

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 | 59 +++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index bc96f28d4807..83103efa5862 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -21,19 +21,27 @@
 #include "gpiolib.h"
 #include "gpiolib-acpi.h"
 
-#define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
-#define QUIRK_NO_WAKEUP				0x02l
+#define QUIRK_IGNORE_WAKE_MASK			GENMASK(15, 0)
+#define QUIRK_IGNORE_WAKE_SET			BIT(16)
+#define QUIRK_NO_EDGE_EVENTS_ON_BOOT		BIT(17)
+
+#define QUIRK_IGNORE_WAKE(x) \
+	(((x) & QUIRK_IGNORE_WAKE_MASK) | QUIRK_IGNORE_WAKE_SET)
+
+#define IGNORE_WAKE_AUTO			-1
+#define IGNORE_WAKE_ALL				-2
+#define IGNORE_WAKE_NONE			-3
+
+static int ignore_wake = IGNORE_WAKE_AUTO;
+module_param(ignore_wake, int, 0444);
+MODULE_PARM_DESC(ignore_wake,
+	"Ignore ACPI wake flag: x=ignore-for-pin-x, -1=auto, -2=all, -3=none");
 
 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
  *
@@ -214,6 +222,7 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 	irq_handler_t handler = NULL;
 	struct gpio_desc *desc;
 	int ret, pin, irq;
+	bool honor_wakeup;
 
 	if (!acpi_gpio_get_irq_resource(ares, &agpio))
 		return AE_OK;
@@ -286,6 +295,17 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		}
 	}
 
+	switch (ignore_wake) {
+	case IGNORE_WAKE_ALL:
+		honor_wakeup = false;
+		break;
+	case IGNORE_WAKE_NONE:
+		honor_wakeup = true;
+		break;
+	default:
+		honor_wakeup = ignore_wake != pin;
+	}
+
 	event->handle = evt_handle;
 	event->handler = handler;
 	event->irq = irq;
@@ -1363,7 +1383,22 @@ 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 = (void *)QUIRK_IGNORE_WAKE(IGNORE_WAKE_ALL),
+	},
+	{
+		/*
+		 * HP X2 10 models with Bay Trail SoC + AXP288 PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI
+		 * GPIO event handler for pin 0x1c, causing spurious wakeups.
+		 * Unlike the Cherry Trail + TI PMIC models, we do want to
+		 * honor the ACPI wake flag on the other GPIOs.
+		 */
+		.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 = (void *)QUIRK_IGNORE_WAKE(0x1c),
 	},
 	{} /* Terminating entry */
 };
@@ -1384,11 +1419,11 @@ 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;
+	if (ignore_wake == IGNORE_WAKE_AUTO) {
+		if (quirks & QUIRK_IGNORE_WAKE_SET)
+			ignore_wake = (s16)(quirks & QUIRK_IGNORE_WAKE_MASK);
 		else
-			honor_wakeup = 1;
+			ignore_wake = IGNORE_WAKE_NONE;
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Tue, Feb 25, 2020 at 11:27:52AM +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 variants of the HP x2 10 models:
> 
> Bay Trail SoC + AXP288 PMIC
> Cherry Trail SoC + AXP288 PMIC
> Cherry Trail SoC + TI PMIC
> 
> It turns out that the need to ignore wakeup on *all* ACPI GPIO event
> handlers is unique to the Cherry Trail SoC + TI PMIC variant for which
> the first quirk was added.
> 
> The 2 variants with the AXP288 PMIC only need to have wakeup disabled on
> the embedded-controller event handler. We want to e.g. keep wakeup on the
> event handler connected to the GPIO for the lid open/closed sensor.
> 
> 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,
> this version of the option has te following possible values:
> 
> values >= 0: a pin number on which to ignore wakeups, the ACPI wake flag
> will still be honored on all other pins
> value -1: auto: check for DMI quirk, otherwise honor the flag on all pins
> value -2: all:  ignore the flag on all pins
> value -3: none: honor wakeups on all pins
> 
> Note that it is possible for an ACPI table to request events on the same
> pin-number on multiple GPIO controllers, in that case if such a pin-number
> is used as ignore_wake value then wakeups will be ignored for that pin on
> all GPIO controllers.
> 
> The existing quirk for the Cherry Trail + TI PMIC models is changed to
> IGNORE_WAKE_ALL, keeping the current behavior; and a new quirk is added
> for the Bay Trail + AXP288 model, ignoring wakeups on the EC GPIO pin only.

In general I'm fine with this, but looking to the history of your changes I'm
afraid that in future it will require more than one pin to be listed or
something like this.

...

> +static int ignore_wake = IGNORE_WAKE_AUTO;
> +module_param(ignore_wake, int, 0444);
> +MODULE_PARM_DESC(ignore_wake,
> +	"Ignore ACPI wake flag: x=ignore-for-pin-x, -1=auto, -2=all, -3=none");

Perhaps we may take list of pins or a bitmap (see bitmap list parsers API).

...

> -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");

Isn't it now a part of ABI? I don't think we may remove it, though we may ignore it.
Or do something else.

(One of the reasons why I hate module parameters)

> +			ignore_wake = (s16)(quirks & QUIRK_IGNORE_WAKE_MASK);

It's casted to signed because ..?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Hans de Goede]

Hi,

Thank you for looking at this.

On 2/25/20 11:54 AM, Andy Shevchenko wrote:
> On Tue, Feb 25, 2020 at 11:27:52AM +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 variants of the HP x2 10 models:
>>
>> Bay Trail SoC + AXP288 PMIC
>> Cherry Trail SoC + AXP288 PMIC
>> Cherry Trail SoC + TI PMIC
>>
>> It turns out that the need to ignore wakeup on *all* ACPI GPIO event
>> handlers is unique to the Cherry Trail SoC + TI PMIC variant for which
>> the first quirk was added.
>>
>> The 2 variants with the AXP288 PMIC only need to have wakeup disabled on
>> the embedded-controller event handler. We want to e.g. keep wakeup on the
>> event handler connected to the GPIO for the lid open/closed sensor.
>>
>> 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,
>> this version of the option has te following possible values:
>>
>> values >= 0: a pin number on which to ignore wakeups, the ACPI wake flag
>> will still be honored on all other pins
>> value -1: auto: check for DMI quirk, otherwise honor the flag on all pins
>> value -2: all:  ignore the flag on all pins
>> value -3: none: honor wakeups on all pins
>>
>> Note that it is possible for an ACPI table to request events on the same
>> pin-number on multiple GPIO controllers, in that case if such a pin-number
>> is used as ignore_wake value then wakeups will be ignored for that pin on
>> all GPIO controllers.
>>
>> The existing quirk for the Cherry Trail + TI PMIC models is changed to
>> IGNORE_WAKE_ALL, keeping the current behavior; and a new quirk is added
>> for the Bay Trail + AXP288 model, ignoring wakeups on the EC GPIO pin only.
> 
> In general I'm fine with this, but looking to the history of your changes I'm
> afraid that in future it will require more than one pin to be listed or
> something like this.

The only models which need this so far are the weird HP X2 models which
use an external embedded controller with the tablet version of BYT / CHT
which is just al sorts of hacked together. Also see:

https://lore.kernel.org/stable/20200223153208.312005-1-hdegoede@redhat.com/T/#u

For other parts of the same device which also rather "hacked together"
HP made these models really really interesting...

With that said I cannot guarantee that we won't need something similar
for some other botched-up device.

> ...
> 
>> +static int ignore_wake = IGNORE_WAKE_AUTO;
>> +module_param(ignore_wake, int, 0444);
>> +MODULE_PARM_DESC(ignore_wake,
>> +	"Ignore ACPI wake flag: x=ignore-for-pin-x, -1=auto, -2=all, -3=none");
> 
> Perhaps we may take list of pins or a bitmap (see bitmap list parsers API).

I guess you mean bitmap_parse_user / bitmap_print_to_pagebuf, the problem
is that for a more generic solution we need a wat to specify the
GPIO controller + the pin, so we would get a list of <name>,<pin> pairs
and then need to parse that, e.g. :

	gpiolib_acpi.ignore_wake=INT33FC:00,0x1c;INT33FC:01;0x12

I agree that if we really want to future proof this that then this is
the way we should go. This does mean adding a bunch of extra code for
parsing this, but I guess that would be better then my current hack.

Please let me know if you prefer going this route then I will respin
the patches to work this way.

> ...
> 
>> -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");
> 
> Isn't it now a part of ABI? I don't think we may remove it, though we may ignore it.
> Or do something else.
> 
> (One of the reasons why I hate module parameters)

I personally do not subscribe to the module parameters are part of the kernel ABI
crowd. I do not think Linus has ever stated something like that ?  For long existing
often used module parameters treating them as such makes a ton of sense, but this
one is quite new and AFAIK almost nobody is using it. So my vote would be to just
drop it. If we get push back we can easily restore it in some form.

> 
>> +			ignore_wake = (s16)(quirks & QUIRK_IGNORE_WAKE_MASK);
> 
> It's casted to signed because ..?

The high 32 bits of the quirk field is used for flags, and ignore_wake can
have a special negative value, so we need to sign extend the value stored
in the lower 16 bits of the quirk in case it is a negative value such as
IGNORE_WAKE_ALL.

Note this ugliness would go away if we switch to the string format for
the module param. Then the driver_data in the dmi matches would point
to a struct with a separate unsigned long flags field and a const char
*ignore_wake field...

Regards,

Hans

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> On 2/25/20 11:54 AM, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2020 at 11:27:52AM +0100, Hans de Goede wrote:

...

> > In general I'm fine with this, but looking to the history of your changes I'm
> > afraid that in future it will require more than one pin to be listed or
> > something like this.
> 
> The only models which need this so far are the weird HP X2 models which
> use an external embedded controller with the tablet version of BYT / CHT
> which is just al sorts of hacked together. Also see:
> 
> https://lore.kernel.org/stable/20200223153208.312005-1-hdegoede@redhat.com/T/#u
> 
> For other parts of the same device which also rather "hacked together"
> HP made these models really really interesting...
> 
> With that said I cannot guarantee that we won't need something similar
> for some other botched-up device.

...

> > > +static int ignore_wake = IGNORE_WAKE_AUTO;
> > > +module_param(ignore_wake, int, 0444);
> > > +MODULE_PARM_DESC(ignore_wake,
> > > +	"Ignore ACPI wake flag: x=ignore-for-pin-x, -1=auto, -2=all, -3=none");
> > 
> > Perhaps we may take list of pins or a bitmap (see bitmap list parsers API).
> 
> I guess you mean bitmap_parse_user / bitmap_print_to_pagebuf, the problem
> is that for a more generic solution we need a wat to specify the
> GPIO controller + the pin, so we would get a list of <name>,<pin> pairs
> and then need to parse that, e.g. :
> 
> 	gpiolib_acpi.ignore_wake=INT33FC:00,0x1c;INT33FC:01;0x12
> 
> I agree that if we really want to future proof this that then this is
> the way we should go. This does mean adding a bunch of extra code for
> parsing this, but I guess that would be better then my current hack.
> 
> Please let me know if you prefer going this route then I will respin
> the patches to work this way.

Let's do it as a list of pairs, but in slightly different format (I see some
potential to derive a generic parser, based on users described in
Documentation/admin-guide/kernel-parameters.txt), i.e.

	ignore_wake=pin:controller[,pin:controller[,...]]

...

> > > -MODULE_PARM_DESC(honor_wakeup,
> > > -		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");

> > Isn't it now a part of ABI? I don't think we may remove it, though we may ignore it.
> > Or do something else.
> > 
> > (One of the reasons why I hate module parameters)
> 
> I personally do not subscribe to the module parameters are part of the kernel ABI
> crowd. I do not think Linus has ever stated something like that ?  For long existing
> often used module parameters treating them as such makes a ton of sense, but this
> one is quite new and AFAIK almost nobody is using it. So my vote would be to just
> drop it. If we get push back we can easily restore it in some form.

I'm fine with dropping it, perhaps add a phrase to the commit message about
(safe) removal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:

> Let's do it as a list of pairs, but in slightly different format (I see some
> potential to derive a generic parser, based on users described in
> Documentation/admin-guide/kernel-parameters.txt), i.e.
> 
> 	ignore_wake=pin:controller[,pin:controller[,...]]

Another possible format

	ignore_wake=controller@pin[;controller@pin[;...]]

The second one, while having less users, can be extended to have list of pins
of the same controller, like

	ignore_wake=controller@pin[:pin2:pin3][;controller@pin[:...][;...]]

(colon to match existing user, but can be, of course, changed)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Hans de Goede]

Hi,

On 2/25/20 1:57 PM, Andy Shevchenko wrote:
> On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
>> On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> 
>> Let's do it as a list of pairs, but in slightly different format (I see some
>> potential to derive a generic parser, based on users described in
>> Documentation/admin-guide/kernel-parameters.txt), i.e.
>>
>> 	ignore_wake=pin:controller[,pin:controller[,...]]
> 
> Another possible format
> 
> 	ignore_wake=controller@pin[;controller@pin[;...]]

I like this one, the other one with the pin first feels wrong, the pin is
part of the controller, not the other way around.

I will rework the patch series to use the ignore_wake=controller@pin format.

> The second one, while having less users, can be extended to have list of pins
> of the same controller, like
> 
> 	ignore_wake=controller@pin[:pin2:pin3][;controller@pin[:...][;...]]

I don't really see a need for this, the controller name can be repeated
in the unlikely case where more then one pin needs to be blacklisted
from wakeup and I would like to keep the parsing as KISS as possible.

I guess we can always add this later if people feel like adding it.

Regards,

Hans

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Fri, Feb 28, 2020 at 12:22:45PM +0100, Hans de Goede wrote:
> On 2/25/20 1:57 PM, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
> > > On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> > 
> > > Let's do it as a list of pairs, but in slightly different format (I see some
> > > potential to derive a generic parser, based on users described in
> > > Documentation/admin-guide/kernel-parameters.txt), i.e.
> > > 
> > > 	ignore_wake=pin:controller[,pin:controller[,...]]
> > 
> > Another possible format
> > 
> > 	ignore_wake=controller@pin[;controller@pin[;...]]
> 
> I like this one, the other one with the pin first feels wrong, the pin is
> part of the controller, not the other way around.

Yes, I agree.

> I will rework the patch series to use the ignore_wake=controller@pin format.
> 
> > The second one, while having less users, can be extended to have list of pins
> > of the same controller, like
> > 
> > 	ignore_wake=controller@pin[:pin2:pin3][;controller@pin[:...][;...]]
> 
> I don't really see a need for this, the controller name can be repeated
> in the unlikely case where more then one pin needs to be blacklisted
> from wakeup and I would like to keep the parsing as KISS as possible.
> 
> I guess we can always add this later if people feel like adding it.

Right.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Hans de Goede]

Hi,

On 2/28/20 12:22 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/25/20 1:57 PM, Andy Shevchenko wrote:
>> On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
>>
>>> Let's do it as a list of pairs, but in slightly different format (I see some
>>> potential to derive a generic parser, based on users described in
>>> Documentation/admin-guide/kernel-parameters.txt), i.e.
>>>
>>>     ignore_wake=pin:controller[,pin:controller[,...]]
>>
>> Another possible format
>>
>>     ignore_wake=controller@pin[;controller@pin[;...]]
> 
> I like this one, the other one with the pin first feels wrong, the pin is
> part of the controller, not the other way around.
> 
> I will rework the patch series to use the ignore_wake=controller@pin format.

Just a quick note. I've changed the separator from ; to , for some reason
grub, at least as used in Fedora with Fedora's grub2 BLS (boot loader spec)
implementation does not like it when there is a ; in the kernel commandline.

I will also send an email about this to Fedora grub maintainer, but for
now it is easiest to just avoid the problem.

Regards,

Hans

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Sat, Feb 29, 2020 at 09:57:52PM +0100, Hans de Goede wrote:
> On 2/28/20 12:22 PM, Hans de Goede wrote:
> > On 2/25/20 1:57 PM, Andy Shevchenko wrote:
> > > On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> > > 
> > > > Let's do it as a list of pairs, but in slightly different format (I see some
> > > > potential to derive a generic parser, based on users described in
> > > > Documentation/admin-guide/kernel-parameters.txt), i.e.
> > > > 
> > > >     ignore_wake=pin:controller[,pin:controller[,...]]
> > > 
> > > Another possible format
> > > 
> > >     ignore_wake=controller@pin[;controller@pin[;...]]
> > 
> > I like this one, the other one with the pin first feels wrong, the pin is
> > part of the controller, not the other way around.
> > 
> > I will rework the patch series to use the ignore_wake=controller@pin format.
> 
> Just a quick note. I've changed the separator from ; to , for some reason
> grub, at least as used in Fedora with Fedora's grub2 BLS (boot loader spec)
> implementation does not like it when there is a ; in the kernel commandline.

Hmm... I think it would be harder then to have less possible formats in the
command line. Do you really need right now several pins to be listed?

If it's about testing, perhaps we may do it with other means.

> I will also send an email about this to Fedora grub maintainer, but for
> now it is easiest to just avoid the problem.

It's definitely bug in Grub due to existing kernel users with such format.
It means Grub is unable to support kernel command line in full.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Hans de Goede]

Hi,

On 3/2/20 10:30 AM, Andy Shevchenko wrote:
> On Sat, Feb 29, 2020 at 09:57:52PM +0100, Hans de Goede wrote:
>> On 2/28/20 12:22 PM, Hans de Goede wrote:
>>> On 2/25/20 1:57 PM, Andy Shevchenko wrote:
>>>> On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
>>>>
>>>>> Let's do it as a list of pairs, but in slightly different format (I see some
>>>>> potential to derive a generic parser, based on users described in
>>>>> Documentation/admin-guide/kernel-parameters.txt), i.e.
>>>>>
>>>>>      ignore_wake=pin:controller[,pin:controller[,...]]
>>>>
>>>> Another possible format
>>>>
>>>>      ignore_wake=controller@pin[;controller@pin[;...]]
>>>
>>> I like this one, the other one with the pin first feels wrong, the pin is
>>> part of the controller, not the other way around.
>>>
>>> I will rework the patch series to use the ignore_wake=controller@pin format.
>>
>> Just a quick note. I've changed the separator from ; to , for some reason
>> grub, at least as used in Fedora with Fedora's grub2 BLS (boot loader spec)
>> implementation does not like it when there is a ; in the kernel commandline.
> 
> Hmm... I think it would be harder then to have less possible formats in the
> command line. Do you really need right now several pins to be listed?

Yes, the existing quirk for the HP X2 10 with Cherry Trail SoC + TI PMIC,
which currently ignores wakeups on all pins needs to ignore wakeup on 2 pins.

> If it's about testing, perhaps we may do it with other means.

Well it is possible to pass the ; by putting quotes around it, so we could
go with the ; if you insist, but it really makes life harder for

>> I will also send an email about this to Fedora grub maintainer, but for
>> now it is easiest to just avoid the problem.
> 
> It's definitely bug in Grub due to existing kernel users with such format.
> It means Grub is unable to support kernel command line in full.

So I discussed this with the Fedora Grub maintainer, he says the problem
exists in upstream grub2 too, grub2 uses a shell like command syntax
both in its config file and in interactive mode, so if you do e.g.:

linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake=INT33FF:01@0;INT0002:00@2

Then grub will see the INT0002:00@2 as a new separate commaond, this should
work:

linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake="INT33FF:01@0;INT0002:00@2"

But the recommended way to edit the cmdline is by editing /etc/default/grub and
then re-running grub2-mkconfig, which clears the quotes unless we escape them
and since grub2-mkconfig is shell script inside shell script inside shell script
I don't even want to think about how many times I need to escape the quotes.

TL;DR: Using ; in kernel commandline options makes life much harder for users
and as such is something which we should try to avoid.

I appreciate that you are trying to come up with a format for the option which
looks like existing options and I like the @ use, but using ; really is not a
good example to follow and IMHO that (not a good example / idea) trumps keeping
the syntax identical to an existing option.

Regards,

Hans

Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko]

On Mon, Mar 02, 2020 at 10:46:57AM +0100, Hans de Goede wrote:
> On 3/2/20 10:30 AM, Andy Shevchenko wrote:
> > On Sat, Feb 29, 2020 at 09:57:52PM +0100, Hans de Goede wrote:
> > > On 2/28/20 12:22 PM, Hans de Goede wrote:
> > > > On 2/25/20 1:57 PM, Andy Shevchenko wrote:
> > > > > On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> > > > > 
> > > > > > Let's do it as a list of pairs, but in slightly different format (I see some
> > > > > > potential to derive a generic parser, based on users described in
> > > > > > Documentation/admin-guide/kernel-parameters.txt), i.e.
> > > > > > 
> > > > > >      ignore_wake=pin:controller[,pin:controller[,...]]
> > > > > 
> > > > > Another possible format
> > > > > 
> > > > >      ignore_wake=controller@pin[;controller@pin[;...]]
> > > > 
> > > > I like this one, the other one with the pin first feels wrong, the pin is
> > > > part of the controller, not the other way around.
> > > > 
> > > > I will rework the patch series to use the ignore_wake=controller@pin format.
> > > 
> > > Just a quick note. I've changed the separator from ; to , for some reason
> > > grub, at least as used in Fedora with Fedora's grub2 BLS (boot loader spec)
> > > implementation does not like it when there is a ; in the kernel commandline.
> > 
> > Hmm... I think it would be harder then to have less possible formats in the
> > command line. Do you really need right now several pins to be listed?
> 
> Yes, the existing quirk for the HP X2 10 with Cherry Trail SoC + TI PMIC,
> which currently ignores wakeups on all pins needs to ignore wakeup on 2 pins.

Ouch.

> > If it's about testing, perhaps we may do it with other means.
> 
> Well it is possible to pass the ; by putting quotes around it, so we could
> go with the ; if you insist, but it really makes life harder for
> 
> > > I will also send an email about this to Fedora grub maintainer, but for
> > > now it is easiest to just avoid the problem.
> > 
> > It's definitely bug in Grub due to existing kernel users with such format.
> > It means Grub is unable to support kernel command line in full.
> 
> So I discussed this with the Fedora Grub maintainer, he says the problem
> exists in upstream grub2 too, grub2 uses a shell like command syntax
> both in its config file and in interactive mode, so if you do e.g.:
> 
> linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake=INT33FF:01@0;INT0002:00@2
> 
> Then grub will see the INT0002:00@2 as a new separate commaond, this should
> work:
> 
> linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake="INT33FF:01@0;INT0002:00@2"
> 
> But the recommended way to edit the cmdline is by editing /etc/default/grub and
> then re-running grub2-mkconfig, which clears the quotes unless we escape them
> and since grub2-mkconfig is shell script inside shell script inside shell script
> I don't even want to think about how many times I need to escape the quotes.

So, bug is still there...

> TL;DR: Using ; in kernel commandline options makes life much harder for users
> and as such is something which we should try to avoid.
> 
> I appreciate that you are trying to come up with a format for the option which
> looks like existing options and I like the @ use, but using ; really is not a
> good example to follow and IMHO that (not a good example / idea) trumps keeping
> the syntax identical to an existing option.

I see. Since we have to fix real problem, go ahead with comma, but please put
few words in the commit message why this format is being chosen.

-- 
With Best Regards,
Andy Shevchenko

[PATCH resend 3/3] gpiolib: acpi: Add quirk to ignore EC gpio wakeups for 1 more HP x2 10 model

[Hans de Goede]

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

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

Like on the Bay Trail + AXP288 PMIC model, we also need to ignore wakeups
for the GPIO which is (ab)used for embedded-controller events on the
Cherry Trail + AXP288 PMIC 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 83103efa5862..46b2d74c610c 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1400,6 +1400,21 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 		},
 		.driver_data = (void *)QUIRK_IGNORE_WAKE(0x1c),
 	},
+	{
+		/*
+		 * HP X2 10 models with Cherry Trail SoC + AXP288 PMIC use an
+		 * external embedded-controller connected via I2C + an ACPI
+		 * GPIO event handler for pin 0x00, causing spurious wakeups.
+		 * Unlike the Cherry Trail + TI PMIC models, we do want to
+		 * honor the ACPI wake flag on the other GPIOs.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion x2 Detachable"),
+			DMI_MATCH(DMI_BOARD_NAME, "813E"),
+		},
+		.driver_data = (void *)QUIRK_IGNORE_WAKE(0x00),
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.25.1

Re: [PATCH resend 1/3] gpiolib: acpi: ignore-wakeup handling rework

[Hans de Goede]

Subject for this one should be:

"[PATCH resend 0/3] gpiolib: acpi: ignore-wakeup handling rework"

As it is the cover letter, sorry about that.




On 2/25/20 11:27 AM, Hans de Goede wrote:
> Hi All,
> 
> The first patch just updates the comment describing why we are ignoring
> GPIO ACPI event wakeups on HP x2 10 models.
> 
> The second patch is more interesting, in the mean time I've learned their
> are actually at least 3 variants of the HP x2 10, and the original quirk
> only applies to the Cherry Trail with TI PMIC variant (and the original
> DMI match only matches that model). We need a similar quirk for the
> Bay Trail with AXP288 model, but there we only want to ignore the wakeups
> for the GPIO ACPI event which is (ab)used for embedded-controller events
> on this model while still honoring the wakeup flags on other pins.
> 
> I'm not 100% happy with the solution I've come up with to allow ignoring
> events on a single pin. But this was the best KISS thing I could come up
> with. Alternatives would involve string parsing (*), which I would rather
> avoid. I'm very much open to alternatives for the current approach in the
> second patch.
> 
> Since sending out the first 2 patches of this series I've received
> positive testing feedback for the quirk for the HP X2 10 Cherry Trail +
> AXP288 PMIC variant, so here is a resend of the first 2 patches with
> a third patch adding a quirk for the third variant of HP X2 10 added.
> 
> Regards,
> 
> Hans
> 
> 
> *) And more complex DMI quirk handling since now we would need to store
> a string + some other flags in the DMI driver_data
> 
> 

Re: [PATCH resend 1/3] gpiolib: acpi: ignore-wakeup handling rework

[Linus Walleij]

On Tue, Feb 25, 2020 at 11:28 AM Hans de Goede <hdegoede@redhat.com> wrote:

> The first patch just updates the comment describing why we are ignoring
> GPIO ACPI event wakeups on HP x2 10 models.

OK

> The second patch is more interesting, in the mean time I've learned their
> are actually at least 3 variants of the HP x2 10, and the original quirk
> only applies to the Cherry Trail with TI PMIC variant (and the original
> DMI match only matches that model). We need a similar quirk for the
> Bay Trail with AXP288 model, but there we only want to ignore the wakeups
> for the GPIO ACPI event which is (ab)used for embedded-controller events
> on this model while still honoring the wakeup flags on other pins.
>
> I'm not 100% happy with the solution I've come up with to allow ignoring
> events on a single pin. But this was the best KISS thing I could come up
> with. Alternatives would involve string parsing (*), which I would rather
> avoid. I'm very much open to alternatives for the current approach in the
> second patch.
>
> Since sending out the first 2 patches of this series I've received
> positive testing feedback for the quirk for the HP X2 10 Cherry Trail +
> AXP288 PMIC variant, so here is a resend of the first 2 patches with
> a third patch adding a quirk for the third variant of HP X2 10 added.

I'm waiting for some ACPI person to say yes to this,
Mika ideally but the other Intel guys like Andy also works :)

Yours,
Linus Walleij

Re: [PATCH resend 1/3] gpiolib: acpi: ignore-wakeup handling rework

[Hans de Goede]

Hi Linus,

On 2/28/20 11:54 PM, Linus Walleij wrote:
> On Tue, Feb 25, 2020 at 11:28 AM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> The first patch just updates the comment describing why we are ignoring
>> GPIO ACPI event wakeups on HP x2 10 models.
> 
> OK
> 
>> The second patch is more interesting, in the mean time I've learned their
>> are actually at least 3 variants of the HP x2 10, and the original quirk
>> only applies to the Cherry Trail with TI PMIC variant (and the original
>> DMI match only matches that model). We need a similar quirk for the
>> Bay Trail with AXP288 model, but there we only want to ignore the wakeups
>> for the GPIO ACPI event which is (ab)used for embedded-controller events
>> on this model while still honoring the wakeup flags on other pins.
>>
>> I'm not 100% happy with the solution I've come up with to allow ignoring
>> events on a single pin. But this was the best KISS thing I could come up
>> with. Alternatives would involve string parsing (*), which I would rather
>> avoid. I'm very much open to alternatives for the current approach in the
>> second patch.
>>
>> Since sending out the first 2 patches of this series I've received
>> positive testing feedback for the quirk for the HP X2 10 Cherry Trail +
>> AXP288 PMIC variant, so here is a resend of the first 2 patches with
>> a third patch adding a quirk for the third variant of HP X2 10 added.
> 
> I'm waiting for some ACPI person to say yes to this,
> Mika ideally but the other Intel guys like Andy also works :)

In the mean time Anday has reviewed the series and as somewhat expected
he did not like the second patch very much. I'm working on an updated version
fixing Andy's concerns.

Regards,

Hans