* [LART] pc_keyb.c changes
@ 2001-11-30 8:25 Alexander Viro
2001-11-30 9:04 ` Alexander Viro
2001-11-30 21:34 ` David C. Hansen
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Viro @ 2001-11-30 8:25 UTC (permalink / raw)
To: linux-kernel; +Cc: Linus Torvalds
Could the person who switched from BKL to spin_lock_irqsave() in
pc_keyb.c please share whatever the hell he had been smoking? Free clue:
disabling interrupts for long intervals to improve scalability is right up
there with fighting for peace and fucking for virginity.
Linus, could we please revert that crap and feed the authors to
Larry? If they are religious about Scalability At Any Cost, Common Sense
Be Damned(tm) - let's give them a chance to become martyrs...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LART] pc_keyb.c changes
2001-11-30 8:25 [LART] pc_keyb.c changes Alexander Viro
@ 2001-11-30 9:04 ` Alexander Viro
2001-11-30 21:34 ` David C. Hansen
1 sibling, 0 replies; 6+ messages in thread
From: Alexander Viro @ 2001-11-30 9:04 UTC (permalink / raw)
To: linux-kernel; +Cc: Linus Torvalds
On Fri, 30 Nov 2001, Alexander Viro wrote:
> Could the person who switched from BKL to spin_lock_irqsave() in
> pc_keyb.c please share whatever the hell he had been smoking? Free clue:
> disabling interrupts for long intervals to improve scalability is right up
> there with fighting for peace and fucking for virginity.
>
> Linus, could we please revert that crap and feed the authors to
> Larry? If they are religious about Scalability At Any Cost, Common Sense
> Be Damned(tm) - let's give them a chance to become martyrs...
BTW, while we are at it, one is _not_ supposed to do inclusion to/removal from
single-linked list without any exclusion whatsoever. Example:
drivers/input/evdev.c::evdev_{open,release}(). Or drivers/input/joydev.c
and drivers/input/mousedev.c, for that matter. Sigh...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LART] pc_keyb.c changes
2001-11-30 8:25 [LART] pc_keyb.c changes Alexander Viro
2001-11-30 9:04 ` Alexander Viro
@ 2001-11-30 21:34 ` David C. Hansen
2001-12-01 1:04 ` george anzinger
1 sibling, 1 reply; 6+ messages in thread
From: David C. Hansen @ 2001-11-30 21:34 UTC (permalink / raw)
To: Alexander Viro; +Cc: linux-kernel, Linus Torvalds
Alexander Viro wrote:
> Could the person who switched from BKL to spin_lock_irqsave() in
> pc_keyb.c please share whatever the hell he had been smoking? Free clue:
> disabling interrupts for long intervals to improve scalability is right up
> there with fighting for peace and fucking for virginity.
As I slowly raise my hand to take, um credit....
This is definitely one of the drivers I to take a second look at, now
that I know about the BKL being held for block and char device opens.
Do you have any ideas how else to do this safely since aux_count is
referenced during an interrupt?
--
Dave Hansen
dave@sr71.net
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LART] pc_keyb.c changes
2001-11-30 21:34 ` David C. Hansen
@ 2001-12-01 1:04 ` george anzinger
0 siblings, 0 replies; 6+ messages in thread
From: george anzinger @ 2001-12-01 1:04 UTC (permalink / raw)
To: David C. Hansen; +Cc: Alexander Viro, linux-kernel, Linus Torvalds
"David C. Hansen" wrote:
>
> Alexander Viro wrote:
>
> > Could the person who switched from BKL to spin_lock_irqsave() in
> > pc_keyb.c please share whatever the hell he had been smoking? Free clue:
> > disabling interrupts for long intervals to improve scalability is right up
> > there with fighting for peace and fucking for virginity.
> As I slowly raise my hand to take, um credit....
>
> This is definitely one of the drivers I to take a second look at, now
> that I know about the BKL being held for block and char device opens.
> Do you have any ideas how else to do this safely since aux_count is
> referenced during an interrupt?
>
Um, staying as far away from that bit of source as possible, I will
offer:
It depends on how it is referenced. If it is just a counter, you may be
able to just make it atomic. If it needs to "stick" for a little
longer, then consider if there are many readers and only a few writers,
in which case look at the read/write_lockirq code, however, this does
have the down side of irq off. BKL did not protect against interrupts,
so one wonders if the irq bit is needed at all.
--
George george@mvista.com
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Real time sched: http://sourceforge.net/projects/rtsched/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LART] pc_keyb.c changes
2001-12-01 2:05 David C. Hansen
@ 2001-12-01 9:56 ` Alan Cox
0 siblings, 0 replies; 6+ messages in thread
From: Alan Cox @ 2001-12-01 9:56 UTC (permalink / raw)
To: David C. Hansen
Cc: george anzinger, Alexander Viro, linux-kernel, Linus Torvalds,
Rick Lindsley
> if (rqst == PM_RESUME) {
> if (queue) { /* Aux port detected */
> - spin_lock_irqsave(&aux_count_lock, flags);
> + read_lock(&aux_count_lock);
> if ( aux_count == 0) { /* Mouse not in use */
Interrupts are off at this point. Its not an obvious detail without reading
the PM code, but it does cramp the style slightly 8(
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LART] pc_keyb.c changes
@ 2001-12-01 2:05 David C. Hansen
2001-12-01 9:56 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: David C. Hansen @ 2001-12-01 2:05 UTC (permalink / raw)
To: george anzinger
Cc: Alexander Viro, linux-kernel, Linus Torvalds, Rick Lindsley
george anzinger wrote:
> It depends on how it is referenced. If it is just a counter, you
> may be able to just make it atomic.
This was my first instinct in this case. But it appears that some things expect aux_count to stay in it's state while operations are performed. The operations on aux_queue look like they need locking.
> If it needs to "stick" for a little longer, then consider if there
> are many readers and only a few writers, in which case look at the
> read/write_lockirq code, however, this does have the down side of
> irq off.
I think that Al's biggest objection here is that interrupts are disabled. The rwlock idea is a good one because aux_count is only read in the interrupt. So, we don't have to disable interrupts, except when we hold the write. But, sadly, the only place this changes anything is in pckbd_pm_resume() and handle_mouse_event(). Open and release both need a write lock. I've attached a patch that turns aux_count_lock into a rw_lock. Let's hope I don't break 64-bit architectures again :)
> BKL did not protect against interrupts, so one wonders if the irq
> bit is needed at all.
I think that this case is one where the risk was minimal, and ignored.
--
Dave Hansen
haveblue@us.ibm.com
--- linux-2.5.1-pre5/drivers/char/pc_keyb.c Fri Nov 30 17:40:03 2001
+++ linux/drivers/char/pc_keyb.c Fri Nov 30 17:45:58 2001
@@ -90,7 +90,7 @@
static struct aux_queue *queue; /* Mouse data buffer. */
static int aux_count;
-static spinlock_t aux_count_lock = SPIN_LOCK_UNLOCKED;
+static rwlock_t aux_count_lock = RW_LOCK_UNLOCKED;
/* used when we send commands to the mouse that expect an ACK. */
static unsigned char mouse_reply_expected;
@@ -406,9 +406,9 @@
if (rqst == PM_RESUME) {
if (queue) { /* Aux port detected */
- spin_lock_irqsave(&aux_count_lock, flags);
+ read_lock(&aux_count_lock);
if ( aux_count == 0) { /* Mouse not in use */
- spin_lock(&kbd_controller_lock);
+ spin_lock_irqsave(&kbd_controller_lock, flags);
/*
* Dell Lat. C600 A06 enables mouse after resume.
* When user touches the pad, it posts IRQ 12
@@ -420,9 +420,9 @@
kbd_write_command(KBD_CCMD_WRITE_MODE);
kb_wait();
kbd_write_output(AUX_INTS_OFF);
- spin_unlock(&kbd_controller_lock);
+ spin_unlock_irqrestore(&kbd_controller_lock, flags);
}
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ read_unlock(&aux_count_lock);
}
}
#endif
@@ -452,7 +452,7 @@
prev_code = scancode;
add_mouse_randomness(scancode);
- spin_lock_irqsave(&aux_count_lock, flags);
+ read_lock(&aux_count_lock);
if ( aux_count ) {
int head = queue->head;
@@ -464,7 +464,7 @@
wake_up_interruptible(&queue->proc_list);
}
}
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ read_unlock(&aux_count_lock);
#endif
}
@@ -1054,12 +1054,12 @@
{
unsigned long flags;
fasync_aux(-1, file, 0);
- spin_lock_irqsave(&aux_count_lock, flags);
+ write_lock_irqsave(&aux_count_lock, flags);
if ( --aux_count ) {
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
return 0;
}
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
kbd_write_cmd(AUX_INTS_OFF); /* Disable controller ints */
kbd_write_command_w(KBD_CCMD_MOUSE_DISABLE);
aux_free_irq(AUX_DEV);
@@ -1076,18 +1076,18 @@
unsigned long flags;
int ret;
- spin_lock_irqsave(&aux_count_lock, flags);
+ write_lock_irqsave(&aux_count_lock, flags);
if ( aux_count++ ) {
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
return 0;
}
queue->head = queue->tail = 0; /* Flush input queue */
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
ret = aux_request_irq(keyboard_interrupt, AUX_DEV);
- spin_lock_irqsave(&aux_count_lock, flags);
+ write_lock_irqsave(&aux_count_lock, flags);
if (ret) {
aux_count--;
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
return -EBUSY;
}
kbd_write_command_w(KBD_CCMD_MOUSE_ENABLE); /* Enable the
@@ -1099,7 +1099,7 @@
mdelay(2); /* Ensure we follow the kbc access delay rules.. */
send_data(KBD_CMD_ENABLE); /* try to workaround toshiba4030cdt problem */
- spin_unlock_irqrestore(&aux_count_lock, flags);
+ write_unlock_irqrestore(&aux_count_lock, flags);
return 0;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2001-12-01 9:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-11-30 8:25 [LART] pc_keyb.c changes Alexander Viro
2001-11-30 9:04 ` Alexander Viro
2001-11-30 21:34 ` David C. Hansen
2001-12-01 1:04 ` george anzinger
2001-12-01 2:05 David C. Hansen
2001-12-01 9:56 ` Alan Cox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).