Bug 68291

The box is set up for debugging so I can test patches etc on it.

Alan reports that 3.11 worked and 3.13-rc7 fails.
marking this report as a regression.

Alan reports 3.12 works.
So the regression is between 3.12 and 3.13-rc7

Much digging and odd bisect behaviour later which wouldn't give straight answers

The critical thing is the BYT pi mux stuff it seems

I see

byt_gpio: INT33FC:02: Failed to request IRQ 18 ACPI event handler
ACPI: \_SB_.PCIO0: notify handler is installed

Then a bit later the i915 explosion

CONFIG_PINCTRL_BAYTRAIL = y ; crashes
CONFIG_PINCTRL_BAYTRAIL = n ; all works

Can't tell easily if this is a pinmux bug or perhaps the Failed to request.. sets up a future fail on the same IRQ line ?

the baytrail pinctrl/gpio driver tries to set up 
interrupt handlers for gpio triggered ACPI events.

Just to narrow it down, does this help?:

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index 114f5ef..f4a7ecb 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -486,8 +486,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
                irq_set_handler_data(hwirq, vg);
                irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
 
-               /* Register interrupt handlers for gpio signaled acpi events */
-               acpi_gpiochip_request_interrupts(gc);
        }

This function requests interrupts for gpio pins that have ACPI GPIO_INT resources in ACPI DSDT table and an ACPI event handler that should be called when interrupt triggered.

Yes that stops the crash

Seems to resolve the crash here too...and now I'm back to a blank screen when modesetting kicks in, on the v8p.

Thats the other bug.

Interestingly for that one if you remote login or script X to do an xrandr newmode, xrandr addmode and xrandr --output VGA1 --mode blah

you get video.

Yeah, I thought it had been fixed, but I think what happened was actually that this bug showed up and broke KMS entirely.

My report for the blank-screen issue is https://bugs.freedesktop.org/show_bug.cgi?id=71977 - do you have one too?

Alan: should I close my report for this issue - https://bugzilla.kernel.org/show_bug.cgi?id=67861 - as a dupe of this one?

If the same "fix" works for both platforms then yeah - mark it a dup of this

*** Bug 67861 has been marked as a duplicate of this bug. ***

This is triggered on 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 (0x12).
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 first 16 (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.

I'm not familiar with how the inner parts of irq code works, or how i915 driver 
requests its interrupt, but I think that if descriptor number 16 is reserved then it should just get the next virtual irq number and requst that one?

Huh. I don't recall what we did about this bug, but it clearly got fixed somehow at some point, because I don't have it with 3.14rc4 and I'm not carrying any patches for it.

(In reply to Adam Williamson from comment #14)
> Huh. I don't recall what we did about this bug, but it clearly got fixed
> somehow at some point, because I don't have it with 3.14rc4 and I'm not
> carrying any patches for it.

Issue never got fixed, The patch that enables GPIO driver to load
just got reverted. 

commit 2b844ba79f4a114bd228ad6fee040ffd99a0963d
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Fri Jan 17 14:23:29 2014 +0100

    Revert "ACPI: Add BayTrail SoC GPIO and LPSS ACPI IDs"

Ah, right. But: I have that patch re-applied in my current builds, and they don't crash.

See https://github.com/AdamWill/baytrail-m/blob/master/kernel/MANIFEST:

* LPSS support, probably needed for sound to work
* https://lkml.org/lkml/2014/2/18/127
baytrail_acpi_pwm.patch
* https://lkml.org/lkml/2014/1/20/88
lpss_pwm.patch
* https://github.com/rjwysocki/linux-pm/commit/b2a51a6d0f96308251cfa41b793c43d0316e3b16
baytrail_gpio_lpss_ids.patch

I'm applying those three patches together, against 3.14rc4, and not hitting this issue.

Looks the IRQ value (16) is hardwired by T100 BIOS. 

Dump debug info in acpi_pci_irq_check_entry() before crash:
        0000:00:02[A] -> [16]

Dump part of ACPI DSDT table:
        Method (_PRT, 0, NotSerialized)  // _PRT: PCI Routing Table
        {
            If (PICM)
            {
                Return (AR00)
            }

            Return (PR00)
        }

        Name (AR00, Package (0x11)
        {
            Package (0x04)
            {
                0x0002FFFF, 
                Zero, 
                Zero, 
                0x10
            }, 
            ......
        }

That can explain why IRQ is used again after GPIO driver loading.

(In reply to Adam Williamson from comment #16)
> Ah, right. But: I have that patch re-applied in my current builds, and they
> don't crash.
> 
> See https://github.com/AdamWill/baytrail-m/blob/master/kernel/MANIFEST:
> 
> * LPSS support, probably needed for sound to work
> * https://lkml.org/lkml/2014/2/18/127
> baytrail_acpi_pwm.patch
> * https://lkml.org/lkml/2014/1/20/88
> lpss_pwm.patch
> *
> https://github.com/rjwysocki/linux-pm/commit/
> b2a51a6d0f96308251cfa41b793c43d0316e3b16
> baytrail_gpio_lpss_ids.patch
> 
> I'm applying those three patches together, against 3.14rc4, and not hitting
> this issue.

Oh, right, 3.14rc4 has some modifications already in it.

Short story:

You need one more patch to trigger this, which is not out anywhere,
I'll attach it here.

Long story:

3.14rc4 has some modifications to how gpio triggered acpi event handlers are registered, gpio driver no longer calls acpi_gpiochip_request_interrupts() itself, it's now done in gpiochip_add() automatically. Because pinctrl-baytrail.c calls gpiochip_add() before it sets up interrupts (creates the new irq domain etc.) the gpio triggered acpi events will not work on this version, (and not crash either) so a patch that moves  gpiochip_add() to after interrupt initialization is needed.    

The patches that changes the gpio triggered acpi event registration is here https://lkml.org/lkml/2013/11/26/104

the patch that needs to be applied to make it crash/work is attached

Created attachment 128281 [details]
patch  to trigger crash on 3.14.rc4

I think I got the root cause. Let's see 2 execution flows (based on 3.13 kernel):

Flow 1: GPIO
	acpi_gpiochip_request_interrupts() -> byt_gpio_to_irq() -> irq_create_mapping() -> 
		irq_alloc_desc_from()	/* IRQ16 is allocated */
		irq_domain_associate() ->
			domain->ops->map() /* Execute the callback byt_gpio_irq_map() */

			static int byt_gpio_irq_map(struct irq_domain *d, unsigned int virq,
						    irq_hw_number_t hw)
			{
				struct byt_gpio *vg = d->host_data;
				......			
				irq_set_chip_data(virq, vg);	
				......	
			}
			
			int irq_set_chip_data(unsigned int irq, void *data)
			{
				struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);
				......
				desc->irq_data.chip_data = data;
				......
			}

	Now the chip_data in irq_desc of IRQ16 is pointing to "byt_gpio".

Flow 2: i915
	acpi_pci_irq_enable()	/* From ACPI, we know the gsi=16 */
		-> acpi_register_gsi() -> mp_register_gsi() -> io_apic_set_pci_routing() ->
		io_apic_setup_irq_pin_once() -> io_apic_setup_irq_pin()
		
		/* irq = 16 here */
		io_apic_setup_irq_pin(unsigned int irq, int node, struct io_apic_irq_attr *attr)
		{
		/* A */	struct irq_cfg *cfg = alloc_irq_and_cfg_at(irq, node);
			int ret;
		
			if (!cfg)
				return -EINVAL;
			ret = __add_pin_to_irq_node(cfg, node, attr->ioapic, attr->ioapic_pin);
			if (!ret)
				setup_ioapic_irq(irq, cfg, attr);
			return ret;
		}

	The problem is at line A, actually the returned cfg is the chip_data in irq_desc of IRQ16, it's pointing to "byt_gpio" in flow 1.

Created attachment 128821 [details]
patch to solve crash

great catch, so io_apic code assumes all irqs descriptors belong to a io_apic irq chip, and that all chip_data is of type irq_cfg.

This patch checks that the interrupt belongs to ioapic before touching chip data.

With this patch device boots, but graphics probing fails because the legacy interrupt it wants is reserved.

Guess this is a better way instead of panic.. Do you think some debug printk's would benefit? Simply because 

If this happens it appears this is a bug in ACPI DSL in the platform, or Linux. In this case it appears its a Linux bug? 

looking at the dsl for GPIO below, the IRQ number is 0x31... why would linux allocate 0x10 instead? 


        Device (GPO0)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "INT33FC")  // _HID: Hardware ID
            Name (_CID, "INT33FC")  // _CID: Compatible ID
            Name (_DDN, "ValleyView General Purpose Input/Output (GPIO) controller")  // _DDN: DOS Device Name
            Name (_UID, One)  // _UID: Unique ID
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (RBUF, ResourceTemplate ()
                {
                    Memory32Fixed (ReadWrite,
                        0xFED0C000,         // Address Base
                        0x00001000,         // Address Length
                        )
                    Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
                    {
                        0x00000031,
                    }
                })
                Return (RBUF)
            }

(In reply to Ashok Raj from comment #22)

> looking at the dsl for GPIO below, the IRQ number is 0x31... why would linux
> allocate 0x10 instead? 
> 
>                     Interrupt (ResourceConsumer, Level, ActiveLow, Shared,
> ,, )
>                     {
>                         0x00000031,
>                     }

This is the interrupt resource the gpio controller uses, this is not the issue in this case.

the GPIO controller works as an interrupt controller, the gpio controller is connected to only one interrupt, and driver only listens to that one (0x31). but the driver provides lots of interrupts to the system, one for each gpio line if needed. So when interrupt 0x31 is called the gpio interrupt handler checks which pin was asserted and calls the handler for that specific interrupt.

For example when sd card detection is connected to GPIO pin 38, the sd card driver asks the gpio driver what interrupt that corresponds to by calling gpio_to_irq().  gpio_to_irq() in the gpio driver maps gpio 38 to a irq descriptor by finding a free interrupt descriptor from a global bitmap of all descriptor. Once a free one is found it is returned to SD card driver. for example descriptor 40

SD card driver registers a handler for irq 40. When a sd card is inserted gpio line 38 is set high, the gpio controller fires an interrupt on (0x31), gpio driver interrupt handler is called, it checks which gpio was asserted (was line 38, it finds which interrupts gpio 38 is mapped to (40) and calls the interrupt handler for 40 (the interrupt handler registered by SD card)

Issue here is that on x86 the ioapic used to be the only one providing interrupts (well, int these ranges atleast I think) so ioapic code assumes that if a intrrupt descriptor is occupied in the global bitmap it belongs to an ioapic, and it accesses data that is only valid for ioapics which causes failures.

In this specific case we collide on interrupt descriptor 16. And it turns out the graphic driver does not even need interrupt.

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 (graphics driver use msi interrupts)

-Mathias

Another question confuses me for a while why the GPIO controller registered IRQ can't be visible in /proc/interrupts? They should be 48/49/50 on ASUS T100. 

Following are the log of /proc/interrupts on my T100:

            CPU0       CPU1       CPU2       CPU3       
   0:         26          0          0          0   IO-APIC-edge      timer
   4:          2          0          0          0   IO-APIC-edge    
  16:          0          0          0          0  BYT-GPIO    6-demux     GPIO-signaled-ACPI-event
  18:          0          0          0          0  BYT-GPIO   18-demux     GPIO-signaled-ACPI-event
  32:          0          0          0          0   IO-APIC-fasteoi   80860F41:00
  33:          0          0          0          0   IO-APIC-fasteoi   80860F41:01
  34:          0          0          0          0   IO-APIC-fasteoi   80860F41:02
  35:          0          0          0          0   IO-APIC-fasteoi   80860F41:03
  36:          0          0          0          0   IO-APIC-fasteoi   80860F41:04
  37:       1033          0          0          0   IO-APIC-fasteoi   80860F41:05
  38:          0          0          0          0   IO-APIC-fasteoi   80860F41:06
  40:          0          0          0          0  BYT-GPIO   38-demux     sd_cd
  44:       2824          0          0          0   IO-APIC-fasteoi   mmc0
  46:        732          0          0          0   IO-APIC-fasteoi   mmc1
  47:          0          0          0          0   IO-APIC-fasteoi   mmc2
  69:          1          0          0          0   IO-APIC-fasteoi   ATML1000:00
 103:      26387          0          0          0   PCI-MSI-edge      xhci_hcd
 NMI:          0          0          0          0   Non-maskable interrupts
 LOC:       3110       1788       1278       1685   Local timer interrupts
 SPU:          0          0          0          0   Spurious interrupts
 PMI:          0          0          0          0   Performance monitoring interrupts
 IWI:        204        215        119        288   IRQ work interrupts
 RTR:          0          0          0          0   APIC ICR read retries
 RES:       2802       3989       2722       4779   Rescheduling interrupts
 CAL:        723        587        917       4521   Function call interrupts
 TLB:         36         68         18         43   TLB shootdowns
 TRM:          0          0          0          0   Thermal event interrupts
 THR:          0          0          0          0   Threshold APIC interrupts
 MCE:          0          0          0          0   Machine check exceptions
 MCP:          1          1          1          1   Machine check polls
 ERR:          0
 MIS:          0

My understanding for Mathias's explanation is, when a sd card is inserted, interrupt 49 is fired, but interrupt 40 will not be fired. 

Interrupt 49 is belong to GPIO controller. In interrupt 49 handler it directly calls the interrupt 40 registered handler.

For IRQ16, my understanding it's different than interrupt 40. It has an individual interrupt handler thread which will execute the predefined ACPI handler.

For example, if pin6 on GPIO is triggered, interrupt 16 is fired. The interrupt handler thread of IRQ16 will execute the ACPI handler.

Correct me if my understanding is wrong.

(In reply to Jin Yao from comment #25)
> 
> For IRQ16, my understanding it's different than interrupt 40. It has an
> individual interrupt handler thread which will execute the predefined ACPI
> handler.
> 
> For example, if pin6 on GPIO is triggered, interrupt 16 is fired. The
> interrupt handler thread of IRQ16 will execute the ACPI handler.
> 
> Correct me if my understanding is wrong.

Same still goes for the gpio triggered ACPI events as for the sd card. Early when gpio driver calls gpiochip_add() we register interrupt handlers for gpio triggered acpi events.

registration simplified:

gpiochip_add()   // pinctrl-baytraul.c
 acpi_gpiochip_add();   // gpiolib.c
  acpi_gpiochip_request_interrupts()  // gpiolib-acpi.c
  // search for gpio interrupts and acpi handlers from ACPI tables.
  // finds pin 6, pin 6 will get mapped to irq 16 in here: 
  irq = chip->to_irq(chip, pin);  // calls baytrail gpio_to_irq() which maps.
  devm_request_threaded_irq(..,irq,.., acpi_gpio_irq_handler); //set handler for interrupt descriptor 16. (irq = 16, pin = 6))

When gpio line 6 is set high, interrupt (0x31) fires, gpio driver interrupt handler is called -> finds line 6 was high -> line 6 maps to interrupt descriptor 16, calls the interrupt handler set for interrupt 16 -> acpi_gpio_irq_handler()

static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
{
	acpi_handle handle = data;

        acpi_evaluate_object(handle, NULL, NULL, NULL);

        return IRQ_HANDLED;
}

Ready.

(In reply to Jin Yao from comment #24)
> Another question confuses me for a while why the GPIO controller registered
> IRQ can't be visible in /proc/interrupts? They should be 48/49/50 on ASUS
> T100. 

I'm not sure, I remember something about proc/interrupts not showing chained handlers.

irq_set_chained_handler(hwirq, byt_gpio_irq_handler);

I just can't understand what the point in creating a irq domain, irq controller, but yet collide with a global name space for irq. 

since the gpio handler is the one manually calling the others, the irq's serviced by GPIO is just one required to get things going. 

pci seems to have the smarts to not request if msi was enabled, but i suspect its in a path which hits before the msi is enabled for graphics.

a simpler chance to avoid (again a hack) would be maybe have a base for the GPIO, and you request irq_create_mapping(vdomain, base+offset)

the real way to fix should be to have the irqs allocated from the interrupt controller domain, and then manage it that way.

Anoher issue I found is devm_request_threaded_irq() returning with failure. 

genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16
byt_gpio INT33FC:02: Failed to request IRQ 16 ACPI event handler
Return value of devm_request_threaded_irq() is -22.

Following is my modification, then it works.

/* Assume BIOS sets the triggering, so no flags */
ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
-                               0, "GPIO-signaled-ACPI-event",
+                               IRQF_ONESHOT, "GPIO-signaled-ACPI-event",
                                data);

(In reply to Jin Yao from comment #29)
> Anoher issue I found is devm_request_threaded_irq() returning with failure. 
> 
> genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 16
> byt_gpio INT33FC:02: Failed to request IRQ 16 ACPI event handler
> Return value of devm_request_threaded_irq() is -22.
> 
> Following is my modification, then it works.
> 
> /* Assume BIOS sets the triggering, so no flags */
> ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
> -                               0, "GPIO-signaled-ACPI-event",
> +                               IRQF_ONESHOT, "GPIO-signaled-ACPI-event",
>                                 data);

This is a known issue being resolved by Mika W. in a patchseries:

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

I also use the "IRQF_ONESHOT" flag while working on this issue until
patches are accepted.

(In reply to Ashok Raj from comment #28)
> I just can't understand what the point in creating a irq domain, irq
> controller, but yet collide with a global name space for irq. 

here's some explanation:
https://www.kernel.org/doc/Documentation/IRQ-domain.txt

I think the issue is at the "__irq_alloc_descs()".

__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
		  struct module *owner)
{
	int start, ret;

	if (!cnt)
		return -EINVAL;

	if (irq >= 0) {
		if (from > irq)
			return -EINVAL;
		from = irq;
	}

	mutex_lock(&sparse_irq_lock);

	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;

	if (start + cnt > nr_irqs) {
		ret = irq_expand_nr_irqs(start + cnt);
		if (ret)
			goto err;
	}

	bitmap_set(allocated_irqs, start, cnt);
	mutex_unlock(&sparse_irq_lock);
	return alloc_descs(start, cnt, node, owner);

err:
	mutex_unlock(&sparse_irq_lock);
	return ret;
}

At the case of i915 driver probing, the irq is 16 and bitmap_find_next_zero_area() returns 17. That's correct, because 16 is occupied by GPIO pin. The problem is at the line A, the checking is failed and goto "err:" directly.

Line A should be buggy code for this case, but I'm not sure if other code may be impacted if I just removed the line A here.

If the fix at the area of IRQ or graphics is not easy, can gpio take the responsiblity? I mean, since the irq16 is just a virtual interrupt, can it be replaced by GPIO internal variable ? 

For example, when gpio line 6 is set high, interrupt (0x31) fires, gpio driver interrupt handler is called -> finds line 6 was high -> line 6 maps to internal variable A -> calls the handler registered for internal variable A -> acpi_gpio_irq_handler()

Given we now know this is low level irq naughtiness in the x86 tree I've moved this to an x86 bug so that the x86 maintainers also notice and hopefully comment.

"low level irq naughtiness" :-) nicely put!

The whole irqdomain seems falling short of keeping these allocators local to the domain without clobbering with global irq. couple of ugly fixes possible, but long term i think irqdomain needs to be enhanced to do the right work.

So https://lkml.org/lkml/2014/4/13/190 has been submitted as a workaround for this (without a note in the bug...), but it completely and utterly fails to apply against current 3.15, for me:

5 out of 5 hunks FAILED -- saving rejects to file drivers/pinctrl/pinctrl-baytrail.c.rej

was it written against / intended for 3.14? The email doesn't seem to make it clear.

Thanks!

Created attachment 132331 [details]
IRQ conflict workaround patch

IRQ conflict workaround patch by Jin Yao

Comment on attachment 132331 [details]
IRQ conflict workaround patch

Hey Adam

I couldn't get the patch to apply, so I applied it manually. Here's the diff against my 3.15 tree. See if it helps.

7 out of 8 hunks fail, when applied against the current Fedora kernel tree, identified as 3.15.0-0.rc1.git2.1 , "Linux v3.15-rc1-49-g10ec34fcb100". That's whether I apply it before or after the patch from https://bugzilla.kernel.org/show_bug.cgi?id=67921 . No other patches in the Fedora kernel build seem to touch pinctrl-baytrail.c .

OK, so I just rediffed the https://lkml.org/lkml/2014/4/13/190 version of the patch. I'm now building a kernel with both that patch and the fix for https://bugzilla.kernel.org/show_bug.cgi?id=67921 from Doug Johnson, with the bit of Doug's patch that flat out disabled IRQ allocation (to avoid *this* bug) taken out - the idea being to see if we can get SDIO devices showing up and avoid the IRQ allocation conflict problem, all at once. I'll see how that goes.

Could you try the patch https://lkml.org/lkml/2014/4/16/278?

As noted elsewhere, I built a kernel as described in c#40 - with a rediffed version of the patch sent to ML, plus Doug's fix for 67921 - and it seems to 'work', as in SDIO devices are enumerated and the tablet doesn't crash. However, the touchscreen doesn't work, and Benjamin Tissoires believes this patch is the cause.

I'm now building a kernel without this patch, and with the version of Doug's fix that disables IRQ setup in pinctrl-baytrail.c , to see how that behaves.

I don't see how the version of the patch linked in c#41 is significantly different from the one I tested; is there some important difference?

It now appears to be Doug Johnson's patch from https://bugzilla.kernel.org/show_bug.cgi?id=67921 which causes the touchscreen issue, not this one. I will confirm this soon, but we think that's the case.

Well, hum.

First, I built a 3.15rc2 kernel that's stock Fedora Rawhide, except for disabling the backported hid-rmi driver which is also known to cause touchscreen issues. With that one, touchscreen works fine.

Then I added v3 of Doug Johnson's patch - http://dougvj.net/baytrail_gpio_quirk_v3.patch . With that kernel, touchscreen also works fine.

Then I added the patch for this bug, the version linked from c#41 (so the build has *both* the c#41 patch *and* Doug's v3 patch)...and the touchscreen stopped working again.

So, it looks to me like this patch really can break the touchscreen.

Proper solution to x86 io_apic code suggested by Thomas Gleixner

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

It moves the region where the dynamic interrupt are started from to
an area after the hardwired GSI interrupts. This way the interrupt descriptors should not conflict

with this fix no workaround/hack should be needed to pinctrl-baytrail

Code untested.

3.18-rc4 with CONFIG_PINCTRL_BAYTRAIL=y works for me on hardware that used to break with this option enabled and work with it disabled based on 3.15 or so (Aava Inari 8 tablet in 64-bit mode).  Anyone?