Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 07:21:04PM -0400, Jeff Garzik wrote: > I wonder if q40_keyb has the same thing to worry about It seems no, it looks like we can remove the irqsave from there. Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Andrea Arcangeli wrote: > > On Tue, Oct 17, 2000 at 05:26:23AM -0400, Jeff Garzik wrote: > > Well, the -spin lock- exists for serialization. My question is Why > > does pc_keyb irq handler disable local irqs for this case? What is the > > race/deadlock that exists with spin_lock in the irq handler, but does > > not exist with spin_lock_irqsave in the irq handler? > > As said the save part isn't necessary. > > This is a trace of the deadlock: > > irq 2 runs > keyboard_interrupt > irqs are been left enabled > spin_lock() > here irq 12 runs > keyboard_interrupt > here doesn't matter if irqs are enabled or disabled of course > spin_lock() -> dealdock Thanks a bunch Andrea. That's the piece I was looking for -- I didn't know that two different irqs were calling the same code. Learn something new every day :) I wonder if q40_keyb has the same thing to worry about -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 05:26:23AM -0400, Jeff Garzik wrote:
> Well, the -spin lock- exists for serialization. My question is Why
> does pc_keyb irq handler disable local irqs for this case? What is the
> race/deadlock that exists with spin_lock in the irq handler, but does
> not exist with spin_lock_irqsave in the irq handler?
As said the save part isn't necessary.
This is a trace of the deadlock:
irq 2 runs
keyboard_interrupt
irqs are been left enabled
spin_lock()
here irq 12 runs
keyboard_interrupt
here doesn't matter if irqs are enabled or disabled of course
spin_lock() -> dealdock
Really if you really want to, you could remove the local irq disable by
replacing it with a disable_irq_nosync(other irq). Something like this is safe
and obviously right. But I don't think the keyboard_interrupt is long enough to
justify playing with the irq controller [that in IA32 UP isn't memory mapped]
instead of using the fast in CPU core local cli, also the cli version allows
the other irq to be started in parallel to us in another CPU.
keyboard_interrupt(irq, ...) {
int other_irq;
switch(irq) {
case KEYBOARD_IRQ:
other_irq = AUX_IRQ;
break;
case AUX_IRQ:
other_irq = KEYBOARD_IRQ;
break;
default:
panic("wrong irq");
}
disable_irq_nosync(other_irq);
spin_lock();
-- critical section --
spin_unlock();
enable_irq(other_irq);
}
BTW, after reading the code some more it seems you could remove the irq disable
also if CONFIG_PSMOUSE is not defined. So you can probably also do:
keyboard_interrupt(irq, ...) {
#ifdef CONFIG_PSMOUSE
int other_irq;
switch(irq) {
case KEYBOARD_IRQ:
other_irq = AUX_IRQ;
break;
case AUX_IRQ:
other_irq = KEYBOARD_IRQ;
break;
default:
panic("wrong irq");
}
disable_irq_nosync(other_irq);
#endif
spin_lock();
-- critical section --
spin_unlock();
#ifdef CONFIG_PSMOUSE
enable_irq(other_irq);
#endif
}
Or the __cli version:
keyboard_interrupt(irq, ...) {
#ifdef CONFIG_PSMOUSE
__cli();
#endif
spin_lock();
-- critical section --
spin_unlock();
#ifdef CONFIG_PSMOUSE
__sti(); /* this wouldn't be necessary if 2.4.x would
honour SA_INTERRUPT for the shared irqs
as 2.2.x */
#endif
}
Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 05:43:34PM +0200, Jes Sorensen wrote: > Which shouldn't matter as the irq source should be disabled. In fact I > thought we were guaranteed not to be re-interrupted in a handler > unless one explicitly does __sti(), has this changed? A single irq handler won't be re-interrupted, correct. Not even if you do __sti(). (in the edge triggered IO-APIC case, the irq source is not disabled to avoid missing events but the highlevel irq logic makes sure that the irq _handler_ won't be run if it was just in-progress somewhere in the system, even if in another CPU) But the fact the irq handler is single threaded with respect to itself is irrelevant with the keyboard_interrupt case because that irq handler will be recalled by _two_ indipendent irq lines (irq 2 for the keyboard and irq 12 for the PS/2 Mouse). Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
> "Andrea" == Andrea Arcangeli <[EMAIL PROTECTED]> writes: Andrea> On Mon, Oct 16, 2000 at 05:31:47PM -0400, Jeff Garzik wrote: Andrea> if you do: Andrea> request_irq(_irq_handler,... 0, ...) Andrea> then my_irq_handler will be recalled with irq enabled. Which shouldn't matter as the irq source should be disabled. In fact I thought we were guaranteed not to be re-interrupted in a handler unless one explicitly does __sti(), has this changed? Jes - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Andrea Arcangeli wrote: > On Mon, Oct 16, 2000 at 09:31:42PM -0400, Jeff Garzik wrote: > >[..] Why are local interrupts disabled [in pc_keyb.c]? > > To avoid to deadlock on the kbd_controller_lock in SMP or to race in UP. > > Both irq handler and normal kernel context (open/close) will play with the keyb > controller at around address 0x6x and accesses are serialized via the > kbd_controller_lock. Well, the -spin lock- exists for serialization. My question is Why does pc_keyb irq handler disable local irqs for this case? What is the race/deadlock that exists with spin_lock in the irq handler, but does not exist with spin_lock_irqsave in the irq handler? For UP, using spin_lock_irqsave in user context, and spin_lock in the irq handler, irqs are disabled for user context, and user context is disabled while the irq handler is running, so no race, and no additional synchronization needed. For SMP, using spin_lock_irqsave in user context, and spin_lock in the irq handler, the irq handler and user context need only to spin_lock to protect the kbd controller. If the above two paragraphs are correct... then why was my patch incorrect? Sorry that I'm a bit slow here, but I just don't see why s/spin_lock_irqsave/spin_lock/ is wrong... Jeff -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
"Andrea" == Andrea Arcangeli [EMAIL PROTECTED] writes: Andrea On Mon, Oct 16, 2000 at 05:31:47PM -0400, Jeff Garzik wrote: Andrea if you do: Andrea request_irq(my_irq_handler,... 0, ...) Andrea then my_irq_handler will be recalled with irq enabled. Which shouldn't matter as the irq source should be disabled. In fact I thought we were guaranteed not to be re-interrupted in a handler unless one explicitly does __sti(), has this changed? Jes - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 05:43:34PM +0200, Jes Sorensen wrote: Which shouldn't matter as the irq source should be disabled. In fact I thought we were guaranteed not to be re-interrupted in a handler unless one explicitly does __sti(), has this changed? A single irq handler won't be re-interrupted, correct. Not even if you do __sti(). (in the edge triggered IO-APIC case, the irq source is not disabled to avoid missing events but the highlevel irq logic makes sure that the irq _handler_ won't be run if it was just in-progress somewhere in the system, even if in another CPU) But the fact the irq handler is single threaded with respect to itself is irrelevant with the keyboard_interrupt case because that irq handler will be recalled by _two_ indipendent irq lines (irq 2 for the keyboard and irq 12 for the PS/2 Mouse). Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 05:26:23AM -0400, Jeff Garzik wrote:
Well, the -spin lock- exists for serialization. My question is Why
does pc_keyb irq handler disable local irqs for this case? What is the
race/deadlock that exists with spin_lock in the irq handler, but does
not exist with spin_lock_irqsave in the irq handler?
As said the save part isn't necessary.
This is a trace of the deadlock:
irq 2 runs
keyboard_interrupt
irqs are been left enabled
spin_lock()
here irq 12 runs
keyboard_interrupt
here doesn't matter if irqs are enabled or disabled of course
spin_lock() - dealdock
Really if you really want to, you could remove the local irq disable by
replacing it with a disable_irq_nosync(other irq). Something like this is safe
and obviously right. But I don't think the keyboard_interrupt is long enough to
justify playing with the irq controller [that in IA32 UP isn't memory mapped]
instead of using the fast in CPU core local cli, also the cli version allows
the other irq to be started in parallel to us in another CPU.
keyboard_interrupt(irq, ...) {
int other_irq;
switch(irq) {
case KEYBOARD_IRQ:
other_irq = AUX_IRQ;
break;
case AUX_IRQ:
other_irq = KEYBOARD_IRQ;
break;
default:
panic("wrong irq");
}
disable_irq_nosync(other_irq);
spin_lock();
-- critical section --
spin_unlock();
enable_irq(other_irq);
}
BTW, after reading the code some more it seems you could remove the irq disable
also if CONFIG_PSMOUSE is not defined. So you can probably also do:
keyboard_interrupt(irq, ...) {
#ifdef CONFIG_PSMOUSE
int other_irq;
switch(irq) {
case KEYBOARD_IRQ:
other_irq = AUX_IRQ;
break;
case AUX_IRQ:
other_irq = KEYBOARD_IRQ;
break;
default:
panic("wrong irq");
}
disable_irq_nosync(other_irq);
#endif
spin_lock();
-- critical section --
spin_unlock();
#ifdef CONFIG_PSMOUSE
enable_irq(other_irq);
#endif
}
Or the __cli version:
keyboard_interrupt(irq, ...) {
#ifdef CONFIG_PSMOUSE
__cli();
#endif
spin_lock();
-- critical section --
spin_unlock();
#ifdef CONFIG_PSMOUSE
__sti(); /* this wouldn't be necessary if 2.4.x would
honour SA_INTERRUPT for the shared irqs
as 2.2.x */
#endif
}
Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Andrea Arcangeli wrote: On Tue, Oct 17, 2000 at 05:26:23AM -0400, Jeff Garzik wrote: Well, the -spin lock- exists for serialization. My question is Why does pc_keyb irq handler disable local irqs for this case? What is the race/deadlock that exists with spin_lock in the irq handler, but does not exist with spin_lock_irqsave in the irq handler? As said the save part isn't necessary. This is a trace of the deadlock: irq 2 runs keyboard_interrupt irqs are been left enabled spin_lock() here irq 12 runs keyboard_interrupt here doesn't matter if irqs are enabled or disabled of course spin_lock() - dealdock Thanks a bunch Andrea. That's the piece I was looking for -- I didn't know that two different irqs were calling the same code. Learn something new every day :) I wonder if q40_keyb has the same thing to worry about -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Tue, Oct 17, 2000 at 07:21:04PM -0400, Jeff Garzik wrote: I wonder if q40_keyb has the same thing to worry about It seems no, it looks like we can remove the irqsave from there. Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Mon, Oct 16, 2000 at 09:31:42PM -0400, Jeff Garzik wrote: > I understand SA_INTERRUPT, my question in the previous e-mail was more > basic: keyboard_interrupt calls handle_kbd_event with local interrupts > disabled. [..] Woops sorry, I misunderstood your question. >[..] Why are local interrupts disabled? To avoid to deadlock on the kbd_controller_lock in SMP or to race in UP. Both irq handler and normal kernel context (open/close) will play with the keyb controller at around address 0x6x and accesses are serialized via the kbd_controller_lock. Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Andrea Arcangeli wrote: > 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). I understand SA_INTERRUPT, my question in the previous e-mail was more basic: keyboard_interrupt calls handle_kbd_event with local interrupts disabled. Why are local interrupts disabled? -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
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? I don't see handle_kbd_event() being called from a timer or anything special like that.. -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Mon, 16 Oct 2000, 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. And don't try to use SA_INTERRUPT: it is a no-op on many architectures, including, in the future, x86. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Mon, 16 Oct 2000, 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. And don't try to use SA_INTERRUPT: it is a no-op on many architectures, including, in the future, x86. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
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? I don't see handle_kbd_event() being called from a timer or anything special like that.. -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Andrea Arcangeli wrote: 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). I understand SA_INTERRUPT, my question in the previous e-mail was more basic: keyboard_interrupt calls handle_kbd_event with local interrupts disabled. Why are local interrupts disabled? -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
On Mon, Oct 16, 2000 at 09:31:42PM -0400, Jeff Garzik wrote: I understand SA_INTERRUPT, my question in the previous e-mail was more basic: keyboard_interrupt calls handle_kbd_event with local interrupts disabled. [..] Woops sorry, I misunderstood your question. [..] Why are local interrupts disabled? To avoid to deadlock on the kbd_controller_lock in SMP or to race in UP. Both irq handler and normal kernel context (open/close) will play with the keyb controller at around address 0x6x and accesses are serialized via the kbd_controller_lock. Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
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. Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Changes:
* both: we know we are in an interrupt, so
s/spin_lock_irqsave/spin_lock/
* both: kbd_controller_lock is local to the module, mark it 'static'
* pc_keyb: move the printk out of the loop
--
Jeff Garzik| The difference between laziness and
Building 1024 | prioritization is the end result.
MandrakeSoft |
diff -urN vanilla/linux-2.4.0-test10-pre3/drivers/char/pc_keyb.c
linux_2_3/drivers/char/pc_keyb.c
--- vanilla/linux-2.4.0-test10-pre3/drivers/char/pc_keyb.c Mon Aug 28 15:06:33
2000
+++ linux_2_3/drivers/char/pc_keyb.cSun Oct 15 12:15:36 2000
@@ -65,7 +65,7 @@
static void __aux_write_ack(int val);
#endif
-spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
+static spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
static unsigned char handle_kbd_event(void);
/* used only by send_data - set by keyboard_interrupt */
@@ -448,7 +448,7 @@
unsigned char status = kbd_read_status();
unsigned int work = 1;
- while (status & KBD_STAT_OBF) {
+ while ((--work > 0) && (status & KBD_STAT_OBF)) {
unsigned char scancode;
scancode = kbd_read_input();
@@ -467,13 +467,10 @@
}
status = kbd_read_status();
-
- if (!--work) {
- printk(KERN_ERR "pc_keyb: controller jammed (0x%02X).\n",
- status);
- break;
- }
}
+
+ if (!work)
+ printk(KERN_ERR "pc_keyb: controller jammed (0x%02X).\n", status);
return status;
}
@@ -481,14 +478,13 @@
static void keyboard_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- unsigned long flags;
-
#ifdef CONFIG_VT
kbd_pt_regs = regs;
#endif
- spin_lock_irqsave(_controller_lock, flags);
+
+ spin_lock(_controller_lock);
handle_kbd_event();
- spin_unlock_irqrestore(_controller_lock, flags);
+ spin_unlock(_controller_lock);
}
/*
diff -urN vanilla/linux-2.4.0-test10-pre3/drivers/char/q40_keyb.c
linux_2_3/drivers/char/q40_keyb.c
--- vanilla/linux-2.4.0-test10-pre3/drivers/char/q40_keyb.c Wed Sep 8 14:51:22
1999
+++ linux_2_3/drivers/char/q40_keyb.c Sun Oct 15 12:15:36 2000
@@ -94,7 +94,7 @@
};
-spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
+static spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
/*
@@ -340,11 +340,10 @@
static void keyboard_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- unsigned long flags;
unsigned char status;
disable_keyboard();
- spin_lock_irqsave(_controller_lock, flags);
+ spin_lock(_controller_lock);
kbd_pt_regs = regs;
status = IRQ_KEYB_MASK & master_inb(INTERRUPT_REG);
@@ -386,7 +385,7 @@
keyup=1;
}
exit:
- spin_unlock_irqrestore(_controller_lock, flags);
+ spin_unlock(_controller_lock);
master_outb(-1,KEYBOARD_UNLOCK_REG); /* keyb ints reenabled herewith */
enable_keyboard();
}
PATCH 2.4.0.10.3: pc_keyb and q40_keyb cleanup
Changes:
* both: we know we are in an interrupt, so
s/spin_lock_irqsave/spin_lock/
* both: kbd_controller_lock is local to the module, mark it 'static'
* pc_keyb: move the printk out of the loop
--
Jeff Garzik| The difference between laziness and
Building 1024 | prioritization is the end result.
MandrakeSoft |
diff -urN vanilla/linux-2.4.0-test10-pre3/drivers/char/pc_keyb.c
linux_2_3/drivers/char/pc_keyb.c
--- vanilla/linux-2.4.0-test10-pre3/drivers/char/pc_keyb.c Mon Aug 28 15:06:33
2000
+++ linux_2_3/drivers/char/pc_keyb.cSun Oct 15 12:15:36 2000
@@ -65,7 +65,7 @@
static void __aux_write_ack(int val);
#endif
-spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
+static spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
static unsigned char handle_kbd_event(void);
/* used only by send_data - set by keyboard_interrupt */
@@ -448,7 +448,7 @@
unsigned char status = kbd_read_status();
unsigned int work = 1;
- while (status KBD_STAT_OBF) {
+ while ((--work 0) (status KBD_STAT_OBF)) {
unsigned char scancode;
scancode = kbd_read_input();
@@ -467,13 +467,10 @@
}
status = kbd_read_status();
-
- if (!--work) {
- printk(KERN_ERR "pc_keyb: controller jammed (0x%02X).\n",
- status);
- break;
- }
}
+
+ if (!work)
+ printk(KERN_ERR "pc_keyb: controller jammed (0x%02X).\n", status);
return status;
}
@@ -481,14 +478,13 @@
static void keyboard_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- unsigned long flags;
-
#ifdef CONFIG_VT
kbd_pt_regs = regs;
#endif
- spin_lock_irqsave(kbd_controller_lock, flags);
+
+ spin_lock(kbd_controller_lock);
handle_kbd_event();
- spin_unlock_irqrestore(kbd_controller_lock, flags);
+ spin_unlock(kbd_controller_lock);
}
/*
diff -urN vanilla/linux-2.4.0-test10-pre3/drivers/char/q40_keyb.c
linux_2_3/drivers/char/q40_keyb.c
--- vanilla/linux-2.4.0-test10-pre3/drivers/char/q40_keyb.c Wed Sep 8 14:51:22
1999
+++ linux_2_3/drivers/char/q40_keyb.c Sun Oct 15 12:15:36 2000
@@ -94,7 +94,7 @@
};
-spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
+static spinlock_t kbd_controller_lock = SPIN_LOCK_UNLOCKED;
/*
@@ -340,11 +340,10 @@
static void keyboard_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- unsigned long flags;
unsigned char status;
disable_keyboard();
- spin_lock_irqsave(kbd_controller_lock, flags);
+ spin_lock(kbd_controller_lock);
kbd_pt_regs = regs;
status = IRQ_KEYB_MASK master_inb(INTERRUPT_REG);
@@ -386,7 +385,7 @@
keyup=1;
}
exit:
- spin_unlock_irqrestore(kbd_controller_lock, flags);
+ spin_unlock(kbd_controller_lock);
master_outb(-1,KEYBOARD_UNLOCK_REG); /* keyb ints reenabled herewith */
enable_keyboard();
}
