[PATCH v2] Skip deferred request irqs for devices known to fail

[PATCH v2] Skip deferred request irqs for devices known to fail

[Ian W MORRISON]

Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
least once on boot" causes the MINIX family of mini PCs to fail to boot
resulting in a "black screen". 

This patch excludes MINIX devices from executing this trigger in order
to successfully boot.

Cc: stable@vger.kernel.org
Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index fdee8afa5339..f6c3dcdc91c9 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/machine.h>
 #include <linux/export.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/mutex.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -20,6 +21,17 @@
 #include "gpiolib.h"
 #include "gpiolib-acpi.h"
 
+static const struct dmi_system_id skip_deferred_request_irqs_table[] = {
+	{
+		.ident = "MINIX Z83-4",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
+		},
+	},
+	{}
+};
+
 /**
  * struct acpi_gpio_event - ACPI GPIO event handler data
  *
@@ -1273,19 +1285,28 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
 	return con_id == NULL;
 }
 
-/* Run deferred acpi_gpiochip_request_irqs() */
+/*
+ * Run deferred acpi_gpiochip_request_irqs()
+ * but exclude devices known to fail
+*/
 static int acpi_gpio_handle_deferred_request_irqs(void)
 {
 	struct acpi_gpio_chip *acpi_gpio, *tmp;
+	const struct dmi_system_id *dmi_id;
 
-	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
-	list_for_each_entry_safe(acpi_gpio, tmp,
+	dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
+	if (dmi_id)
+		return 0;
+	else {
+		mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
+		list_for_each_entry_safe(acpi_gpio, tmp,
 				 &acpi_gpio_deferred_req_irqs_list,
 				 deferred_req_irqs_list_entry)
-		acpi_gpiochip_request_irqs(acpi_gpio);
+			acpi_gpiochip_request_irqs(acpi_gpio);
 
-	acpi_gpio_deferred_req_irqs_done = true;
-	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
+		acpi_gpio_deferred_req_irqs_done = true;
+		mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
+	}
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v2] Skip deferred request irqs for devices known to fail

[Hans de Goede]

Hi All,

On 19-08-19 13:26, Ian W MORRISON wrote:
> Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
> least once on boot" causes the MINIX family of mini PCs to fail to boot
> resulting in a "black screen".
> 
> This patch excludes MINIX devices from executing this trigger in order
> to successfully boot.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Erm, you cannot just add someone's Reviewed-by tag because they have
looked at the patch, you are only supposed to do that when the reviewer
actually puts that in his reply, then you can copy the line and
add it to your commit msg. Please drop these.

Also I might be able to get my hands on a Minix Neo Z83-4 myself
in a couple of days and then I can try to reproduce this, so lets
wait a bit for that and see how that goes.

Regards,

Hans




> ---
>   drivers/gpio/gpiolib-acpi.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index fdee8afa5339..f6c3dcdc91c9 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -13,6 +13,7 @@
>   #include <linux/gpio/machine.h>
>   #include <linux/export.h>
>   #include <linux/acpi.h>
> +#include <linux/dmi.h>
>   #include <linux/interrupt.h>
>   #include <linux/mutex.h>
>   #include <linux/pinctrl/pinctrl.h>
> @@ -20,6 +21,17 @@
>   #include "gpiolib.h"
>   #include "gpiolib-acpi.h"
>   
> +static const struct dmi_system_id skip_deferred_request_irqs_table[] = {
> +	{
> +		.ident = "MINIX Z83-4",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
> +		},
> +	},
> +	{}
> +};
> +
>   /**
>    * struct acpi_gpio_event - ACPI GPIO event handler data
>    *
> @@ -1273,19 +1285,28 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
>   	return con_id == NULL;
>   }
>   
> -/* Run deferred acpi_gpiochip_request_irqs() */
> +/*
> + * Run deferred acpi_gpiochip_request_irqs()
> + * but exclude devices known to fail
> +*/
>   static int acpi_gpio_handle_deferred_request_irqs(void)
>   {
>   	struct acpi_gpio_chip *acpi_gpio, *tmp;
> +	const struct dmi_system_id *dmi_id;
>   
> -	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> -	list_for_each_entry_safe(acpi_gpio, tmp,
> +	dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
> +	if (dmi_id)
> +		return 0;
> +	else {
> +		mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> +		list_for_each_entry_safe(acpi_gpio, tmp,
>   				 &acpi_gpio_deferred_req_irqs_list,
>   				 deferred_req_irqs_list_entry)
> -		acpi_gpiochip_request_irqs(acpi_gpio);
> +			acpi_gpiochip_request_irqs(acpi_gpio);
>   
> -	acpi_gpio_deferred_req_irqs_done = true;
> -	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +		acpi_gpio_deferred_req_irqs_done = true;
> +		mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +	}
>   
>   	return 0;
>   }
> 

Re: [PATCH v2] Skip deferred request irqs for devices known to fail

[Hans de Goede]

Hi All,

On 19-08-19 13:31, Hans de Goede wrote:
> Also I might be able to get my hands on a Minix Neo Z83-4 myself
> in a couple of days and then I can try to reproduce this, so lets
> wait a bit for that and see how that goes.

So I've access to a Minix Neo z83-4 myself now. The problem is
the DSDT contains an _E03 handler on the second (INT33FF UID 2)
GPIO controller which is clearly copy pasted from some DSDT
from a tablet as it deals with the ID pin of the micro-usb
connector, which the Minix Neo z83-4 mini-PC does not have.

This _E03 method switches the XHCI role switch between
host and device roles (those data lines are nor used, so don't
care) *and* for some reason it sets GN66 to 0 or 1, with GN66
being defined as:

                 Connection (
                     GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
                         "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                         )
                         {   // Pin list
                             0x0042
                         }
                 ),

This leads to the following difference in a pinctrl debug dump
between a good (running of ACPI edge GPIO handlers at boot disabled)
and bad run:

@@ -51,7 +51,7 @@
  pin 63 (PANEL1_BKLTCTL) GPIO 0x00008102 0x04c00000
  pin 64 (HV_DDI1_HPD) mode 1 0x03010000 0x04c00020
  pin 65 (PANEL0_BKLTCTL) GPIO 0x30008202 0x04c00003
-pin 66 (HV_DDI0_DDC_SDA) GPIO 0x00018000 0x04c00000
+pin 66 (HV_DDI0_DDC_SDA) mode 1 0x00010001 0x04c00000
  pin 67 (HV_DDI2_DDC_SCL) mode 3 0x00930301 0x04c00000
  pin 68 (HV_DDI2_HPD) mode 1 0x03010001 0x04c00020
  pin 69 (PANEL1_VDDEN) GPIO 0x00008102 0x04c00000

With a bad run ssh still works, basically everything still works except
for DDC on the  HDMI conector which is causing the blackscreen.

Through ssh I could get the above pinctrl difference and
also see this new errors in the logs:

kernel: i915 0000:00:02.0: HDMI-A-1: EDID is invalid:
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel:         [00] ZERO 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
kernel: [drm] Cannot find any crtc or sizes
kernel: [drm] Cannot find any crtc or sizes

Which matches with the DDC data pin being changes from connected
to the DDC i2c-controller into a generic (G)PIO

So this is really a case of a broken DSDT I am afraid and as such
the DMI blacklist seems the best (least bad) we can do.

But I do not believe that the current patch is a good fix, this problem
first surfaced when we started running edge ACPI GPIO event handlers at
boot to ensure that any state which is set by the handler matches the
current value of the pin. So that e.g. USB host/device role switches are
set the right value.

Where as the fix proposed by Ian, disabled us from registering a
handler all together, not only for the troublesome _E03 (which will
never trigger normally since there is no id-pin), but also for the
e.g. the INT0002 vgpio device.

And not registering a handler for the INT0002 vgpio device causes
an interrupt storm on irq 9, although for some reason that storm
stops after a 100000 interrupts or so on the Minix Neo Z83-4.
which is different from other devices where it never stops and we
get millions of interrupts.

So I believe a better fix would be to:

1) Add a kernel parameter to disable the run of edge ACPI
GPIO events at startup:

gpiolib_acpi_run_edge_events_on_startup

2) Make this default to auto which uses a DMI blacklist

This will allow us to easily test for similar problems on other
hardware and it fixes the issue at hand without disabling all
ACPI GPIO event handlers.

I will prep a patch implementing this approach sometime this
weekend.

Regards,

Hans



>> ---
>>   drivers/gpio/gpiolib-acpi.c | 33 +++++++++++++++++++++++++++------
>>   1 file changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
>> index fdee8afa5339..f6c3dcdc91c9 100644
>> --- a/drivers/gpio/gpiolib-acpi.c
>> +++ b/drivers/gpio/gpiolib-acpi.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/gpio/machine.h>
>>   #include <linux/export.h>
>>   #include <linux/acpi.h>
>> +#include <linux/dmi.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/mutex.h>
>>   #include <linux/pinctrl/pinctrl.h>
>> @@ -20,6 +21,17 @@
>>   #include "gpiolib.h"
>>   #include "gpiolib-acpi.h"
>> +static const struct dmi_system_id skip_deferred_request_irqs_table[] = {
>> +    {
>> +        .ident = "MINIX Z83-4",
>> +        .matches = {
>> +            DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
>> +            DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
>> +        },
>> +    },
>> +    {}
>> +};
>> +
>>   /**
>>    * struct acpi_gpio_event - ACPI GPIO event handler data
>>    *
>> @@ -1273,19 +1285,28 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
>>       return con_id == NULL;
>>   }
>> -/* Run deferred acpi_gpiochip_request_irqs() */
>> +/*
>> + * Run deferred acpi_gpiochip_request_irqs()
>> + * but exclude devices known to fail
>> +*/
>>   static int acpi_gpio_handle_deferred_request_irqs(void)
>>   {
>>       struct acpi_gpio_chip *acpi_gpio, *tmp;
>> +    const struct dmi_system_id *dmi_id;
>> -    mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
>> -    list_for_each_entry_safe(acpi_gpio, tmp,
>> +    dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
>> +    if (dmi_id)
>> +        return 0;
>> +    else {
>> +        mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
>> +        list_for_each_entry_safe(acpi_gpio, tmp,
>>                    &acpi_gpio_deferred_req_irqs_list,
>>                    deferred_req_irqs_list_entry)
>> -        acpi_gpiochip_request_irqs(acpi_gpio);
>> +            acpi_gpiochip_request_irqs(acpi_gpio);
>> -    acpi_gpio_deferred_req_irqs_done = true;
>> -    mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
>> +        acpi_gpio_deferred_req_irqs_done = true;
>> +        mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
>> +    }
>>       return 0;
>>   }
>>

Re: [PATCH v2] Skip deferred request irqs for devices known to fail

[Andy Shevchenko]

On Mon, Aug 19, 2019 at 09:26:37PM +1000, Ian W MORRISON wrote:
> Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
> least once on boot" causes the MINIX family of mini PCs to fail to boot
> resulting in a "black screen".
> 
> This patch excludes MINIX devices from executing this trigger in order
> to successfully boot.

Thanks for an update.

> Cc: stable@vger.kernel.org
> Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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

Hmm... Did I really give the tag?
Too many stuff is going on, anyway, please consider more comments below.

First of all, the subject should start from "gpiolib: acpi: " prefix.

Then, Fixes tag seems to be missed.

> +/*
> + * Run deferred acpi_gpiochip_request_irqs()
> + * but exclude devices known to fail

Missed period.

> +*/

Missed leading space (the column of stars).

>  static int acpi_gpio_handle_deferred_request_irqs(void)
>  {
>  	struct acpi_gpio_chip *acpi_gpio, *tmp;
> +	const struct dmi_system_id *dmi_id;
>  
> -	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> -	list_for_each_entry_safe(acpi_gpio, tmp,
> +	dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
> +	if (dmi_id)
> +		return 0;

The idea of positive check is exactly for...

> +	else {

...getting rid of this redundant 'else' followed by unneeded level of indentation.

> +		mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> +		list_for_each_entry_safe(acpi_gpio, tmp,
>  				 &acpi_gpio_deferred_req_irqs_list,
>  				 deferred_req_irqs_list_entry)
> -		acpi_gpiochip_request_irqs(acpi_gpio);
> +			acpi_gpiochip_request_irqs(acpi_gpio);
>  
> -	acpi_gpio_deferred_req_irqs_done = true;
> -	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +		acpi_gpio_deferred_req_irqs_done = true;
> +		mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +	}
>  
>  	return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko