Hi,
On 18-08-19 00:26, Linus Walleij wrote:
On Fri, Aug 16, 2019 at 9:42 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
On 8/16/19 7:16 PM, Linus Walleij wrote:
Sorry but just so I understand this report: when you say the
interrupt storm is back, do you mean that this patch brings
it back, or do you mean the interrupt storm is there
even before this patch?
I mean that the patch brings it back, iow the patch causes
a regression.
Gnah that's too bad. :/
This patch does bring semantic differences, but very
small ones.
Could it be that the parent device of the IRQ controller is
different after this?
If you mean parent_device in struct irq_chip then no,
the gpiolib core doesn't touch that neither before or
after this patch.
I think that the ACPI subsys relies
on the parent being the INT0002 ACPI instantiated platform
device.
But this driver never sets up parent_device in struct
irq_chip even today... it just passes in in pretty open coded
with NULL as parent_device (compiletime default).
Notice that the driver has attached itself as shared irq-handler
to the ACPI IRQ
What is it sharing it with?
With the ACPI subsys, this IRQ is called the GPE which is an
interrupt normally reserved for events to be handled by the
ACPI subsys.
Aha I get it, I think.
The ACPI subsys has the ability to attach an event handler
written in ACPI byte code (AML code) to GPIO events (GPIO
triggered IRQs), quoting the same piece of AML as before:
Device (GPED)
{
Name (_ADR, Zero) // _ADR: Address
Name (_HID, "INT0002" /* Virtual GPIO Controller */) // _HID: Hardw
Name (_CID, "INT0002" /* Virtual GPIO Controller */) // _CID: Compa
Name (_DDN, "Virtual GPIO controller") // _DDN: DOS Device Name
...
Method (_AEI, 0, NotSerialized) // _AEI: ACPI Event Interrupts
{
Name (RBUF, ResourceTemplate ()
{
GpioInt (Level, ActiveHigh, ExclusiveAndWake, PullDown, 0x00
"\\_SB.GPED", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0002
}
})
Return (RBUF) /* \_SB_.GPED._AEI.RBUF */
}
Method (_L02, 0, NotSerialized) // _Lxx: Level-Triggered GPE
{
...
}
}
So what we are seeing here is an AEI (ACPI Event Interrupt) entry pointing
to pin 2 of the INT0002 GPIO chip, note this is not a real GPIO chip, but
a way to attach an ACPI event handler to GPE interrupts, while only
running the event handler when a specific status bit is set.
I see ... that's a bit complex way to solve an easy problem but I
guess the ACPI gods want it that way.
So what the ACPI subsys does is it looksup the GPIO with index 2
on the INT0003 gpiochip (which is unchanged) and the calls gpio_to_irq
on the found GPIO, it seems that the gpio_to_irq is no longer working,
causing the:
123: 0 0 0 0 INT0002 Virtual GPIO 2 ACPI:Event
Line in cat/ /proc/interrupts to disappear. That or gpio_to_irq works
but then for some reason the subsequent request irq fails.
OK I get it, I try to see what the problem may come from.
I suspect gpio[d]_to_irq isn't working as expected for some
reason.
We are checking the valid_mask to see if the IRQ is valid for
mapping. Could it be that something is wrong with the valid_mask?
It used to be that we:
1. Set up the (only) GPIO chip
2. Set up the valid mask (now allocated)
3. Register the irqchip
4. Register the irqhandler
and now we instead:
1. Set up the (only) GPIO chip
2. Register the irqchip
3. Register the irqhandler
4. Set up the valid mask (now allocated)
The valid_mask in the GPIO chip itself has a special callback
to set up the mask, maybe we need to have the same for
the irqchip if that needs to happen in the same flow as
before.
It's a weird driver cascading a single GPIO IRQ that isn't
really a GPIO so my head is spinning a bit :D
Ok, so the change to when the valid mask is set sounds like a possible
reason for the regression.
The GPIO lookup and gpio[d]_to_irq call happen from
acpi_gpiochip_request_interrupts which gets called at the end of
gpiochip_add_irqchip.
So yes it sounds like the irqmask getting set too late may very well
be the problem here.
Note that the acpi_gpiochip_request_interrupts call also does a
gpiochip_lock_as_irq call on the GPIO.
Basically the 3 relevant calls are:
desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event",
GPIO_ACTIVE_HIGH, GPIOD_IN);
ret = gpiochip_lock_as_irq(chip, pin);
irq = gpiod_to_irq(desc);
I see now that all 3 of these have error handling + dev_err calls,
so if you want I can retest and look for errors in dmesg, I guess I
should have done that right away...
Regards,
Hans
