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

[Ingo Molnar <mingo@elte.hu>]

* 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