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
Yours,
Linus Walleij
