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

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

[Jin, 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 <yao.jin@linux.intel.com>
---
  drivers/pinctrl/pinctrl-baytrail.c | 37 
+++++++++++++++++++++++++++++--------
  1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c 
b/drivers/pinctrl/pinctrl-baytrail.c
index 6e8301f..45b2d81 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -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;
@@ -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 = {

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

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Linus Walleij]

On Mon, Apr 14, 2014 at 4:48 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:

> 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 <yao.jin@linux.intel.com>
> ---
>  drivers/pinctrl/pinctrl-baytrail.c | 37
> +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> b/drivers/pinctrl/pinctrl-baytrail.c
> index 6e8301f..45b2d81 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -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;
> @@ -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 = {
>
>  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

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/22 21:18, Linus Walleij wrote:
> On Mon, Apr 14, 2014 at 4:48 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> 
>> 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 <yao.jin@linux.intel.com>
>> ---
>>  drivers/pinctrl/pinctrl-baytrail.c | 37
>> +++++++++++++++++++++++++++++--------
>>  1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
>> b/drivers/pinctrl/pinctrl-baytrail.c
>> index 6e8301f..45b2d81 100644
>> --- a/drivers/pinctrl/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/pinctrl-baytrail.c
>> @@ -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;
>> @@ -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 = {
>>
>>  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...
> 
Thanks for reminding. I will keep in mind for next time.

> 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 :-(

GPIO driver is loaded before graphics. It's no problem for it to get the
first free irq, which number is 16. After a while, graphics driver is
loaded. The IRQ number (16) it needs is hardwired by BIOS. Here is part
of DSDT table:

Name (AR00, Package (0x11)
{
    Package (0x04)
    {
        0x0002FFFF,
        Zero,
        Zero,
        0x10	/* Jin Yao: IRQ number 16 */
    },
    ......
}

The problem happens at __irq_alloc_descs().

__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int
node, struct module *owner)
{
	......
	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
from, cnt, 0);
	ret = -EEXIST;

	/* Jin Yao: irq = 16, start = 17 */
/* A */ if (irq >=0 && start != irq)
		goto err;
	......
err:
}

When graphics driver probing, the irq is 16 and
bitmap_find_next_zero_area() returns 17. That's correct, because 16 has
been allocated to GPIO pin. The problem is at the line A, the checking
is failed and goto "err:" directly. The next io_apic processing code
(not list here) assumes all irq descriptors belongs to a io_apic chip,
and that all chip_data is of type irq_cfg. But unfortunately in this
case, the chip_data in irq_desc of IRQ16 is pointing to byt_gpio, then
crash happens.

So this needs to be fixed in two places:
1. make sure io_apic code does not access data in interrupt descriptors
that belong to other "interrupt controllers", eg gpio than io_apic.

2. fix graphics driver / bios to not request the not needed irq 16

Probably the above fixes need long time, so I decide to use a simple and
direct way by just shifting the irq for GPIO pins to avoid the conflict.

This patch "[PATCH] pinctrl-baytrail: workaround for irq descriptor
conflict on ASUS T100TA" has format issue. I post it by forwarding via
Thunderbird, but lead to format issue, I'm so sorry for that.

I post another patch "[PATCH] pinctrl-baytrail: fix for irq descriptor
conflict on ASUS T100TA" to solve the format issue.

Thanks
Jin Yao

> 
> Yours,
> Linus Walleij
> 

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Mathias Nyman]

>
> 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.

> 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

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Linus Walleij]

On Wed, Apr 23, 2014 at 1:35 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:

> 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.

This looks pretty scary.

io_apic.c is maintained by every single kernel bigshot and I sure
as hell would like to not under any circumstance step on its toes
so whatever solution we can think of until the next merge window
will be applied. (AFICT this affects all baytrails, no desktops etc).

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?

Other archs doesn't seem to have this problem, but they also
do not have the same legacy.

Yours,
Linus Walleij

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Thomas Gleixner]

On Wed, 23 Apr 2014, Linus Walleij wrote:
> 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?

The issue on x86 is, that we have basically two classes of interrupts.

 - The hardwired ones (in the chipset and we have no way to
   change that except with MSI)

 - The dynamic ones, i.e. MSI[X]

So for allocating the MSI interrupts we use the range above the
possible hardwired interrupts.

Unfortunaly the information where the reserved/hardwired range ends is
not available to drivers, but it should be simple to do so.

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,

	tglx

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Linus Walleij]

On Thu, Apr 24, 2014 at 9:25 AM, Thomas Gleixner <tglx@linutronix.de> wrote:

> 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.

Yours,
Linus Walleij

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Thomas Gleixner]

On Thu, 24 Apr 2014, Linus Walleij wrote:
> On Thu, Apr 24, 2014 at 9:25 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > 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 <tglx@linutronix.de>
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 <tglx@linutronix.de>
---
 arch/x86/kernel/apic/io_apic.c |    5 +++++
 include/linux/irq.h            |    2 ++
 kernel/irq/irqdesc.c           |    2 ++
 kernel/softirq.c               |    5 +++++
 4 files changed, 14 insertions(+)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -3425,6 +3425,11 @@ int get_nr_irqs_gsi(void)
 	return nr_irqs_gsi;
 }
 
+unsigned int arch_dynirq_lower_bound(unsigned int from)
+{
+	return from < nr_irqs_gsi ? nr_irqs_gsi : from;
+}
+
 int __init arch_probe_nr_irqs(void)
 {
 	int nr;
Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -602,6 +602,8 @@ static inline u32 irq_get_trigger_type(u
 	return d ? irqd_get_trigger_type(d) : 0;
 }
 
+unsigned int arch_dynirq_lower_bound(unsigned int from);
+
 int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 		struct module *owner);
 
Index: linux-2.6/kernel/irq/irqdesc.c
===================================================================
--- linux-2.6.orig/kernel/irq/irqdesc.c
+++ linux-2.6/kernel/irq/irqdesc.c
@@ -363,6 +363,8 @@ __irq_alloc_descs(int irq, unsigned int
 		if (from > irq)
 			return -EINVAL;
 		from = irq;
+	} else {
+		from = arch_dynirq_lower_bound(from);
 	}
 
 	mutex_lock(&sparse_irq_lock);
Index: linux-2.6/kernel/softirq.c
===================================================================
--- linux-2.6.orig/kernel/softirq.c
+++ linux-2.6/kernel/softirq.c
@@ -779,3 +779,8 @@ int __init __weak arch_early_irq_init(vo
 {
 	return 0;
 }
+
+unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
+{
+	return from;
+}

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Mika Westerberg]

On Thu, Apr 24, 2014 at 04:40:25PM +0200, Thomas Gleixner wrote:
> On Thu, 24 Apr 2014, Linus Walleij wrote:
> > On Thu, Apr 24, 2014 at 9:25 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > 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 <tglx@linutronix.de>
> 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 <tglx@linutronix.de>

Works here on my T100,

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[tip:irq/urgent] genirq: x86: Ensure that dynamic irq allocation does not conflict

[tip-bot for Thomas Gleixner]

Commit-ID:  62a08ae2a5763aabeee98264605236b001503e0c
Gitweb:     http://git.kernel.org/tip/62a08ae2a5763aabeee98264605236b001503e0c
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 24 Apr 2014 09:50:53 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 28 Apr 2014 12:20:00 +0200

genirq: x86: Ensure that dynamic irq allocation does not conflict

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.

Reported-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Krogerus Heikki <heikki.krogerus@intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.02.1404241617360.28206@ionos.tec.linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apic/io_apic.c | 5 +++++
 include/linux/irq.h            | 2 ++
 kernel/irq/irqdesc.c           | 7 +++++++
 kernel/softirq.c               | 5 +++++
 4 files changed, 19 insertions(+)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6ad4658..d23aa82 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3425,6 +3425,11 @@ int get_nr_irqs_gsi(void)
 	return nr_irqs_gsi;
 }
 
+unsigned int arch_dynirq_lower_bound(unsigned int from)
+{
+	return from < nr_irqs_gsi ? nr_irqs_gsi : from;
+}
+
 int __init arch_probe_nr_irqs(void)
 {
 	int nr;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 10a0b1a..5c57efb 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -603,6 +603,8 @@ static inline u32 irq_get_trigger_type(unsigned int irq)
 	return d ? irqd_get_trigger_type(d) : 0;
 }
 
+unsigned int arch_dynirq_lower_bound(unsigned int from);
+
 int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 		struct module *owner);
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a7174617..bb07f29 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -363,6 +363,13 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 		if (from > irq)
 			return -EINVAL;
 		from = irq;
+	} else {
+		/*
+		 * For interrupts which are freely allocated the
+		 * architecture can force a lower bound to the @from
+		 * argument. x86 uses this to exclude the GSI space.
+		 */
+		from = arch_dynirq_lower_bound(from);
 	}
 
 	mutex_lock(&sparse_irq_lock);
diff --git a/kernel/softirq.c b/kernel/softirq.c
index b50990a..33e4648 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -779,3 +779,8 @@ int __init __weak arch_early_irq_init(void)
 {
 	return 0;
 }
+
+unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
+{
+	return from;
+}

Re: [PATCH] pinctrl-baytrail: workaround for irq descriptor conflict on ASUS T100TA

[Andy Shevchenko]

On Wed, 2014-04-23 at 14:35 +0300, Mathias Nyman wrote:
> >
> > 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.

This patch and discussion reminds me an old one where I was trying to
solve quite similar issue on Medfield [1], in particular [2, and 3]. For
my opinion irq mapping framework should be redesigned to solve the issue
regarding to devices which require fixed interrupt lines.

[1] https://lkml.org/lkml/2012/7/5/152
[2] https://lkml.org/lkml/2012/7/5/155
[3] https://lkml.org/lkml/2012/7/16/173
> 
> > 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
> 


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy