Re: [patch] irq rewrite

Richard Henderson (rth@twiddle.net)
Tue, 7 Dec 1999 21:39:36 -0800


Some comments on all this (some of this wonders about the x86
stuff too, since Andrea is trying to share hw_interrupt_type).

(1) What in the world is the difference between shutdown/disable
end/enable supposed to be in hw_interrupt_type? There appear to
be zero instances of such a difference in the existing code base.

(2) Anyone notice that the visws_apic code apparently hasn't been
updated to some change in hw_interrupt_type?

(3) I think you should continue to manage _alpha_irq_masks from
generic code instead of having a separate instance of cached_irq_mask
from virtually every sys_foo.c.

(4)
+ if (irqflags & SA_SHIRQ) {
+ if (!dev_id)
+ printk("Bad boy: %s (at 0x%x) called us without \
+ a dev_id\n", devname, (&irq)[-1]);

Careful, that should be %p and __builtin_return_address.
Obviously directly copied from x86, but it's technically
wrong there too.

(5) I don't think you can get rid of PROBE_MASK, though it's kind
of hard to see from just the patch. Does your replacement code
have some mechanism to prevent certain IRQs from being probed?

Similarly,

+ for (i = NR_IRQS-1; i > 0; i--)
+ if (!irq_desc[i].action)
+ irq_desc[i].handler->startup(i);

should probably be iterating over ACTUAL_NR_IRQS instead.

(6)
case 1:
- handle_irq(RTC_IRQ, -1, &regs);
+ handle_irq(RTC_IRQ, &regs);

A timer tick that comes in through entInt(1,...) need not
and should not be acked. PALcode has already done that.
I don't see a mechanism for this.

That's all I see for now. If I get ambitious this evening
I may see about updating the patch for all of sys_foo.c and
try it out on some other systems.

r~