pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

A crash is triggered on the ASUS T100TA Baytrail-T because of a IRQ descriptor conflict. There are two gpio triggered acpi events in this device, GPIO 6 and 18. These gpios are translated to irqs by calling gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset). irq_create_mapping will take care of allocating the irq descriptor, taking the first available number starting from the given value (6 in our case). The 0-15 are already reserved by legacy ISA code, so it gets the first free irq descriptor which is number 16. The i915 driver also uses irq 16, it loads later than gpio and crashes in probe.

The bug is reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=68291

The rootcause we know now is a low level irq issue. It needs a long term solution to fix the issue in irq system.

This patch is a workaround which changes the Baytrail GPIO driver to avoid the IRQ conflict. It still uses the irq domain to allocate irq descriptor but start from a predefined irq base number (256).

Signed-off-by Jin Yao <[email protected]>

---
 drivers/pinctrl/pinctrl-baytrail.c | 37
+++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/pinctrl/pinctrl-baytrail.c +22 -8

index 6e8301f..45b2d81 100644
@@ -124,6 +124,18 @@ static struct pinctrl_gpio_range byt_ranges[] = {
        },
 };
 
+/*
+ * Start from an irq base number above x86 ioapic range to work around some
+ * nasty, which is still in 3.14 unresolved irq descriptor conflicts.
+ */
+#define BYT_GPIO_PIN_IRQBASE   256
+
+static int byt_pin_irqbase[] = {
+       BYT_GPIO_PIN_IRQBASE,
+       BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE,
+       BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE + BYT_NGPIO_NCORE,
+};
+
 struct byt_gpio {
        struct gpio_chip                chip;
        struct irq_domain               *domain;

@@ -131,6 +143,7 @@ struct byt_gpio {
        spinlock_t                      lock;
        void __iomem                    *reg_base;
        struct pinctrl_gpio_range       *range;
+       int                             pin_irqbase;
 };
 
 #define to_byt_gpio(c) container_of(c, struct byt_gpio, chip)

@@ -481,7 +494,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
        struct pinctrl_gpio_range *range;
        acpi_handle handle = ACPI_HANDLE(dev);
        unsigned hwirq;
-       int ret;
+       int ret, i;
 
        if (acpi_bus_get_device(handle, &acpi_dev))
                return -ENODEV;

@@ -496,6 +509,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
                if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
                        vg->chip.ngpio = range->npins;
                        vg->range = range;
+                       ret = kstrtol(range->name, 10, &i);
+                       if (ret != 0)
+                               return ret;
+
+                       i--;
+                       vg->pin_irqbase = byt_pin_irqbase[i];
                        break;
                }
        }

@@ -527,19 +546,14 @@ static int byt_gpio_probe(struct platform_device
 *pdev)
        gc->can_sleep = false;
        gc->dev = dev;
 
-       ret = gpiochip_add(gc);
-       if (ret) {
-               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-               return ret;
-       }
-
        /* set up interrupts  */
        irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
        if (irq_rc && irq_rc->start) {
                hwirq = irq_rc->start;
                gc->to_irq = byt_gpio_to_irq;
 
-               vg->domain = irq_domain_add_linear(NULL, gc->ngpio,
+               vg->domain = irq_domain_add_simple(NULL, gc->ngpio,
+                                                  vg->pin_irqbase,
                                                   &byt_gpio_irq_ops, vg);
                if (!vg->domain)

                        return -ENXIO;

drivers/pinctrl/pinctrl-baytrail.c +6 -0

@@ -550,6 +564,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
                irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
        }
 
+       ret = gpiochip_add(gc);
+       if (ret) {
+               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+               return ret;
+       }
+
        pm_runtime_enable(dev);
 
        return 0;

Quoted 118 lines · Yao

A crash is triggered on the ASUS T100TA Baytrail-T because of a IRQ descriptor conflict. There are two gpio triggered acpi events in this device, GPIO 6 and 18. These gpios are translated to irqs by calling gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset). irq_create_mapping will take care of allocating the irq descriptor, taking the first available number starting from the given value (6 in our case). The 0-15 are already reserved by legacy ISA code, so it gets the first free irq descriptor which is number 16. The i915 driver also uses irq 16, it loads later than gpio and crashes in probe.

The bug is reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=68291

The rootcause we know now is a low level irq issue. It needs a long term solution to fix the issue in irq system.

This patch is a workaround which changes the Baytrail GPIO driver to avoid the IRQ conflict. It still uses the irq domain to allocate irq descriptor but start from a predefined irq base number (256).

Signed-off-by Jin Yao <[email protected]>

---
 drivers/pinctrl/pinctrl-baytrail.c | 37
+++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/pinctrl/pinctrl-baytrail.c +22 -8

index 6e8301f..45b2d81 100644
@@ -124,6 +124,18 @@ static struct pinctrl_gpio_range byt_ranges[] = {
        },
 };
 
+/*
+ * Start from an irq base number above x86 ioapic range to work around some
+ * nasty, which is still in 3.14 unresolved irq descriptor conflicts.
+ */
+#define BYT_GPIO_PIN_IRQBASE   256
+
+static int byt_pin_irqbase[] = {
+       BYT_GPIO_PIN_IRQBASE,
+       BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE,
+       BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE + BYT_NGPIO_NCORE,
+};
+
 struct byt_gpio {
        struct gpio_chip                chip;
        struct irq_domain               *domain;

@@ -131,6 +143,7 @@ struct byt_gpio {
        spinlock_t                      lock;
        void __iomem                    *reg_base;
        struct pinctrl_gpio_range       *range;
+       int                             pin_irqbase;
 };
 
 #define to_byt_gpio(c) container_of(c, struct byt_gpio, chip)

@@ -481,7 +494,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
        struct pinctrl_gpio_range *range;
        acpi_handle handle = ACPI_HANDLE(dev);
        unsigned hwirq;
-       int ret;
+       int ret, i;
 
        if (acpi_bus_get_device(handle, &acpi_dev))
                return -ENODEV;

@@ -496,6 +509,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
                if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
                        vg->chip.ngpio = range->npins;
                        vg->range = range;
+                       ret = kstrtol(range->name, 10, &i);
+                       if (ret != 0)
+                               return ret;
+
+                       i--;
+                       vg->pin_irqbase = byt_pin_irqbase[i];
                        break;
                }
        }

@@ -527,19 +546,14 @@ static int byt_gpio_probe(struct platform_device
 *pdev)
        gc->can_sleep = false;
        gc->dev = dev;
 
-       ret = gpiochip_add(gc);
-       if (ret) {
-               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-               return ret;
-       }
-
        /* set up interrupts  */
        irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
        if (irq_rc && irq_rc->start) {
                hwirq = irq_rc->start;
                gc->to_irq = byt_gpio_to_irq;
 
-               vg->domain = irq_domain_add_linear(NULL, gc->ngpio,
+               vg->domain = irq_domain_add_simple(NULL, gc->ngpio,
+                                                  vg->pin_irqbase,
                                                   &byt_gpio_irq_ops, vg);
                if (!vg->domain)

                        return -ENXIO;

drivers/pinctrl/pinctrl-baytrail.c +6 -0

@@ -550,6 +564,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
                irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
        }
 
+       ret = gpiochip_add(gc);
+       if (ret) {
+               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+               return ret;
+       }
+
        pm_runtime_enable(dev);
 
        return 0;

@@ -572,6 +592,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
 

drivers/pinctrl/pinctrl-baytrail.c static const struct dev_pm_ops byt_gpio_pm_ops = { · line 598

Quoted 6 lines

drivers/pinctrl/pinctrl-baytrail.c +1 -0

 static const struct acpi_device_id byt_gpio_acpi_match[] = {
        { "INT33B2", 0 },
+       { "INT33FC", 0 },
        { }
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);

Urgent fix and the maintainers did not react in a week? Well maybe they need to be on the To: line...

Mathias: can you send a patch adding yourself as maintainer of this driver in the MAINTAINERS file so stuff like this does not fall to the floor (me)?

Second: this fix is ugly like hell, is it really the best we can think of, plus in the commit message I'd very much like to know the real issue behind this as people in the x86 camp seem to be using some strange static IRQ line assignments that I cannot really understand so I don't know what the proper fix for this is :-(

Yours,
Linus Walleij

Urgent fix and the maintainers did not react in a week? Well maybe they need to be on the To: line...

Mathias: can you send a patch adding yourself as maintainer of this driver in the MAINTAINERS file so stuff like this does not fall to the floor (me)?

Second: this fix is ugly like hell, is it really the best we can think of, plus in the commit message I'd very much like to know the real issue behind this as people in the x86 camp seem to be using some strange static IRQ line assignments that I cannot really understand so I don't know what the proper fix for this is :-(

The real issue to my understanding is that the x86 io-apic code is not really capable of living together with other interrupt controllers at the same time. There are some assumptions that interrupts 0-15 belong to the legacy ISA world, from there on we got io-apic PCI interrupts ( I think io-apic interrupt controller handles something like 24 - 48 interrupts?) we want to be outside that range until this is fixed.

The x86 io-apic code does nasty things, for example the function that allocates the irq and the private chip data assumes that if the irq descriptor is taken, (returns -EEXISTS) then io-apic just must have reserved it earlier and can anyway use it, and access the chip data in a format only io-apic (struct irq_cfg) uses. This is of course not the case if a gpio interrupt controller like pinctrl-baytrail reserved the interrupt descriptor earler. -> oops

here's the io_apic.c function:

static struct irq_cfg *alloc_irq_and_cfg_at(unsigned int at, int node)

{
        int res = irq_alloc_desc_at(at, node);
        struct irq_cfg *cfg;

        if (res < 0) {
                if (res != -EEXIST)
                        return NULL;
                cfg = irq_get_chip_data(at);
                if (cfg)
                        return cfg;
        }

        cfg = alloc_irq_cfg(at, node);
        if (cfg)
                irq_set_chip_data(at, cfg);
        else
                irq_free_desc(at);
        return cfg;
}

We tried to fix this particular issue (make sure the interrupt belongs to a io_apic controller before letting io_apic touch it), but we just opened a can of worms of other issues I don't know how to deal with.

However this is a first time for an embedded irqchip (coupled with GPIO ACPI) creeping into the x86 world, so it needs some attention I think, do we have a direction forward for peaceful coexistence of several irq controllers picking some IRQ numbers/descriptors dynamically as they probe, also on x86 systems?

I'm traveling until friday, so please wait before you commit that fugly hack. I'll have a closer look how we can handle that at the core level.

Quoted 3 lines · Thomas Gleixner

I'm traveling until friday, so please wait before you commit that fugly hack. I'll have a closer look how we can handle that at the core level.

Thanks a *lot* Thomas, I'll stand by for action.

Quoted 7 lines · Linus Walleij

Quoted 3 lines · Thomas Gleixner

I'm traveling until friday, so please wait before you commit that fugly hack. I'll have a closer look how we can handle that at the core level.

Thanks a *lot* Thomas, I'll stand by for action.

Find an untested patch below. It should cure the issue.

I went through all code which can be affected by this and except for some other places, which might erroneously allocate inside the hardwired space, I can't see any possible fallout.

Thanks,

tglx

-------------------->

Subject: genirq: x86: Ensure that dynamic irq allocation does not conflict
From: Thomas Gleixner <[email protected]>
Date: Thu, 24 Apr 2014 09:50:53 +0200

On x86 the allocation of irq descriptors may allocate interrupts which are in the range of the GSI interrupts. That's wrong as those interrupts are hardwired and we don't have the irq domain translation like PPC. So one of these interrupts can be hooked up later to one of the devices which are hard wired to it and the io_apic init code for that particular interrupt line happily reuses that descriptor with a completely different configuration so hell breaks lose.

Inside x86 we allocate dynamic interrupts from above nr_gsi_irqs, except for a few usage sites which have not yet blown up in our face for whatever reason. But for drivers which need an irq range, like the GPIO drivers, we have no limit in place and we don't want to expose such a detail to a driver.

To cure this introduce a function which an architecture can implement to impose a lower bound on the dynamic interrupt allocations.

Implement it for x86 and set the lower bound to nr_gsi_irqs, which is the end of the hardwired interrupt space, so all dynamic allocations happen above.

That not only allows the GPIO driver to work sanely, it also protects the bogus callsites of create_irq_nr() in hpet, uv, irq_remapping and htirq code. They need to be cleaned up as well, but that's a separate issue.

Signed-off-by Thomas Gleixner <[email protected]>

Quoted 8 lines

Urgent fix and the maintainers did not react in a week? Well maybe they need to be on the To: line...

Mathias: can you send a patch adding yourself as maintainer of this driver in the MAINTAINERS file so stuff like this does not fall to the floor (me)?

Hi,

Sorry about the delay. I'm taking over Sarah's role as usb3 xhci maintainer and got my hands full over there. I can look at these patches now and then but you might need to kick me.

In general, I'd agree with Mika, Andy and Heikki (and Rafael obviously) opinion regarding baytrail gpio / acpi related matters.

Quoted 7 lines

Second: this fix is ugly like hell, is it really the best we can think of, plus in the commit message I'd very much like to know the real issue behind this as people in the x86 camp seem to be using some strange static IRQ line assignments that I cannot really understand so I don't know what the proper fix for this is :-(

This patch went out a bit early, a new version (which is not mangled) can be found at:

http://marc.info/?l=linux-kernel&m=139765010717522&w=2

But that one still produces some compiler warning which should be fixed.

There might be some touchscreen related issues recently found related to this patch that need to be investigated ( see bug link in commit message)

This is a hack to get T100TA working. This is one of many bad ways to workaround the real issue instead of fixing it, and I hope it can be reverted later, but this is the best we can do with our (my) limited io-apic and interrupt knowledge. But in the end, with this the Asus T100TA gpio works somehow, without it, it doesn't.

The real issue to my understanding is that the x86 io-apic code is not really capable of living together with other interrupt controllers at the same time. There are some assumptions that interrupts 0-15 belong to the legacy ISA world, from there on we got io-apic PCI interrupts ( I think io-apic interrupt controller handles something like 24 - 48 interrupts?) we want to be outside that range until this is fixed.

The x86 io-apic code does nasty things, for example the function that allocates the irq and the private chip data assumes that if the irq descriptor is taken, (returns -EEXISTS) then io-apic just must have reserved it earlier and can anyway use it, and access the chip data in a format only io-apic (struct irq_cfg) uses. This is of course not the case if a gpio interrupt controller like pinctrl-baytrail reserved the interrupt descriptor earler. -> oops

here's the io_apic.c function:

static struct irq_cfg *alloc_irq_and_cfg_at(unsigned int at, int node) {

  	int res = irq_alloc_desc_at(at, node);
         struct irq_cfg *cfg;

         if (res < 0) {
                 if (res != -EEXIST)
                         return NULL;
                 cfg = irq_get_chip_data(at);
                 if (cfg)
                         return cfg;
         }

         cfg = alloc_irq_cfg(at, node);
	if (cfg)
                 irq_set_chip_data(at, cfg);
         else
             	irq_free_desc(at);
         return cfg;
}

We tried to fix this particular issue (make sure the interrupt belongs to a io_apic controller before letting io_apic touch it), but we just opened a can of worms of other issues I don't know how to deal with.

-Mathias