[PATCH 0/6] Remove tty_lock from the console drivers

[PATCH 0/6] Remove tty_lock from the console drivers

[Alan Cox]

This needs a fair bit of testing and a bit of review wouldn't go amiss, but
it does drive the tty_lock() mess out of the console and correct chunks of
console locking in the process.

---

Alan Cox (6):
      vt: push the tty_lock down into the map handling
      vt: tackle the main part of the selection logic
      vt: waitevent is self locked so drop the tty_lock
      vt: push down tioclinux cases
      vt: push down the tty lock so we can see what is left to tackle
      vt: sort out locking for font handling


 drivers/tty/vt/consolemap.c |  119 ++++++++++++++++++++++++++++++++-----------
 drivers/tty/vt/selection.c  |   49 +++++++++++++-----
 drivers/tty/vt/vc_screen.c  |    4 +
 drivers/tty/vt/vt.c         |   37 ++++++++++---
 drivers/tty/vt/vt_ioctl.c   |   68 ++++++++++++-------------
 include/linux/vt_kern.h     |    1 
 6 files changed, 186 insertions(+), 92 deletions(-)

-- 
	"It's 106 miles to Chicago, we've got a full tank of gas, the
	 sat-nav has crashed, it's dark and we're completely lost"

[PATCH 1/6] vt: sort out locking for font handling

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

The font methods are console_lock covered. Unfortunately they don't extend
the lock over all the needed tests.

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

 drivers/tty/vt/vt.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)


diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 8205578..8439303 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3972,9 +3972,6 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
 	int rc = -EINVAL;
 	int c;
 
-	if (vc->vc_mode != KD_TEXT)
-		return -EINVAL;
-
 	if (op->data) {
 		font.data = kmalloc(max_font_size, GFP_KERNEL);
 		if (!font.data)
@@ -3983,7 +3980,9 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
 		font.data = NULL;
 
 	console_lock();
-	if (vc->vc_sw->con_font_get)
+	if (vc->vc_mode != KD_TEXT)
+		rc = -EINVAL;
+	else if (vc->vc_sw->con_font_get)
 		rc = vc->vc_sw->con_font_get(vc, &font);
 	else
 		rc = -ENOSYS;
@@ -4065,7 +4064,9 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
 	if (IS_ERR(font.data))
 		return PTR_ERR(font.data);
 	console_lock();
-	if (vc->vc_sw->con_font_set)
+	if (vc->vc_mode != KD_TEXT)
+		rc = -EINVAL;
+	else if (vc->vc_sw->con_font_set)
 		rc = vc->vc_sw->con_font_set(vc, &font, op->flags);
 	else
 		rc = -ENOSYS;
@@ -4081,8 +4082,6 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
 	char *s = name;
 	int rc;
 
-	if (vc->vc_mode != KD_TEXT)
-		return -EINVAL;
 
 	if (!op->data)
 		s = NULL;
@@ -4092,6 +4091,10 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
 		name[MAX_FONT_NAME - 1] = 0;
 
 	console_lock();
+	if (vc->vc_mode != KD_TEXT) {
+		console_unlock();
+		return -EINVAL;
+	}
 	if (vc->vc_sw->con_font_default)
 		rc = vc->vc_sw->con_font_default(vc, &font, s);
 	else
@@ -4109,11 +4112,11 @@ static int con_font_copy(struct vc_data *vc, struct console_font_op *op)
 	int con = op->height;
 	int rc;
 
-	if (vc->vc_mode != KD_TEXT)
-		return -EINVAL;
 
 	console_lock();
-	if (!vc->vc_sw->con_font_copy)
+	if (vc->vc_mode != KD_TEXT)
+		rc = -EINVAL;
+	else if (!vc->vc_sw->con_font_copy)
 		rc = -ENOSYS;
 	else if (con < 0 || !vc_cons_allocated(con))
 		rc = -ENOTTY;

[PATCH 2/6] vt: push down the tty lock so we can see what is left to tackle

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

At this point we have the tty_lock guarding a couple of oddities, plus the
translation and unimap still.

We also extend the console_lock in a couple of spots where coverage is wrong
and switch vcs_open to use the right lock !

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

 drivers/tty/vt/vc_screen.c |    4 +-
 drivers/tty/vt/vt_ioctl.c  |   87 +++++++++++++++++++++++++++-----------------
 2 files changed, 55 insertions(+), 36 deletions(-)


diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 7a367ff..fa7268a 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -608,10 +608,10 @@ vcs_open(struct inode *inode, struct file *filp)
 	unsigned int currcons = iminor(inode) & 127;
 	int ret = 0;
 	
-	tty_lock();
+	console_lock();
 	if(currcons && !vc_cons_allocated(currcons-1))
 		ret = -ENXIO;
-	tty_unlock();
+	console_unlock();
 	return ret;
 }
 
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 28ca0aa..16ad235 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -281,7 +281,6 @@ int vt_ioctl(struct tty_struct *tty,
 
 	console = vc->vc_num;
 
-	tty_lock();
 
 	if (!vc_cons_allocated(console)) { 	/* impossible? */
 		ret = -ENOIOCTLCMD;
@@ -299,16 +298,18 @@ int vt_ioctl(struct tty_struct *tty,
  
 	switch (cmd) {
 	case TIOCLINUX:
+		tty_lock();
 		ret = tioclinux(tty, arg);
+		tty_unlock();
 		break;
 	case KIOCSOUND:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		/*
 		 * The use of PIT_TICK_RATE is historic, it used to be
 		 * the platform-dependent CLOCK_TICK_RATE between 2.6.12
 		 * and 2.6.36, which was a minor but unfortunate ABI
-		 * change.
+		 * change. kd_mksound is locked by the input layer.
 		 */
 		if (arg)
 			arg = PIT_TICK_RATE / arg;
@@ -317,7 +318,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 	case KDMKTONE:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 	{
 		unsigned int ticks, count;
 		
@@ -335,7 +336,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 	case KDGKBTYPE:
 		/*
-		 * this is naive.
+		 * this is naïve.
 		 */
 		ucval = KB_101;
 		ret = put_user(ucval, (char __user *)arg);
@@ -353,6 +354,8 @@ int vt_ioctl(struct tty_struct *tty,
 		/*
 		 * KDADDIO and KDDELIO may be able to add ports beyond what
 		 * we reject here, but to be safe...
+		 *
+		 * These are locked internally via sys_ioperm
 		 */
 		if (arg < GPFIRST || arg > GPLAST) {
 			ret = -EINVAL;
@@ -375,7 +378,7 @@ int vt_ioctl(struct tty_struct *tty,
 		struct kbd_repeat kbrep;
 		
 		if (!capable(CAP_SYS_TTY_CONFIG))
-			goto eperm;
+			return -EPERM;
 
 		if (copy_from_user(&kbrep, up, sizeof(struct kbd_repeat))) {
 			ret =  -EFAULT;
@@ -399,7 +402,7 @@ int vt_ioctl(struct tty_struct *tty,
 		 * need to restore their engine state. --BenH
 		 */
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		switch (arg) {
 		case KD_GRAPHICS:
 			break;
@@ -412,6 +415,7 @@ int vt_ioctl(struct tty_struct *tty,
 			ret = -EINVAL;
 			goto out;
 		}
+		/* FIXME: this needs the console lock extending */
 		if (vc->vc_mode == (unsigned char) arg)
 			break;
 		vc->vc_mode = (unsigned char) arg;
@@ -443,7 +447,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 	case KDSKBMODE:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		ret = vt_do_kdskbmode(console, arg);
 		if (ret == 0)
 			tty_ldisc_flush(tty);
@@ -512,7 +516,7 @@ int vt_ioctl(struct tty_struct *tty,
 	case KDSIGACCEPT:
 	{
 		if (!perm || !capable(CAP_KILL))
-			goto eperm;
+			return -EPERM;
 		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
 			ret = -EINVAL;
 		else {
@@ -530,7 +534,7 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_mode tmp;
 
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		if (copy_from_user(&tmp, up, sizeof(struct vt_mode))) {
 			ret = -EFAULT;
 			goto out;
@@ -576,6 +580,7 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_stat __user *vtstat = up;
 		unsigned short state, mask;
 
+		/* Review: FIXME: Console lock ? */
 		if (put_user(fg_console + 1, &vtstat->v_active))
 			ret = -EFAULT;
 		else {
@@ -593,6 +598,7 @@ int vt_ioctl(struct tty_struct *tty,
 	 * Returns the first available (non-opened) console.
 	 */
 	case VT_OPENQRY:
+		/* FIXME: locking ? - but then this is a stupid API */
 		for (i = 0; i < MAX_NR_CONSOLES; ++i)
 			if (! VT_IS_IN_USE(i))
 				break;
@@ -606,7 +612,7 @@ int vt_ioctl(struct tty_struct *tty,
 	 */
 	case VT_ACTIVATE:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		if (arg == 0 || arg > MAX_NR_CONSOLES)
 			ret =  -ENXIO;
 		else {
@@ -625,7 +631,7 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_setactivate vsa;
 
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 
 		if (copy_from_user(&vsa, (struct vt_setactivate __user *)arg,
 					sizeof(struct vt_setactivate))) {
@@ -653,6 +659,7 @@ int vt_ioctl(struct tty_struct *tty,
 			if (ret)
 				break;
 			/* Commence switch and lock */
+			/* Review set_console locks */
 			set_console(vsa.console);
 		}
 		break;
@@ -663,7 +670,7 @@ int vt_ioctl(struct tty_struct *tty,
 	 */
 	case VT_WAITACTIVE:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		if (arg == 0 || arg > MAX_NR_CONSOLES)
 			ret = -ENXIO;
 		else
@@ -682,16 +689,17 @@ int vt_ioctl(struct tty_struct *tty,
 	 */
 	case VT_RELDISP:
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 
+		console_lock();
 		if (vc->vt_mode.mode != VT_PROCESS) {
+			console_unlock();
 			ret = -EINVAL;
 			break;
 		}
 		/*
 		 * Switching-from response
 		 */
-		console_lock();
 		if (vc->vt_newvt >= 0) {
 			if (arg == 0)
 				/*
@@ -768,7 +776,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 		ushort ll,cc;
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		if (get_user(ll, &vtsizes->v_rows) ||
 		    get_user(cc, &vtsizes->v_cols))
 			ret = -EFAULT;
@@ -779,6 +787,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 				if (vc) {
 					vc->vc_resize_user = 1;
+					/* FIXME: review v tty lock */
 					vc_resize(vc_cons[i].d, cc, ll);
 				}
 			}
@@ -792,7 +801,7 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_consize __user *vtconsize = up;
 		ushort ll,cc,vlin,clin,vcol,ccol;
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		if (!access_ok(VERIFY_READ, vtconsize,
 				sizeof(struct vt_consize))) {
 			ret = -EFAULT;
@@ -848,7 +857,7 @@ int vt_ioctl(struct tty_struct *tty,
 
 	case PIO_FONT: {
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		op.op = KD_FONT_OP_SET;
 		op.flags = KD_FONT_FLAG_OLD | KD_FONT_FLAG_DONT_RECALC;	/* Compatibility */
 		op.width = 8;
@@ -889,7 +898,7 @@ int vt_ioctl(struct tty_struct *tty,
 	case PIO_FONTRESET:
 	{
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 
 #ifdef BROKEN_GRAPHICS_PROGRAMS
 		/* With BROKEN_GRAPHICS_PROGRAMS defined, the default
@@ -915,7 +924,7 @@ int vt_ioctl(struct tty_struct *tty,
 			break;
 		}
 		if (!perm && op.op != KD_FONT_OP_GET)
-			goto eperm;
+			return -EPERM;
 		ret = con_font_op(vc, &op);
 		if (ret)
 			break;
@@ -927,50 +936,65 @@ int vt_ioctl(struct tty_struct *tty,
 	case PIO_SCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else
+		else {
+			tty_lock();
 			ret = con_set_trans_old(up);
+			tty_unlock();
+		}
 		break;
 
 	case GIO_SCRNMAP:
+		tty_lock();
 		ret = con_get_trans_old(up);
+		tty_unlock();
 		break;
 
 	case PIO_UNISCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else
+		else {
+			tty_lock();
 			ret = con_set_trans_new(up);
+			tty_unlock();
+		}
 		break;
 
 	case GIO_UNISCRNMAP:
+		tty_lock();
 		ret = con_get_trans_new(up);
+		tty_unlock();
 		break;
 
 	case PIO_UNIMAPCLR:
 	      { struct unimapinit ui;
 		if (!perm)
-			goto eperm;
+			return -EPERM;
 		ret = copy_from_user(&ui, up, sizeof(struct unimapinit));
 		if (ret)
 			ret = -EFAULT;
-		else
+		else {
+			tty_lock();
 			con_clear_unimap(vc, &ui);
+			tty_unlock();
+		}
 		break;
 	      }
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
+		tty_lock();
 		ret = do_unimap_ioctl(cmd, up, perm, vc);
+		tty_unlock();
 		break;
 
 	case VT_LOCKSWITCH:
 		if (!capable(CAP_SYS_TTY_CONFIG))
-			goto eperm;
+			return -EPERM;
 		vt_dont_switch = 1;
 		break;
 	case VT_UNLOCKSWITCH:
 		if (!capable(CAP_SYS_TTY_CONFIG))
-			goto eperm;
+			return -EPERM;
 		vt_dont_switch = 0;
 		break;
 	case VT_GETHIFONTMASK:
@@ -984,11 +1008,7 @@ int vt_ioctl(struct tty_struct *tty,
 		ret = -ENOIOCTLCMD;
 	}
 out:
-	tty_unlock();
 	return ret;
-eperm:
-	ret = -EPERM;
-	goto out;
 }
 
 void reset_vc(struct vc_data *vc)
@@ -1150,8 +1170,6 @@ long vt_compat_ioctl(struct tty_struct *tty,
 
 	console = vc->vc_num;
 
-	tty_lock();
-
 	if (!vc_cons_allocated(console)) { 	/* impossible? */
 		ret = -ENOIOCTLCMD;
 		goto out;
@@ -1180,7 +1198,9 @@ long vt_compat_ioctl(struct tty_struct *tty,
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
+		tty_lock();
 		ret = compat_unimap_ioctl(cmd, up, perm, vc);
+		tty_unlock();
 		break;
 
 	/*
@@ -1217,11 +1237,9 @@ long vt_compat_ioctl(struct tty_struct *tty,
 		goto fallback;
 	}
 out:
-	tty_unlock();
 	return ret;
 
 fallback:
-	tty_unlock();
 	return vt_ioctl(tty, cmd, arg);
 }
 
@@ -1407,6 +1425,7 @@ int vt_move_to_console(unsigned int vt, int alloc)
 		return -EIO;
 	}
 	console_unlock();
+	/* Review: I don't see why we need tty_lock here FIXME */
 	tty_lock();
 	if (vt_waitactive(vt + 1)) {
 		pr_debug("Suspend: Can't switch VCs.");

[PATCH 3/6] vt: push down tioclinux cases

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Some of this ventures into selection which is still a complete lost cause. We
are not making it any worse. It's completely busted anyway.

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

 drivers/tty/vt/selection.c |   12 ++++++------
 drivers/tty/vt/vt.c        |   12 ++++++++++++
 drivers/tty/vt/vt_ioctl.c  |    2 --
 3 files changed, 18 insertions(+), 8 deletions(-)


diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 738e45a..2a50916 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -75,7 +75,7 @@ clear_selection(void) {
 
 /*
  * User settable table: what characters are to be considered alphabetic?
- * 256 bits
+ * 256 bits. FIXME: Needs a locking model.
  */
 static u32 inwordLut[8]={
   0x00000000, /* control chars     */
@@ -307,7 +307,8 @@ int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *t
  * queue of the tty associated with the current console.
  * Invoked by ioctl().
  *
- * Locking: always called with BTM from vt_ioctl
+ * Locking: called without locks. Calls the ldisc wrongly with
+ * unsafe methods,
  */
 int paste_selection(struct tty_struct *tty)
 {
@@ -322,13 +323,12 @@ int paste_selection(struct tty_struct *tty)
 	poke_blanked_console();
 	console_unlock();
 
+	/* FIXME: wtf is this supposed to achieve ? */
 	ld = tty_ldisc_ref(tty);
-	if (!ld) {
-		tty_unlock();
+	if (!ld)
 		ld = tty_ldisc_ref_wait(tty);
-		tty_lock();
-	}
 
+	/* FIXME: this is completely unsafe */
 	add_wait_queue(&vc->paste_wait, &wait);
 	while (sel_buffer && sel_buffer_lth > pasted) {
 		set_current_state(TASK_INTERRUPTIBLE);
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 8439303..280a4c4 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2637,11 +2637,15 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 			ret = __put_user(data, p);
 			break;
 		case TIOCL_GETMOUSEREPORTING:
+			console_lock();	/* May be overkill */
 			data = mouse_reporting();
+			console_unlock();
 			ret = __put_user(data, p);
 			break;
 		case TIOCL_SETVESABLANK:
+			console_lock();
 			ret = set_vesa_blanking(p);
+			console_unlock();
 			break;
 		case TIOCL_GETKMSGREDIRECT:
 			data = vt_get_kmsg_redirect();
@@ -2658,13 +2662,21 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 			}
 			break;
 		case TIOCL_GETFGCONSOLE:
+			/* No locking needed as this is a transiently
+			   correct return anyway if the caller hasn't
+			   disabled switching */
 			ret = fg_console;
 			break;
 		case TIOCL_SCROLLCONSOLE:
 			if (get_user(lines, (s32 __user *)(p+4))) {
 				ret = -EFAULT;
 			} else {
+				/* Need the console lock here. Note that lots
+				   of other calls need fixing before the lock
+				   is actually useful ! */
+				console_lock();
 				scrollfront(vc_cons[fg_console].d, lines);
+				console_unlock();
 				ret = 0;
 			}
 			break;
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 16ad235..f0b5143 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -298,9 +298,7 @@ int vt_ioctl(struct tty_struct *tty,
  
 	switch (cmd) {
 	case TIOCLINUX:
-		tty_lock();
 		ret = tioclinux(tty, arg);
-		tty_unlock();
 		break;
 	case KIOCSOUND:
 		if (!perm)

[PATCH 4/6] vt: waitevent is self locked so drop the tty_lock

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

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

 drivers/tty/vt/vt_ioctl.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)


diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index f0b5143..ede2ef1 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -130,7 +130,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_tty(vt_event_waitqueue, vw->done);
+	wait_event_interruptible(vt_event_waitqueue, vw->done);
 	/* Dequeue it */
 	spin_lock_irqsave(&vt_event_lock, flags);
 	list_del(&vw->list);
@@ -1423,14 +1423,10 @@ int vt_move_to_console(unsigned int vt, int alloc)
 		return -EIO;
 	}
 	console_unlock();
-	/* Review: I don't see why we need tty_lock here FIXME */
-	tty_lock();
 	if (vt_waitactive(vt + 1)) {
 		pr_debug("Suspend: Can't switch VCs.");
-		tty_unlock();
 		return -EINTR;
 	}
-	tty_unlock();
 	return prev;
 }
 

[PATCH 5/6] vt: tackle the main part of the selection logic

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

We leave the existing paste mess alone and just fix up the vt side of
things.

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

 drivers/tty/vt/selection.c |   39 +++++++++++++++++++++++++++++++--------
 drivers/tty/vt/vt.c        |    2 ++
 2 files changed, 33 insertions(+), 8 deletions(-)


diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 2a50916..8e9b4be 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -62,10 +62,14 @@ sel_pos(int n)
 				use_unicode);
 }
 
-/* remove the current selection highlight, if any,
-   from the console holding the selection. */
-void
-clear_selection(void) {
+/**
+ *	clear_selection		-	remove current selection
+ *
+ *	Remove the current selection highlight, if any from the console
+ *	holding the selection. The caller must hold the console lock.
+ */
+void clear_selection(void)
+{
 	highlight_pointer(-1); /* hide the pointer */
 	if (sel_start != -1) {
 		highlight(sel_start, sel_end);
@@ -75,7 +79,7 @@ clear_selection(void) {
 
 /*
  * User settable table: what characters are to be considered alphabetic?
- * 256 bits. FIXME: Needs a locking model.
+ * 256 bits. Locked by the console lock.
  */
 static u32 inwordLut[8]={
   0x00000000, /* control chars     */
@@ -92,10 +96,20 @@ static inline int inword(const u16 c) {
 	return c > 0xff || (( inwordLut[c>>5] >> (c & 0x1F) ) & 1);
 }
 
-/* set inwordLut contents. Invoked by ioctl(). */
+/**
+ *	set loadlut		-	load the LUT table
+ *	@p: user table
+ *
+ *	Load the LUT table from user space. The caller must hold the console
+ *	lock. Make a temporary copy so a partial update doesn't make a mess.
+ */
 int sel_loadlut(char __user *p)
 {
-	return copy_from_user(inwordLut, (u32 __user *)(p+4), 32) ? -EFAULT : 0;
+	u32 tmplut[8];
+	if (copy_from_user(tmplut, (u32 __user *)(p+4), 32))
+		return -EFAULT;
+	memcpy(inwordLut, tmplut, 32);
+	return 0;
 }
 
 /* does screen address p correspond to character at LH/RH edge of screen? */
@@ -131,7 +145,16 @@ static int store_utf8(u16 c, char *p)
     	}
 }
 
-/* set the current selection. Invoked by ioctl() or by kernel code. */
+/**
+ *	set_selection		- 	set the current selection.
+ *	@sel: user selection info
+ *	@tty: the console tty
+ *
+ *	Invoked by the ioctl handle for the vt layer.
+ *
+ *	The entire selection process is managed under the console_lock. It's
+ *	 a lot under the lock but its hardly a performance path
+ */
 int set_selection(const struct tiocl_selection __user *sel, struct tty_struct *tty)
 {
 	struct vc_data *vc = vc_cons[fg_console].d;
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 280a4c4..7d79ca8 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2623,7 +2623,9 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 			console_unlock();
 			break;
 		case TIOCL_SELLOADLUT:
+			console_lock();
 			ret = sel_loadlut(p);
+			console_unlock();
 			break;
 		case TIOCL_GETSHIFTSTATE:
 

[PATCH 6/6] vt: push the tty_lock down into the map handling

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

When we do this it becomes clear the lock we should be holding is the vc
lock, and in fact many of our other helpers are properly invoked this way.

We don't at this point guarantee not to race the keyboard code but the results
of that appear harmless and that was true before we started as well.

We now have no users of tty_lock in the console driver...

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

 drivers/tty/vt/consolemap.c |  119 ++++++++++++++++++++++++++++++++-----------
 drivers/tty/vt/vt_ioctl.c   |   25 ++-------
 include/linux/vt_kern.h     |    1 
 3 files changed, 93 insertions(+), 52 deletions(-)


diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index a0f3d6c..299fa08 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -312,6 +312,7 @@ int con_set_trans_old(unsigned char __user * arg)
 	if (!access_ok(VERIFY_READ, arg, E_TABSZ))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++) {
 		unsigned char uc;
 		__get_user(uc, arg+i);
@@ -319,6 +320,7 @@ int con_set_trans_old(unsigned char __user * arg)
 	}
 
 	update_user_maps();
+	console_unlock();
 	return 0;
 }
 
@@ -330,11 +332,13 @@ int con_get_trans_old(unsigned char __user * arg)
 	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++)
-	  {
-	    ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
-	    __put_user((ch & ~0xff) ? 0 : ch, arg+i);
-	  }
+	{
+		ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
+		__put_user((ch & ~0xff) ? 0 : ch, arg+i);
+	}
+	console_unlock();
 	return 0;
 }
 
@@ -346,6 +350,7 @@ int con_set_trans_new(ushort __user * arg)
 	if (!access_ok(VERIFY_READ, arg, E_TABSZ*sizeof(unsigned short)))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++) {
 		unsigned short us;
 		__get_user(us, arg+i);
@@ -353,6 +358,7 @@ int con_set_trans_new(ushort __user * arg)
 	}
 
 	update_user_maps();
+	console_unlock();
 	return 0;
 }
 
@@ -364,8 +370,10 @@ int con_get_trans_new(ushort __user * arg)
 	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ*sizeof(unsigned short)))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++)
 	  __put_user(p[i], arg+i);
+	console_unlock();
 	
 	return 0;
 }
@@ -407,6 +415,7 @@ static void con_release_unimap(struct uni_pagedir *p)
 	}
 }
 
+/* Caller must hold the console lock */
 void con_free_unimap(struct vc_data *vc)
 {
 	struct uni_pagedir *p;
@@ -487,17 +496,21 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
 	return 0;
 }
 
-/* ui is a leftover from using a hashtable, but might be used again */
-int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
+/* ui is a leftover from using a hashtable, but might be used again
+   Caller must hold the lock */
+static int con_do_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
 {
 	struct uni_pagedir *p, *q;
-  
+
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	if (p && p->readonly) return -EIO;
+	if (p && p->readonly)
+		return -EIO;
+
 	if (!p || --p->refcount) {
 		q = kzalloc(sizeof(*p), GFP_KERNEL);
 		if (!q) {
-			if (p) p->refcount++;
+			if (p)
+				p->refcount++;
 			return -ENOMEM;
 		}
 		q->refcount=1;
@@ -511,22 +524,41 @@ int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
 	return 0;
 }
 
+int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
+{
+	int ret;
+	console_lock();
+	ret = con_do_clear_unimap(vc, ui);
+	console_unlock();
+	return ret;
+}
+	
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 {
 	int err = 0, err1, i;
 	struct uni_pagedir *p, *q;
 
+	console_lock();
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	if (p->readonly) return -EIO;
+	if (p->readonly) {
+		console_unlock();
+		return -EIO;
+	}
 	
-	if (!ct) return 0;
+	if (!ct) {
+		console_unlock();
+		return 0;
+	}
 	
 	if (p->refcount > 1) {
 		int j, k;
 		u16 **p1, *p2, l;
 		
-		err1 = con_clear_unimap(vc, NULL);
-		if (err1) return err1;
+		err1 = con_do_clear_unimap(vc, NULL);
+		if (err1) {
+			console_unlock();
+			return err1;
+		}
 		
 		q = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 		for (i = 0, l = 0; i < 32; i++)
@@ -541,6 +573,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 						*vc->vc_uni_pagedir_loc = (unsigned long)p;
 						con_release_unimap(q);
 						kfree(q);
+						console_unlock();
 						return err1; 
 					}
               			}
@@ -557,21 +590,30 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		list++;
 	}
 	
-	if (con_unify_unimap(vc, p))
+	if (con_unify_unimap(vc, p)) {
+		console_unlock();
 		return err;
+	}
 
 	for (i = 0; i <= 3; i++)
 		set_inverse_transl(vc, p, i); /* Update all inverse translations */
 	set_inverse_trans_unicode(vc, p);
-  
+
+	console_unlock();
 	return err;
 }
 
-/* Loads the unimap for the hardware font, as defined in uni_hash.tbl.
-   The representation used was the most compact I could come up
-   with.  This routine is executed at sys_setup time, and when the
-   PIO_FONTRESET ioctl is called. */
-
+/**
+ *	con_set_default_unimap	-	set default unicode map
+ *	@vc: the console we are updating
+ *
+ *	Loads the unimap for the hardware font, as defined in uni_hash.tbl.
+ *	The representation used was the most compact I could come up
+ *	with.  This routine is executed at video setup, and when the
+ *	PIO_FONTRESET ioctl is called. 
+ *
+ *	The caller must hold the console lock
+ */
 int con_set_default_unimap(struct vc_data *vc)
 {
 	int i, j, err = 0, err1;
@@ -582,6 +624,7 @@ int con_set_default_unimap(struct vc_data *vc)
 		p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 		if (p == dflt)
 			return 0;
+
 		dflt->refcount++;
 		*vc->vc_uni_pagedir_loc = (unsigned long)dflt;
 		if (p && !--p->refcount) {
@@ -593,8 +636,9 @@ int con_set_default_unimap(struct vc_data *vc)
 	
 	/* The default font is always 256 characters */
 
-	err = con_clear_unimap(vc, NULL);
-	if (err) return err;
+	err = con_do_clear_unimap(vc, NULL);
+	if (err)
+		return err;
     
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 	q = dfont_unitable;
@@ -619,6 +663,13 @@ int con_set_default_unimap(struct vc_data *vc)
 }
 EXPORT_SYMBOL(con_set_default_unimap);
 
+/**
+ *	con_copy_unimap		-	copy unimap between two vts
+ *	@dst_vc: target
+ *	@src_vt: source
+ *
+ *	The caller must hold the console lock when invoking this method
+ */
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 {
 	struct uni_pagedir *q;
@@ -633,13 +684,23 @@ int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 	*dst_vc->vc_uni_pagedir_loc = (long)q;
 	return 0;
 }
+EXPORT_SYMBOL(con_copy_unimap);
 
+/**
+ *	con_get_unimap		-	get the unicode map
+ *	@vc: the console to read from
+ *
+ *	Read the console unicode data for this console. Called from the ioctl
+ *	handlers.
+ */
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
 {
 	int i, j, k, ect;
 	u16 **p1, *p2;
 	struct uni_pagedir *p;
 
+	console_lock();
+
 	ect = 0;
 	if (*vc->vc_uni_pagedir_loc) {
 		p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
@@ -659,22 +720,19 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 				}
 	}
 	__put_user(ect, uct);
+	console_unlock();
 	return ((ect <= ct) ? 0 : -ENOMEM);
 }
 
-void con_protect_unimap(struct vc_data *vc, int rdonly)
-{
-	struct uni_pagedir *p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	
-	if (p)
-		p->readonly = rdonly;
-}
-
 /*
  * Always use USER_MAP. These functions are used by the keyboard,
  * which shouldn't be affected by G0/G1 switching, etc.
  * If the user map still contains default values, i.e. the
  * direct-to-font mapping, then assume user is using Latin1.
+ *
+ * FIXME: at some point we need to decide if we want to lock the table
+ * update element itself via the keyboard_event_lock for consistency with the
+ * keyboard driver as well as the consoles
  */
 /* may be called during an interrupt */
 u32 conv_8bit_to_uni(unsigned char c)
@@ -742,4 +800,3 @@ console_map_init(void)
 			con_set_default_unimap(vc_cons[i].d);
 }
 
-EXPORT_SYMBOL(con_copy_unimap);
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index ede2ef1..6461854 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -910,7 +910,9 @@ int vt_ioctl(struct tty_struct *tty,
 		ret = con_font_op(vc_cons[fg_console].d, &op);
 		if (ret)
 			break;
+		console_lock();
 		con_set_default_unimap(vc_cons[fg_console].d);
+		console_unlock();
 		break;
 		}
 #endif
@@ -934,33 +936,23 @@ int vt_ioctl(struct tty_struct *tty,
 	case PIO_SCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else {
-			tty_lock();
+		else
 			ret = con_set_trans_old(up);
-			tty_unlock();
-		}
 		break;
 
 	case GIO_SCRNMAP:
-		tty_lock();
 		ret = con_get_trans_old(up);
-		tty_unlock();
 		break;
 
 	case PIO_UNISCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else {
-			tty_lock();
+		else
 			ret = con_set_trans_new(up);
-			tty_unlock();
-		}
 		break;
 
 	case GIO_UNISCRNMAP:
-		tty_lock();
 		ret = con_get_trans_new(up);
-		tty_unlock();
 		break;
 
 	case PIO_UNIMAPCLR:
@@ -970,19 +962,14 @@ int vt_ioctl(struct tty_struct *tty,
 		ret = copy_from_user(&ui, up, sizeof(struct unimapinit));
 		if (ret)
 			ret = -EFAULT;
-		else {
-			tty_lock();
+		else
 			con_clear_unimap(vc, &ui);
-			tty_unlock();
-		}
 		break;
 	      }
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
-		tty_lock();
 		ret = do_unimap_ioctl(cmd, up, perm, vc);
-		tty_unlock();
 		break;
 
 	case VT_LOCKSWITCH:
@@ -1196,9 +1183,7 @@ long vt_compat_ioctl(struct tty_struct *tty,
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
-		tty_lock();
 		ret = compat_unimap_ioctl(cmd, up, perm, vc);
-		tty_unlock();
 		break;
 
 	/*
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index e33d77f..50ae7d0 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -70,7 +70,6 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list);
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list);
 int con_set_default_unimap(struct vc_data *vc);
 void con_free_unimap(struct vc_data *vc);
-void con_protect_unimap(struct vc_data *vc, int rdonly);
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc);
 
 #define vc_translate(vc, c) ((vc)->vc_translate[(c) |			\

Re: [PATCH 2/6] vt: push down the tty lock so we can see what is left to tackle

[Jiri Slaby]

On 03/01/2012 08:50 PM, Alan Cox wrote:
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -281,7 +281,6 @@ int vt_ioctl(struct tty_struct *tty,
>  
>  	console = vc->vc_num;
>  
> -	tty_lock();

Note that this breaks bisection. Further in vt_ioctl, there is a call to
vt_event_wait_ioctl which calls vt_event_wait and that calls
wait_event_interruptible_tty which unlocks BTM.

You change wait_event_interruptible_tty to wait_event_interruptible even
in 4/6.

The VT_WAITEVENT case should have tty_lock until 4/6 (and not longer).

thanks,
-- 
js
suse labs

Re: [PATCH 2/6] vt: push down the tty lock so we can see what is left to tackle

[Arnd Bergmann]

On Thursday 01 March 2012, Alan Cox wrote:
> @@ -1407,6 +1425,7 @@ int vt_move_to_console(unsigned int vt, int alloc)
>                 return -EIO;
>         }
>         console_unlock();
> +       /* Review: I don't see why we need tty_lock here FIXME */
>         tty_lock();
>         if (vt_waitactive(vt + 1)) {
>                 pr_debug("Suspend: Can't switch VCs.");

I'm pretty sure I only added this in be1bc288 "tty: introduce
wait_event_interruptible_tty" so I could release the mutex again in
vt_event_wait.

All other callers of that function hold it, so it needs to be released
in vt_event_wait and that can only be done if it's known to be held first.

	Arnd

Re: [PATCH 0/6] Remove tty_lock from the console drivers

[Arnd Bergmann]

On Thursday 01 March 2012, Alan Cox wrote:
> 
> This needs a fair bit of testing and a bit of review wouldn't go amiss, but
> it does drive the tty_lock() mess out of the console and correct chunks of
> console locking in the process.

I've spent an hour looking at the patches and trying to understand the code
paths you touch.  The use of console_lock for more stuff than it's currently
used for is slightly worrying, mostly because it's still an old-style
semaphore and not covered by lockdep because of that, which might make testing
harder, but it also seems to be the logical thing to do as you write.

Other than that and the VT_WAITEVENT issue that Jiri pointed out, nothing
else caught my eye, it all looks good.

	Arnd

[PATCH 6/6] vt: push the tty_lock down into the map handling

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

When we do this it becomes clear the lock we should be holding is the vc
lock, and in fact many of our other helpers are properly invoked this way.

We don't at this point guarantee not to race the keyboard code but the results
of that appear harmless and that was true before we started as well.

We now have no users of tty_lock in the console driver...

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

 drivers/tty/vt/consolemap.c |  119 ++++++++++++++++++++++++++++++++-----------
 drivers/tty/vt/vt_ioctl.c   |   25 ++-------
 include/linux/vt_kern.h     |    1 
 3 files changed, 93 insertions(+), 52 deletions(-)


diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index a0f3d6c..299fa08 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -312,6 +312,7 @@ int con_set_trans_old(unsigned char __user * arg)
 	if (!access_ok(VERIFY_READ, arg, E_TABSZ))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++) {
 		unsigned char uc;
 		__get_user(uc, arg+i);
@@ -319,6 +320,7 @@ int con_set_trans_old(unsigned char __user * arg)
 	}
 
 	update_user_maps();
+	console_unlock();
 	return 0;
 }
 
@@ -330,11 +332,13 @@ int con_get_trans_old(unsigned char __user * arg)
 	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++)
-	  {
-	    ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
-	    __put_user((ch & ~0xff) ? 0 : ch, arg+i);
-	  }
+	{
+		ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
+		__put_user((ch & ~0xff) ? 0 : ch, arg+i);
+	}
+	console_unlock();
 	return 0;
 }
 
@@ -346,6 +350,7 @@ int con_set_trans_new(ushort __user * arg)
 	if (!access_ok(VERIFY_READ, arg, E_TABSZ*sizeof(unsigned short)))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++) {
 		unsigned short us;
 		__get_user(us, arg+i);
@@ -353,6 +358,7 @@ int con_set_trans_new(ushort __user * arg)
 	}
 
 	update_user_maps();
+	console_unlock();
 	return 0;
 }
 
@@ -364,8 +370,10 @@ int con_get_trans_new(ushort __user * arg)
 	if (!access_ok(VERIFY_WRITE, arg, E_TABSZ*sizeof(unsigned short)))
 		return -EFAULT;
 
+	console_lock();
 	for (i=0; i<E_TABSZ ; i++)
 	  __put_user(p[i], arg+i);
+	console_unlock();
 	
 	return 0;
 }
@@ -407,6 +415,7 @@ static void con_release_unimap(struct uni_pagedir *p)
 	}
 }
 
+/* Caller must hold the console lock */
 void con_free_unimap(struct vc_data *vc)
 {
 	struct uni_pagedir *p;
@@ -487,17 +496,21 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
 	return 0;
 }
 
-/* ui is a leftover from using a hashtable, but might be used again */
-int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
+/* ui is a leftover from using a hashtable, but might be used again
+   Caller must hold the lock */
+static int con_do_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
 {
 	struct uni_pagedir *p, *q;
-  
+
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	if (p && p->readonly) return -EIO;
+	if (p && p->readonly)
+		return -EIO;
+
 	if (!p || --p->refcount) {
 		q = kzalloc(sizeof(*p), GFP_KERNEL);
 		if (!q) {
-			if (p) p->refcount++;
+			if (p)
+				p->refcount++;
 			return -ENOMEM;
 		}
 		q->refcount=1;
@@ -511,22 +524,41 @@ int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
 	return 0;
 }
 
+int con_clear_unimap(struct vc_data *vc, struct unimapinit *ui)
+{
+	int ret;
+	console_lock();
+	ret = con_do_clear_unimap(vc, ui);
+	console_unlock();
+	return ret;
+}
+	
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 {
 	int err = 0, err1, i;
 	struct uni_pagedir *p, *q;
 
+	console_lock();
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	if (p->readonly) return -EIO;
+	if (p->readonly) {
+		console_unlock();
+		return -EIO;
+	}
 	
-	if (!ct) return 0;
+	if (!ct) {
+		console_unlock();
+		return 0;
+	}
 	
 	if (p->refcount > 1) {
 		int j, k;
 		u16 **p1, *p2, l;
 		
-		err1 = con_clear_unimap(vc, NULL);
-		if (err1) return err1;
+		err1 = con_do_clear_unimap(vc, NULL);
+		if (err1) {
+			console_unlock();
+			return err1;
+		}
 		
 		q = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 		for (i = 0, l = 0; i < 32; i++)
@@ -541,6 +573,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 						*vc->vc_uni_pagedir_loc = (unsigned long)p;
 						con_release_unimap(q);
 						kfree(q);
+						console_unlock();
 						return err1; 
 					}
               			}
@@ -557,21 +590,30 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		list++;
 	}
 	
-	if (con_unify_unimap(vc, p))
+	if (con_unify_unimap(vc, p)) {
+		console_unlock();
 		return err;
+	}
 
 	for (i = 0; i <= 3; i++)
 		set_inverse_transl(vc, p, i); /* Update all inverse translations */
 	set_inverse_trans_unicode(vc, p);
-  
+
+	console_unlock();
 	return err;
 }
 
-/* Loads the unimap for the hardware font, as defined in uni_hash.tbl.
-   The representation used was the most compact I could come up
-   with.  This routine is executed at sys_setup time, and when the
-   PIO_FONTRESET ioctl is called. */
-
+/**
+ *	con_set_default_unimap	-	set default unicode map
+ *	@vc: the console we are updating
+ *
+ *	Loads the unimap for the hardware font, as defined in uni_hash.tbl.
+ *	The representation used was the most compact I could come up
+ *	with.  This routine is executed at video setup, and when the
+ *	PIO_FONTRESET ioctl is called. 
+ *
+ *	The caller must hold the console lock
+ */
 int con_set_default_unimap(struct vc_data *vc)
 {
 	int i, j, err = 0, err1;
@@ -582,6 +624,7 @@ int con_set_default_unimap(struct vc_data *vc)
 		p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 		if (p == dflt)
 			return 0;
+
 		dflt->refcount++;
 		*vc->vc_uni_pagedir_loc = (unsigned long)dflt;
 		if (p && !--p->refcount) {
@@ -593,8 +636,9 @@ int con_set_default_unimap(struct vc_data *vc)
 	
 	/* The default font is always 256 characters */
 
-	err = con_clear_unimap(vc, NULL);
-	if (err) return err;
+	err = con_do_clear_unimap(vc, NULL);
+	if (err)
+		return err;
     
 	p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
 	q = dfont_unitable;
@@ -619,6 +663,13 @@ int con_set_default_unimap(struct vc_data *vc)
 }
 EXPORT_SYMBOL(con_set_default_unimap);
 
+/**
+ *	con_copy_unimap		-	copy unimap between two vts
+ *	@dst_vc: target
+ *	@src_vt: source
+ *
+ *	The caller must hold the console lock when invoking this method
+ */
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 {
 	struct uni_pagedir *q;
@@ -633,13 +684,23 @@ int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 	*dst_vc->vc_uni_pagedir_loc = (long)q;
 	return 0;
 }
+EXPORT_SYMBOL(con_copy_unimap);
 
+/**
+ *	con_get_unimap		-	get the unicode map
+ *	@vc: the console to read from
+ *
+ *	Read the console unicode data for this console. Called from the ioctl
+ *	handlers.
+ */
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
 {
 	int i, j, k, ect;
 	u16 **p1, *p2;
 	struct uni_pagedir *p;
 
+	console_lock();
+
 	ect = 0;
 	if (*vc->vc_uni_pagedir_loc) {
 		p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
@@ -659,22 +720,19 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 				}
 	}
 	__put_user(ect, uct);
+	console_unlock();
 	return ((ect <= ct) ? 0 : -ENOMEM);
 }
 
-void con_protect_unimap(struct vc_data *vc, int rdonly)
-{
-	struct uni_pagedir *p = (struct uni_pagedir *)*vc->vc_uni_pagedir_loc;
-	
-	if (p)
-		p->readonly = rdonly;
-}
-
 /*
  * Always use USER_MAP. These functions are used by the keyboard,
  * which shouldn't be affected by G0/G1 switching, etc.
  * If the user map still contains default values, i.e. the
  * direct-to-font mapping, then assume user is using Latin1.
+ *
+ * FIXME: at some point we need to decide if we want to lock the table
+ * update element itself via the keyboard_event_lock for consistency with the
+ * keyboard driver as well as the consoles
  */
 /* may be called during an interrupt */
 u32 conv_8bit_to_uni(unsigned char c)
@@ -742,4 +800,3 @@ console_map_init(void)
 			con_set_default_unimap(vc_cons[i].d);
 }
 
-EXPORT_SYMBOL(con_copy_unimap);
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index ede2ef1..6461854 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -910,7 +910,9 @@ int vt_ioctl(struct tty_struct *tty,
 		ret = con_font_op(vc_cons[fg_console].d, &op);
 		if (ret)
 			break;
+		console_lock();
 		con_set_default_unimap(vc_cons[fg_console].d);
+		console_unlock();
 		break;
 		}
 #endif
@@ -934,33 +936,23 @@ int vt_ioctl(struct tty_struct *tty,
 	case PIO_SCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else {
-			tty_lock();
+		else
 			ret = con_set_trans_old(up);
-			tty_unlock();
-		}
 		break;
 
 	case GIO_SCRNMAP:
-		tty_lock();
 		ret = con_get_trans_old(up);
-		tty_unlock();
 		break;
 
 	case PIO_UNISCRNMAP:
 		if (!perm)
 			ret = -EPERM;
-		else {
-			tty_lock();
+		else
 			ret = con_set_trans_new(up);
-			tty_unlock();
-		}
 		break;
 
 	case GIO_UNISCRNMAP:
-		tty_lock();
 		ret = con_get_trans_new(up);
-		tty_unlock();
 		break;
 
 	case PIO_UNIMAPCLR:
@@ -970,19 +962,14 @@ int vt_ioctl(struct tty_struct *tty,
 		ret = copy_from_user(&ui, up, sizeof(struct unimapinit));
 		if (ret)
 			ret = -EFAULT;
-		else {
-			tty_lock();
+		else
 			con_clear_unimap(vc, &ui);
-			tty_unlock();
-		}
 		break;
 	      }
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
-		tty_lock();
 		ret = do_unimap_ioctl(cmd, up, perm, vc);
-		tty_unlock();
 		break;
 
 	case VT_LOCKSWITCH:
@@ -1196,9 +1183,7 @@ long vt_compat_ioctl(struct tty_struct *tty,
 
 	case PIO_UNIMAP:
 	case GIO_UNIMAP:
-		tty_lock();
 		ret = compat_unimap_ioctl(cmd, up, perm, vc);
-		tty_unlock();
 		break;
 
 	/*
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index e33d77f..50ae7d0 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -70,7 +70,6 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list);
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list);
 int con_set_default_unimap(struct vc_data *vc);
 void con_free_unimap(struct vc_data *vc);
-void con_protect_unimap(struct vc_data *vc, int rdonly);
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc);
 
 #define vc_translate(vc, c) ((vc)->vc_translate[(c) |			\

Re: [PATCH 6/6] vt: push the tty_lock down into the map handling

[Greg KH]

On Fri, Mar 02, 2012 at 03:00:14PM +0000, Alan Cox wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> When we do this it becomes clear the lock we should be holding is the vc
> lock, and in fact many of our other helpers are properly invoked this way.
> 
> We don't at this point guarantee not to race the keyboard code but the results
> of that appear harmless and that was true before we started as well.
> 
> We now have no users of tty_lock in the console driver...
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
>  drivers/tty/vt/consolemap.c |  119 ++++++++++++++++++++++++++++++++-----------
>  drivers/tty/vt/vt_ioctl.c   |   25 ++-------
>  include/linux/vt_kern.h     |    1 
>  3 files changed, 93 insertions(+), 52 deletions(-)
> 

With this patch applied, I get the following build error:

drivers/tty/vt/consolemap.c: In function ‘con_set_trans_old’:
drivers/tty/vt/consolemap.c:315:2: error: implicit declaration of function ‘console_lock’ [-Werror=implicit-function-declaration]
drivers/tty/vt/consolemap.c:323:2: error: implicit declaration of function ‘console_unlock’ [-Werror=implicit-function-declaration]

what went wrong?

I've applied the first 5 in this series to my tree now, but not this
one.

thanks,

greg k-h