[PATCH v3 00/10] BKL conversion in tty layer

[PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

This is the third attempt to get the BKL out of the
TTY code. This version goes much further than the
previous one, and eliminates most of the code I
had introduced there.
 
What remains of the series are changes to the existing
BKL usage in the TTY layer to avoid depending on
the autorelease-during-sleep or the recursive
behavior. The final patch simply adds the option
to replace the BKL with a plain mutex.

The series is available as a git tree in the bkl/tty branch of
git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
There is also a bkl/tty-history branch with a much
more detailed series that I used to get to this point.
Both series are bisectable.

Arnd Bergmann (10):
  tty: replace BKL with a new tty_lock
  tty: never hold BTM while getting tty_mutex
  tty: fix console_sem lock order
  cdc-acm: remove dead code
  tty: introduce wait_event_interruptible_tty
  tty: annotate tty_write_lock
  tty: reorder ldisc locking
  tty: untangle locking of wait_until_sent
  tty: remove tty_lock_nested
  tty: implement BTM as mutex instead of BKL

 drivers/char/Makefile          |    1 +
 drivers/char/amiserial.c       |   23 ++++--
 drivers/char/briq_panel.c      |    6 +-
 drivers/char/cyclades.c        |    2 +-
 drivers/char/istallion.c       |   12 ++-
 drivers/char/n_hdlc.c          |   16 ++--
 drivers/char/n_r3964.c         |   10 ++--
 drivers/char/pty.c             |   24 +++----
 drivers/char/selection.c       |   13 +++-
 drivers/char/serial167.c       |    4 +-
 drivers/char/stallion.c        |    2 +
 drivers/char/sx.c              |   12 ++--
 drivers/char/tty_io.c          |  138 ++++++++++++++++++++++-----------------
 drivers/char/tty_ldisc.c       |   43 +++++++++----
 drivers/char/tty_mutex.c       |   47 ++++++++++++++
 drivers/char/tty_port.c        |    2 +-
 drivers/char/vc_screen.c       |    4 +-
 drivers/char/vt_ioctl.c        |   12 ++--
 drivers/serial/68360serial.c   |    2 -
 drivers/serial/crisv10.c       |    6 +-
 drivers/serial/serial_core.c   |   39 ++++++++----
 drivers/usb/class/cdc-acm.c    |   12 +---
 drivers/video/console/vgacon.c |    2 -
 include/linux/tty.h            |   64 ++++++++++++++++++
 lib/Kconfig.debug              |   10 +++
 25 files changed, 338 insertions(+), 168 deletions(-)
 create mode 100644 drivers/char/tty_mutex.c

[PATCH 01/10] tty: replace BKL with a new tty_lock

[Arnd Bergmann]

As a preparation for replacing the big kernel lock
in the TTY layer, wrap all the callers in new
macros tty_lock, tty_lock_nested and tty_unlock.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/amiserial.c       |   16 +++---
 drivers/char/briq_panel.c      |    6 +-
 drivers/char/n_hdlc.c          |   16 +++---
 drivers/char/n_r3964.c         |    8 ++--
 drivers/char/pty.c             |    4 +-
 drivers/char/selection.c       |    4 +-
 drivers/char/serial167.c       |    4 +-
 drivers/char/sx.c              |   12 ++--
 drivers/char/tty_io.c          |  113 ++++++++++++++++++++++------------------
 drivers/char/tty_ldisc.c       |   24 +++++----
 drivers/char/vc_screen.c       |    4 +-
 drivers/char/vt_ioctl.c        |   10 ++--
 drivers/serial/68360serial.c   |    4 +-
 drivers/serial/crisv10.c       |    4 +-
 drivers/serial/serial_core.c   |   10 ++--
 drivers/video/console/vgacon.c |    4 +-
 include/linux/tty.h            |   28 ++++++++++
 17 files changed, 157 insertions(+), 114 deletions(-)

diff --git a/drivers/char/amiserial.c b/drivers/char/amiserial.c
index 56b2767..5bd382e 100644
--- a/drivers/char/amiserial.c
+++ b/drivers/char/amiserial.c
@@ -1071,7 +1071,7 @@ static int get_serial_info(struct async_struct * info,
 	if (!retinfo)
 		return -EFAULT;
 	memset(&tmp, 0, sizeof(tmp));
-	lock_kernel();
+	tty_lock();
 	tmp.type = state->type;
 	tmp.line = state->line;
 	tmp.port = state->port;
@@ -1082,7 +1082,7 @@ static int get_serial_info(struct async_struct * info,
 	tmp.close_delay = state->close_delay;
 	tmp.closing_wait = state->closing_wait;
 	tmp.custom_divisor = state->custom_divisor;
-	unlock_kernel();
+	tty_unlock();
 	if (copy_to_user(retinfo,&tmp,sizeof(*retinfo)))
 		return -EFAULT;
 	return 0;
@@ -1099,14 +1099,14 @@ static int set_serial_info(struct async_struct * info,
 	if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
 		return -EFAULT;
 
-	lock_kernel();
+	tty_lock();
 	state = info->state;
 	old_state = *state;
   
 	change_irq = new_serial.irq != state->irq;
 	change_port = (new_serial.port != state->port);
 	if(change_irq || change_port || (new_serial.xmit_fifo_size != state->xmit_fifo_size)) {
-	  unlock_kernel();
+	  tty_unlock();
 	  return -EINVAL;
 	}
   
@@ -1126,7 +1126,7 @@ static int set_serial_info(struct async_struct * info,
 	}
 
 	if (new_serial.baud_base < 9600) {
-		unlock_kernel();
+		tty_unlock();
 		return -EINVAL;
 	}
 
@@ -1162,7 +1162,7 @@ check_and_exit:
 		}
 	} else
 		retval = startup(info);
-	unlock_kernel();
+	tty_unlock();
 	return retval;
 }
 
@@ -1537,7 +1537,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 
 	orig_jiffies = jiffies;
 
-	lock_kernel();
+	tty_lock_nested(); /* tty_wait_until_sent is called from lots of places */
 	/*
 	 * Set the check interval to be 1/5 of the estimated time to
 	 * send a single character, and make it at least 1.  The check
@@ -1578,7 +1578,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 			break;
 	}
 	__set_current_state(TASK_RUNNING);
-	unlock_kernel();
+	tty_unlock();
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
 #endif
diff --git a/drivers/char/briq_panel.c b/drivers/char/briq_panel.c
index 555cd93..d5fa113 100644
--- a/drivers/char/briq_panel.c
+++ b/drivers/char/briq_panel.c
@@ -67,15 +67,15 @@ static void set_led(char state)
 
 static int briq_panel_open(struct inode *ino, struct file *filep)
 {
-	lock_kernel();
+	tty_lock();
 	/* enforce single access, vfd_is_open is protected by BKL */
 	if (vfd_is_open) {
-		unlock_kernel();
+		tty_unlock();
 		return -EBUSY;
 	}
 	vfd_is_open = 1;
 
-	unlock_kernel();
+	tty_unlock();
 	return 0;
 }
 
diff --git a/drivers/char/n_hdlc.c b/drivers/char/n_hdlc.c
index c68118e..47d3228 100644
--- a/drivers/char/n_hdlc.c
+++ b/drivers/char/n_hdlc.c
@@ -598,18 +598,18 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 		return -EFAULT;
 	}
 
-	lock_kernel();
+	tty_lock();
 
 	for (;;) {
 		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
-			unlock_kernel();
+			tty_unlock();
 			return -EIO;
 		}
 
 		n_hdlc = tty2n_hdlc (tty);
 		if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
 			 tty != n_hdlc->tty) {
-			unlock_kernel();
+			tty_unlock();
 			return 0;
 		}
 
@@ -619,13 +619,13 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 			
 		/* no data */
 		if (file->f_flags & O_NONBLOCK) {
-			unlock_kernel();
+			tty_unlock();
 			return -EAGAIN;
 		}
 			
 		interruptible_sleep_on (&tty->read_wait);
 		if (signal_pending(current)) {
-			unlock_kernel();
+			tty_unlock();
 			return -EINTR;
 		}
 	}
@@ -648,7 +648,7 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 		kfree(rbuf);
 	else	
 		n_hdlc_buf_put(&n_hdlc->rx_free_buf_list,rbuf);
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 	
 }	/* end of n_hdlc_tty_read() */
@@ -691,7 +691,7 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
 		count = maxframe;
 	}
 	
-	lock_kernel();
+	tty_lock();
 
 	add_wait_queue(&tty->write_wait, &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
@@ -731,7 +731,7 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
 		n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
 		n_hdlc_send_frames(n_hdlc,tty);
 	}
-	unlock_kernel();
+	tty_unlock();
 	return error;
 	
 }	/* end of n_hdlc_tty_write() */
diff --git a/drivers/char/n_r3964.c b/drivers/char/n_r3964.c
index c1d8b54..f4bd259 100644
--- a/drivers/char/n_r3964.c
+++ b/drivers/char/n_r3964.c
@@ -1067,7 +1067,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
 
 	TRACE_L("read()");
 
-	lock_kernel();
+	tty_lock();
 
 	pClient = findClient(pInfo, task_pid(current));
 	if (pClient) {
@@ -1109,7 +1109,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
 	}
 	ret = -EPERM;
 unlock:
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 }
 
@@ -1158,7 +1158,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file,
 	pHeader->locks = 0;
 	pHeader->owner = NULL;
 
-	lock_kernel();
+	tty_lock();
 
 	pClient = findClient(pInfo, task_pid(current));
 	if (pClient) {
@@ -1177,7 +1177,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file,
 	add_tx_queue(pInfo, pHeader);
 	trigger_transmit(pInfo);
 
-	unlock_kernel();
+	tty_unlock();
 
 	return 0;
 }
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index d83a431..384e79f 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -671,9 +671,9 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 
-	lock_kernel();
+	tty_lock();
 	ret = __ptmx_open(inode, filp);
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 }
 
diff --git a/drivers/char/selection.c b/drivers/char/selection.c
index 6e79340..85211a3 100644
--- a/drivers/char/selection.c
+++ b/drivers/char/selection.c
@@ -313,7 +313,7 @@ int paste_selection(struct tty_struct *tty)
 	struct  tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	lock_kernel();
+	tty_lock_nested(); /* always called with BTM from vt_ioctl */
 
 	acquire_console_sem();
 	poke_blanked_console();
@@ -338,6 +338,6 @@ int paste_selection(struct tty_struct *tty)
 	__set_current_state(TASK_RUNNING);
 
 	tty_ldisc_deref(ld);
-	unlock_kernel();
+	tty_unlock();
 	return 0;
 }
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 8dfd247..26a7089 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -1537,7 +1537,7 @@ cy_ioctl(struct tty_struct *tty, struct file *file,
 	printk("cy_ioctl %s, cmd = %x arg = %lx\n", tty->name, cmd, arg);	/* */
 #endif
 
-	lock_kernel();
+	tty_lock();
 
 	switch (cmd) {
 	case CYGETMON:
@@ -1593,7 +1593,7 @@ cy_ioctl(struct tty_struct *tty, struct file *file,
 	default:
 		ret_val = -ENOIOCTLCMD;
 	}
-	unlock_kernel();
+	tty_unlock();
 
 #ifdef SERIAL_DEBUG_OTHER
 	printk("cy_ioctl done\n");
diff --git a/drivers/char/sx.c b/drivers/char/sx.c
index a81ec4f..5b24db4 100644
--- a/drivers/char/sx.c
+++ b/drivers/char/sx.c
@@ -1699,7 +1699,7 @@ static long sx_fw_ioctl(struct file *filp, unsigned int cmd,
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
 
-	lock_kernel();
+	tty_lock();
 
 	sx_dprintk(SX_DEBUG_FIRMWARE, "IOCTL %x: %lx\n", cmd, arg);
 
@@ -1848,7 +1848,7 @@ static long sx_fw_ioctl(struct file *filp, unsigned int cmd,
 		break;
 	}
 out:
-	unlock_kernel();
+	tty_unlock();
 	func_exit();
 	return rc;
 }
@@ -1859,7 +1859,7 @@ static int sx_break(struct tty_struct *tty, int flag)
 	int rv;
 
 	func_enter();
-	lock_kernel();
+	tty_lock();
 
 	if (flag)
 		rv = sx_send_command(port, HS_START, -1, HS_IDLE_BREAK);
@@ -1868,7 +1868,7 @@ static int sx_break(struct tty_struct *tty, int flag)
 	if (rv != 1)
 		printk(KERN_ERR "sx: couldn't send break (%x).\n",
 			read_sx_byte(port->board, CHAN_OFFSET(port, hi_hstat)));
-	unlock_kernel();
+	tty_unlock();
 	func_exit();
 	return 0;
 }
@@ -1909,7 +1909,7 @@ static int sx_ioctl(struct tty_struct *tty, struct file *filp,
 	/* func_enter2(); */
 
 	rc = 0;
-	lock_kernel();
+	tty_lock();
 	switch (cmd) {
 	case TIOCGSERIAL:
 		rc = gs_getserial(&port->gs, argp);
@@ -1921,7 +1921,7 @@ static int sx_ioctl(struct tty_struct *tty, struct file *filp,
 		rc = -ENOIOCTLCMD;
 		break;
 	}
-	unlock_kernel();
+	tty_unlock();
 
 	/* func_exit(); */
 	return rc;
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 6da962c..3bf2c75 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -149,6 +149,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 #else
 #define tty_compat_ioctl NULL
 #endif
+static int __tty_fasync(int fd, struct file *filp, int on);
 static int tty_fasync(int fd, struct file *filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
 static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
@@ -483,7 +484,7 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
  *	remains intact.
  *
  *	Locking:
- *		BKL
+ *		BTM
  *		  redirect lock for undoing redirection
  *		  file list lock for manipulating list of ttys
  *		  tty_ldisc_lock from called functions
@@ -513,8 +514,11 @@ static void do_tty_hangup(struct work_struct *work)
 	}
 	spin_unlock(&redirect_lock);
 
-	/* inuse_filps is protected by the single kernel lock */
-	lock_kernel();
+	/* inuse_filps is protected by the single tty lock,
+	   this really needs to change if we want to flush the
+	   workqueue with the lock held */
+	tty_lock_nested(); /* called with BTM held from pty_close and
+				others */
 	check_tty_count(tty, "do_tty_hangup");
 
 	file_list_lock();
@@ -525,7 +529,7 @@ static void do_tty_hangup(struct work_struct *work)
 		if (filp->f_op->write != tty_write)
 			continue;
 		closecount++;
-		tty_fasync(-1, filp, 0);	/* can't block */
+		__tty_fasync(-1, filp, 0);	/* can't block */
 		filp->f_op = &hung_up_tty_fops;
 	}
 	file_list_unlock();
@@ -594,7 +598,7 @@ static void do_tty_hangup(struct work_struct *work)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	tty_ldisc_enable(tty);
-	unlock_kernel();
+	tty_unlock();
 	if (f)
 		fput(f);
 }
@@ -696,7 +700,8 @@ static void session_clear_tty(struct pid *session)
  *	exiting; it is 0 if called by the ioctl TIOCNOTTY.
  *
  *	Locking:
- *		BKL is taken for hysterical raisins
+ *		BTM is taken for hysterical raisins, and held when
+ *		  called from no_tty().
  *		  tty_mutex is taken to protect tty
  *		  ->siglock is taken to protect ->signal/->sighand
  *		  tasklist_lock is taken to walk process list for sessions
@@ -714,10 +719,10 @@ void disassociate_ctty(int on_exit)
 	tty = get_current_tty();
 	if (tty) {
 		tty_pgrp = get_pid(tty->pgrp);
-		lock_kernel();
+		tty_lock_nested(); /* see above */
 		if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY)
 			tty_vhangup(tty);
-		unlock_kernel();
+		tty_unlock();
 		tty_kref_put(tty);
 	} else if (on_exit) {
 		struct pid *old_pgrp;
@@ -774,9 +779,9 @@ void disassociate_ctty(int on_exit)
 void no_tty(void)
 {
 	struct task_struct *tsk = current;
-	lock_kernel();
+	tty_lock();
 	disassociate_ctty(0);
-	unlock_kernel();
+	tty_unlock();
 	proc_clear_tty(tsk);
 }
 
@@ -1013,19 +1018,19 @@ out:
  * We don't put it into the syslog queue right now maybe in the future if
  * really needed.
  *
- * We must still hold the BKL and test the CLOSING flag for the moment.
+ * We must still hold the BTM and test the CLOSING flag for the moment.
  */
 
 void tty_write_message(struct tty_struct *tty, char *msg)
 {
 	if (tty) {
 		mutex_lock(&tty->atomic_write_lock);
-		lock_kernel();
+		tty_lock();
 		if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) {
-			unlock_kernel();
+			tty_unlock();
 			tty->ops->write(tty, msg, strlen(msg));
 		} else
-			unlock_kernel();
+			tty_unlock();
 		tty_write_unlock(tty);
 	}
 	return;
@@ -1208,18 +1213,18 @@ static int tty_driver_install_tty(struct tty_driver *driver,
 	int ret;
 
 	if (driver->ops->install) {
-		lock_kernel();
+		tty_lock_nested(); /* already called with BTM held */
 		ret = driver->ops->install(driver, tty);
-		unlock_kernel();
+		tty_unlock();
 		return ret;
 	}
 
 	if (tty_init_termios(tty) == 0) {
-		lock_kernel();
+		tty_lock_nested();
 		tty_driver_kref_get(driver);
 		tty->count++;
 		driver->ttys[idx] = tty;
-		unlock_kernel();
+		tty_unlock();
 		return 0;
 	}
 	return -ENOMEM;
@@ -1312,14 +1317,15 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
 	struct tty_struct *tty;
 	int retval;
 
-	lock_kernel();
+	tty_lock_nested(); /* always called with tty lock held already */
+
 	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
-		unlock_kernel();
+		tty_unlock();
 		return ERR_PTR(-EIO);
 	}
-	unlock_kernel();
+	tty_unlock();
 
 	/*
 	 * First time open is complex, especially for PTY devices.
@@ -1363,9 +1369,9 @@ release_mem_out:
 	if (printk_ratelimit())
 		printk(KERN_INFO "tty_init_dev: ldisc open failed, "
 				 "clearing slot %d\n", idx);
-	lock_kernel();
+	tty_lock_nested();
 	release_tty(tty, idx);
-	unlock_kernel();
+	tty_unlock();
 	return ERR_PTR(retval);
 }
 
@@ -1512,10 +1518,10 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (tty_paranoia_check(tty, inode, "tty_release_dev"))
 		return 0;
 
-	lock_kernel();
+	tty_lock();
 	check_tty_count(tty, "tty_release_dev");
 
-	tty_fasync(-1, filp, 0);
+	__tty_fasync(-1, filp, 0);
 
 	idx = tty->index;
 	pty_master = (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
@@ -1527,18 +1533,18 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (idx < 0 || idx >= tty->driver->num) {
 		printk(KERN_DEBUG "tty_release_dev: bad idx when trying to "
 				  "free (%s)\n", tty->name);
-		unlock_kernel();
+		tty_unlock();
 		return 0;
 	}
 	if (!devpts) {
 		if (tty != tty->driver->ttys[idx]) {
-			unlock_kernel();
+			tty_unlock();
 			printk(KERN_DEBUG "tty_release_dev: driver.table[%d] not tty "
 			       "for (%s)\n", idx, tty->name);
 			return 0;
 		}
 		if (tty->termios != tty->driver->termios[idx]) {
-			unlock_kernel();
+			tty_unlock();
 			printk(KERN_DEBUG "tty_release_dev: driver.termios[%d] not termios "
 			       "for (%s)\n",
 			       idx, tty->name);
@@ -1556,21 +1562,21 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (tty->driver->other &&
 	     !(tty->driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
 		if (o_tty != tty->driver->other->ttys[idx]) {
-			unlock_kernel();
+			tty_unlock();
 			printk(KERN_DEBUG "tty_release_dev: other->table[%d] "
 					  "not o_tty for (%s)\n",
 			       idx, tty->name);
 			return 0 ;
 		}
 		if (o_tty->termios != tty->driver->other->termios[idx]) {
-			unlock_kernel();
+			tty_unlock();
 			printk(KERN_DEBUG "tty_release_dev: other->termios[%d] "
 					  "not o_termios for (%s)\n",
 			       idx, tty->name);
 			return 0;
 		}
 		if (o_tty->link != tty) {
-			unlock_kernel();
+			tty_unlock();
 			printk(KERN_DEBUG "tty_release_dev: bad pty pointers\n");
 			return 0;
 		}
@@ -1579,7 +1585,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (tty->ops->close)
 		tty->ops->close(tty, filp);
 
-	unlock_kernel();
+	tty_unlock();
 	/*
 	 * Sanity check: if tty->count is going to zero, there shouldn't be
 	 * any waiters on tty->read_wait or tty->write_wait.  We test the
@@ -1602,7 +1608,7 @@ int tty_release(struct inode *inode, struct file *filp)
 		   opens on /dev/tty */
 
 		mutex_lock(&tty_mutex);
-		lock_kernel();
+		tty_lock();
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
@@ -1633,7 +1639,7 @@ int tty_release(struct inode *inode, struct file *filp)
 
 		printk(KERN_WARNING "tty_release_dev: %s: read/write wait queue "
 				    "active!\n", tty_name(tty, buf));
-		unlock_kernel();
+		tty_unlock();
 		mutex_unlock(&tty_mutex);
 		schedule();
 	}
@@ -1698,7 +1704,7 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	/* check whether both sides are closing ... */
 	if (!tty_closing || (o_tty && !o_tty_closing)) {
-		unlock_kernel();
+		tty_unlock();
 		return 0;
 	}
 
@@ -1718,7 +1724,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	/* Make this pty number available for reallocation */
 	if (devpts)
 		devpts_kill_index(inode, idx);
-	unlock_kernel();
+	tty_unlock();
 	return 0;
 }
 
@@ -1760,12 +1766,12 @@ retry_open:
 	retval = 0;
 
 	mutex_lock(&tty_mutex);
-	lock_kernel();
+	tty_lock();
 
 	if (device == MKDEV(TTYAUX_MAJOR, 0)) {
 		tty = get_current_tty();
 		if (!tty) {
-			unlock_kernel();
+			tty_unlock();
 			mutex_unlock(&tty_mutex);
 			return -ENXIO;
 		}
@@ -1797,14 +1803,14 @@ retry_open:
 				goto got_driver;
 			}
 		}
-		unlock_kernel();
+		tty_unlock();
 		mutex_unlock(&tty_mutex);
 		return -ENODEV;
 	}
 
 	driver = get_tty_driver(device, &index);
 	if (!driver) {
-		unlock_kernel();
+		tty_unlock();
 		mutex_unlock(&tty_mutex);
 		return -ENODEV;
 	}
@@ -1814,7 +1820,7 @@ got_driver:
 		tty = tty_driver_lookup_tty(driver, inode, index);
 
 		if (IS_ERR(tty)) {
-			unlock_kernel();
+			tty_unlock();
 			mutex_unlock(&tty_mutex);
 			return PTR_ERR(tty);
 		}
@@ -1830,7 +1836,7 @@ got_driver:
 	mutex_unlock(&tty_mutex);
 	tty_driver_kref_put(driver);
 	if (IS_ERR(tty)) {
-		unlock_kernel();
+		tty_unlock();
 		return PTR_ERR(tty);
 	}
 
@@ -1862,11 +1868,11 @@ got_driver:
 #endif
 		tty_release(inode, filp);
 		if (retval != -ERESTARTSYS) {
-			unlock_kernel();
+			tty_unlock();
 			return retval;
 		}
 		if (signal_pending(current)) {
-			unlock_kernel();
+			tty_unlock();
 			return retval;
 		}
 		schedule();
@@ -1877,11 +1883,11 @@ got_driver:
 			filp->f_op = &tty_fops;
 		goto retry_open;
 	}
-	unlock_kernel();
+	tty_unlock();
 
 
 	mutex_lock(&tty_mutex);
-	lock_kernel();
+	tty_lock();
 	spin_lock_irq(&current->sighand->siglock);
 	if (!noctty &&
 	    current->signal->leader &&
@@ -1889,7 +1895,7 @@ got_driver:
 	    tty->session == NULL)
 		__proc_set_tty(current, tty);
 	spin_unlock_irq(&current->sighand->siglock);
-	unlock_kernel();
+	tty_unlock();
 	mutex_unlock(&tty_mutex);
 	return 0;
 }
@@ -1925,13 +1931,12 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 	return ret;
 }
 
-static int tty_fasync(int fd, struct file *filp, int on)
+static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty;
 	unsigned long flags;
 	int retval = 0;
 
-	lock_kernel();
 	tty = (struct tty_struct *)filp->private_data;
 	if (tty_paranoia_check(tty, filp->f_path.dentry->d_inode, "tty_fasync"))
 		goto out;
@@ -1965,7 +1970,15 @@ static int tty_fasync(int fd, struct file *filp, int on)
 	}
 	retval = 0;
 out:
-	unlock_kernel();
+	return retval;
+}
+
+static int tty_fasync(int fd, struct file *filp, int on)
+{
+	int retval;
+	tty_lock();
+	retval = __tty_fasync(fd, filp, on);
+	tty_unlock();
 	return retval;
 }
 
diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 500e740..97681ff 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -440,6 +440,8 @@ static void tty_set_termios_ldisc(struct tty_struct *tty, int num)
  *
  *	A helper opening method. Also a convenient debugging and check
  *	point.
+ *
+ *	Locking: always called with BTM already held.
  */
 
 static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
@@ -447,10 +449,10 @@ static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
 	WARN_ON(test_and_set_bit(TTY_LDISC_OPEN, &tty->flags));
 	if (ld->ops->open) {
 		int ret;
-                /* BKL here locks verus a hangup event */
-		lock_kernel();
+                /* BTM here locks versus a hangup event */
+		tty_lock_nested(); /* always held here already */
 		ret = ld->ops->open(tty);
-		unlock_kernel();
+		tty_unlock();
 		return ret;
 	}
 	return 0;
@@ -553,7 +555,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (IS_ERR(new_ldisc))
 		return PTR_ERR(new_ldisc);
 
-	lock_kernel();
+	tty_lock();
 	/*
 	 *	We need to look at the tty locking here for pty/tty pairs
 	 *	when both sides try to change in parallel.
@@ -567,12 +569,12 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	 */
 
 	if (tty->ldisc->ops->num == ldisc) {
-		unlock_kernel();
+		tty_unlock();
 		tty_ldisc_put(new_ldisc);
 		return 0;
 	}
 
-	unlock_kernel();
+	tty_unlock();
 	/*
 	 *	Problem: What do we do if this blocks ?
 	 *	We could deadlock here
@@ -594,7 +596,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 		mutex_lock(&tty->ldisc_mutex);
 	}
 
-	lock_kernel();
+	tty_lock();
 
 	set_bit(TTY_LDISC_CHANGING, &tty->flags);
 
@@ -607,7 +609,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	o_ldisc = tty->ldisc;
 
-	unlock_kernel();
+	tty_unlock();
 	/*
 	 *	Make sure we don't change while someone holds a
 	 *	reference to the line discipline. The TTY_LDISC bit
@@ -633,14 +635,14 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	flush_scheduled_work();
 
 	mutex_lock(&tty->ldisc_mutex);
-	lock_kernel();
+	tty_lock();
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
 		clear_bit(TTY_LDISC_CHANGING, &tty->flags);
 		mutex_unlock(&tty->ldisc_mutex);
 		tty_ldisc_put(new_ldisc);
-		unlock_kernel();
+		tty_unlock();
 		return -EIO;
 	}
 
@@ -682,7 +684,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (o_work)
 		schedule_delayed_work(&o_tty->buf.work, 1);
 	mutex_unlock(&tty->ldisc_mutex);
-	unlock_kernel();
+	tty_unlock();
 	return retval;
 }
 
diff --git a/drivers/char/vc_screen.c b/drivers/char/vc_screen.c
index c1791a6..bcce46c 100644
--- a/drivers/char/vc_screen.c
+++ b/drivers/char/vc_screen.c
@@ -463,10 +463,10 @@ vcs_open(struct inode *inode, struct file *filp)
 	unsigned int currcons = iminor(inode) & 127;
 	int ret = 0;
 	
-	lock_kernel();
+	tty_lock();
 	if(currcons && !vc_cons_allocated(currcons-1))
 		ret = -ENXIO;
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 }
 
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 625f77d..cc07fab 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -509,7 +509,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
 
 	console = vc->vc_num;
 
-	lock_kernel();
+	tty_lock();
 
 	if (!vc_cons_allocated(console)) { 	/* impossible? */
 		ret = -ENOIOCTLCMD;
@@ -1334,7 +1334,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
 		ret = -ENOIOCTLCMD;
 	}
 out:
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 eperm:
 	ret = -EPERM;
@@ -1501,7 +1501,7 @@ long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
 
 	console = vc->vc_num;
 
-	lock_kernel();
+	tty_lock();
 
 	if (!vc_cons_allocated(console)) { 	/* impossible? */
 		ret = -ENOIOCTLCMD;
@@ -1569,11 +1569,11 @@ long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
 		goto fallback;
 	}
 out:
-	unlock_kernel();
+	tty_unlock();
 	return ret;
 
 fallback:
-	unlock_kernel();
+	tty_unlock();
 	return vt_ioctl(tty, file, cmd, arg);
 }
 
diff --git a/drivers/serial/68360serial.c b/drivers/serial/68360serial.c
index 24661cd..c17a595 100644
--- a/drivers/serial/68360serial.c
+++ b/drivers/serial/68360serial.c
@@ -1705,7 +1705,7 @@ static void rs_360_wait_until_sent(struct tty_struct *tty, int timeout)
 	printk("jiff=%lu...", jiffies);
 #endif
 
-	lock_kernel();
+	tty_lock_nested(); /* always held already since we come from ->close */
 	/* We go through the loop at least once because we can't tell
 	 * exactly when the last character exits the shifter.  There can
 	 * be at least two characters waiting to be sent after the buffers
@@ -1734,7 +1734,7 @@ static void rs_360_wait_until_sent(struct tty_struct *tty, int timeout)
 			bdp--;
 	} while (bdp->status & BD_SC_READY);
 	current->state = TASK_RUNNING;
-	unlock_kernel();
+	tty_unlock();
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
 #endif
diff --git a/drivers/serial/crisv10.c b/drivers/serial/crisv10.c
index 31f1723..d11d898 100644
--- a/drivers/serial/crisv10.c
+++ b/drivers/serial/crisv10.c
@@ -3924,7 +3924,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 	 * Check R_DMA_CHx_STATUS bit 0-6=number of available bytes in FIFO
 	 * R_DMA_CHx_HWSW bit 31-16=nbr of bytes left in DMA buffer (0=64k)
 	 */
-	lock_kernel();
+	tty_lock_nested(); /* locked already when coming from close */
 	orig_jiffies = jiffies;
 	while (info->xmit.head != info->xmit.tail || /* More in send queue */
 	       (*info->ostatusadr & 0x007f) ||  /* more in FIFO */
@@ -3941,7 +3941,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 			curr_time_usec - info->last_tx_active_usec;
 	}
 	set_current_state(TASK_RUNNING);
-	unlock_kernel();
+	tty_unlock();
 }
 
 /*
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index a55751a..4b151a4 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -1274,7 +1274,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	struct uart_port *uport;
 	unsigned long flags;
 
-	BUG_ON(!kernel_locked());
+	BUG_ON(!tty_locked());
 
 	if (!state)
 		return;
@@ -1382,7 +1382,7 @@ static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
 	if (port->type == PORT_UNKNOWN || port->fifosize == 0)
 		return;
 
-	lock_kernel();
+	tty_lock_nested(); /* already locked when coming from close */
 
 	/*
 	 * Set the check interval to be 1/5 of the estimated time to
@@ -1429,7 +1429,7 @@ static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
 			break;
 	}
 	set_current_state(TASK_RUNNING); /* might not be needed */
-	unlock_kernel();
+	tty_unlock();
 }
 
 /*
@@ -1444,7 +1444,7 @@ static void uart_hangup(struct tty_struct *tty)
 	struct tty_port *port = &state->port;
 	unsigned long flags;
 
-	BUG_ON(!kernel_locked());
+	BUG_ON(!tty_locked());
 	pr_debug("uart_hangup(%d)\n", state->uart_port->line);
 
 	mutex_lock(&port->mutex);
@@ -1570,7 +1570,7 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 	struct tty_port *port;
 	int retval, line = tty->index;
 
-	BUG_ON(!kernel_locked());
+	BUG_ON(!tty_locked());
 	pr_debug("uart_open(%d) called\n", line);
 
 	/*
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 182dd6f..7197005 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -1108,7 +1108,7 @@ static int vgacon_do_font_op(struct vgastate *state,char *arg,int set,int ch512)
 		charmap += 4 * cmapsz;
 #endif
 
-	unlock_kernel();
+	tty_unlock();
 	spin_lock_irq(&vga_lock);
 	/* First, the Sequencer */
 	vga_wseq(state->vgabase, VGA_SEQ_RESET, 0x1);
@@ -1192,7 +1192,7 @@ static int vgacon_do_font_op(struct vgastate *state,char *arg,int set,int ch512)
 		vga_wattr(state->vgabase, VGA_AR_ENABLE_DISPLAY, 0);	
 	}
 	spin_unlock_irq(&vga_lock);
-	lock_kernel();
+	tty_lock();
 	return 0;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4409967..c35ad81 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -13,6 +13,7 @@
 #include <linux/tty_driver.h>
 #include <linux/tty_ldisc.h>
 #include <linux/mutex.h>
+#include <linux/smp_lock.h>
 
 #include <asm/system.h>
 
@@ -571,5 +572,32 @@ extern int vt_ioctl(struct tty_struct *tty, struct file *file,
 extern long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
 		     unsigned int cmd, unsigned long arg);
 
+/* functions for preparation of BKL removal */
+
+/*
+ * tty_lock_nested get the tty_lock while potentially holding it
+ *
+ * The Big TTY Mutex is a recursive lock, meaning you can take it
+ * from a thread that is already holding it.
+ * This is bad for a number of reasons, so tty_lock_nested should
+ * really be used as rarely as possible. If a code location can
+ * be shown to never get called with this held already, it should
+ * use tty_lock() instead.
+ */
+static inline void __lockfunc tty_lock_nested(void) __acquires(kernel_lock)
+{
+	lock_kernel();
+}
+static inline void tty_lock(void) __acquires(kernel_lock)
+{
+	WARN_ON(kernel_locked());
+	lock_kernel();
+}
+static inline void tty_unlock(void) __releases(kernel_lock)
+{
+	unlock_kernel();
+}
+#define tty_locked()		(kernel_locked())
+
 #endif /* __KERNEL__ */
 #endif
-- 
1.7.0.4

[PATCH 02/10] tty: never hold BTM while getting tty_mutex

[Arnd Bergmann]

tty_mutex is never taken with the BTM held, except for
two corner cases that are worked around here.
We give up the BTM before calling tty_release() in the
error path of tty_open().
Similarly, we reorder the locking in ptmx_open()
to get tty_mutex before the BTM.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/pty.c    |   22 ++++++++--------------
 drivers/char/tty_io.c |   13 +++++++------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index 384e79f..b441ebd 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -626,7 +626,7 @@ static const struct tty_operations pty_unix98_ops = {
  *		allocated_ptys_lock handles the list of free pty numbers
  */
 
-static int __ptmx_open(struct inode *inode, struct file *filp)
+static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
 	int retval;
@@ -635,11 +635,14 @@ static int __ptmx_open(struct inode *inode, struct file *filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
+	tty_lock();
 	index = devpts_new_index(inode);
+	tty_unlock();
 	if (index < 0)
 		return index;
 
 	mutex_lock(&tty_mutex);
+	tty_lock();
 	tty = tty_init_dev(ptm_driver, index, 1);
 	mutex_unlock(&tty_mutex);
 
@@ -657,24 +660,15 @@ static int __ptmx_open(struct inode *inode, struct file *filp)
 		goto out1;
 
 	retval = ptm_driver->ops->open(tty, filp);
-	if (!retval)
-		return 0;
+	if (retval)
+		tty_release(inode, filp);
 out1:
-	tty_release(inode, filp);
+	tty_unlock();
 	return retval;
 out:
 	devpts_kill_index(inode, index);
-	return retval;
-}
-
-static int ptmx_open(struct inode *inode, struct file *filp)
-{
-	int ret;
-
-	tty_lock();
-	ret = __ptmx_open(inode, filp);
 	tty_unlock();
-	return ret;
+	return retval;
 }
 
 static struct file_operations ptmx_fops;
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 3bf2c75..d51993b 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1866,21 +1866,22 @@ got_driver:
 		printk(KERN_DEBUG "error %d in opening %s...", retval,
 		       tty->name);
 #endif
+		tty_unlock(); /* need to call tty_release without BTM */
 		tty_release(inode, filp);
-		if (retval != -ERESTARTSYS) {
-			tty_unlock();
+		if (retval != -ERESTARTSYS)
 			return retval;
-		}
-		if (signal_pending(current)) {
-			tty_unlock();
+
+		if (signal_pending(current))
 			return retval;
-		}
+
 		schedule();
 		/*
 		 * Need to reset f_op in case a hangup happened.
 		 */
+		tty_lock();
 		if (filp->f_op == &hung_up_tty_fops)
 			filp->f_op = &tty_fops;
+		tty_unlock();
 		goto retry_open;
 	}
 	tty_unlock();
-- 
1.7.0.4

[PATCH 03/10] tty: fix console_sem lock order

[Arnd Bergmann]

vgacon_do_font_op releases and reacquires the BTM while holding
console_sem. This violates the rule that BTM has to be the
outer lock whenever we hold both.

There does not seem to be any reason to give up the BTM here,
so just stop doing that.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/video/console/vgacon.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 7197005..54e32c5 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -1108,7 +1108,6 @@ static int vgacon_do_font_op(struct vgastate *state,char *arg,int set,int ch512)
 		charmap += 4 * cmapsz;
 #endif
 
-	tty_unlock();
 	spin_lock_irq(&vga_lock);
 	/* First, the Sequencer */
 	vga_wseq(state->vgabase, VGA_SEQ_RESET, 0x1);
@@ -1192,7 +1191,6 @@ static int vgacon_do_font_op(struct vgastate *state,char *arg,int set,int ch512)
 		vga_wattr(state->vgabase, VGA_AR_ENABLE_DISPLAY, 0);	
 	}
 	spin_unlock_irq(&vga_lock);
-	tty_lock();
 	return 0;
 }
 
-- 
1.7.0.4

[PATCH 04/10] cdc-acm: remove dead code

[Arnd Bergmann]

The wait_event_interruptible_timeout in acm_port_down is
never reached. Remove it to avoid possible deadlocks
with the big tty mutex if someone were to start using
the blocking version of acm_port_down.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/class/cdc-acm.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 5e1a253..7b50bf5 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -636,19 +636,13 @@ static void acm_tty_unregister(struct acm *acm)
 
 static int acm_tty_chars_in_buffer(struct tty_struct *tty);
 
-static void acm_port_down(struct acm *acm, int drain)
+static void acm_port_down(struct acm *acm)
 {
 	int i, nr = acm->rx_buflimit;
 	mutex_lock(&open_mutex);
 	if (acm->dev) {
 		usb_autopm_get_interface(acm->control);
 		acm_set_control(acm, acm->ctrlout = 0);
-		/* try letting the last writes drain naturally */
-		if (drain) {
-			wait_event_interruptible_timeout(acm->drain_wait,
-				(ACM_NW == acm_wb_is_avail(acm)) || !acm->dev,
-					ACM_CLOSE_TIMEOUT * HZ);
-		}
 		usb_kill_urb(acm->ctrlurb);
 		for (i = 0; i < ACM_NW; i++)
 			usb_kill_urb(acm->wb[i].urb);
@@ -664,7 +658,7 @@ static void acm_tty_hangup(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
 	tty_port_hangup(&acm->port);
-	acm_port_down(acm, 0);
+	acm_port_down(acm);
 }
 
 static void acm_tty_close(struct tty_struct *tty, struct file *filp)
@@ -685,7 +679,7 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
 		mutex_unlock(&open_mutex);
 		return;
 	}
-	acm_port_down(acm, 0);
+	acm_port_down(acm);
 	tty_port_close_end(&acm->port, tty);
 	tty_port_tty_set(&acm->port, NULL);
 }
-- 
1.7.0.4

[PATCH 05/10] tty: introduce wait_event_interruptible_tty

[Arnd Bergmann]

Calling wait_event_interruptible implicitly
releases the BKL when it sleeps, but we need
to do this explcitly when we have converted
it to a mutex.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/cyclades.c  |    2 +-
 drivers/char/istallion.c |   12 ++++++++----
 drivers/char/n_r3964.c   |    2 +-
 drivers/char/tty_port.c  |    2 +-
 drivers/char/vt_ioctl.c  |    2 +-
 drivers/serial/crisv10.c |    4 ++--
 include/linux/tty.h      |   42 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/char/cyclades.c b/drivers/char/cyclades.c
index 51acfe3..27aad94 100644
--- a/drivers/char/cyclades.c
+++ b/drivers/char/cyclades.c
@@ -1607,7 +1607,7 @@ static int cy_open(struct tty_struct *tty, struct file *filp)
 	 * If the port is the middle of closing, bail out now
 	 */
 	if (tty_hung_up_p(filp) || (info->port.flags & ASYNC_CLOSING)) {
-		wait_event_interruptible(info->port.close_wait,
+		wait_event_interruptible_tty(info->port.close_wait,
 				!(info->port.flags & ASYNC_CLOSING));
 		return (info->port.flags & ASYNC_HUP_NOTIFY) ? -EAGAIN: -ERESTARTSYS;
 	}
diff --git a/drivers/char/istallion.c b/drivers/char/istallion.c
index 5e9a81d..be28391 100644
--- a/drivers/char/istallion.c
+++ b/drivers/char/istallion.c
@@ -954,7 +954,7 @@ static int stli_rawopen(struct stlibrd *brdp, struct stliport *portp, unsigned l
  *	order of opens and closes may not be preserved across shared
  *	memory, so we must wait until it is complete.
  */
-	wait_event_interruptible(portp->raw_wait,
+	wait_event_interruptible_tty(portp->raw_wait,
 			!test_bit(ST_CLOSING, &portp->state));
 	if (signal_pending(current)) {
 		return -ERESTARTSYS;
@@ -989,7 +989,7 @@ static int stli_rawopen(struct stlibrd *brdp, struct stliport *portp, unsigned l
 	set_bit(ST_OPENING, &portp->state);
 	spin_unlock_irqrestore(&brd_lock, flags);
 
-	wait_event_interruptible(portp->raw_wait,
+	wait_event_interruptible_tty(portp->raw_wait,
 			!test_bit(ST_OPENING, &portp->state));
 	if (signal_pending(current))
 		rc = -ERESTARTSYS;
@@ -1020,7 +1020,7 @@ static int stli_rawclose(struct stlibrd *brdp, struct stliport *portp, unsigned
  *	occurs on this port.
  */
 	if (wait) {
-		wait_event_interruptible(portp->raw_wait,
+		wait_event_interruptible_tty(portp->raw_wait,
 				!test_bit(ST_CLOSING, &portp->state));
 		if (signal_pending(current)) {
 			return -ERESTARTSYS;
@@ -1052,7 +1052,7 @@ static int stli_rawclose(struct stlibrd *brdp, struct stliport *portp, unsigned
  *	to come back.
  */
 	rc = 0;
-	wait_event_interruptible(portp->raw_wait,
+	wait_event_interruptible_tty(portp->raw_wait,
 			!test_bit(ST_CLOSING, &portp->state));
 	if (signal_pending(current))
 		rc = -ERESTARTSYS;
@@ -1073,6 +1073,10 @@ static int stli_rawclose(struct stlibrd *brdp, struct stliport *portp, unsigned
 
 static int stli_cmdwait(struct stlibrd *brdp, struct stliport *portp, unsigned long cmd, void *arg, int size, int copyback)
 {
+	/*
+	 * no need for wait_event_tty because clearing ST_CMDING cannot block
+	 * on BTM
+	 */
 	wait_event_interruptible(portp->raw_wait,
 			!test_bit(ST_CMDING, &portp->state));
 	if (signal_pending(current))
diff --git a/drivers/char/n_r3964.c b/drivers/char/n_r3964.c
index f4bd259..a98290d 100644
--- a/drivers/char/n_r3964.c
+++ b/drivers/char/n_r3964.c
@@ -1079,7 +1079,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
 				goto unlock;
 			}
 			/* block until there is a message: */
-			wait_event_interruptible(pInfo->read_wait,
+			wait_event_interruptible_tty(pInfo->read_wait,
 					(pMsg = remove_msg(pInfo, pClient)));
 		}
 
diff --git a/drivers/char/tty_port.c b/drivers/char/tty_port.c
index a3bd1d0..35eb304 100644
--- a/drivers/char/tty_port.c
+++ b/drivers/char/tty_port.c
@@ -231,7 +231,7 @@ int tty_port_block_til_ready(struct tty_port *port,
 
 	/* block if port is in the process of being closed */
 	if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING) {
-		wait_event_interruptible(port->close_wait,
+		wait_event_interruptible_tty(port->close_wait,
 				!(port->flags & ASYNC_CLOSING));
 		if (port->flags & ASYNC_HUP_NOTIFY)
 			return -EAGAIN;
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index cc07fab..c56c4f3 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -133,7 +133,7 @@ static void vt_event_wait(struct vt_event_wait *vw)
 	list_add(&vw->list, &vt_events);
 	spin_unlock_irqrestore(&vt_event_lock, flags);
 	/* Wait for it to pass */
-	wait_event_interruptible(vt_event_waitqueue, vw->done);
+	wait_event_interruptible_tty(vt_event_waitqueue, vw->done);
 	/* Dequeue it */
 	spin_lock_irqsave(&vt_event_lock, flags);
 	list_del(&vw->list);
diff --git a/drivers/serial/crisv10.c b/drivers/serial/crisv10.c
index d11d898..e6a1cb7 100644
--- a/drivers/serial/crisv10.c
+++ b/drivers/serial/crisv10.c
@@ -3981,7 +3981,7 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
 	 */
 	if (tty_hung_up_p(filp) ||
 	    (info->flags & ASYNC_CLOSING)) {
-		wait_event_interruptible(info->close_wait,
+		wait_event_interruptible_tty(info->close_wait,
 			!(info->flags & ASYNC_CLOSING));
 #ifdef SERIAL_DO_RESTART
 		if (info->flags & ASYNC_HUP_NOTIFY)
@@ -4139,7 +4139,7 @@ rs_open(struct tty_struct *tty, struct file * filp)
 	 */
 	if (tty_hung_up_p(filp) ||
 	    (info->flags & ASYNC_CLOSING)) {
-		wait_event_interruptible(info->close_wait,
+		wait_event_interruptible_tty(info->close_wait,
 			!(info->flags & ASYNC_CLOSING));
 #ifdef SERIAL_DO_RESTART
 		return ((info->flags & ASYNC_HUP_NOTIFY) ?
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c35ad81..9d4c87f 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -599,5 +599,47 @@ static inline void tty_unlock(void) __releases(kernel_lock)
 }
 #define tty_locked()		(kernel_locked())
 
+/*
+ * wait_event_interruptible_tty -- wait for a condition with the tty lock held
+ *
+ * The condition we are waiting for might take a long time to
+ * become true, or might depend on another thread taking the
+ * BTM. In either case, we need to drop the BTM to guarantee
+ * forward progress. This is a leftover from the conversion
+ * from the BKL and should eventually get removed as the BTM
+ * falls out of use.
+ *
+ * Do not use in new code.
+ */
+#define wait_event_interruptible_tty(wq, condition)			\
+({									\
+	int __ret = 0;							\
+	if (!(condition)) {						\
+		__wait_event_interruptible_tty(wq, condition, __ret);	\
+	}								\
+	__ret;								\
+})
+
+#define __wait_event_interruptible_tty(wq, condition, ret)		\
+do {									\
+	DEFINE_WAIT(__wait);						\
+									\
+	for (;;) {							\
+		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		if (!signal_pending(current)) {				\
+			tty_unlock();					\
+			schedule();					\
+			tty_lock();					\
+			continue;					\
+		}							\
+		ret = -ERESTARTSYS;					\
+		break;							\
+	}								\
+	finish_wait(&wq, &__wait);					\
+} while (0)
+
+
 #endif /* __KERNEL__ */
 #endif
-- 
1.7.0.4

[PATCH 06/10] tty: annotate tty_write_lock

[Arnd Bergmann]

atomic_write_lock never nests below BTM, so
there are no lock order problems between the
two.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/tty_io.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index d51993b..36aecba 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -912,6 +912,11 @@ void tty_write_unlock(struct tty_struct *tty)
 
 int tty_write_lock(struct tty_struct *tty, int ndelay)
 {
+	/*
+	 * code inspection has shown that this is never called
+	 * with the BTM held. Make sure this stays that way.
+	 */
+	WARN_ON_ONCE(tty_locked());
 	if (!mutex_trylock(&tty->atomic_write_lock)) {
 		if (ndelay)
 			return -EAGAIN;
-- 
1.7.0.4

[PATCH 07/10] tty: reorder ldisc locking

[Arnd Bergmann]

We need to release the BTM in paste_selection() when
sleeping in tty_ldisc_ref_wait to avoid deadlocks
with tty_ldisc_enable.

In tty_set_ldisc, we now always grab the BTM before
taking the ldisc_mutex in order to avoid AB-BA
deadlocks between the two.

tty_ldisc_halt potentially blocks on a workqueue
function that takes the BTM, so we must release
the BTM before calling it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/selection.c |    9 +++++++--
 drivers/char/tty_ldisc.c |   24 ++++++++++++++++++++----
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/char/selection.c b/drivers/char/selection.c
index 85211a3..75889cd 100644
--- a/drivers/char/selection.c
+++ b/drivers/char/selection.c
@@ -319,8 +319,13 @@ int paste_selection(struct tty_struct *tty)
 	poke_blanked_console();
 	release_console_sem();
 
-	ld = tty_ldisc_ref_wait(tty);
-	
+	ld = tty_ldisc_ref(tty);
+	if (!ld) {
+		tty_unlock();
+		ld = tty_ldisc_ref_wait(tty);
+		tty_lock();
+	}
+
 	add_wait_queue(&vc->paste_wait, &wait);
 	while (sel_buffer && sel_buffer_lth > pasted) {
 		set_current_state(TASK_INTERRUPTIBLE);
diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 97681ff..0f49479 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -582,6 +582,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	tty_wait_until_sent(tty, 0);
 
+	tty_lock();
 	mutex_lock(&tty->ldisc_mutex);
 
 	/*
@@ -591,13 +592,13 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	while (test_bit(TTY_LDISC_CHANGING, &tty->flags)) {
 		mutex_unlock(&tty->ldisc_mutex);
+		tty_unlock();
 		wait_event(tty_ldisc_wait,
 			test_bit(TTY_LDISC_CHANGING, &tty->flags) == 0);
+		tty_lock();
 		mutex_lock(&tty->ldisc_mutex);
 	}
 
-	tty_lock();
-
 	set_bit(TTY_LDISC_CHANGING, &tty->flags);
 
 	/*
@@ -634,8 +635,8 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	flush_scheduled_work();
 
-	mutex_lock(&tty->ldisc_mutex);
 	tty_lock();
+	mutex_lock(&tty->ldisc_mutex);
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
@@ -782,7 +783,20 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 * Avoid racing set_ldisc or tty_ldisc_release
 	 */
 	mutex_lock(&tty->ldisc_mutex);
-	tty_ldisc_halt(tty);
+
+	/*
+	 * this is like tty_ldisc_halt, but we need to give up
+	 * the BTM before calling cancel_delayed_work_sync,
+	 * which may need to wait for another function taking the BTM
+	 */
+	clear_bit(TTY_LDISC, &tty->flags);
+	tty_unlock();
+	cancel_delayed_work_sync(&tty->buf.work);
+	mutex_unlock(&tty->ldisc_mutex);
+
+	tty_lock();
+	mutex_lock(&tty->ldisc_mutex);
+
 	/* At this point we have a closed ldisc and we want to
 	   reopen it. We could defer this to the next open but
 	   it means auditing a lot of other paths so this is
@@ -853,8 +867,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	 * race with the set_ldisc code path.
 	 */
 
+	tty_unlock();
 	tty_ldisc_halt(tty);
 	flush_scheduled_work();
+	tty_lock();
 
 	mutex_lock(&tty->ldisc_mutex);
 	/*
-- 
1.7.0.4

[PATCH 08/10] tty: untangle locking of wait_until_sent

[Arnd Bergmann]

Some wait_until_sent versions require the big
tty mutex, others don't and some callers of
wait_until_sent already hold it while other don't.
That leads to recursive use of the BTM in these
functions, which we're trying to get rid of.

This turns all cleans up the locking there so
that the driver's wait_until_sent function
never takes the BTM itself if it is already
called with that lock held.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/amiserial.c     |   11 +++++++++--
 drivers/serial/68360serial.c |    2 --
 drivers/serial/crisv10.c     |    2 --
 drivers/serial/serial_core.c |   31 ++++++++++++++++++++++---------
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/char/amiserial.c b/drivers/char/amiserial.c
index 5bd382e..3c0231a 100644
--- a/drivers/char/amiserial.c
+++ b/drivers/char/amiserial.c
@@ -1527,6 +1527,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	struct async_struct * info = tty->driver_data;
 	unsigned long orig_jiffies, char_time;
+	int tty_was_locked = tty_locked();
 	int lsr;
 
 	if (serial_paranoia_check(info, tty->name, "rs_wait_until_sent"))
@@ -1537,7 +1538,12 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 
 	orig_jiffies = jiffies;
 
-	tty_lock_nested(); /* tty_wait_until_sent is called from lots of places */
+	/*
+	 * tty_wait_until_sent is called from lots of places,
+	 * with or without the BTM.
+	 */
+	if (!tty_was_locked)
+		tty_lock();
 	/*
 	 * Set the check interval to be 1/5 of the estimated time to
 	 * send a single character, and make it at least 1.  The check
@@ -1578,7 +1584,8 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 			break;
 	}
 	__set_current_state(TASK_RUNNING);
-	tty_unlock();
+	if (!tty_was_locked())
+		tty_unlock();
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
 #endif
diff --git a/drivers/serial/68360serial.c b/drivers/serial/68360serial.c
index c17a595..5b7b801 100644
--- a/drivers/serial/68360serial.c
+++ b/drivers/serial/68360serial.c
@@ -1705,7 +1705,6 @@ static void rs_360_wait_until_sent(struct tty_struct *tty, int timeout)
 	printk("jiff=%lu...", jiffies);
 #endif
 
-	tty_lock_nested(); /* always held already since we come from ->close */
 	/* We go through the loop at least once because we can't tell
 	 * exactly when the last character exits the shifter.  There can
 	 * be at least two characters waiting to be sent after the buffers
@@ -1734,7 +1733,6 @@ static void rs_360_wait_until_sent(struct tty_struct *tty, int timeout)
 			bdp--;
 	} while (bdp->status & BD_SC_READY);
 	current->state = TASK_RUNNING;
-	tty_unlock();
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
 #endif
diff --git a/drivers/serial/crisv10.c b/drivers/serial/crisv10.c
index e6a1cb7..0825d4a 100644
--- a/drivers/serial/crisv10.c
+++ b/drivers/serial/crisv10.c
@@ -3924,7 +3924,6 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 	 * Check R_DMA_CHx_STATUS bit 0-6=number of available bytes in FIFO
 	 * R_DMA_CHx_HWSW bit 31-16=nbr of bytes left in DMA buffer (0=64k)
 	 */
-	tty_lock_nested(); /* locked already when coming from close */
 	orig_jiffies = jiffies;
 	while (info->xmit.head != info->xmit.tail || /* More in send queue */
 	       (*info->ostatusadr & 0x007f) ||  /* more in FIFO */
@@ -3941,7 +3940,6 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
 			curr_time_usec - info->last_tx_active_usec;
 	}
 	set_current_state(TASK_RUNNING);
-	tty_unlock();
 }
 
 /*
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 4b151a4..78b1eac 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -60,7 +60,7 @@ static struct lock_class_key port_lock_key;
 
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios);
-static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
+static void __uart_wait_until_sent(struct uart_port *port, int timeout);
 static void uart_change_pm(struct uart_state *state, int pm_state);
 
 /*
@@ -1322,8 +1322,16 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, msecs_to_jiffies(port->closing_wait));
+	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE) {
+		/*
+		 * hack: open-coded tty_wait_until_sent to avoid
+		 * recursive tty_lock
+		 */
+		long timeout = msecs_to_jiffies(port->closing_wait);
+		if (wait_event_interruptible_timeout(tty->write_wait,
+				!tty_chars_in_buffer(tty), timeout) >= 0)
+			__uart_wait_until_sent(uport, timeout);
+	}
 
 	/*
 	 * At this point, we stop accepting input.  To do this, we
@@ -1339,7 +1347,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 		 * has completely drained; this is especially
 		 * important if there is a transmit FIFO!
 		 */
-		uart_wait_until_sent(tty, uport->timeout);
+		__uart_wait_until_sent(uport, uport->timeout);
 	}
 
 	uart_shutdown(tty, state);
@@ -1373,17 +1381,13 @@ done:
 	mutex_unlock(&port->mutex);
 }
 
-static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
+static void __uart_wait_until_sent(struct uart_port *port, int timeout)
 {
-	struct uart_state *state = tty->driver_data;
-	struct uart_port *port = state->uart_port;
 	unsigned long char_time, expire;
 
 	if (port->type == PORT_UNKNOWN || port->fifosize == 0)
 		return;
 
-	tty_lock_nested(); /* already locked when coming from close */
-
 	/*
 	 * Set the check interval to be 1/5 of the estimated time to
 	 * send a single character, and make it at least 1.  The check
@@ -1429,6 +1433,15 @@ static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
 			break;
 	}
 	set_current_state(TASK_RUNNING); /* might not be needed */
+}
+
+static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
+{
+	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->uart_port;
+
+	tty_lock();
+	__uart_wait_until_sent(port, timeout);
 	tty_unlock();
 }
 
-- 
1.7.0.4

Re: [PATCH 08/10] tty: untangle locking of wait_until_sent

[Daniel K.]

Arnd Bergmann wrote:
> diff --git a/drivers/char/amiserial.c b/drivers/char/amiserial.c
> index 5bd382e..3c0231a 100644
> --- a/drivers/char/amiserial.c
> +++ b/drivers/char/amiserial.c
> @@ -1527,6 +1527,7 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
>  {
>  	struct async_struct * info = tty->driver_data;
>  	unsigned long orig_jiffies, char_time;
> +	int tty_was_locked = tty_locked();
>  	int lsr;
>  
>  	if (serial_paranoia_check(info, tty->name, "rs_wait_until_sent"))
> @@ -1578,7 +1584,8 @@ static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
>  			break;
>  	}
>  	__set_current_state(TASK_RUNNING);
> -	tty_unlock();
> +	if (!tty_was_locked())

tty_was_locked is not a function.


Daniel K.

[PATCH 09/10] tty: remove tty_lock_nested

[Arnd Bergmann]

This changes all remaining users of tty_lock_nested
to be non-recursive, which lets us kill this function.
As a consequence, we won't need to keep the lock count
any more, which allows more simplifications later.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/pty.c       |    2 +-
 drivers/char/selection.c |    4 ++--
 drivers/char/stallion.c  |    2 ++
 drivers/char/tty_io.c    |   41 ++++++++++++++++++++---------------------
 drivers/char/tty_ldisc.c |    3 +--
 include/linux/tty.h      |   16 +---------------
 6 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index b441ebd..89c13eb 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -62,7 +62,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 		if (tty->driver == ptm_driver)
 			devpts_pty_kill(tty->link);
 #endif
-		tty_vhangup(tty->link);
+		tty_vhangup_locked(tty->link);
 	}
 }
 
diff --git a/drivers/char/selection.c b/drivers/char/selection.c
index 75889cd..ebae344 100644
--- a/drivers/char/selection.c
+++ b/drivers/char/selection.c
@@ -313,7 +313,8 @@ int paste_selection(struct tty_struct *tty)
 	struct  tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	tty_lock_nested(); /* always called with BTM from vt_ioctl */
+	/* always called with BTM from vt_ioctl */
+	WARN_ON(!tty_locked());
 
 	acquire_console_sem();
 	poke_blanked_console();
@@ -343,6 +344,5 @@ int paste_selection(struct tty_struct *tty)
 	__set_current_state(TASK_RUNNING);
 
 	tty_ldisc_deref(ld);
-	tty_unlock();
 	return 0;
 }
diff --git a/drivers/char/stallion.c b/drivers/char/stallion.c
index f2167f8..feed311 100644
--- a/drivers/char/stallion.c
+++ b/drivers/char/stallion.c
@@ -1656,7 +1656,9 @@ static void stl_cleanup_panels(struct stlbrd *brdp)
 				continue;
 			tty = tty_port_tty_get(&portp->port);
 			if (tty != NULL) {
+				tty_lock(); /* call tty_port_hangup with BTM */
 				stl_hangup(tty);
+				tty_unlock();
 				tty_kref_put(tty);
 			}
 			kfree(portp->tx.buf);
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 36aecba..7bb9685 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -492,10 +492,8 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-static void do_tty_hangup(struct work_struct *work)
+void tty_vhangup_locked(struct tty_struct *tty)
 {
-	struct tty_struct *tty =
-		container_of(work, struct tty_struct, hangup_work);
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
 	struct task_struct *p;
@@ -517,8 +515,6 @@ static void do_tty_hangup(struct work_struct *work)
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
-	tty_lock_nested(); /* called with BTM held from pty_close and
-				others */
 	check_tty_count(tty, "do_tty_hangup");
 
 	file_list_lock();
@@ -598,11 +594,20 @@ static void do_tty_hangup(struct work_struct *work)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	tty_ldisc_enable(tty);
-	tty_unlock();
 	if (f)
 		fput(f);
 }
 
+static void do_tty_hangup(struct work_struct *work)
+{
+	struct tty_struct *tty =
+		container_of(work, struct tty_struct, hangup_work);
+
+	tty_lock();
+	tty_vhangup_locked(tty);
+	tty_unlock();
+}
+
 /**
  *	tty_hangup		-	trigger a hangup event
  *	@tty: tty to hangup
@@ -638,7 +643,9 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	do_tty_hangup(&tty->hangup_work);
+	tty_lock();
+	tty_vhangup_locked(tty);
+	tty_unlock();
 }
 
 EXPORT_SYMBOL(tty_vhangup);
@@ -719,10 +726,12 @@ void disassociate_ctty(int on_exit)
 	tty = get_current_tty();
 	if (tty) {
 		tty_pgrp = get_pid(tty->pgrp);
-		tty_lock_nested(); /* see above */
-		if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY)
-			tty_vhangup(tty);
-		tty_unlock();
+		if (on_exit) {
+			tty_lock();
+			if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
+				tty_vhangup_locked(tty);
+			tty_unlock();
+		}
 		tty_kref_put(tty);
 	} else if (on_exit) {
 		struct pid *old_pgrp;
@@ -1218,18 +1227,14 @@ static int tty_driver_install_tty(struct tty_driver *driver,
 	int ret;
 
 	if (driver->ops->install) {
-		tty_lock_nested(); /* already called with BTM held */
 		ret = driver->ops->install(driver, tty);
-		tty_unlock();
 		return ret;
 	}
 
 	if (tty_init_termios(tty) == 0) {
-		tty_lock_nested();
 		tty_driver_kref_get(driver);
 		tty->count++;
 		driver->ttys[idx] = tty;
-		tty_unlock();
 		return 0;
 	}
 	return -ENOMEM;
@@ -1322,15 +1327,11 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
 	struct tty_struct *tty;
 	int retval;
 
-	tty_lock_nested(); /* always called with tty lock held already */
-
 	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) {
-		tty_unlock();
 		return ERR_PTR(-EIO);
 	}
-	tty_unlock();
 
 	/*
 	 * First time open is complex, especially for PTY devices.
@@ -1374,9 +1375,7 @@ release_mem_out:
 	if (printk_ratelimit())
 		printk(KERN_INFO "tty_init_dev: ldisc open failed, "
 				 "clearing slot %d\n", idx);
-	tty_lock_nested();
 	release_tty(tty, idx);
-	tty_unlock();
 	return ERR_PTR(retval);
 }
 
diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 0f49479..412f977 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -450,9 +450,8 @@ static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
 	if (ld->ops->open) {
 		int ret;
                 /* BTM here locks versus a hangup event */
-		tty_lock_nested(); /* always held here already */
+		WARN_ON(!tty_locked());
 		ret = ld->ops->open(tty);
-		tty_unlock();
 		return ret;
 	}
 	return 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9d4c87f..b227616 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -415,6 +415,7 @@ extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
+extern void tty_vhangup_locked(struct tty_struct *tty);
 extern void tty_vhangup_self(void);
 extern void tty_unhangup(struct file *filp);
 extern int tty_hung_up_p(struct file *filp);
@@ -573,21 +574,6 @@ extern long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
 		     unsigned int cmd, unsigned long arg);
 
 /* functions for preparation of BKL removal */
-
-/*
- * tty_lock_nested get the tty_lock while potentially holding it
- *
- * The Big TTY Mutex is a recursive lock, meaning you can take it
- * from a thread that is already holding it.
- * This is bad for a number of reasons, so tty_lock_nested should
- * really be used as rarely as possible. If a code location can
- * be shown to never get called with this held already, it should
- * use tty_lock() instead.
- */
-static inline void __lockfunc tty_lock_nested(void) __acquires(kernel_lock)
-{
-	lock_kernel();
-}
 static inline void tty_lock(void) __acquires(kernel_lock)
 {
 	WARN_ON(kernel_locked());
-- 
1.7.0.4

[PATCH 10/10] tty: implement BTM as mutex instead of BKL

[Arnd Bergmann]

The tty locking now follows the rules for mutexes, so
we can replace the BKL usage with a new subsystem
wide mutex.

This patch for now makes the new behaviour an optional
experimental feature that can be enabled for testing
purposes.

Using a regular mutex here will change the behaviour
when blocked on the BTM from spinning to sleeping,
but that should not be visible to the user.

Using the mutex also means that all the BTM is now
covered by lockdep.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/Makefile    |    1 +
 drivers/char/tty_mutex.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/tty.h      |    8 +++++++
 lib/Kconfig.debug        |   10 +++++++++
 4 files changed, 66 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/tty_mutex.c

diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index f957edf..74ee3fa 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -9,6 +9,7 @@ FONTMAPFILE = cp437.uni
 
 obj-y	 += mem.o random.o tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o tty_buffer.o tty_port.o
 
+obj-$(CONFIG_TTY_MUTEX)		+= tty_mutex.o
 obj-$(CONFIG_LEGACY_PTYS)	+= pty.o
 obj-$(CONFIG_UNIX98_PTYS)	+= pty.o
 obj-y				+= misc.o
diff --git a/drivers/char/tty_mutex.c b/drivers/char/tty_mutex.c
new file mode 100644
index 0000000..f66dfdf
--- /dev/null
+++ b/drivers/char/tty_mutex.c
@@ -0,0 +1,47 @@
+/*
+ * drivers/char/tty_lock.c
+ */
+#include <linux/tty.h>
+#include <linux/module.h>
+#include <linux/kallsyms.h>
+#include <linux/semaphore.h>
+#include <linux/sched.h>
+
+/*
+ * The 'big tty semaphore'
+ *
+ * This mutex is taken and released by tty_lock() and tty_unlock(),
+ * replacing the older big kernel mutex.
+ * It can no longer be taken recursively, and does not get
+ * released implicitly while sleeping.
+ *
+ * Don't use in new code.
+ */
+static DEFINE_MUTEX(big_tty_mutex);
+struct task_struct *__big_tty_mutex_owner;
+EXPORT_SYMBOL_GPL(__big_tty_mutex_owner);
+
+/*
+ * Getting the big tty mutex.
+ */
+void __lockfunc tty_lock(void)
+{
+	struct task_struct *task = current;
+
+	WARN_ON(__big_tty_mutex_owner == task);
+
+	mutex_lock(&big_tty_mutex);
+	__big_tty_mutex_owner = task;
+}
+EXPORT_SYMBOL(tty_lock);
+
+void __lockfunc tty_unlock(void)
+{
+	struct task_struct *task = current;
+
+	WARN_ON(__big_tty_mutex_owner != task);
+	__big_tty_mutex_owner = NULL;
+
+	mutex_unlock(&big_tty_mutex);
+}
+EXPORT_SYMBOL(tty_unlock);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index b227616..d8d90f6 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -573,7 +573,14 @@ extern int vt_ioctl(struct tty_struct *tty, struct file *file,
 extern long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
 		     unsigned int cmd, unsigned long arg);
 
+/* tty_mutex.c */
 /* functions for preparation of BKL removal */
+#ifdef CONFIG_TTY_MUTEX
+extern void __lockfunc tty_lock(void) __acquires(tty_lock);
+extern void __lockfunc tty_unlock(void) __releases(tty_lock);
+extern struct task_struct *__big_tty_mutex_owner;
+#define tty_locked()		(current == __big_tty_mutex_owner)
+#else
 static inline void tty_lock(void) __acquires(kernel_lock)
 {
 	WARN_ON(kernel_locked());
@@ -584,6 +591,7 @@ static inline void tty_unlock(void) __releases(kernel_lock)
 	unlock_kernel();
 }
 #define tty_locked()		(kernel_locked())
+#endif
 
 /*
  * wait_event_interruptible_tty -- wait for a condition with the tty lock held
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 935248b..0b3e1ad 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -428,6 +428,16 @@ config RT_MUTEX_TESTER
 	help
 	  This option enables a rt-mutex tester.
 
+config TTY_MUTEX
+	bool "Use a mutex instead of BKL for TTY locking"
+	depends on EXPERIMENTAL && SMP
+	help
+	  The TTY subsystem traditionally depends on the big kernel lock
+	  for serialization. Saying Y here replaces the BKL with the Big
+	  TTY Mutex (BTM).
+	  Building a kernel without the BKL is only possible with TTY_MUTEX
+	  enabled.
+
 config DEBUG_SPINLOCK
 	bool "Spinlock and rw-lock debugging: basic checks"
 	depends on DEBUG_KERNEL
-- 
1.7.0.4

Re: [PATCH 10/10] tty: implement BTM as mutex instead of BKL

[Daniel K.]

Arnd Bergmann wrote:
> diff --git a/drivers/char/tty_mutex.c b/drivers/char/tty_mutex.c
> new file mode 100644
> index 0000000..f66dfdf
> --- /dev/null
> +++ b/drivers/char/tty_mutex.c
> @@ -0,0 +1,47 @@
> +/*
> + * drivers/char/tty_lock.c
> + */
> +#include <linux/tty.h>
> +#include <linux/module.h>
> +#include <linux/kallsyms.h>
> +#include <linux/semaphore.h>
> +#include <linux/sched.h>
> +
> +/*
> + * The 'big tty semaphore'

Referred to as Big TTY Mutex or BTM elsewhere.


> + * This mutex is taken and released by tty_lock() and tty_unlock(),
> + * replacing the older big kernel mutex.

big kernel lock, or BKL?


> + * It can no longer be taken recursively, and does not get
> + * released implicitly while sleeping.
> + *
> + * Don't use in new code.
> + */
> +static DEFINE_MUTEX(big_tty_mutex);
> +struct task_struct *__big_tty_mutex_owner;
> +EXPORT_SYMBOL_GPL(__big_tty_mutex_owner);

> +config TTY_MUTEX
> +	bool "Use a mutex instead of BKL for TTY locking"
> +	depends on EXPERIMENTAL && SMP
> +	help
> +	  The TTY subsystem traditionally depends on the big kernel lock
> +	  for serialization. Saying Y here replaces the BKL with the Big
> +	  TTY Mutex (BTM).
> +	  Building a kernel without the BKL is only possible with TTY_MUTEX
> +	  enabled.
> +


Daniel K.

Re: [PATCH 10/10] tty: implement BTM as mutex instead of BKL

[Arnd Bergmann]

On Sunday 16 May 2010 05:33:14 Daniel K. wrote:
> Arnd Bergmann wrote:
> > diff --git a/drivers/char/tty_mutex.c b/drivers/char/tty_mutex.c
> > new file mode 100644
> > index 0000000..f66dfdf
> > --- /dev/null
> > +++ b/drivers/char/tty_mutex.c
> > @@ -0,0 +1,47 @@
> > +/*
> > + * drivers/char/tty_lock.c
> > + */
> > +#include <linux/tty.h>
> > +#include <linux/module.h>
> > +#include <linux/kallsyms.h>
> > +#include <linux/semaphore.h>
> > +#include <linux/sched.h>
> > +
> > +/*
> > + * The 'big tty semaphore'
> 
> Referred to as Big TTY Mutex or BTM elsewhere.
> 
> 
> > + * This mutex is taken and released by tty_lock() and tty_unlock(),
> > + * replacing the older big kernel mutex.
> 
> big kernel lock, or BKL?
> 

Thanks for looking at this, this (and the bug you found in amiserial)
were caused by changing it a few times from one implementation to the next.

Fixed now and pushed out to git.

	Anrd

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Alan Cox]

On Sat, 15 May 2010 22:59:46 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> This is the third attempt to get the BKL out of the
> TTY code. This version goes much further than the
> previous one, and eliminates most of the code I
> had introduced there.

At this point I think the only way to make further progress is to
actually push this stuff into the kernel on top of the BKL removal
patches for the drivers and see what happens. Something will no doubt
break but we can try and nail them in time or if not revert the series
and try again next kernel.

The big nasty remaining after this is drivers/serial, which probably
wants some serious reconstructive surgery to use kfifo etc. Either way
its an independent problem to the stuff this lot tackles.

So this series

Acked-by: Alan Cox <alan@linux.intel.com>

Alan

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Greg KH]

On Mon, May 17, 2010 at 02:41:30PM +0100, Alan Cox wrote:
> On Sat, 15 May 2010 22:59:46 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > This is the third attempt to get the BKL out of the
> > TTY code. This version goes much further than the
> > previous one, and eliminates most of the code I
> > had introduced there.
> 
> At this point I think the only way to make further progress is to
> actually push this stuff into the kernel on top of the BKL removal
> patches for the drivers and see what happens. Something will no doubt
> break but we can try and nail them in time or if not revert the series
> and try again next kernel.
> 
> The big nasty remaining after this is drivers/serial, which probably
> wants some serious reconstructive surgery to use kfifo etc. Either way
> its an independent problem to the stuff this lot tackles.
> 
> So this series
> 
> Acked-by: Alan Cox <alan@linux.intel.com>

I'd prefer to do this for .36, not for .35.  Arnd, I'll be glad to queue
these patches up to the tty development tree after .35-rc1 is out, so it
gets lots of testing in the linux-next tree, is that ok?  If so, can you
resend them to me at that time?

thanks,

greg k-h

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Monday 17 May 2010, Greg KH wrote:
> 
> I'd prefer to do this for .36, not for .35.  Arnd, I'll be glad to queue
> these patches up to the tty development tree after .35-rc1 is out, so it
> gets lots of testing in the linux-next tree, is that ok?  If so, can you
> resend them to me at that time?

Ok, I'll make sure it still works with .35-rc1 when it's out and add
Alan's ACK, then send a you a pull request. Did you already merge the
series from Alan that this is based on, or do you want those patches
to wait as well? They are in my git tree, so they will be included
in my pull request if they are not in -rc1.

Also, it would be nice if you could merge the first patch of my
series, which should be completely harmless (it just renames
function calls) but having it upstream means that we can
keep the remaining patches that are required for BKL-less
kernels a lot smaller.

	Arnd

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Greg KH]

On Mon, May 17, 2010 at 08:30:23PM +0200, Arnd Bergmann wrote:
> On Monday 17 May 2010, Greg KH wrote:
> > 
> > I'd prefer to do this for .36, not for .35.  Arnd, I'll be glad to queue
> > these patches up to the tty development tree after .35-rc1 is out, so it
> > gets lots of testing in the linux-next tree, is that ok?  If so, can you
> > resend them to me at that time?
> 
> Ok, I'll make sure it still works with .35-rc1 when it's out and add
> Alan's ACK, then send a you a pull request. Did you already merge the
> series from Alan that this is based on, or do you want those patches
> to wait as well? They are in my git tree, so they will be included
> in my pull request if they are not in -rc1.

No, I do not have his patches anywhere, as I didn't expect that they
were being sent to me to apply :)

> Also, it would be nice if you could merge the first patch of my
> series, which should be completely harmless (it just renames
> function calls) but having it upstream means that we can
> keep the remaining patches that are required for BKL-less
> kernels a lot smaller.

Can you resend this patch to me so that I can include it?

thanks,

greg k-h

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Tuesday 18 May 2010, Greg KH wrote:
> On Mon, May 17, 2010 at 08:30:23PM +0200, Arnd Bergmann wrote:
> > Ok, I'll make sure it still works with .35-rc1 when it's out and add
> > Alan's ACK, then send a you a pull request. Did you already merge the
> > series from Alan that this is based on, or do you want those patches
> > to wait as well? They are in my git tree, so they will be included
> > in my pull request if they are not in -rc1.
> 
> No, I do not have his patches anywhere, as I didn't expect that they
> were being sent to me to apply :)
> 
> > Also, it would be nice if you could merge the first patch of my
> > series, which should be completely harmless (it just renames
> > function calls) but having it upstream means that we can
> > keep the remaining patches that are required for BKL-less
> > kernels a lot smaller.
> 
> Can you resend this patch to me so that I can include it?

I think we should only merge this patch if you also want to
take the patches from Alan mentioned above, otherwise it would
cause more trouble to rebase everything than we can gain from
merging it now.

If this makes sense to you, please pull

git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git tty-for-2.6.35

This contains all of Alan's patches, plus the first one of my
series on top of that.

	Arnd

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Greg KH]

On Tue, May 18, 2010 at 11:52:01PM +0200, Arnd Bergmann wrote:
> > 
> > Can you resend this patch to me so that I can include it?
> 
> I think we should only merge this patch if you also want to
> take the patches from Alan mentioned above, otherwise it would
> cause more trouble to rebase everything than we can gain from
> merging it now.

Then I'll just wait, is that ok?

> If this makes sense to you, please pull
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git tty-for-2.6.35
> 
> This contains all of Alan's patches, plus the first one of my
> series on top of that.

I'll wait for you all to redo this and resend it to me for the .36
kernel.

Oh, and I really don't like to have a git pull, can you just email me
the patches?

thanks,

greg k-h

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Wednesday 19 May 2010, Greg KH wrote:
> On Tue, May 18, 2010 at 11:52:01PM +0200, Arnd Bergmann wrote:
> > > 
> > > Can you resend this patch to me so that I can include it?
> > 
> > I think we should only merge this patch if you also want to
> > take the patches from Alan mentioned above, otherwise it would
> > cause more trouble to rebase everything than we can gain from
> > merging it now.
> 
> Then I'll just wait, is that ok?

ok

> > If this makes sense to you, please pull
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git tty-for-2.6.35
> > 
> > This contains all of Alan's patches, plus the first one of my
> > series on top of that.
> 
> I'll wait for you all to redo this and resend it to me for the .36
> kernel.
> 
> Oh, and I really don't like to have a git pull, can you just email me
> the patches?

Yes, of course. That also answers the question whether to rebase or to
merge with -rc1.

	Arnd

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Monday 17 May 2010, Alan Cox wrote:
> At this point I think the only way to make further progress is to
> actually push this stuff into the kernel on top of the BKL removal
> patches for the drivers and see what happens. Something will no doubt
> break but we can try and nail them in time or if not revert the series
> and try again next kernel.

Ok, thanks. Most of the breakage that I've seen while testing this (after
I first got it working) was either trivially breaking the build, or
causing lockdep splats for things that are correct with CONFIG_TTY_MUTEX
disabled but violating the lock order with it enabled.

I'm ok with having the series in -next for another kernel to improve
the quality, as long as no new major changes to the tty locking get
merged that break the assumptions I made. Your last series did make
my work easier (or maybe even possible) to get to this point, but most
of the time was spent checking every possible code path by hand to
validate the locking rules, and that would need to be done again
if the code changes.

	Arnd

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Tony Luck]

On Mon, May 17, 2010 at 6:41 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> At this point I think the only way to make further progress is to
> actually push this stuff into the kernel on top of the BKL removal
> patches for the drivers and see what happens. Something will no doubt
> break but we can try and nail them in time or if not revert the series
> and try again next kernel.

These changes showed up in linux-next (tag: next-20100617) ... and I'm seeing
a few WARN_ON messages on ia64 while booting:

WARNING: at include/linux/tty.h:589 tty_open+0x160/0xc60()
WARNING: at include/linux/tty.h:589 tty_open+0x9d0/0xc60()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()

Stack trace for the first of these looks like:
Call Trace:
 [<a0000001000159d0>] show_stack+0x50/0xa0
 [<a00000010090f250>] dump_stack+0x30/0x50
 [<a00000010008e2c0>] warn_slowpath_common+0xc0/0x120
 [<a00000010008e360>] warn_slowpath_null+0x40/0x60
 [<a00000010053ebe0>] tty_open+0x160/0xc60
 [<a0000001001af9b0>] chrdev_open+0x310/0x360
 [<a0000001001a58b0>] __dentry_open+0x350/0x680
 [<a0000001001a5d80>] nameidata_to_filp+0x80/0xc0
 [<a0000001001bfee0>] finish_open+0x160/0x380
 [<a0000001001c0cc0>] do_last+0xbc0/0xce0
 [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
 [<a0000001001a5290>] do_sys_open+0x90/0x200
 [<a0000001001a54d0>] sys_open+0x50/0x80
 [<a000000100b907e0>] kernel_init+0x340/0x420
 [<a000000100013c10>] kernel_thread_helper+0x30/0x60
 [<a00000010000a0c0>] start_kernel_thread+0x20/0x40

Does anyone see anything similar on other architectures?  Or is ia64 doing
something "special" here?

-Tony

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Thursday 17 June 2010 21:13:52 Tony Luck wrote:

> These changes showed up in linux-next (tag: next-20100617) ... and I'm seeing
> a few WARN_ON messages on ia64 while booting:
> 
> WARNING: at include/linux/tty.h:589 tty_open+0x160/0xc60()
> ...
> Stack trace for the first of these looks like:
> Call Trace:
> ...
>  [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
>  [<a0000001001a5290>] do_sys_open+0x90/0x200
>  [<a0000001001a54d0>] sys_open+0x50/0x80
>  [<a000000100b907e0>] kernel_init+0x340/0x420
>  [<a000000100013c10>] kernel_thread_helper+0x30/0x60
>  [<a00000010000a0c0>] start_kernel_thread+0x20/0x40
> 
> Does anyone see anything similar on other architectures?  Or is ia64 doing
> something "special" here?

Ah, I forgot to test the tty series without the other patches I have
in my bkl removal repository and without CONFIG_TTY_MUTEX.

Does the patch below make this go away? We should probably include
the 'misc' branch of my BKL repository in -next to fix this.

	Arnd

--
>From 99b699e56b23775c0c9a131208e1a5e13f6cfad3 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 15 Mar 2010 19:00:34 +0100
Subject: [PATCH] init: remove the BKL from startup code

I have shown by code review that no driver takes
the BKL at init time any more, so whatever the
init code was locking against is no longer there
and it is now safe to remove the BKL there.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/init/main.c b/init/main.c
index 3bdb152..81821e1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -434,7 +434,6 @@ static noinline void __init_refok rest_init(void)
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
-	unlock_kernel();
 
 	/*
 	 * The boot idle thread must execute schedule()
@@ -555,7 +554,6 @@ asmlinkage void __init start_kernel(void)
  * Interrupts are still disabled. Do necessary setups, then
  * enable them
  */
-	lock_kernel();
 	tick_init();
 	boot_cpu_init();
 	page_address_init();
@@ -819,7 +817,6 @@ static noinline int init_post(void)
 	/* need to finish all async __init code before freeing the memory */
 	async_synchronize_full();
 	free_initmem();
-	unlock_kernel();
 	mark_rodata_ro();
 	system_state = SYSTEM_RUNNING;
 	numa_default_policy();
@@ -855,8 +852,6 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
-	lock_kernel();
-
 	/*
 	 * init can allocate pages on any node
 	 */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 086d363..8047ca5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -734,13 +734,6 @@ __acquires(kernel_lock)
 		return -1;
 	}
 
-	/*
-	 * When this gets called we hold the BKL which means that
-	 * preemption is disabled. Various trace selftests however
-	 * need to disable and enable preemption for successful tests.
-	 * So we drop the BKL here and grab it after the tests again.
-	 */
-	unlock_kernel();
 	mutex_lock(&trace_types_lock);
 
 	tracing_selftest_running = true;
@@ -822,7 +815,6 @@ __acquires(kernel_lock)
 #endif
 
  out_unlock:
-	lock_kernel();
 	return ret;
 }
 

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Tony Luck]

> Does the patch below make this go away? We should probably include
> the 'misc' branch of my BKL repository in -next to fix this.

That fixes a couple of instances (the tty_open() cases).  I still have:

WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()

Stack traces for these all look the same:

Call Trace:
 [<a000000100015990>] show_stack+0x50/0xa0
 [<a00000010090f1f0>] dump_stack+0x30/0x50
 [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
 [<a00000010008e320>] warn_slowpath_null+0x40/0x60
 [<a00000010053d910>] tty_release+0x90/0xbc0
 [<a0000001001ab200>] __fput+0x260/0x420
 [<a0000001001ab400>] fput+0x40/0x60
 [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
 [<a00000010054f3f0>] pty_close+0x350/0x3a0
 [<a00000010053ddd0>] tty_release+0x550/0xbc0
 [<a0000001001ab200>] __fput+0x260/0x420
 [<a0000001001ab400>] fput+0x40/0x60
 [<a0000001001a4dc0>] filp_close+0x120/0x140
 [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
 [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20

-Tony

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Arnd Bergmann]

On Thursday 17 June 2010 22:15:32 Tony Luck wrote:
> Call Trace:
>  [<a000000100015990>] show_stack+0x50/0xa0
>  [<a00000010090f1f0>] dump_stack+0x30/0x50
>  [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
>  [<a00000010008e320>] warn_slowpath_null+0x40/0x60
>  [<a00000010053d910>] tty_release+0x90/0xbc0
>  [<a0000001001ab200>] __fput+0x260/0x420
>  [<a0000001001ab400>] fput+0x40/0x60
>  [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
>  [<a00000010054f3f0>] pty_close+0x350/0x3a0
>  [<a00000010053ddd0>] tty_release+0x550/0xbc0
>  [<a0000001001ab200>] __fput+0x260/0x420
>  [<a0000001001ab400>] fput+0x40/0x60
>  [<a0000001001a4dc0>] filp_close+0x120/0x140
>  [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
>  [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20
> 

Ah, this sucks. I think Alan actually tried to warn me of this problem and I
thought I had it right, but obviously I got it wrong in the end. I really
should have run into this during testing though, not sure why I didn't.

The good news is that the warning message is harmless for the normal
case where CONFIG_TTY_MUTEX remains disabled, it's only debugging code
to warn that there is a bug once the option gets turned on.

Unfortunately however the only fix I see is to push the BTM further down
into the hangup function, which makes the pty code slightly more complex
and which I'm sure is an equivalent transformation.

I'll try doing some more tests with this patch and CONFIG_TTY_LOCK disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index c9af9ff..0902127 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 		if (tty->driver == ptm_driver)
 			devpts_pty_kill(tty->link);
 #endif
-		tty_vhangup_locked(tty->link);
+		unlock_kernel();
+		tty_vhangup(tty->link);
+		lock_kernel();
 	}
 }
 
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 5db354d..852ccb4 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -471,7 +471,7 @@ void tty_wakeup(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
- *	do_tty_hangup		-	actual handler for hangup events
+ *	__tty_hangup		-	actual handler for hangup events
  *	@work: tty device
  *
  *	This can be called by the "eventd" kernel thread.  That is process
@@ -492,7 +492,7 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-void tty_vhangup_locked(struct tty_struct *tty)
+void __tty_hangup(struct tty_struct *tty)
 {
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
@@ -512,10 +512,12 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	}
 	spin_unlock(&redirect_lock);
 
+	tty_lock();
+
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
-	check_tty_count(tty, "do_tty_hangup");
+	check_tty_count(tty, "tty_hangup");
 
 	file_list_lock();
 	/* This breaks for file handles being sent over AF_UNIX sockets ? */
@@ -594,6 +596,9 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	tty_ldisc_enable(tty);
+
+	tty_unlock();
+
 	if (f)
 		fput(f);
 }
@@ -603,9 +608,7 @@ static void do_tty_hangup(struct work_struct *work)
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 /**
@@ -643,13 +646,12 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 EXPORT_SYMBOL(tty_vhangup);
 
+
 /**
  *	tty_vhangup_self	-	process vhangup for own ctty
  *
@@ -727,10 +729,8 @@ void disassociate_ctty(int on_exit)
 	if (tty) {
 		tty_pgrp = get_pid(tty->pgrp);
 		if (on_exit) {
-			tty_lock();
 			if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
-				tty_vhangup_locked(tty);
-			tty_unlock();
+				tty_vhangup(tty);
 		}
 		tty_kref_put(tty);
 	} else if (on_exit) {

Re: [PATCH v3 00/10] BKL conversion in tty layer

[Frederic Weisbecker]

On Thu, Jun 17, 2010 at 11:48:07PM +0200, Arnd Bergmann wrote:
> On Thursday 17 June 2010 22:15:32 Tony Luck wrote:
> > Call Trace:
> >  [<a000000100015990>] show_stack+0x50/0xa0
> >  [<a00000010090f1f0>] dump_stack+0x30/0x50
> >  [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
> >  [<a00000010008e320>] warn_slowpath_null+0x40/0x60
> >  [<a00000010053d910>] tty_release+0x90/0xbc0
> >  [<a0000001001ab200>] __fput+0x260/0x420
> >  [<a0000001001ab400>] fput+0x40/0x60
> >  [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
> >  [<a00000010054f3f0>] pty_close+0x350/0x3a0
> >  [<a00000010053ddd0>] tty_release+0x550/0xbc0
> >  [<a0000001001ab200>] __fput+0x260/0x420
> >  [<a0000001001ab400>] fput+0x40/0x60
> >  [<a0000001001a4dc0>] filp_close+0x120/0x140
> >  [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
> >  [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20
> > 
> 
> Ah, this sucks. I think Alan actually tried to warn me of this problem and I
> thought I had it right, but obviously I got it wrong in the end. I really
> should have run into this during testing though, not sure why I didn't.
> 
> The good news is that the warning message is harmless for the normal
> case where CONFIG_TTY_MUTEX remains disabled, it's only debugging code
> to warn that there is a bug once the option gets turned on.
> 
> Unfortunately however the only fix I see is to push the BTM further down
> into the hangup function, which makes the pty code slightly more complex
> and which I'm sure is an equivalent transformation.
> 
> I'll try doing some more tests with this patch and CONFIG_TTY_LOCK disabled.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/char/pty.c b/drivers/char/pty.c
> index c9af9ff..0902127 100644
> --- a/drivers/char/pty.c
> +++ b/drivers/char/pty.c
> @@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
>  		if (tty->driver == ptm_driver)
>  			devpts_pty_kill(tty->link);
>  #endif
> -		tty_vhangup_locked(tty->link);
> +		unlock_kernel();
> +		tty_vhangup(tty->link);
> +		lock_kernel();



Don't you mean tty_unlock / tty_lock there?

[PATCH] tty: avoid recursive BTM in pty_close

[Arnd Bergmann]

When the console has been redirected, a hangup of the tty
will cause tty_release to be called under the big tty_mutex,
which leads to a deadlock because hangup is also called
under the BTM.

This moves the BTM deeper into the tty_hangup function so
we can close the redirected tty without holding the BTM.
In case of pty, we now need to drop the BTM before
calling tty_vhangup.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/pty.c    |    4 +++-
 drivers/char/tty_io.c |   24 ++++++++++++------------
 2 files changed, 15 insertions(+), 13 deletions(-)

---
On Friday 18 June 2010, Frederic Weisbecker wrote:
> > -             tty_vhangup_locked(tty->link);
> > +             unlock_kernel();
> > +             tty_vhangup(tty->link);
> > +             lock_kernel();
> 
> 
> 
> Don't you mean tty_unlock / tty_lock there?

Yes, thanks for catching this.

I did test this patch yesterday night (it was late) with CONFIG_TTY_MUTEX
disabled, as in Tony's configuration, but I did not realize that I
broke the other configuration in turn.

I'd like to get Alan's opinion on whether he considers the change in
pty.c safe.

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index c9af9ff..acff19f 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 		if (tty->driver == ptm_driver)
 			devpts_pty_kill(tty->link);
 #endif
-		tty_vhangup_locked(tty->link);
+		tty_unlock();
+		tty_vhangup(tty->link);
+		tty_lock();
 	}
 }
 
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 5db354d..6bf6d36 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -471,7 +471,7 @@ void tty_wakeup(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
- *	do_tty_hangup		-	actual handler for hangup events
+ *	__tty_hangup		-	actual handler for hangup events
  *	@work: tty device
  *
  *	This can be called by the "eventd" kernel thread.  That is process
@@ -492,7 +492,7 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-void tty_vhangup_locked(struct tty_struct *tty)
+void __tty_hangup(struct tty_struct *tty)
 {
 	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
@@ -512,10 +512,12 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	}
 	spin_unlock(&redirect_lock);
 
+	tty_lock();
+
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
-	check_tty_count(tty, "do_tty_hangup");
+	check_tty_count(tty, "tty_hangup");
 
 	file_list_lock();
 	/* This breaks for file handles being sent over AF_UNIX sockets ? */
@@ -594,6 +596,9 @@ void tty_vhangup_locked(struct tty_struct *tty)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	tty_ldisc_enable(tty);
+
+	tty_unlock();
+
 	if (f)
 		fput(f);
 }
@@ -603,9 +608,7 @@ static void do_tty_hangup(struct work_struct *work)
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 /**
@@ -643,13 +646,12 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	tty_lock();
-	tty_vhangup_locked(tty);
-	tty_unlock();
+	__tty_hangup(tty);
 }
 
 EXPORT_SYMBOL(tty_vhangup);
 
+
 /**
  *	tty_vhangup_self	-	process vhangup for own ctty
  *
@@ -727,10 +729,8 @@ void disassociate_ctty(int on_exit)
 	if (tty) {
 		tty_pgrp = get_pid(tty->pgrp);
 		if (on_exit) {
-			tty_lock();
 			if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
-				tty_vhangup_locked(tty);
-			tty_unlock();
+				tty_vhangup(tty);
 		}
 		tty_kref_put(tty);
 	} else if (on_exit) {
-- 
1.7.0.4

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Alan Cox]

> I'd like to get Alan's opinion on whether he considers the change in
> pty.c safe.

I considered it a while ago when trying to work out removing the BKL from
this path. After much head banging and an overwhelming desire to go and
get drunk instead I concluded it wasn't possible to tell by analysis.

So I ack this patch - it's the only way to find out 8)

Alan

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Tony Luck]

On Fri, Jun 18, 2010 at 9:21 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> So I ack this patch - it's the only way to find out 8)

Here's the results from this patch on top of next-20100618

The "WARNING: at include/linux/tty.h:589 tty_open+0x9d0/0xc60()" messages
are back, two of them, both with same stack trace:


Call Trace:
 [<a0000001000159d0>] show_stack+0x50/0xa0
 [<a00000010090f270>] dump_stack+0x30/0x50
 [<a00000010008e2c0>] warn_slowpath_common+0xc0/0x120
 [<a00000010008e360>] warn_slowpath_null+0x40/0x60
 [<a00000010053eb40>] tty_open+0x160/0xc60
 [<a0000001001af9b0>] chrdev_open+0x310/0x360
 [<a0000001001a58b0>] __dentry_open+0x350/0x680
 [<a0000001001a5d80>] nameidata_to_filp+0x80/0xc0
 [<a0000001001bfee0>] finish_open+0x160/0x380
 [<a0000001001c0cc0>] do_last+0xbc0/0xce0
 [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
 [<a0000001001a5290>] do_sys_open+0x90/0x200
 [<a0000001001a54d0>] sys_open+0x50/0x80
 [<a000000100b907e0>] kernel_init+0x340/0x420
 [<a000000100013c10>] kernel_thread_helper+0x30/0x60
 [<a00000010000a0c0>] start_kernel_thread+0x20/0x40

The tty_release() ones have all gone though.

I've also just noticed that the serial console output gets garbled
(like it is at the wrong baud rate) when userland code starts
printing messages. Stays garbled until /sbin/agetty starts up
to print the "Welcome ... login:" banner.

Similar garbage when shutting down with "reboot" command.
I can see "INIT", then it all goes to pieces until I see the kernel
print "Restarting system".

-Tony

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Arnd Bergmann]

On Friday 18 June 2010, Tony Luck wrote:
> On Fri, Jun 18, 2010 at 9:21 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > So I ack this patch - it's the only way to find out 8)
> 
> Here's the results from this patch on top of next-20100618
> 
> The "WARNING: at include/linux/tty.h:589 tty_open+0x9d0/0xc60()" messages
> are back, two of them, both with same stack trace:
> 
> 
> Call Trace:
>  [<a0000001000159d0>] show_stack+0x50/0xa0
>  [<a00000010090f270>] dump_stack+0x30/0x50
>  [<a00000010008e2c0>] warn_slowpath_common+0xc0/0x120
>  [<a00000010008e360>] warn_slowpath_null+0x40/0x60
>  [<a00000010053eb40>] tty_open+0x160/0xc60
>  [<a0000001001af9b0>] chrdev_open+0x310/0x360
>  [<a0000001001a58b0>] __dentry_open+0x350/0x680
>  [<a0000001001a5d80>] nameidata_to_filp+0x80/0xc0
>  [<a0000001001bfee0>] finish_open+0x160/0x380
>  [<a0000001001c0cc0>] do_last+0xbc0/0xce0
>  [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
>  [<a0000001001a5290>] do_sys_open+0x90/0x200
>  [<a0000001001a54d0>] sys_open+0x50/0x80
>  [<a000000100b907e0>] kernel_init+0x340/0x420
>  [<a000000100013c10>] kernel_thread_helper+0x30/0x60
>  [<a00000010000a0c0>] start_kernel_thread+0x20/0x40

Ok, this is the same one you reported at first. I forgot to
mention that the other patch I sent as a reply to your report
is still needed and not yet in -next since I'm trying to
sort through the other BKL removal patches now.
This instance of the WARN_ON is completely harmless though,
you could consider this one a false positive.

> The tty_release() ones have all gone though.

ok, good.

> I've also just noticed that the serial console output gets garbled
> (like it is at the wrong baud rate) when userland code starts
> printing messages. Stays garbled until /sbin/agetty starts up
> to print the "Welcome ... login:" banner.
>
> Similar garbage when shutting down with "reboot" command.
> I can see "INIT", then it all goes to pieces until I see the kernel
> print "Restarting system".

Do you know if this was a problem with the original series or
something that got introduced by my last patch?
Also, if you don't mind, could you try if the problem also exists
with CONFIG_TTY_MUTEX enabled?
Which serial driver do you use?

I'll try to set up a serial console here to reproduce and bisect
this problem on my own system.

	Arnd

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Tony Luck]

On Fri, Jun 18, 2010 at 11:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> Ok, this is the same one you reported at first. I forgot to
> mention that the other patch I sent as a reply to your report
> is still needed and not yet in -next since I'm trying to
> sort through the other BKL removal patches now.
> This instance of the WARN_ON is completely harmless though,
> you could consider this one a false positive.

Ok. Adding your first patch into todays mix cleaned away
these messages,

> Do you know if this was a problem with the original series or
> something that got introduced by my last patch?

Problem was there before. Looks like it was not in next-20100616
but began with next-20100617.

> Also, if you don't mind, could you try if the problem also exists
> with CONFIG_TTY_MUTEX enabled?
With CONFIG_TTY_MUTEX=y kernel with both your patches builds
and boots with no unusual messages.  Application output still gets
garbled.

> Which serial driver do you use?
I boot with a "console=uart,io,0x3f8" argument, so I start out using
8250_early.c. Looks like it switched to 8250.c based on seeing
this:

Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled

on the console log.

-Tony

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Arnd Bergmann]

On Friday 18 June 2010 22:25:22 Tony Luck wrote:
> On Fri, Jun 18, 2010 at 11:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > Do you know if this was a problem with the original series or
> > something that got introduced by my last patch?
> 
> Problem was there before. Looks like it was not in next-20100616
> but began with next-20100617.

Ok, so it's certainly in one of the patches from Alan or me.

> > Also, if you don't mind, could you try if the problem also exists
> > with CONFIG_TTY_MUTEX enabled?
> With CONFIG_TTY_MUTEX=y kernel with both your patches builds
> and boots with no unusual messages.  Application output still gets
> garbled.

ok.
 
> > Which serial driver do you use?
> I boot with a "console=uart,io,0x3f8" argument, so I start out using
> 8250_early.c. Looks like it switched to 8250.c based on seeing
> this:
> 
> Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> 
> on the console log.

Good, so it's something I should be able to reproduce on a PC.

	Arnd

[PATCH] serial: revert "Use block_til_ready helper"

[Arnd Bergmann]

On Saturday 19 June 2010 14:32:04 Arnd Bergmann wrote:
> > I boot with a "console=uart,io,0x3f8" argument, so I start out using
> > 8250_early.c. Looks like it switched to 8250.c based on seeing
> > this:
> > 
> > Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> > 
> > on the console log.
> 
> Good, so it's something I should be able to reproduce on a PC.

I could reproduce the problem now and bisected it down to a cleanup
patch that can fortunately get reverted. The symptom that I saw
is that when I open a serial port for the second time (e.g. starting
getty and logging in), the output gets garbled, while the first one
worked fine. This does not rely on the console code at all.

Original commit message:
    Our code now rather closely resembles the helper, so switch to it.

Apparently, the two functions are not really doing the same,
so revert the commit for now. This includes the addition of
a tty_unlock()/tty_lock() pair when blocking on the wait queue.

Greg, do you want to apply this patch on top of the series, or
do you prefer to get a replacement for the original patch, once
we know what's wrong with it?

Tony, can you confirm that with this patch added, everything's
fine for you?

Alan, any idea where the problem may be with the broken patch?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/serial/serial_core.c |   89 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 88 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 78b1eac..55e5769 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -1539,6 +1539,93 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 }
 
+/*
+ * Block the open until the port is ready.  We must be called with
+ * the per-port semaphore held.
+ */
+static int
+uart_block_til_ready(struct file *filp, struct uart_state *state)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	struct tty_port *port = &state->port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	if (!tty_hung_up_p(filp))
+		port->count--;
+	port->blocked_open++;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	add_wait_queue(&port->open_wait, &wait);
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		/*
+		 * If we have been hung up, tell userspace/restart open.
+		 */
+		if (tty_hung_up_p(filp) || port->tty == NULL)
+			break;
+
+		/*
+		 * If the port has been closed, tell userspace/restart open.
+		 */
+		if (!(port->flags & ASYNC_INITIALIZED))
+			break;
+
+		/*
+		 * If non-blocking mode is set, or CLOCAL mode is set,
+		 * we don't want to wait for the modem status lines to
+		 * indicate that the port is ready.
+		 *
+		 * Also, if the port is not enabled/configured, we want
+		 * to allow the open to succeed here.  Note that we will
+		 * have set TTY_IO_ERROR for a non-existant port.
+		 */
+		if ((filp->f_flags & O_NONBLOCK) ||
+		    (port->tty->termios->c_cflag & CLOCAL) ||
+		    (port->tty->flags & (1 << TTY_IO_ERROR)))
+			break;
+
+		/*
+		 * Set DTR to allow modem to know we're waiting.  Do
+		 * not set RTS here - we want to make sure we catch
+		 * the data from the modem.
+		 */
+		if (port->tty->termios->c_cflag & CBAUD)
+			tty_port_raise_dtr_rts(port);
+
+		/*
+		 * and wait for the carrier to indicate that the
+		 * modem is ready for us.
+		 */
+		if (tty_port_carrier_raised(port))
+			break;
+
+		tty_unlock();
+		schedule();
+		tty_lock();
+
+		if (signal_pending(current))
+			break;
+	}
+	set_current_state(TASK_RUNNING);
+	remove_wait_queue(&port->open_wait, &wait);
+
+	spin_lock_irqsave(&port->lock, flags);
+	if (!tty_hung_up_p(filp))
+		port->count++;
+	port->blocked_open--;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	if (signal_pending(current))
+		return -ERESTARTSYS;
+
+	if (!port->tty || tty_hung_up_p(filp))
+		return -EAGAIN;
+
+	return 0;
+}
+
 static struct uart_state *uart_get(struct uart_driver *drv, int line)
 {
 	struct uart_state *state;
@@ -1647,7 +1734,7 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 	 */
 	mutex_unlock(&port->mutex);
 	if (retval == 0)
-		retval = tty_port_block_til_ready(port, tty, filp);
+		retval = uart_block_til_ready(filp, state);
 
 	/*
 	 * If this is the first open to succeed, adjust things to suit.
-- 
1.7.0.4

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Alan Cox]

O> I could reproduce the problem now and bisected it down to a cleanup
> patch that can fortunately get reverted. The symptom that I saw
> is that when I open a serial port for the second time (e.g. starting
> getty and logging in), the output gets garbled, while the first one
> worked fine. This does not rely on the console code at all.

So the init code isn't getting run somewhere for some reason. Almost
certainly a count bug.

> Alan, any idea where the problem may be with the broken patch?

I'll take a look. We really don't want to revert this as it fixes some
nasty things the BKL covers over so I'd be worried the races would become
exposed (and it possibly allows NULL->func() calls if it can which would
be bad)

First guess would be something in port->count going astray and the port
not getting re-initialised.

Important question - does it need a getty or will a simple cat < /dev/foo
twice get garbled (or cat > if its garbling the other way)

Which direction matters and whether a hangup is needed matters

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Arnd Bergmann]

On Saturday 19 June 2010 23:57:06 Alan Cox wrote:
> 
> Important question - does it need a getty or will a simple cat < /dev/foo
> twice get garbled (or cat > if its garbling the other way)
>
> Which direction matters and whether a hangup is needed matters

What I see is totally reproducible but does not make any sense to me.
In my test setup I have a serial cable between /dev/ttyS0 (on-board
16550A) and /dev/ttyUSB0 on the same machine.

When I start minicom on ttyS0 (or cat, for that matter), and start
a getty on ttyUSB0 (or write into it any other way), everything is
totally fine.

I can observe the following problems when I start minicom on ttyUSB0:
- writing to ttyS0 using cat alone gives garbled output
- writing to ttyS0 using minicom always works
- running a getty on ttyS0 lets me log in once
- writing to ttyS0 using cat works fine while a working getty or minicom
  also has ttyS0 open
- running the getty on ttyS0 a second time gives me the same garbled
  output as cat gives me

The way that the output is garbled seems to be just missing characters,
in a mostly reproducible way. Repeatedly writing the string
abcdefghijklmnopqrstuvwxyz1234567890 into ttyS0 gives an output like
'akq7m3iy', where only every sixteenth character is shown, plus
an extra character in the second position.

	Arnd

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Alan Cox]

> The way that the output is garbled seems to be just missing characters,
> in a mostly reproducible way. Repeatedly writing the string
> abcdefghijklmnopqrstuvwxyz1234567890 into ttyS0 gives an output like
> 'akq7m3iy', where only every sixteenth character is shown, plus
> an extra character in the second position.

Ok this sort of makes sense. Something isn't getting initialised and both
getty and minicom will do a termios set which is sorting it out.


And this is occurring because the generic block_til_ready sets
ASYNCB_NORMAL_ACTIVE so the termios updating gets skipped.

Ok the quickfix for this kernel is to delete this

       /*
         * If this is the first open to succeed, adjust things to suit.
         */
        if (retval == 0 && !(port->flags & ASYNC_NORMAL_ACTIVE)) {
                set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);

                uart_update_termios(state);
        }

and paste it into

void tty_port_raise_dtr_rts(struct tty_port *port)
{
        if (port->ops->dtr_rts)
                port->ops->dtr_rts(port, 1);

		-->> HERE <<--
}


(the mutex is held by the caller)

That should cure it and then we can think about doing it more elegantly
by getting the serial layer to use tty_port_open, kfifo and the like and
removing the tons of repeated crap in all the drivers.

Alan

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Arnd Bergmann]

On Monday 21 June 2010 16:13:09 Alan Cox wrote:

> and paste it into
> 
> void tty_port_raise_dtr_rts(struct tty_port *port)
> {
>         if (port->ops->dtr_rts)
>                 port->ops->dtr_rts(port, 1);
> 
> 		-->> HERE <<--
> }
> 
> 
> (the mutex is held by the caller)
> 
> That should cure it and then we can think about doing it more elegantly
> by getting the serial layer to use tty_port_open, kfifo and the like and
> removing the tons of repeated crap in all the drivers.

I had to do it slightly different, see below. This patch also fixes
the garbled output and lets me run cat> and getty on the onboard 16550a,
but unlike the previous patch, I can't get a minicom connection between
the two ports using hardware flow control now, so it seems that there
is still another problem.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 78b1eac..cd85112 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -1533,8 +1533,16 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
 	struct uart_state *state = container_of(port, struct uart_state, port);
 	struct uart_port *uport = state->uart_port;
 
-	if (onoff)
+	if (onoff) {
 		uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
+
+		/*
+		 * If this is the first open to succeed,
+		 * adjust things to suit.
+		 */
+		if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags))
+			uart_update_termios(port->tty, state);
+	}
 	else
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 }
@@ -1649,15 +1657,6 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 	if (retval == 0)
 		retval = tty_port_block_til_ready(port, tty, filp);
 
-	/*
-	 * If this is the first open to succeed, adjust things to suit.
-	 */
-	if (retval == 0 && !(port->flags & ASYNC_NORMAL_ACTIVE)) {
-		set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
-
-		uart_update_termios(tty, state);
-	}
-
 fail:
 	return retval;
 }

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Arnd Bergmann]

On Monday 21 June 2010 22:57:03 Arnd Bergmann wrote:
> I had to do it slightly different, see below. This patch also fixes
> the garbled output and lets me run cat> and getty on the onboard 16550a,
> but unlike the previous patch, I can't get a minicom connection between
> the two ports using hardware flow control now, so it seems that there
> is still another problem.

For clarification, the case that did not work was using minicom on both
ends. Disabling flow control works, enabling hardware flow control on
both ends fails but it worked fine with the patch I posted on Saturday.

	Arnd

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Tony Luck]

On Sat, Jun 19, 2010 at 1:29 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Tony, can you confirm that with this patch added, everything's
> fine for you?

Yes.  My user mode output now looks fine with that patch applied.

Thanks

-Tony

Re: [PATCH] serial: revert "Use block_til_ready helper"

[Greg KH]

On Sat, Jun 19, 2010 at 10:29:21PM +0200, Arnd Bergmann wrote:
> On Saturday 19 June 2010 14:32:04 Arnd Bergmann wrote:
> > > I boot with a "console=uart,io,0x3f8" argument, so I start out using
> > > 8250_early.c. Looks like it switched to 8250.c based on seeing
> > > this:
> > > 
> > > Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> > > 
> > > on the console log.
> > 
> > Good, so it's something I should be able to reproduce on a PC.
> 
> I could reproduce the problem now and bisected it down to a cleanup
> patch that can fortunately get reverted. The symptom that I saw
> is that when I open a serial port for the second time (e.g. starting
> getty and logging in), the output gets garbled, while the first one
> worked fine. This does not rely on the console code at all.
> 
> Original commit message:
>     Our code now rather closely resembles the helper, so switch to it.
> 
> Apparently, the two functions are not really doing the same,
> so revert the commit for now. This includes the addition of
> a tty_unlock()/tty_lock() pair when blocking on the wait queue.
> 
> Greg, do you want to apply this patch on top of the series, or
> do you prefer to get a replacement for the original patch, once
> we know what's wrong with it?

A replacement for the original one might be best if we can figure it
out.

Just let me know what you need me to do.

thanks,

greg k-h

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Tony Luck]

On Fri, Jun 18, 2010 at 11:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Call Trace:
>>  [<a0000001000159d0>] show_stack+0x50/0xa0
>>  [<a00000010090f270>] dump_stack+0x30/0x50
>>  [<a00000010008e2c0>] warn_slowpath_common+0xc0/0x120
>>  [<a00000010008e360>] warn_slowpath_null+0x40/0x60
>>  [<a00000010053eb40>] tty_open+0x160/0xc60
>>  [<a0000001001af9b0>] chrdev_open+0x310/0x360
>>  [<a0000001001a58b0>] __dentry_open+0x350/0x680
>>  [<a0000001001a5d80>] nameidata_to_filp+0x80/0xc0
>>  [<a0000001001bfee0>] finish_open+0x160/0x380
>>  [<a0000001001c0cc0>] do_last+0xbc0/0xce0
>>  [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
>>  [<a0000001001a5290>] do_sys_open+0x90/0x200
>>  [<a0000001001a54d0>] sys_open+0x50/0x80
>>  [<a000000100b907e0>] kernel_init+0x340/0x420
>>  [<a000000100013c10>] kernel_thread_helper+0x30/0x60
>>  [<a00000010000a0c0>] start_kernel_thread+0x20/0x40
>
> Ok, this is the same one you reported at first. I forgot to
> mention that the other patch I sent as a reply to your report
> is still needed and not yet in -next since I'm trying to
> sort through the other BKL removal patches now.
> This instance of the WARN_ON is completely harmless though,
> you could consider this one a false positive.

Quick status check. I'm still seeing these messages in next-20100628.
Are you still working on a fix?

-Tony

Re: [PATCH] tty: avoid recursive BTM in pty_close

[Arnd Bergmann]

On Monday 28 June 2010 19:17:39 Tony Luck wrote:
> On Fri, Jun 18, 2010 at 11:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> Call Trace:
> >>  [<a0000001000159d0>] show_stack+0x50/0xa0
> >>  [<a00000010090f270>] dump_stack+0x30/0x50
> >>  [<a00000010008e2c0>] warn_slowpath_common+0xc0/0x120
> >>  [<a00000010008e360>] warn_slowpath_null+0x40/0x60
> >>  [<a00000010053eb40>] tty_open+0x160/0xc60
> >>  [<a0000001001af9b0>] chrdev_open+0x310/0x360
> >>  [<a0000001001a58b0>] __dentry_open+0x350/0x680
> >>  [<a0000001001a5d80>] nameidata_to_filp+0x80/0xc0
> >>  [<a0000001001bfee0>] finish_open+0x160/0x380
> >>  [<a0000001001c0cc0>] do_last+0xbc0/0xce0
> >>  [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
> >>  [<a0000001001a5290>] do_sys_open+0x90/0x200
> >>  [<a0000001001a54d0>] sys_open+0x50/0x80
> >>  [<a000000100b907e0>] kernel_init+0x340/0x420
> >>  [<a000000100013c10>] kernel_thread_helper+0x30/0x60
> >>  [<a00000010000a0c0>] start_kernel_thread+0x20/0x40
> >
> > Ok, this is the same one you reported at first. I forgot to
> > mention that the other patch I sent as a reply to your report
> > is still needed and not yet in -next since I'm trying to
> > sort through the other BKL removal patches now.
> > This instance of the WARN_ON is completely harmless though,
> > you could consider this one a false positive.
> 
> Quick status check. I'm still seeing these messages in next-20100628.
> Are you still working on a fix?

I got distracted and it wasn't on my list of pressing issues because
this particular warning is harmless. I should get the base bkl series
into -next to fix this.

For the other issue (garbled output), I'm not sure how to proceed.
Alan didn't like the easy workaround of reverting the commit that
introduced it and the fix that Alan suggested only solved the
problem partially as far as I can tell.

	Arnd