[PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist

[PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist

[Hans de Goede]

Another day; another DSDT bug we need to workaround...

Since commit ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events
at least once on boot") we call _AEI edge handlers at boot.

In some rare cases this causes problems. One example of this is the Minix
Neo Z83-4 mini PC, this device has a clear DSDT bug where it has some copy
and pasted code for dealing with Micro USB-B connector host/device role
switching, while the mini PC does not even have a micro-USB connector.
This code, which should not be there, messes with the DDC data pin from
the HDMI connector (switching it to GPIO mode) breaking HDMI support.

To avoid problems like this, this commit adds a new
gpiolib_acpi_run_edge_events_on_boot kernel commandline option which
can be "on", "off", or "auto" (default).

In auto mode the default is on and a DMI based blacklist is used,
the initial version of this blacklist contains the Minix Neo Z83-4
fixing the HDMI being broken on this device.

Cc: stable@vger.kernel.org
Cc: Daniel Drake <drake@endlessm.com>
Cc: Ian W MORRISON <ianwmorrison@gmail.com>
Reported-by: Ian W MORRISON <ianwmorrison@gmail.com>
Suggested-by: Ian W MORRISON <ianwmorrison@gmail.com>
Fixes: ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events at least once on boot")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 52 ++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 39f2f9035c11..546dc2c1f3f1 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -7,6 +7,7 @@
  *          Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
+#include <linux/dmi.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
@@ -19,6 +20,23 @@
 
 #include "gpiolib.h"
 
+static int gpiolib_acpi_run_edge_events_on_boot = -1;
+
+static int __init gpiolib_acpi_run_edge_events_on_boot_setup(char *arg)
+{
+	if (!strcmp(arg, "on"))
+		gpiolib_acpi_run_edge_events_on_boot = 1;
+	else if (!strcmp(arg, "off"))
+		gpiolib_acpi_run_edge_events_on_boot = 0;
+	else if (!strcmp(arg, "auto"))
+		gpiolib_acpi_run_edge_events_on_boot = -1;
+
+	return 1;
+}
+
+__setup("gpiolib_acpi_run_edge_events_on_boot=",
+        gpiolib_acpi_run_edge_events_on_boot_setup);
+
 /**
  * struct acpi_gpio_event - ACPI GPIO event handler data
  *
@@ -150,6 +168,29 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 }
 EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
 
+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"),
+		}
+	},
+	{} /* Terminating entry */
+};
+
+static bool acpi_gpiochip_run_edge_events_on_boot(void)
+{
+	if (gpiolib_acpi_run_edge_events_on_boot == -1) {
+		if (dmi_check_system(run_edge_events_on_boot_blacklist))
+			gpiolib_acpi_run_edge_events_on_boot = 0;
+		else
+			gpiolib_acpi_run_edge_events_on_boot = 1;
+	}
+
+	return gpiolib_acpi_run_edge_events_on_boot;
+}
+
 static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
 				      struct acpi_gpio_event *event)
 {
@@ -170,10 +211,13 @@ static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
 	event->irq_requested = true;
 
 	/* Make sure we trigger the initial state of edge-triggered IRQs */
-	value = gpiod_get_raw_value_cansleep(event->desc);
-	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
-	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
-		event->handler(event->irq, event);
+	if (acpi_gpiochip_run_edge_events_on_boot() &&
+	    (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
+		value = gpiod_get_raw_value_cansleep(event->desc);
+		if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
+		    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
+			event->handler(event->irq, event);
+	}
 }
 
 static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
-- 
2.22.0

Re: [PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist

[Ian W MORRISON]

On Sat, 24 Aug 2019 at 07:53, Hans de Goede <hdegoede@redhat.com> wrote:
>
> To avoid problems like this, this commit adds a new
> gpiolib_acpi_run_edge_events_on_boot kernel commandline option which
> can be "on", "off", or "auto" (default).
>
> In auto mode the default is on and a DMI based blacklist is used,
> the initial version of this blacklist contains the Minix Neo Z83-4
> fixing the HDMI being broken on this device.
>

Many thanks Hans. I've tested the patch including the command line
option with both v5.3-rc5 and linux-next on a Minix Neo Z83-4 and it
works fine.

Best regards,
Ian

Re: [PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist

[Andy Shevchenko]

On Fri, Aug 23, 2019 at 11:52:55PM +0200, Hans de Goede wrote:
> Another day; another DSDT bug we need to workaround...
> 
> Since commit ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events
> at least once on boot") we call _AEI edge handlers at boot.
> 
> In some rare cases this causes problems. One example of this is the Minix
> Neo Z83-4 mini PC, this device has a clear DSDT bug where it has some copy
> and pasted code for dealing with Micro USB-B connector host/device role
> switching, while the mini PC does not even have a micro-USB connector.
> This code, which should not be there, messes with the DDC data pin from
> the HDMI connector (switching it to GPIO mode) breaking HDMI support.
> 
> To avoid problems like this, this commit adds a new
> gpiolib_acpi_run_edge_events_on_boot kernel commandline option which
> can be "on", "off", or "auto" (default).
> 
> In auto mode the default is on and a DMI based blacklist is used,
> the initial version of this blacklist contains the Minix Neo Z83-4
> fixing the HDMI being broken on this device.

> +static int gpiolib_acpi_run_edge_events_on_boot = -1;
> +
> +static int __init gpiolib_acpi_run_edge_events_on_boot_setup(char *arg)
> +{

> +	if (!strcmp(arg, "on"))
> +		gpiolib_acpi_run_edge_events_on_boot = 1;
> +	else if (!strcmp(arg, "off"))
> +		gpiolib_acpi_run_edge_events_on_boot = 0;

kstrtobool() ?

> +	else if (!strcmp(arg, "auto"))
> +		gpiolib_acpi_run_edge_events_on_boot = -1;
> +
> +	return 1;
> +}

> +
> +__setup("gpiolib_acpi_run_edge_events_on_boot=",
> +        gpiolib_acpi_run_edge_events_on_boot_setup);

Can't we use module_param() ?
The resulting option would be 'gpiolib_acpi.foo=...'

> +{

> +	if (gpiolib_acpi_run_edge_events_on_boot == -1) {
> +		if (dmi_check_system(run_edge_events_on_boot_blacklist))
> +			gpiolib_acpi_run_edge_events_on_boot = 0;
> +		else
> +			gpiolib_acpi_run_edge_events_on_boot = 1;
> +	}

Can we run this at an initcall once and use variable instead of calling a
method below?

> +	return gpiolib_acpi_run_edge_events_on_boot;
> +}
> +
>  static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>  				      struct acpi_gpio_event *event)
>  {
> @@ -170,10 +211,13 @@ static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>  	event->irq_requested = true;
>  
>  	/* Make sure we trigger the initial state of edge-triggered IRQs */
> -	value = gpiod_get_raw_value_cansleep(event->desc);
> -	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> -	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> -		event->handler(event->irq, event);
> +	if (acpi_gpiochip_run_edge_events_on_boot() &&
> +	    (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> +		value = gpiod_get_raw_value_cansleep(event->desc);
> +		if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> +		    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> +			event->handler(event->irq, event);
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist

[Hans de Goede]

Hi Andy,

On 26-08-19 11:11, Andy Shevchenko wrote:
> On Fri, Aug 23, 2019 at 11:52:55PM +0200, Hans de Goede wrote:
>> Another day; another DSDT bug we need to workaround...
>>
>> Since commit ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events
>> at least once on boot") we call _AEI edge handlers at boot.
>>
>> In some rare cases this causes problems. One example of this is the Minix
>> Neo Z83-4 mini PC, this device has a clear DSDT bug where it has some copy
>> and pasted code for dealing with Micro USB-B connector host/device role
>> switching, while the mini PC does not even have a micro-USB connector.
>> This code, which should not be there, messes with the DDC data pin from
>> the HDMI connector (switching it to GPIO mode) breaking HDMI support.
>>
>> To avoid problems like this, this commit adds a new
>> gpiolib_acpi_run_edge_events_on_boot kernel commandline option which
>> can be "on", "off", or "auto" (default).
>>
>> In auto mode the default is on and a DMI based blacklist is used,
>> the initial version of this blacklist contains the Minix Neo Z83-4
>> fixing the HDMI being broken on this device.
> 
>> +static int gpiolib_acpi_run_edge_events_on_boot = -1;
>> +
>> +static int __init gpiolib_acpi_run_edge_events_on_boot_setup(char *arg)
>> +{
> 
>> +	if (!strcmp(arg, "on"))
>> +		gpiolib_acpi_run_edge_events_on_boot = 1;
>> +	else if (!strcmp(arg, "off"))
>> +		gpiolib_acpi_run_edge_events_on_boot = 0;
> 
> kstrtobool() ?
> 
>> +	else if (!strcmp(arg, "auto"))
>> +		gpiolib_acpi_run_edge_events_on_boot = -1;
>> +
>> +	return 1;
>> +}
> 
>> +
>> +__setup("gpiolib_acpi_run_edge_events_on_boot=",
>> +        gpiolib_acpi_run_edge_events_on_boot_setup);
> 
> Can't we use module_param() ?
> The resulting option would be 'gpiolib_acpi.foo=...'

I was expecting that would not work, since gpiolib is a bool
not a tristate, but it seems that if there is no module-name
to use as prefix for module-parameters the kernel simply uses
the .c file name, so this works and yes, this is better then
using __setup, will fix for v2.

>> +{
> 
>> +	if (gpiolib_acpi_run_edge_events_on_boot == -1) {
>> +		if (dmi_check_system(run_edge_events_on_boot_blacklist))
>> +			gpiolib_acpi_run_edge_events_on_boot = 0;
>> +		else
>> +			gpiolib_acpi_run_edge_events_on_boot = 1;
>> +	}
> 
> Can we run this at an initcall once and use variable instead of calling a
> method below?

I was a bit worried about init ordering, but I've checked and dmi_setup()
is done as a core_initcall, so we can do this once as a postcore_initcall
which should be early enough, will fix for v2.

> 
>> +	return gpiolib_acpi_run_edge_events_on_boot;
>> +}
>> +
>>   static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>>   				      struct acpi_gpio_event *event)
>>   {
>> @@ -170,10 +211,13 @@ static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>>   	event->irq_requested = true;
>>   
>>   	/* Make sure we trigger the initial state of edge-triggered IRQs */
>> -	value = gpiod_get_raw_value_cansleep(event->desc);
>> -	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>> -	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>> -		event->handler(event->irq, event);
>> +	if (acpi_gpiochip_run_edge_events_on_boot() &&
>> +	    (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>> +		value = gpiod_get_raw_value_cansleep(event->desc);
>> +		if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>> +		    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>> +			event->handler(event->irq, event);
>> +	}
> 

Regards,

Hans