[PATCH][RFC] 2.6.6 tty_io.c hangup locking

[Paul Fulghum]

The following patch removes unnecessary disabling of
interrupts when processing hangup for tty devices.

This was introduced way back in August 1998
with patch 2.1.115 when the original hangup processing was
changed from cli/sti to lock_kernel()/unlock_kernel().

Up to now, this did not cause any actual problems.
In 2.6.X this causes a warning if any of the called functions
(flush_buffer/write_wakeup) call spin_xxx_bh() functions.

Warning is in spin_unlock_bh (kernel/softirq.c:122)

An example is drivers/net/ppp_synctty.c write_wakeup.
This has been reported several times by different people.

The flush_buffer/write_wakeup calls are also called
when processing ioctl for tty devices, without disabling
interrupts using only lock_kernel()/unlock_kernel().

tty->ldisc.flush_buffer()
	tty_ioctl.c
		ioctl(TCSETSF) -> set_termios()
		ioctl(TCSETAF) -> set_termios()
		ioctl(TCFLSH)

tty->driver.flush_buffer()
	tty_ioctl.c
		ioctl(TCFLSH)

tty->ldisc.write_wakeup()
	tty_ioctl.c
		ioctl(TCOON)  -> start_tty()
		ioctl(TCIOFF) -> send_prio_char() -> start_tty()
		ioctl(TCION)  -> send_prio_char() -> start_tty()

So removal of the interrupt disable/enable from around
these calls in tty_io.c do_tty_hangup(), implements locking
the same as the ioctl calls.

I have tested this patch on an SMP machine with 2.6.6.

I welcome comments and testing by others, particularly those
 who have seen the softirq.c message when doing ppp
(synchronous PPP, PPPoATM etc) that use the ppp_synctty.c

Paul Fulghum
paulkf@xxxxxxxxxxxxx


--- linux-2.6.6/drivers/char/tty_io.c	2004-05-28 08:26:47.000000000 -0500
+++ linux-2.6.6-mg1/drivers/char/tty_io.c	2004-05-28 08:31:05.000000000 -0500
@@ -442,21 +442,13 @@
 	}
 	file_list_unlock();
 	
-	/* FIXME! What are the locking issues here? This may me overdoing things..
-	* this question is especially important now that we've removed the irqlock. */
-	{
-		unsigned long flags;
-
-		local_irq_save(flags); // FIXME: is this safe?
-		if (tty->ldisc.flush_buffer)
-			tty->ldisc.flush_buffer(tty);
-		if (tty->driver->flush_buffer)
-			tty->driver->flush_buffer(tty);
-		if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
-		    tty->ldisc.write_wakeup)
-			(tty->ldisc.write_wakeup)(tty);
-		local_irq_restore(flags); // FIXME: is this safe?
-	}
+	if (tty->ldisc.flush_buffer)
+		tty->ldisc.flush_buffer(tty);
+	if (tty->driver->flush_buffer)
+		tty->driver->flush_buffer(tty);
+	if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
+	    tty->ldisc.write_wakeup)
+		(tty->ldisc.write_wakeup)(tty);
 
 	wake_up_interruptible(&tty->write_wait);
 	wake_up_interruptible(&tty->read_wait);


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