* [patch] small tty irq race fix
@ 2003-03-03 18:09 Nicolas Pitre
2003-03-03 20:06 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Pitre @ 2003-03-03 18:09 UTC (permalink / raw)
To: torvalds; +Cc: lkml
--- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003
+++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 13:02:31 2003
@@ -1947,17 +1947,17 @@
if (tty->flip.buf_num) {
cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
- tty->flip.buf_num = 0;
local_irq_save(flags); // FIXME: is this safe?
+ tty->flip.buf_num = 0;
tty->flip.char_buf_ptr = tty->flip.char_buf;
tty->flip.flag_buf_ptr = tty->flip.flag_buf;
} else {
cp = tty->flip.char_buf;
fp = tty->flip.flag_buf;
- tty->flip.buf_num = 1;
local_irq_save(flags); // FIXME: is this safe?
+ tty->flip.buf_num = 1;
tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
}
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] small tty irq race fix 2003-03-03 18:09 [patch] small tty irq race fix Nicolas Pitre @ 2003-03-03 20:06 ` Alan Cox 2003-03-03 19:15 ` Nicolas Pitre 2003-03-03 21:20 ` Nicolas Pitre 0 siblings, 2 replies; 6+ messages in thread From: Alan Cox @ 2003-03-03 20:06 UTC (permalink / raw) To: Nicolas Pitre; +Cc: Linus Torvalds, lkml > fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; > - tty->flip.buf_num = 0; > > local_irq_save(flags); // FIXME: is this safe? > + tty->flip.buf_num = 0; The other CPU can be touching these fields too surely. Its a useful note that the spinlocks need putting in the right spot but its still broken 8( ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] small tty irq race fix 2003-03-03 20:06 ` Alan Cox @ 2003-03-03 19:15 ` Nicolas Pitre 2003-03-03 21:20 ` Nicolas Pitre 1 sibling, 0 replies; 6+ messages in thread From: Nicolas Pitre @ 2003-03-03 19:15 UTC (permalink / raw) To: Alan Cox; +Cc: Linus Torvalds, lkml On 3 Mar 2003, Alan Cox wrote: > > > fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; > > - tty->flip.buf_num = 0; > > > > local_irq_save(flags); // FIXME: is this safe? > > + tty->flip.buf_num = 0; > > The other CPU can be touching these fields too surely. Its a > useful note that the spinlocks need putting in the right spot > but its still broken 8( Well I only care about UP at the moment and the patch makes it right for UP at least. Someone with his brain around the tty locking requirements can look at replacing the local_irq_save() (there and elsewhere as well), which is sort of a different issue. Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] small tty irq race fix 2003-03-03 20:06 ` Alan Cox 2003-03-03 19:15 ` Nicolas Pitre @ 2003-03-03 21:20 ` Nicolas Pitre 2003-03-03 21:39 ` Linus Torvalds 1 sibling, 1 reply; 6+ messages in thread From: Nicolas Pitre @ 2003-03-03 21:20 UTC (permalink / raw) To: Alan Cox; +Cc: Linus Torvalds, lkml On Mon, 3 Mar 2003, Alan Cox wrote: > > > fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; > > - tty->flip.buf_num = 0; > > > > local_irq_save(flags); // FIXME: is this safe? > > + tty->flip.buf_num = 0; > > The other CPU can be touching these fields too surely. Its a > useful note that the spinlocks need putting in the right spot > but its still broken 8( What about this one? It just happens that tty->read_lock is actually used deeper in the same call instance (in n_tty.c) so this looks to be the best lock to use. --- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003 +++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 16:13:30 2003 @@ -1947,23 +1947,23 @@ if (tty->flip.buf_num) { cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE; fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; - tty->flip.buf_num = 0; - local_irq_save(flags); // FIXME: is this safe? + spin_lock_irqsave(&tty->read_lock, flags); + tty->flip.buf_num = 0; tty->flip.char_buf_ptr = tty->flip.char_buf; tty->flip.flag_buf_ptr = tty->flip.flag_buf; } else { cp = tty->flip.char_buf; fp = tty->flip.flag_buf; - tty->flip.buf_num = 1; - local_irq_save(flags); // FIXME: is this safe? + spin_lock_irqsave(&tty->read_lock, flags); + tty->flip.buf_num = 1; tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE; tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; } count = tty->flip.count; tty->flip.count = 0; - local_irq_restore(flags); // FIXME: is this safe? + spin_unlock_irqrestore(&tty->read_lock, flags); tty->ldisc.receive_buf(tty, cp, fp, count); } Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] small tty irq race fix 2003-03-03 21:20 ` Nicolas Pitre @ 2003-03-03 21:39 ` Linus Torvalds 2003-03-03 21:51 ` Nicolas Pitre 0 siblings, 1 reply; 6+ messages in thread From: Linus Torvalds @ 2003-03-03 21:39 UTC (permalink / raw) To: Nicolas Pitre; +Cc: Alan Cox, lkml On Mon, 3 Mar 2003, Nicolas Pitre wrote: > > What about this one? It just happens that tty->read_lock is actually used > deeper in the same call instance (in n_tty.c) so this looks to be the best > lock to use. Looks ok. I would suggest moving the "spin_lock_irqsave()" to outside the 'if'-statement, though, since that should make the code a lot more readable, and if the lock is supposed to protect tty->flip.buf_num, then let's do it right and protect the read as well, no? Linus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] small tty irq race fix 2003-03-03 21:39 ` Linus Torvalds @ 2003-03-03 21:51 ` Nicolas Pitre 0 siblings, 0 replies; 6+ messages in thread From: Nicolas Pitre @ 2003-03-03 21:51 UTC (permalink / raw) To: Linus Torvalds; +Cc: Alan Cox, lkml On Mon, 3 Mar 2003, Linus Torvalds wrote: > > On Mon, 3 Mar 2003, Nicolas Pitre wrote: > > > > What about this one? It just happens that tty->read_lock is actually used > > deeper in the same call instance (in n_tty.c) so this looks to be the best > > lock to use. > > Looks ok. I would suggest moving the "spin_lock_irqsave()" to outside the > 'if'-statement, though, since that should make the code a lot more > readable, and if the lock is supposed to protect tty->flip.buf_num, then > let's do it right and protect the read as well, no? Oh sure. --- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003 +++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 16:49:44 2003 @@ -1944,27 +1944,25 @@ schedule_delayed_work(&tty->flip.work, 1); return; } + + spin_lock_irqsave(&tty->read_lock, flags); if (tty->flip.buf_num) { cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE; fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; tty->flip.buf_num = 0; - - local_irq_save(flags); // FIXME: is this safe? tty->flip.char_buf_ptr = tty->flip.char_buf; tty->flip.flag_buf_ptr = tty->flip.flag_buf; } else { cp = tty->flip.char_buf; fp = tty->flip.flag_buf; tty->flip.buf_num = 1; - - local_irq_save(flags); // FIXME: is this safe? tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE; tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE; } count = tty->flip.count; tty->flip.count = 0; - local_irq_restore(flags); // FIXME: is this safe? - + spin_unlock_irqrestore(&tty->read_lock, flags); + tty->ldisc.receive_buf(tty, cp, fp, count); } Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2003-03-03 21:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-03-03 18:09 [patch] small tty irq race fix Nicolas Pitre 2003-03-03 20:06 ` Alan Cox 2003-03-03 19:15 ` Nicolas Pitre 2003-03-03 21:20 ` Nicolas Pitre 2003-03-03 21:39 ` Linus Torvalds 2003-03-03 21:51 ` Nicolas Pitre
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).