Anyone feeling inspired. These three patches move the console to use the
tty_port interface and refcounting. The first two work perfectly, the third
which we really need works beautifully except that it breaks mingetty/login
stuff.
[So not for the main kernel if you like using text consoles]
I have been staring at this problem for ages and I'm clearly missing something
silly because I've still not pinned down why mingetty etc break with the
third patch applied.
---
Alan Cox (3):
vt: Fix refcounting use
tty: Move the vt_tty field from the vc_data into the standard tty_port
tty: Make vt's have a tty_port
This takes all the tty references through the expected interface points so
we can refcount them.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/keyboard.c | 10 +++++-----
drivers/char/vt.c | 10 +++++-----
drivers/char/vt_ioctl.c | 2 +-
include/linux/console_struct.h | 1 -
4 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 11491a3..cd440f8 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -315,7 +315,7 @@ int kbd_rate(struct kbd_repeat *rep)
*/
static void put_queue(struct vc_data *vc, int ch)
{
- struct tty_struct *tty = vc->vc_tty;
+ struct tty_struct *tty = vc->port.tty;
if (tty) {
tty_insert_flip_char(tty, ch, 0);
@@ -325,7 +325,7 @@ static void put_queue(struct vc_data *vc, int ch)
static void puts_queue(struct vc_data *vc, char *cp)
{
- struct tty_struct *tty = vc->vc_tty;
+ struct tty_struct *tty = vc->port.tty;
if (!tty)
return;
@@ -497,7 +497,7 @@ static void fn_show_ptregs(struct vc_data *vc)
static void fn_hold(struct vc_data *vc)
{
- struct tty_struct *tty = vc->vc_tty;
+ struct tty_struct *tty = vc->port.tty;
if (rep || !tty)
return;
@@ -575,7 +575,7 @@ static void fn_inc_console(struct vc_data *vc)
static void fn_send_intr(struct vc_data *vc)
{
- struct tty_struct *tty = vc->vc_tty;
+ struct tty_struct *tty = vc->port.tty;
if (!tty)
return;
@@ -1167,7 +1167,7 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
int shift_final;
struct keyboard_notifier_param param = { .vc = vc, .value = keycode, .down = down };
- tty = vc->vc_tty;
+ tty = vc->port.tty;
if (tty && (!tty->driver_data)) {
/* No driver data? Strange. Okay we fix it then. */
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 8e2cb28..9821628 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -957,12 +957,12 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
* Resize a virtual console as seen from the console end of things. We
* use the common vc_do_resize methods to update the structures. The
* caller must hold the console sem to protect console internals and
- * vc->vc_tty
+ * vc->port.tty
*/
int vc_resize(struct vc_data *vc, unsigned int cols, unsigned int rows)
{
- return vc_do_resize(vc->vc_tty, vc, cols, rows);
+ return vc_do_resize(vc->port.tty, vc, cols, rows);
}
/**
@@ -2794,12 +2794,12 @@ static int con_open(struct tty_struct *tty, struct file *filp)
struct vc_data *vc = vc_cons[currcons].d;
/* Still being freed */
- if (vc->vc_tty) {
+ if (vc->port.tty) {
release_console_sem();
return -ERESTARTSYS;
}
tty->driver_data = vc;
- vc->vc_tty = tty;
+ vc->port.tty = tty;
if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
@@ -2827,7 +2827,7 @@ static void con_shutdown(struct tty_struct *tty)
struct vc_data *vc = tty->driver_data;
BUG_ON(vc == NULL);
acquire_console_sem();
- vc->vc_tty = NULL;
+ vc->port.tty = NULL;
release_console_sem();
tty_shutdown(tty);
}
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 6aa1028..625f77d 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -1367,7 +1367,7 @@ void vc_SAK(struct work_struct *work)
acquire_console_sem();
vc = vc_con->d;
if (vc) {
- tty = vc->vc_tty;
+ tty = vc->port.tty;
/*
* SAK should also work in all raw modes and reset
* them properly.
diff --git a/include/linux/console_struct.h b/include/linux/console_struct.h
index 57e83f1..3364898 100644
--- a/include/linux/console_struct.h
+++ b/include/linux/console_struct.h
@@ -58,7 +58,6 @@ struct vc_data {
/* VT terminal data */
unsigned int vc_state; /* Escape sequence parser state */
unsigned int vc_npar,vc_par[NPAR]; /* Parameters of current escape sequence */
- struct tty_struct *vc_tty; /* TTY we are attached to */
/* data for manual vt switching */
struct vt_mode vt_mode;
struct pid *vt_pid;
The vt layer isn't safely handling reference counts to tty object on the input
side. Add a tty port structure to the vt layer in order to implement this using
the standard helpers.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/vt.c | 2 ++
include/linux/console_struct.h | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index bd1d116..8e2cb28 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -768,6 +768,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */
if (!vc)
return -ENOMEM;
vc_cons[currcons].d = vc;
+ tty_port_init(&vc->port);
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
visual_init(vc, currcons, 1);
if (!*vc->vc_uni_pagedir_loc)
@@ -2908,6 +2909,7 @@ static int __init con_init(void)
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
+ tty_port_init(&vc->port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
vc_init(vc, vc->vc_rows, vc->vc_cols,
diff --git a/include/linux/console_struct.h b/include/linux/console_struct.h
index 38fe59d..57e83f1 100644
--- a/include/linux/console_struct.h
+++ b/include/linux/console_struct.h
@@ -21,6 +21,8 @@ struct vt_struct;
#define NPAR 16
struct vc_data {
+ struct tty_port port; /* Upper level data */
+
unsigned short vc_num; /* Console number */
unsigned int vc_cols; /* [#] Console size */
unsigned int vc_rows;
We want to properly refcount the vc tty because of the cases where a vc is
disassociated from a tty (eg destroyed). At the moment the code isn't safe
and in fact input even tries to patch up broken pointers as a result!
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/keyboard.c | 47 +++++++++++-------
drivers/char/vt.c | 126 ++++++++++++++++++++++++++++++++---------------
drivers/char/vt_ioctl.c | 6 +-
3 files changed, 119 insertions(+), 60 deletions(-)
diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index cd440f8..46d0b1a 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -315,17 +315,18 @@ int kbd_rate(struct kbd_repeat *rep)
*/
static void put_queue(struct vc_data *vc, int ch)
{
- struct tty_struct *tty = vc->port.tty;
+ struct tty_struct *tty = tty_port_tty_get(&vc->port);
if (tty) {
tty_insert_flip_char(tty, ch, 0);
con_schedule_flip(tty);
+ tty_kref_put(tty);
}
}
static void puts_queue(struct vc_data *vc, char *cp)
{
- struct tty_struct *tty = vc->port.tty;
+ struct tty_struct *tty = tty_port_tty_get(&vc->port);
if (!tty)
return;
@@ -335,6 +336,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
cp++;
}
con_schedule_flip(tty);
+ tty_kref_put(tty);
}
static void applkey(struct vc_data *vc, int key, char mode)
@@ -497,10 +499,12 @@ static void fn_show_ptregs(struct vc_data *vc)
static void fn_hold(struct vc_data *vc)
{
- struct tty_struct *tty = vc->port.tty;
+ struct tty_struct *tty = tty_port_tty_get(&vc->port);
- if (rep || !tty)
+ if (rep || !tty) {
+ tty_kref_put(tty);
return;
+ }
/*
* Note: SCROLLOCK will be set (cleared) by stop_tty (start_tty);
@@ -511,6 +515,7 @@ static void fn_hold(struct vc_data *vc)
start_tty(tty);
else
stop_tty(tty);
+ tty_kref_put(tty);
}
static void fn_num(struct vc_data *vc)
@@ -575,12 +580,13 @@ static void fn_inc_console(struct vc_data *vc)
static void fn_send_intr(struct vc_data *vc)
{
- struct tty_struct *tty = vc->port.tty;
+ struct tty_struct *tty = tty_port_tty_get(&vc->port);
if (!tty)
return;
tty_insert_flip_char(tty, 0, TTY_BREAK);
con_schedule_flip(tty);
+ tty_kref_put(tty);
}
static void fn_scroll_forw(struct vc_data *vc)
@@ -1167,11 +1173,14 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
int shift_final;
struct keyboard_notifier_param param = { .vc = vc, .value = keycode, .down = down };
- tty = vc->port.tty;
+ tty = tty_port_tty_get(&vc->port);
- if (tty && (!tty->driver_data)) {
- /* No driver data? Strange. Okay we fix it then. */
- tty->driver_data = vc;
+ if (tty && !tty->driver_data) {
+ /* No driver data? - can't continue */
+ WARN_ON(1);
+ /* If this race still exists we want to know */
+ tty_kref_put(tty);
+ return;
}
kbd = kbd_table + vc->vc_num;
@@ -1201,13 +1210,13 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
sysrq_down = down;
sysrq_alt_use = sysrq_alt;
}
- return;
+ goto put;
}
if (sysrq_down && !down && keycode == sysrq_alt_use)
sysrq_down = 0;
if (sysrq_down && down && !rep) {
handle_sysrq(kbd_sysrq_xlate[keycode], tty);
- return;
+ goto put;
}
#endif
#ifdef CONFIG_SPARC
@@ -1245,7 +1254,7 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
* characters get aren't echoed locally. This makes key repeat
* usable with slow applications and under heavy loads.
*/
- return;
+ goto put;
}
param.shift = shift_final = (shift_state | kbd->slockstate) ^ kbd->lockstate;
@@ -1256,14 +1265,14 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
atomic_notifier_call_chain(&keyboard_notifier_list, KBD_UNBOUND_KEYCODE, ¶m);
compute_shiftstate();
kbd->slockstate = 0;
- return;
+ goto put;
}
if (keycode >= NR_KEYS)
if (keycode >= KEY_BRL_DOT1 && keycode <= KEY_BRL_DOT8)
keysym = U(K(KT_BRL, keycode - KEY_BRL_DOT1 + 1));
else
- return;
+ goto put;
else
keysym = key_map[keycode];
@@ -1272,10 +1281,10 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
if (type < 0xf0) {
param.value = keysym;
if (atomic_notifier_call_chain(&keyboard_notifier_list, KBD_UNICODE, ¶m) == NOTIFY_STOP)
- return;
+ goto put;
if (down && !raw_mode)
to_utf8(vc, keysym);
- return;
+ goto put;
}
type -= 0xf0;
@@ -1291,10 +1300,10 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
param.value = keysym;
if (atomic_notifier_call_chain(&keyboard_notifier_list, KBD_KEYSYM, ¶m) == NOTIFY_STOP)
- return;
+ goto put;
if (raw_mode && type != KT_SPEC && type != KT_SHIFT)
- return;
+ goto put;
(*k_handler[type])(vc, keysym & 0xff, !down);
@@ -1303,6 +1312,8 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
if (type != KT_SLOCK)
kbd->slockstate = 0;
+put:
+ tty_kref_put(tty);
}
static void kbd_event(struct input_handle *handle, unsigned int event_type,
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 9821628..bc9f1fd 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -157,7 +157,7 @@ static void hide_cursor(struct vc_data *vc);
static void console_callback(struct work_struct *ignored);
static void blank_screen_t(unsigned long dummy);
static void set_palette(struct vc_data *vc);
-
+static int con_activate(struct tty_port *port, struct tty_struct *tty);
static int printable; /* Is console ready for printing? */
int default_utf8 = true;
module_param(default_utf8, int, S_IRUGO | S_IWUSR);
@@ -744,6 +744,10 @@ static void visual_init(struct vc_data *vc, int num, int init)
vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;
}
+static const struct tty_port_operations con_port_ops = {
+ .activate = con_activate,
+};
+
int vc_allocate(unsigned int currcons) /* return 0 on success */
{
WARN_CONSOLE_UNLOCKED();
@@ -769,6 +773,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */
return -ENOMEM;
vc_cons[currcons].d = vc;
tty_port_init(&vc->port);
+ vc->port.ops = &con_port_ops;
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
visual_init(vc, currcons, 1);
if (!*vc->vc_uni_pagedir_loc)
@@ -2696,6 +2701,7 @@ static int con_write(struct tty_struct *tty, const unsigned char *buf, int count
retval = do_con_write(tty, buf, count);
con_flush_chars(tty);
+ printk("con_write: %s %d\n", current->comm, retval);
return retval;
}
@@ -2771,7 +2777,6 @@ static void con_flush_chars(struct tty_struct *tty)
if (in_interrupt()) /* from flush_to_ldisc */
return;
- /* if we race with con_close(), vt may be null */
acquire_console_sem();
vc = tty->driver_data;
if (vc)
@@ -2779,61 +2784,100 @@ static void con_flush_chars(struct tty_struct *tty)
release_console_sem();
}
-/*
- * Allocate the console screen memory.
+/**
+ * con_install - called when a tty is being opened
+ * @driver: the driver for the tty
+ * @tty: the new tty being created
+ *
+ * Allocate the console screen memory and install the relevant structures
+ * into the kernel. We don't yet use this to kref consoles but the
+ * tty layer provides the framework for this via the install and shutdown
+ * methods.
*/
-static int con_open(struct tty_struct *tty, struct file *filp)
+
+static int con_install(struct tty_driver *driver, struct tty_struct *tty)
{
- unsigned int currcons = tty->index;
- int ret = 0;
+ int idx = tty->index;
+ int ret = tty_init_termios(tty);
+ struct vc_data *vc;
+
+ if (ret)
+ return ret;
acquire_console_sem();
- if (tty->driver_data == NULL) {
- ret = vc_allocate(currcons);
- if (ret == 0) {
- struct vc_data *vc = vc_cons[currcons].d;
-
- /* Still being freed */
- if (vc->port.tty) {
- release_console_sem();
- return -ERESTARTSYS;
- }
- tty->driver_data = vc;
- vc->port.tty = tty;
+ ret = vc_allocate(idx);
+ if (ret == 0) {
+ printk("install: %s: vc_allocate %d = %d\n", current->comm, idx, ret);
+ vc = vc_cons[idx].d;
+ tty_driver_kref_get(driver);
+ tty->count++;
+ tty->driver_data = vc;
+ driver->ttys[idx] = tty;
+ } else
+ tty_free_termios(tty);
+ release_console_sem();
+ return ret;
+}
- if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
- tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
- tty->winsize.ws_col = vc_cons[currcons].d->vc_cols;
- }
- if (vc->vc_utf)
- tty->termios->c_iflag |= IUTF8;
- else
- tty->termios->c_iflag &= ~IUTF8;
- release_console_sem();
- return ret;
- }
+/**
+ * con_activate - set up for initial open
+ * @port: the port being opened
+ * @tty: the tty we are binding
+ *
+ * At this point tty->driver_data has been set by the install method.
+ * We are called under the port mutex serialized from close and hangup
+ * events. Fill in the various termios and winsize related bits according
+ * to the console itself.
+ */
+
+static int con_activate(struct tty_port *port, struct tty_struct *tty)
+{
+ struct vc_data *vc = tty->driver_data;
+
+ printk("con_activate %d: %s\n", tty->index, current->comm);
+ acquire_console_sem();
+ if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
+ tty->winsize.ws_row = vc->vc_rows;
+ tty->winsize.ws_col = vc->vc_cols;
}
+ if (vc->vc_utf)
+ tty->termios->c_iflag |= IUTF8;
+ else
+ tty->termios->c_iflag &= ~IUTF8;
release_console_sem();
+ return 0;
+}
+
+/*
+ * Use the standard helpers for this part of the process.
+ */
+
+static int con_open(struct tty_struct *tty, struct file *filp)
+{
+ struct vc_data *vc = tty->driver_data;
+ int ret;
+ printk("Console open %s %d\n", current->comm, tty->index);
+ ret = tty_port_open(&vc->port, tty, filp);
+ printk("Ret %d\n", ret);
return ret;
}
static void con_close(struct tty_struct *tty, struct file *filp)
{
- /* Nothing to do - we defer to shutdown */
+ struct vc_data *vc = tty->driver_data;
+ printk("Console close %s %d\n", current->comm, tty->index);
+ tty_port_close(&vc->port, tty, filp);
}
-static void con_shutdown(struct tty_struct *tty)
+static void con_hangup(struct tty_struct *tty)
{
struct vc_data *vc = tty->driver_data;
- BUG_ON(vc == NULL);
- acquire_console_sem();
- vc->port.tty = NULL;
- release_console_sem();
- tty_shutdown(tty);
+ printk("Console hangup %s %d\n", current->comm, tty->index);
+ tty_port_hangup(&vc->port);
}
-static int default_italic_color = 2; // green (ASCII)
-static int default_underline_color = 3; // cyan (ASCII)
+static int default_italic_color = 2; /* green (ASCII) */
+static int default_underline_color = 3; /* cyan (ASCII) */
module_param_named(italic, default_italic_color, int, S_IRUGO | S_IWUSR);
module_param_named(underline, default_underline_color, int, S_IRUGO | S_IWUSR);
@@ -2910,6 +2954,7 @@ static int __init con_init(void)
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
tty_port_init(&vc->port);
+ vc->port.ops = &con_port_ops;
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
vc_init(vc, vc->vc_rows, vc->vc_cols,
@@ -2940,6 +2985,8 @@ console_initcall(con_init);
static const struct tty_operations con_ops = {
.open = con_open,
.close = con_close,
+ .hangup = con_hangup,
+ .install = con_install,
.write = con_write,
.write_room = con_write_room,
.put_char = con_put_char,
@@ -2954,7 +3001,6 @@ static const struct tty_operations con_ops = {
.throttle = con_throttle,
.unthrottle = con_unthrottle,
.resize = vt_resize,
- .shutdown = con_shutdown
};
static struct cdev vc0_cdev;
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 625f77d..8540604 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -1367,13 +1367,15 @@ void vc_SAK(struct work_struct *work)
acquire_console_sem();
vc = vc_con->d;
if (vc) {
- tty = vc->port.tty;
+ tty = tty_port_tty_get(&vc->port);
/*
* SAK should also work in all raw modes and reset
* them properly.
*/
- if (tty)
+ if (tty) {
__do_SAK(tty);
+ tty_kref_put(tty);
+ }
reset_vc(vc);
}
release_console_sem();
On 17/02/10 13:23, Alan Cox wrote:
> We want to properly refcount the vc tty because of the cases where a vc is
> disassociated from a tty (eg destroyed). At the moment the code isn't safe
> and in fact input even tries to patch up broken pointers as a result!
>
> static void applkey(struct vc_data *vc, int key, char mode)
> @@ -497,10 +499,12 @@ static void fn_show_ptregs(struct vc_data *vc)
>
> static void fn_hold(struct vc_data *vc)
> {
> - struct tty_struct *tty = vc->port.tty;
> + struct tty_struct *tty = tty_port_tty_get(&vc->port);
>
> - if (rep || !tty)
> + if (rep || !tty) {
> + tty_kref_put(tty);
> return;
> + }
>
This is the only bit that immediately stands out..
maybe?
if (rep || !tty) {
if (tty)
tty_kref_put(tty);
return;
}
regards
Richard
> This is the only bit that immediately stands out..
> maybe?
> if (rep || !tty) {
> if (tty)
> tty_kref_put(tty);
> return;
> }
tty_kref_put(NULL) is valid - otherwise a lot of other code (and
programmers) would go nuts..
Alan