Re: [PATCH] Use delayed disable mode of ioapic edge triggeredinterrupts

[Linus Torvalds]

On Tue, 14 Nov 2006, Eric W. Biederman wrote:
> 
> The big one I did not set it on is the interrupt if it comes in
> through ExtInt.  I assume the 8259 is sane but I may be wrong.

Yes, ExtInt is ok, i fyou actually mask it at the 8259. As mentioned 
earlier in the thread, the i8259 has its edge detect logic _after_ the 
masking logic, so if the irq is still active, and you unmask it, it will 
see an edge, and re-assert the interrupt in hardware.

So the i8259 is a good interrupt controller, and does not need delayed 
disable and software logic to re-assert the irq.

> The truth is in practice I don't think it matters because I don't
> think anyone actually disables MSI or hypertransport interrupts.

Fair enough, at least for a 2.6.19 kind of release timeframe (and that is 
what I worry about most, at least right now).

> At this point I have two questions.
> - What is the easiest path to get us to a stable 2.6.19 where
>   everything works?

If people don't expect HT and MSI interrupts to be masked (and I can well 
imagine that), then I think your two-liner patch is good to go. Komuro 
seems to have acked it already, and in many ways that's the "minimal 
change" for 2.6.19 right now.

I do like Ingo's patch because it seems "safe" (even if I think it might 
be a bit _overly_ safe), but it changes semantics enough that I don't like 
it for 2.6.19. Even his second version definitely changes semantics for 
level-triggered PCI interrupts, even though he fixed ExtInt/i8259 ones.

So I think I'll go with your patch for now, and we can re-visit Ingo's 
thing after 2.6.19.

> - What is the sanest thing for long term maintenance, of irqs?
> 
>   genirq is less code to maintain overall (a plus).

Oh, I absolutely think genirq is the right thing to do. No question at 
all. I just think that we might want to refactor the code somewhat, and in 
particular I suspect that many irq controller drivers should use separate 
"struct irq_chip" entries for edge and level, because they are 
fundamentally different.

>   My gut feel is that there is room for a lot more cleanup in this
>   area but we probably need to stabilize what we have.

Exactly. Baby steps. Make it work. Then clean up. Slowly.

>   Since you aren't complaining about what the code actually does but
>   rather how the interface looks, I have a proposal.  I assert that
>   the interface for registering an irq is much to general, and broad.
> 
>   Instead of having:
> 
>   irq_desc[irq].status |= IRQ_DELAYED_DISABLE;
>   set_irq_chip_and_handler_name(irq, &ioapic_chip,
>                                      handle_edge_irq, "edge");
> 
>   We should have a set of helper functions one for each common type
>   of interrupt.
> 
>   set_irq_edge_lossy(irq, &ioapic_chip);
>   set_irq_edge(irq, &ioapic_chip);
>   set_irq_level(irq, &ioapic_chip);

Yeah, that might be a fine way too. That's largely what we do for the IO 
schedulers, and it's been fairly successful. Start out by setting common 
defaults, and then allow chip drivers to specify particular details 
explicitly.

Ingo?

			Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/