Skip to contentSkip to conversation

VT locking patch #2

DISCUSSION Benjamin Herrenschmidt 1 participant 2 messages Jan 11, 2004

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

Hi Andrew !

Here's a new version of the patch, against 2.6.1 bk.

I went more in depth into some of the calls in vt_ioctl and I think fixed a few more races along with a possible false-positive. I added the check for oops in progress too.

There are matching bits that have to go to fbdev as well, they'll be part of the fbdev merge.

James: Do no include this patch with your big fbdev patches please, it makes things more confusing. I'll send you separately the patches to apply to fbdev to add locking in the right place.

Ben.

#1 lore raw
Quoted 8 lines · Benjamin Herrenschmidt

Hi Andrew !

Here's a new version of the patch, against 2.6.1 bk.

I went more in depth into some of the calls in vt_ioctl and I think fixed a few more races along with a possible false-positive. I added the check for oops in progress too.

Quoted 6 lines

There are matching bits that have to go to fbdev as well, they'll be part of the fbdev merge.

James: Do no include this patch with your big fbdev patches please, it makes things more confusing. I'll send you separately the patches to apply to fbdev to add locking in the right place.

And with the patch attached...

===== drivers/char/selection.c 1.6 vs edited =====

drivers/char/selection.c +4 -0
@@ -24,6 +24,7 @@
 #include <linux/consolemap.h>
 #include <linux/selection.h>
 #include <linux/tiocl.h>
+#include <linux/console.h>
 
 #ifndef MIN
 #define MIN(a,b)	((a) < (b) ? (a) : (b))
@@ -290,7 +291,10 @@
 	int	pasted = 0, count;
 	DECLARE_WAITQUEUE(wait, current);
 
+	acquire_console_sem();
 	poke_blanked_console();
+	release_console_sem();
+
 	add_wait_queue(&vt->paste_wait, &wait);
 	while (sel_buffer && sel_buffer_lth > pasted) {
 		set_current_state(TASK_INTERRUPTIBLE);

===== drivers/char/tty_io.c 1.127 vs edited =====

drivers/char/tty_io.c +6 -1
@@ -1484,7 +1484,12 @@
 #ifdef CONFIG_VT
 	if (tty->driver->type == TTY_DRIVER_TYPE_CONSOLE) {
 		unsigned int currcons = tty->index;
-		if (vc_resize(currcons, tmp_ws.ws_col, tmp_ws.ws_row))
+		int rc;
+
+		acquire_console_sem();	       
+		rc = vc_resize(currcons, tmp_ws.ws_col, tmp_ws.ws_row);
+		release_console_sem();
+		if (rc)
 			return -ENXIO;
 	}
 #endif

===== drivers/char/vt.c 1.56 vs edited =====

drivers/char/vt.c +111 -46

380 lines not on this page · raw message

===== drivers/char/vt_ioctl.c 1.32 vs edited =====

drivers/char/vt_ioctl.c +46 -15

139 lines not on this page · raw message

===== include/linux/console.h 1.10 vs edited =====

include/linux/console.h +8 -0
@@ -102,6 +102,14 @@
 extern void release_console_sem(void);
 extern void console_conditional_schedule(void);
 extern void console_unblank(void);
+extern int is_console_locked(void);
+
+/* Some debug stub to catch some of the obvious races in the VT code */
+#if 1
+#define WARN_CONSOLE_UNLOCKED()	WARN_ON(!is_console_locked() && !oops_in_progress)
+#else
+#define WARN_CONSOLE_UNLOCKED()
+#endif
 
 /* VESA Blanking Levels */
 #define VESA_NO_BLANKING        0

===== kernel/printk.c 1.30 vs edited =====

kernel/printk.c +20 -0
@@ -62,6 +62,15 @@
  */
 static DECLARE_MUTEX(console_sem);
 struct console *console_drivers;
+/*
+ * This is used for debugging the mess that is the VT code by
+ * keeping track if we have the console semaphore held. It's
+ * definitely not the perfect debug tool (we don't know if _WE_
+ * hold it are racing, but it helps tracking those weird code
+ * path in the console code where we end up in places I want
+ * locked without the console sempahore held
+ */
+static int console_locked;
 
 /*
  * logbuf_lock protects log_buf, log_start, log_end, con_start and logged_chars
@@ -524,6 +533,7 @@
 		goto out;
 	}
 	if (!down_trylock(&console_sem)) {
+		console_locked = 1;
 		/*
 		 * We own the drivers.  We can drop the spinlock and let
 		 * release_console_sem() print the text
@@ -557,10 +567,17 @@
 	if (in_interrupt())
 		BUG();
 	down(&console_sem);
+	console_locked = 1;
 	console_may_schedule = 1;
 }
 EXPORT_SYMBOL(acquire_console_sem);
 
+int is_console_locked(void)
+{
+	return console_locked;
+}
+EXPORT_SYMBOL(is_console_locked);
+
 /**
  * release_console_sem - unlock the console system
  *
@@ -592,12 +609,14 @@
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 		call_console_drivers(_con_start, _log_end);
 	}
+	console_locked = 0;
 	console_may_schedule = 0;
 	up(&console_sem);
 	spin_unlock_irqrestore(&logbuf_lock, flags);
 	if (wake_klogd && !oops_in_progress && waitqueue_active(&log_wait))
 		wake_up_interruptible(&log_wait);
 }
+EXPORT_SYMBOL(release_console_sem);
 
 /** console_conditional_schedule - yield the CPU if required
  *
@@ -633,6 +652,7 @@
 	 */
 	if (down_trylock(&console_sem) != 0)
 		return;
+	console_locked = 1;
 	console_may_schedule = 0;
 	for (c = console_drivers; c != NULL; c = c->next)
 		if ((c->flags & CON_ENABLED) && c->unblank)

===== kernel/power/console.c 1.5 vs edited =====

kernel/power/console.c +10 -6
@@ -6,6 +6,7 @@
 
 #include <linux/vt_kern.h>
 #include <linux/kbd_kern.h>
+#include <linux/console.h>
 #include "power.h"
 
 static int new_loglevel = 10;
@@ -18,14 +19,20 @@
 	console_loglevel = new_loglevel;
 
 #ifdef SUSPEND_CONSOLE
+	acquire_console_sem();
+
 	orig_fgconsole = fg_console;
 
-	if (vc_allocate(SUSPEND_CONSOLE))
+	if (vc_allocate(SUSPEND_CONSOLE)) {
 	  /* we can't have a free VC for now. Too bad,
 	   * we don't want to mess the screen for now. */
+		release_console_sem();
 		return 1;
+	}
 
 	set_console(SUSPEND_CONSOLE);
+	release_console_sem();
+
 	if (vt_waitactive(SUSPEND_CONSOLE)) {
 		pr_debug("Suspend: Can't switch VCs.");
 		return 1;
@@ -40,12 +47,9 @@
 {
 	console_loglevel = orig_loglevel;
 #ifdef SUSPEND_CONSOLE
+	acquire_console_sem();
 	set_console(orig_fgconsole);
-
-	/* FIXME: 
-	 * This following part is left over from swsusp. Is it really needed?
-	 */
-	update_screen(fg_console);
+	release_console_sem();
 #endif
 	return;
 }