Hello!
In article <20000207014830.G31166@sfgoth.com> you wrote:
MBJ> Oleg Drokin wrote:
>> > another ppp_tty_push - uh, I really hope ppp->tty_pushing isn't the only
>> > thing serializing access here.. that doesn't look healthy to me.
>> I think that exactly this happens, we have 2 ppp_tty_push running, one after
>> one.
MBJ> Yeah, I wouldn't doubt it. Probably the solution will be to convert tty_pushing
MBJ> to an atomic_t.
MBJ> Time for the famous linux-kernel mantra: please try this untested patch
Finally bug was triggered again. And guess where it was? It was in
ppp_async_encode.
ppp device ppp1: tpkt set to NULL at point 4
It seems that we have one instance of ppp_tty_wakeup called from
do_serial_bh, and other one from rs_flush_buffer.
I've seen patches on the list to ppp code to check whenether
is other ppp_tty_wakeup running.
My aproach is different. I propose to call ldisc functions only once from
tty layer, this way we don't hurt ppp and other ldisc things,
and have all non SMP safe things in ldisc code fixed.
After all it is does not make sence to call ldisc.write_wakeup on the tty,
if another instance of it is already running on the same tty.
E.g. folowing patch fixes not only ppp, but also SLIP, without even
touching SLIP & PPP code.
--- drivers/char/serial.c.orig Sat Feb 19 13:53:14 2000
+++ drivers/char/serial.c Sat Feb 19 14:51:13 2000
@@ -32,6 +32,9 @@
* 4/98: Added changes to support the ARM architecture proposed by
* Russell King
*
+ * 2/2000: Fix SMP race in ldisc calling.
+ * Oleg Drokin <green@crimea.edu>
+ *
* This module exports the following rs232 io functions:
*
* int rs_init(void);
@@ -812,10 +815,13 @@
return;
if (test_and_clear_bit(RS_EVENT_WRITE_WAKEUP, &info->event)) {
- if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
- tty->ldisc.write_wakeup)
- (tty->ldisc.write_wakeup)(tty);
- wake_up_interruptible(&tty->write_wait);
+ if (!test_and_set_bit(TTY_LDISC_CALLED, &tty->flags)) {
+ if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
+ tty->ldisc.write_wakeup)
+ (tty->ldisc.write_wakeup)(tty);
+ clear_bit(TTY_LDISC_CALLED, &tty->flags);
+ wake_up_interruptible(&tty->write_wait);
+ }
}
}
@@ -1532,9 +1538,12 @@
info->xmit_cnt = info->xmit_head = info->xmit_tail = 0;
restore_flags(flags);
wake_up_interruptible(&tty->write_wait);
- if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
- tty->ldisc.write_wakeup)
- (tty->ldisc.write_wakeup)(tty);
+ if (!test_and_set_bit(TTY_LDISC_CALLED, &tty->flags)) {
+ if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
+ tty->ldisc.write_wakeup)
+ (tty->ldisc.write_wakeup)(tty);
+ clear_bit(TTY_LDISC_CALLED, &tty->flags);
+ }
}
/*
--- include/linux/tty.h.orig Sat Feb 19 13:58:32 2000
+++ include/linux/tty.h Sat Feb 19 14:15:02 2000
@@ -330,6 +330,7 @@
#define TTY_HW_COOK_IN 15
#define TTY_PTY_LOCK 16
#define TTY_NO_WRITE_SPLIT 17
+#define TTY_LDISC_CALLED 18
#define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
Bye,
Oleg
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Feb 23 2000 - 21:00:23 EST