Re: Reworking suspend-resume sequence (was: Re: PCI PM: Restore standard config registers of all devices early)

Ingo Molnar
2009-02-03T19:53:42Z

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, 3 Feb 2009, Ingo Molnar wrote:
> > >
> > > I'm really not sure why that handle_edge_irq thing uses "ack_and_mask()"
> > > instead of just "desc->chip->ack()"? I'm also totally flummoxed as to why
> > > it feels it needs to go all the way out to the device to mask things,
> > > instead of just masking at an apic level, which is much simpler and faster
> > > (especially since masking should never happen in practice anyway).
> >
> > Hm, do you mean mask_ack_irq()?
>
> Yes.
>
> > The ->mask_ack() irqchip method is just a
> > small tweak normally: if we get an irq while the irq was disabled, we can
> > mark it pending and masks it for real.
>
> No, I know why mask_ack_irq() does what it does and I agree with it. What
> I was really reacting to was that handle_edge_irq() calls it at _all_.
> IOW, I'm talking about this code:
>
> handle_edge_irq(unsigned int irq, struct irq_desc *desc)
> ...
> if (unlikely((desc->status & (IRQ_INPROGRESS | IRQ_DISABLED)) ||
> !desc->action)) {
> desc->status |= (IRQ_PENDING | IRQ_MASKED);
> mask_ack_irq(desc, irq);
> ..
>
> where the masking part seems a bit pointless. And in the case of MSI, it
> causes us to go all the way out to the device, which sounds pretty
> expensive too.

hm, i agree that your patched version looks much simpler.

There's two reasons we have this variant:

- huge bikeshed painting thread in the early days of the genirq patchset,
with folks expressing concern about our original plans to keep
edge-triggered unmasked _always_. (which your patch does too)
So we just went with the path of least resistence and used this hybride.

- the screaming-irq observation i had - do you consider that valid?:

>> [ In theory this also solves screaming level-triggered irqs that
>> advertise themselves as edge-triggered [due to firmware/BIOS bug -
>> these do happen] and then keep spamming the system. ]

I wanted to have a pretty much interchangeable flow method between edge
and level triggered - so that the BIOS cannot screw us by enumerating an
irq as edge-triggered while it's level-triggered.

Especially for legacy x86 irqs in the low <16 range the trigger mode can
be influenced by chipset settings and might not always be what we think
it is.

That's my rough recollection - Thomas, is that correct and do you have
anything to add here?

Ingo