Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup

From: Andrea Arcangeli (andrea@suse.de)
Date: Mon Oct 16 2000 - 19:55:52 EST


On Mon, Oct 16, 2000 at 05:31:47PM -0400, Jeff Garzik wrote:
> Andrea Arcangeli wrote:
> >
> > On Sun, Oct 15, 2000 at 03:48:55PM -0400, Jeff Garzik wrote:
> > > Changes:
> > > * both: we know we are in an interrupt, so
> > > s/spin_lock_irqsave/spin_lock/
> >
> > There request_irq is not called passing the SA_INTERRUPT flag so the irq
> > handler is recalled with irqs enabled and in turn irqsave is necessary.
>
> hmmm. Can you elaborate on this a bit?
>
> I didn't know that bit about !SA_INTERRUPT, but why is irqsave necessary
> in these drivers? [..]

if you do:

        request_irq(&my_irq_handler,... 0, ...)

then my_irq_handler will be recalled with irq enabled.

If you do:

        request_irq(&my_irq_handler,... SA_INTERRUPT, ...)

then my_irq_handler will be recalled with irq disabled.

SA_INTERRUPT only controls if an irq handler is recalled with irq
enabled or disabled (locally of course).

keyboard_interrupt is registered w/o SA_INTERRUPT so it will be
recalled with irq enabled and so you someway need to do a __cli() before
spinning on the lock.

>[..] I don't see handle_kbd_event() being called from a
> timer or anything special like that..

Timer, bottomhalves (softirq) and tasklets (and softnet) are always
recalled with irq enabled. So if it would be called by timer/tasklet/bhhandler
it should use irq version of the spinlocks too if it needs to run with irq
locally disabled.

One thing you could safely change in keyboard_interrupt is to remove the save
part of the spinlock by using spin_lock_irq (we don't need to save anything
since keyboard_interrupt is only recalled as an irq handler).

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



This archive was generated by hypermail 2b29 : Mon Oct 23 2000 - 21:00:10 EST