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

Ingo Molnar
2009-02-03T19:14:03Z

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

> On Tue, 3 Feb 2009, Linus Torvalds wrote:
> >
> > That said, we've also never had much reason to _care_ deeply, so it's also
> > possible that we do mask things over some path. I didn't actually walk
> > _all_ the paths, and the logic for irq handling has changed enough over
> > the years that I don't know all the paths any more. Maybe we do that
> > explicit mask in some path I missed. We _shouldn't_, but who knows..
>
> Ok, so I decided to actually try to walk it all. Better look at the actual
> code.
>
> Hmm. The _normal_ simple irq handler does this the way I described, but
> for some reason the "handle_edge_irq()" does not. And the reason is
> actually a buglet: it needs to mask things for the "recursive interrupt"
> case.
>
> But that literally just looks like a small implementation detail (the code
> decided to share the code for IRQ_INPROGRESS and IRQ_DISABLED). We should
> fix it, so that you _can_ disable irqs and not have to worry about this
> all.
>
> 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()? 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.

It's optional for a PIC implementation to provide it and the generic code
does it via ->mask() + ->ack() if the PIC implementation keeps it NULL.

[ 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 have not done a deep audit, normally (on x86 at least) ->mask_ack() should
not touch any lowlevel device bits (only the interrupt controller bits).
Have you found a case where it does?

That would be arguably broken i think - we should not touch lowlevel device
bits from the current generation of PIC code really, there's just no point.

Ingo