Skip to contentSkip to conversation

BKL conversion in tty layer

PATCH In review Arnd Bergmann 6 participants 43 messages May 15, 2010 – Jun 28, 2010

Subsystems (8)
lore
Share & tools
lore

Arnd Bergmann

author maintainer
#0 lore raw

v3 cover BKL conversion in tty layer

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
author maintainer
#1 lore raw

PATCH v3 1/10 tty: replace BKL with a new tty_lock

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.

---
 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(-)
drivers/char/amiserial.c +8 -8
index 56b2767..5bd382e 100644
@@ -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
drivers/char/briq_panel.c +3 -3
index 555cd93..d5fa113 100644
@@ -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;
 }
 
drivers/char/n_hdlc.c +8 -8
index c68118e..47d3228 100644
@@ -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() */
drivers/char/n_r3964.c +4 -4
index c1d8b54..f4bd259 100644
@@ -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;
 }
drivers/char/pty.c +2 -2
index d83a431..384e79f 100644
@@ -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;
 }
 
drivers/char/selection.c +2 -2
index 6e79340..85211a3 100644
@@ -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;
 }
drivers/char/serial167.c +2 -2
index 8dfd247..26a7089 100644
@@ -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");
drivers/char/sx.c +6 -6
index a81ec4f..5b24db4 100644
@@ -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;
drivers/char/tty_io.c +63 -50

357 lines not on this page · raw message

drivers/char/tty_ldisc.c +13 -11
index 500e740..97681ff 100644
@@ -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;
 }
 
drivers/char/vc_screen.c +2 -2
index c1791a6..bcce46c 100644
@@ -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;
 }
 
drivers/char/vt_ioctl.c +5 -5
index 625f77d..cc07fab 100644
@@ -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);
 }
 
drivers/serial/68360serial.c +2 -2
index 24661cd..c17a595 100644
@@ -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
drivers/serial/crisv10.c +2 -2
index 31f1723..d11d898 100644
@@ -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();
 }
 
 /*
drivers/serial/serial_core.c +5 -5
index a55751a..4b151a4 100644
@@ -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);
 
 	/*
drivers/video/console/vgacon.c +2 -2
index 182dd6f..7197005 100644
@@ -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;
 }
 
include/linux/tty.h +28 -0
index 4409967..c35ad81 100644
@@ -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
Signature · 1 line
1.7.0.4
author maintainer
#2 lore raw

PATCH v3 2/10 tty: never hold BTM while getting tty_mutex

In reply to 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.

---
 drivers/char/pty.c    |   22 ++++++++--------------
 drivers/char/tty_io.c |   13 +++++++------
 2 files changed, 15 insertions(+), 20 deletions(-)
drivers/char/pty.c +8 -14
index 384e79f..b441ebd 100644
@@ -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;
drivers/char/tty_io.c +7 -6
index 3bf2c75..d51993b 100644
@@ -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();
Signature · 1 line
1.7.0.4
author maintainer
#3 lore raw

PATCH v3 3/10 tty: fix console_sem lock order

In reply to 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.

---
 drivers/video/console/vgacon.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/video/console/vgacon.c +0 -2
index 7197005..54e32c5 100644
@@ -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;
 }
 
Signature · 1 line
1.7.0.4
author maintainer
#4 lore raw

PATCH v3 4/10 cdc-acm: remove dead code

In reply to 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.

---
 drivers/usb/class/cdc-acm.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)
drivers/usb/class/cdc-acm.c +3 -9
index 5e1a253..7b50bf5 100644
@@ -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);
 }
Signature · 1 line
1.7.0.4
author maintainer
#5 lore raw

PATCH v3 5/10 tty: introduce wait_event_interruptible_tty

In reply to 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.

---
 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(-)
drivers/char/cyclades.c +1 -1
index 51acfe3..27aad94 100644
@@ -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;
 	}
drivers/char/istallion.c +8 -4
index 5e9a81d..be28391 100644
@@ -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))
drivers/char/n_r3964.c +1 -1
index f4bd259..a98290d 100644
@@ -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)));
 		}
 
drivers/char/tty_port.c +1 -1
index a3bd1d0..35eb304 100644
@@ -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;
drivers/char/vt_ioctl.c +1 -1
index cc07fab..c56c4f3 100644
@@ -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);
drivers/serial/crisv10.c +2 -2
index d11d898..e6a1cb7 100644
@@ -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) ?
include/linux/tty.h +42 -0

47 lines not on this page · raw message

Signature · 1 line
1.7.0.4
author maintainer
#6 lore raw

PATCH v3 6/10 tty: annotate tty_write_lock

In reply to Arnd Bergmann

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

---
 drivers/char/tty_io.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/char/tty_io.c +5 -0
index d51993b..36aecba 100644
@@ -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;
Signature · 1 line
1.7.0.4
author maintainer
#7 lore raw

PATCH v3 7/10 tty: reorder ldisc locking

In reply to 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.

---
 drivers/char/selection.c |    9 +++++++--
 drivers/char/tty_ldisc.c |   24 ++++++++++++++++++++----
 2 files changed, 27 insertions(+), 6 deletions(-)
drivers/char/selection.c +7 -2
index 85211a3..75889cd 100644
@@ -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);
drivers/char/tty_ldisc.c +20 -4
index 97681ff..0f49479 100644
@@ -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);
 	/*
Signature · 1 line
1.7.0.4
author maintainer
#8 lore raw

PATCH v3 8/10 tty: untangle locking of wait_until_sent

In reply to 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.

---
 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(-)
drivers/char/amiserial.c +9 -2
index 5bd382e..3c0231a 100644
@@ -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
drivers/serial/68360serial.c +0 -2
index c17a595..5b7b801 100644
@@ -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
drivers/serial/crisv10.c +0 -2
index e6a1cb7..0825d4a 100644
@@ -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();
 }
 
 /*
drivers/serial/serial_core.c +22 -9
index 4b151a4..78b1eac 100644
@@ -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();
 }
 
Signature · 1 line
1.7.0.4
#9 lore raw

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

Quoted 13 lines · Arnd Bergmann
drivers/char/amiserial.c +1 -0
index 5bd382e..3c0231a 100644
@@ -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"))

drivers/char/amiserial.c static void rs_wait_until_sent(struct tty_struct *tty, int timeout) · line 1587

Quoted 5 lines
drivers/char/amiserial.c +1 -1
@@ -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.

author maintainer
#10 lore raw

PATCH v3 9/10 tty: remove tty_lock_nested

In reply to 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.

---
 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(-)
drivers/char/pty.c +1 -1
index b441ebd..89c13eb 100644
@@ -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);
 	}
 }
 
drivers/char/selection.c +2 -2
index 75889cd..ebae344 100644
@@ -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;
 }
drivers/char/stallion.c +2 -0
index f2167f8..feed311 100644
@@ -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);
drivers/char/tty_io.c +20 -21

108 lines not on this page · raw message

drivers/char/tty_ldisc.c +1 -2
index 0f49479..412f977 100644
@@ -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;
include/linux/tty.h +1 -15
index 9d4c87f..b227616 100644
@@ -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());
Signature · 1 line
1.7.0.4
author maintainer
#11 lore raw

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

In reply to 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.

---
 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
drivers/char/Makefile +1 -0
index f957edf..74ee3fa 100644
@@ -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
drivers/char/tty_mutex.c new file +47 -0

47 lines not on this page · raw message

include/linux/tty.h +8 -0
index b227616..d8d90f6 100644
@@ -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
lib/Kconfig.debug +10 -0
index 935248b..0b3e1ad 100644
@@ -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
Signature · 1 line
1.7.0.4
#12 lore raw

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

Quoted 7 lines · Arnd Bergmann
drivers/char/tty_mutex.c new file +1 -0
new file mode 100644
index 0000000..f66dfdf
@@ -0,0 +1,47 @@
+/*

drivers/char/tty_mutex.c line 11

Quoted 10 lines
drivers/char/tty_mutex.c new file +10 -0
+ * 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.

drivers/char/tty_mutex.c line 14

Quoted 2 lines
drivers/char/tty_mutex.c new file +2 -0
+ * This mutex is taken and released by tty_lock() and tty_unlock(),
+ * replacing the older big kernel mutex.

big kernel lock, or BKL?

Quoted 9 lines
drivers/char/tty_mutex.c new file +8 -0
+ * 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);
 

drivers/char/tty_mutex.c

Quoted 10 lines
drivers/char/tty_mutex.c new file +10 -0
+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.

author maintainer
#13 lore raw

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

Quoted 22 lines · On Sunday 16 May 2010 05:33:14 Daniel K. wrote:
Quoted 7 lines · Arnd Bergmann
drivers/char/tty_mutex.c new file +1 -0
new file mode 100644
index 0000000..f66dfdf
@@ -0,0 +1,47 @@
+/*

drivers/char/tty_mutex.c line 11

Quoted 10 lines
drivers/char/tty_mutex.c new file +10 -0
+ * 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.

Quoted 5 lines

drivers/char/tty_mutex.c line 14

Quoted 2 lines
drivers/char/tty_mutex.c new file +2 -0
+ * 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

Acked-by on the v3 series
#14 lore raw
In reply to Arnd Bergmann
Quoted 4 lines · On Sat, 15 May 2010 22:59:46 +0200 Arnd Bergmann <[email protected]> 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

Alan

#15 lore raw
Quoted 14 lines · Alan Cox
Quoted 4 lines · On Sat, 15 May 2010 22:59:46 +0200 Arnd Bergmann <[email protected]> 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.

Quoted 7 lines

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

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

author maintainer
#16 lore raw
Quoted 5 lines · Greg KH

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

#17 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · Greg KH

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?

Quoted 5 lines

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 :)

Quoted 5 lines

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

author maintainer
#18 lore raw
Quoted 10 lines · Greg KH
Quoted 5 lines · Arnd Bergmann

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 :)

Quoted 7 lines
Quoted 5 lines

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

#19 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 2 lines

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?

Quoted 6 lines

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

author maintainer
#20 lore raw
Quoted 10 lines · Greg KH
Quoted 7 lines · Arnd Bergmann
Quoted 2 lines

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

Quoted 4 lines
Quoted 4 lines

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

Quoted 8 lines
Quoted 2 lines

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

author maintainer
#21 lore raw
In reply to Alan Cox
Quoted 5 lines · Alan Cox

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

#22 lore raw
In reply to Alan Cox
Quoted 5 lines · Alan Cox

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

author maintainer
#23 lore raw
Quoted 15 lines · 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
Quoted 2 lines

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

--

Quoted 1 line

From 99b699e56b23775c0c9a131208e1a5e13f6cfad3 Mon Sep 17 00:00:00 2001

From: Arnd Bergmann <[email protected]>
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.

init/main.c +0 -5
index 3bdb152..81821e1 100644
@@ -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
 	 */
kernel/trace/trace.c +0 -8
index 086d363..8047ca5 100644
@@ -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;
 }
 
#24 lore raw
Quoted 2 lines

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

author maintainer
#25 lore raw
Quoted 8 lines · 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
Quoted 9 lines
 [<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.

drivers/char/pty.c +3 -1
index c9af9ff..0902127 100644
@@ -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();
 	}
 }
 
drivers/char/tty_io.c +12 -12
index 5db354d..852ccb4 100644
@@ -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) {
#26 lore raw
Quoted 40 lines · Arnd Bergmann
Quoted 8 lines · 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
Quoted 9 lines
 [<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.

drivers/char/pty.c +0 -0
index c9af9ff..0902127 100644

drivers/char/pty.c static void pty_close(struct tty_struct *tty, struct file *filp) · line 67

Quoted 7 lines
drivers/char/pty.c +3 -1
@@ -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?

author maintainer
#27 lore raw

PATCH 1/1 tty: avoid recursive BTM in pty_close

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.

---
 drivers/char/pty.c    |    4 +++-
 drivers/char/tty_io.c |   24 ++++++++++++------------
 2 files changed, 15 insertions(+), 13 deletions(-)

---

Quoted 8 lines · Frederic Weisbecker

Review

Quoted 4 lines
(quoted diff) +3 -1
-             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.

drivers/char/pty.c +3 -1
index c9af9ff..acff19f 100644
@@ -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();
 	}
 }
 
drivers/char/tty_io.c +12 -12
index 5db354d..6bf6d36 100644
@@ -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) {
Signature · 1 line
1.7.0.4
#28 lore raw

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

Quoted 2 lines

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

#29 lore raw

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

Quoted 1 line · Alan Cox

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

author maintainer
#30 lore raw

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

Quoted 18 lines · Tony Luck
Quoted 1 line · Alan Cox

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
Quoted 8 lines
 [<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.

Quoted 1 line

The tty_release() ones have all gone though.

ok, good.

Quoted 8 lines

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

#31 lore raw

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

Quoted 6 lines · Arnd Bergmann

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,

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

author maintainer
#32 lore raw

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

Quoted 6 lines · On Friday 18 June 2010 22:25:22 Tony Luck wrote:
Quoted 2 lines · Arnd Bergmann

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.

Quoted 5 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 1 line

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

author maintainer
#33 lore raw

PATCH 1/1 serial: revert "Use block_til_ready helper"

Quoted 9 lines · On Saturday 19 June 2010 14:32:04 Arnd Bergmann wrote:
Quoted 7 lines

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?

---
 drivers/serial/serial_core.c |   89 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 88 insertions(+), 1 deletions(-)
drivers/serial/serial_core.c +88 -1
index 78b1eac..55e5769 100644
@@ -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.
Signature · 1 line
1.7.0.4
#34 lore raw

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

O> I could reproduce the problem now and bisected it down to a cleanup

Quoted 4 lines

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.

Quoted 1 line

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

author maintainer
#35 lore raw

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

Quoted 5 lines · 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

#36 lore raw

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

Quoted 5 lines

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

author maintainer
#37 lore raw

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

Quoted 11 lines · 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 <<--
}
Quoted 5 lines

(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.

drivers/serial/serial_core.c +9 -10
index 78b1eac..cd85112 100644
@@ -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;
 }
author maintainer
#38 lore raw

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

Quoted 5 lines · 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

#39 lore raw

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

In reply to Arnd Bergmann
Quoted 2 lines · Arnd Bergmann

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

#40 lore raw

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

In reply to Arnd Bergmann
Quoted 20 lines · Arnd Bergmann
Quoted 9 lines · On Saturday 19 June 2010 14:32:04 Arnd Bergmann wrote:
Quoted 7 lines

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.

Quoted 7 lines

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

#41 lore raw

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

In reply to Arnd Bergmann
Quoted 18 lines · Arnd Bergmann
Quoted 9 lines
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
Quoted 8 lines
 [<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
Quoted 6 lines

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

author maintainer
#42 lore raw

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

Quoted 26 lines · On Monday 28 June 2010 19:17:39 Tony Luck wrote:
Quoted 18 lines · Arnd Bergmann
Quoted 9 lines
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
Quoted 8 lines
 [<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
Quoted 6 lines

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.

Quoted 2 lines

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