| Message ID | 20190819112637.29943-1-ianwmorrison@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Skip deferred request irqs for devices known to fail | expand |
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; > } >
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; > }
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; >> } >>
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; }