TTY: tty_port questions

TTY: tty_port questions

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 12402 bytes --]

Hi!

While moving UML's console driver to tty_port some strange things happened.
So, I have a few questions. :-)

The original driver did not implement tty_operations->hangup().
If I implement it and call tty_port_hangup(), as Alan suggested,
the login fails on all TTYs except tty0.
It fails because the opened TTY returns EIO upon read()/write() after /bin/login
called vhangup().

The call chain is:
vhangup() -> tty_vhangup_self() -> tty_vhangup() -> __tty_hangup()

Within __tty_hangup() something happens that I don't fully understand:

if (cons_filp) {
        if (tty->ops->close)
                for (n = 0; n < closecount; n++)
                        tty->ops->close(tty, cons_filp);
} else if (tty->ops->hangup)
        (tty->ops->hangup)(tty);

Login on tty0 works because cons_filp is not NULL and tty->ops->close() is called.
On the other hand login fails on every other TTY because cons_filp remains NULL
and the TTY hangs up.

Is there something missing in my hangup function?

If I omit tty_operations->hangup() and leave it, like the old driver, NULL non-tty0 TTYs cannot
be opened. (getty terminates immediately because it cannot open any TTY.)
open() retuns -EIO because the TTY_CLOSING is set in tty->flags.

How can this be?

Another fun fact is that the above noted problems do not happen on Debian.
Because Debian's /bin/login does not call vhangup().

Thanks,
//richard

--
diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index c1cf220..24de04c 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -19,19 +19,29 @@ static irqreturn_t line_interrupt(int irq, void *data)
 {
 	struct chan *chan = data;
 	struct line *line = chan->line;
+	struct tty_struct *tty;
+
+	if (line) {
+		tty = tty_port_tty_get(&line->port);
+		chan_interrupt(&line->chan_list, &line->task, tty, irq);
+		tty_kref_put(tty);
+	}

-	if (line)
-		chan_interrupt(&line->chan_list, &line->task, line->tty, irq);
 	return IRQ_HANDLED;
 }

 static void line_timer_cb(struct work_struct *work)
 {
 	struct line *line = container_of(work, struct line, task.work);
+	struct tty_struct *tty;

-	if (!line->throttled)
-		chan_interrupt(&line->chan_list, &line->task, line->tty,
+	if (!line->throttled) {
+		tty = tty_port_tty_get(&line->port);
+		chan_interrupt(&line->chan_list, &line->task, tty,
 			       line->driver->read_irq);
+
+		tty_kref_put(tty);
+	}
 }

 /*
@@ -343,7 +353,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
 {
 	struct chan *chan = data;
 	struct line *line = chan->line;
-	struct tty_struct *tty = line->tty;
+	struct tty_struct *tty = tty_port_tty_get(&line->port);
 	int err;

 	/*
@@ -354,6 +364,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
 	spin_lock(&line->lock);
 	err = flush_buffer(line);
 	if (err == 0) {
+		tty_kref_put(tty);
 		return IRQ_NONE;
 	} else if (err < 0) {
 		line->head = line->buffer;
@@ -365,9 +376,36 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
 		return IRQ_NONE;

 	tty_wakeup(tty);
+	tty_kref_put(tty);
 	return IRQ_HANDLED;
 }

+static int line_activate(struct tty_port *port, struct tty_struct *tty)
+{
+	int ret;
+	struct line *line = tty->driver_data;
+
+	ret = enable_chan(line);
+	if (ret)
+		return ret;
+
+	INIT_DELAYED_WORK(&line->task, line_timer_cb);
+
+	if (!line->sigio) {
+		chan_enable_winch(&line->chan_list, tty);
+		line->sigio = 1;
+	}
+
+	chan_window_size(&line->chan_list, &tty->winsize.ws_row,
+			 &tty->winsize.ws_col);
+
+	return 0;
+}
+
+static const struct tty_port_operations line_port_ops = {
+	.activate = line_activate,
+};
+
 int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
 {
 	const struct line_driver *driver = line->driver;
@@ -404,81 +442,49 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data)
  * first open or last close.  Otherwise, open and close just return.
  */

-int line_open(struct line *lines, struct tty_struct *tty)
+int line_open(struct tty_struct *tty, struct file *filp)
 {
-	struct line *line = &lines[tty->index];
-	int err = -ENODEV;
+	struct line *line = tty->driver_data;
+	return tty_port_open(&line->port, tty, filp);
+}

-	spin_lock(&line->count_lock);
-	if (!line->valid)
-		goto out_unlock;
+int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line)
+{
+	int ret = tty_init_termios(tty);
+	if (ret)
+		return ret;

-	err = 0;
-	if (line->count++)
-		goto out_unlock;
+	tty_driver_kref_get(driver);
+	tty->count++;
+	driver->ttys[tty->index] = tty;

-	BUG_ON(tty->driver_data);
 	tty->driver_data = line;
-	line->tty = tty;
-
-	spin_unlock(&line->count_lock);
-	err = enable_chan(line);
-	if (err) /* line_close() will be called by our caller */
-		return err;
-
-	INIT_DELAYED_WORK(&line->task, line_timer_cb);
-
-	if (!line->sigio) {
-		chan_enable_winch(&line->chan_list, tty);
-		line->sigio = 1;
-	}
-
-	chan_window_size(&line->chan_list, &tty->winsize.ws_row,
-			 &tty->winsize.ws_col);

 	return 0;
-
-out_unlock:
-	spin_unlock(&line->count_lock);
-	return err;
 }

 static void unregister_winch(struct tty_struct *tty);

-void line_close(struct tty_struct *tty, struct file * filp)
+void line_cleanup(struct tty_struct *tty)
 {
 	struct line *line = tty->driver_data;

-	/*
-	 * If line_open fails (and tty->driver_data is never set),
-	 * tty_open will call line_close.  So just return in this case.
-	 */
-	if (line == NULL)
-		return;
-
-	/* We ignore the error anyway! */
-	flush_buffer(line);
-
-	spin_lock(&line->count_lock);
-	BUG_ON(!line->valid);
-
-	if (--line->count)
-		goto out_unlock;
-
-	line->tty = NULL;
-	tty->driver_data = NULL;
-
-	spin_unlock(&line->count_lock);
-
 	if (line->sigio) {
 		unregister_winch(tty);
 		line->sigio = 0;
 	}
+}

-	return;
+void line_close(struct tty_struct *tty, struct file * filp)
+{
+	struct line *line = tty->driver_data;
+	tty_port_close(&line->port, tty, filp);
+}

-out_unlock:
-	spin_unlock(&line->count_lock);
+void line_hangup(struct tty_struct *tty)
+{
+	struct line *line = tty->driver_data;
+	tty_port_hangup(&line->port);
 }

 void close_lines(struct line *lines, int nlines)
@@ -495,13 +501,6 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
 	struct line *line = &lines[n];
 	int err = -EINVAL;

-	spin_lock(&line->count_lock);
-
-	if (line->count) {
-		*error_out = "Device is already open";
-		goto out;
-	}
-
 	if (line->init_pri <= init_prio) {
 		line->init_pri = init_prio;
 		if (!strcmp(init, "none"))
@@ -512,8 +511,7 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio,
 		}
 	}
 	err = 0;
-out:
-	spin_unlock(&line->count_lock);
+
 	return err;
 }

@@ -598,6 +596,7 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,
 	struct line *line;
 	char *end;
 	int dev, n = 0;
+	struct tty_struct *tty;

 	dev = simple_strtoul(name, &end, 0);
 	if ((*end != '\0') || (end == name)) {
@@ -612,13 +611,15 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str,

 	line = &lines[dev];

-	spin_lock(&line->count_lock);
+	tty = tty_port_tty_get(&line->port);
+
 	if (!line->valid)
 		CONFIG_CHUNK(str, size, n, "none", 1);
-	else if (line->tty == NULL)
+	else if (tty == NULL)
 		CONFIG_CHUNK(str, size, n, line->init_str, 1);
 	else n = chan_config_string(&line->chan_list, str, size, error_out);
-	spin_unlock(&line->count_lock);
+
+	tty_kref_put(tty);

 	return n;
 }
@@ -678,8 +679,8 @@ struct tty_driver *register_lines(struct line_driver *line_driver,
 	}

 	for(i = 0; i < nlines; i++) {
-		if (!lines[i].valid)
-			tty_unregister_device(driver, i);
+		tty_port_init(&lines[i].port);
+		lines[i].port.ops = &line_port_ops;
 	}

 	mconsole_register_dev(&line_driver->mc);
@@ -805,7 +806,6 @@ void register_winch_irq(int fd, int tty_fd, int pid, struct tty_struct *tty,
 				   .pid  	= pid,
 				   .tty 	= tty,
 				   .stack	= stack });
-
 	if (um_request_irq(WINCH_IRQ, fd, IRQ_READ, winch_interrupt,
 			   IRQF_DISABLED | IRQF_SHARED | IRQF_SAMPLE_RANDOM,
 			   "winch", winch) < 0) {
diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h
index 63df3ca..54adfc6 100644
--- a/arch/um/drivers/line.h
+++ b/arch/um/drivers/line.h
@@ -31,9 +31,8 @@ struct line_driver {
 };

 struct line {
-	struct tty_struct *tty;
-	spinlock_t count_lock;
-	unsigned long count;
+	struct tty_port port;
+
 	int valid;

 	char *init_str;
@@ -59,15 +58,17 @@ struct line {
 };

 #define LINE_INIT(str, d) \
-	{ .count_lock =	__SPIN_LOCK_UNLOCKED((str).count_lock), \
-	  .init_str =	str,	\
+	{ .init_str =	str,	\
 	  .init_pri =	INIT_STATIC, \
 	  .valid =	1, \
 	  .lock =	__SPIN_LOCK_UNLOCKED((str).lock), \
 	  .driver =	d }

 extern void line_close(struct tty_struct *tty, struct file * filp);
-extern int line_open(struct line *lines, struct tty_struct *tty);
+extern int line_open(struct tty_struct *tty, struct file *filp);
+extern int line_install(struct tty_driver *driver, struct tty_struct *tty, struct line *line);
+extern void line_cleanup(struct tty_struct *tty);
+extern void line_hangup(struct tty_struct *tty);
 extern int line_setup(struct line *lines, unsigned int sizeof_lines,
 		      char *init, char **error_out);
 extern int line_write(struct tty_struct *tty, const unsigned char *buf,
diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
index 9d8c20a..89fdecb 100644
--- a/arch/um/drivers/ssl.c
+++ b/arch/um/drivers/ssl.c
@@ -92,17 +92,6 @@ static int ssl_remove(int n, char **error_out)
 			   error_out);
 }

-static int ssl_open(struct tty_struct *tty, struct file *filp)
-{
-	int err = line_open(serial_lines, tty);
-
-	if (err)
-		printk(KERN_ERR "Failed to open serial line %d, err = %d\n",
-		       tty->index, err);
-
-	return err;
-}
-
 #if 0
 static void ssl_flush_buffer(struct tty_struct *tty)
 {
@@ -124,8 +113,13 @@ void ssl_hangup(struct tty_struct *tty)
 }
 #endif

+static int ssl_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	return line_install(driver, tty, &serial_lines[tty->index]);
+}
+
 static const struct tty_operations ssl_ops = {
-	.open 	 		= ssl_open,
+	.open 	 		= line_open,
 	.close 	 		= line_close,
 	.write 	 		= line_write,
 	.put_char 		= line_put_char,
@@ -134,9 +128,12 @@ static const struct tty_operations ssl_ops = {
 	.flush_buffer 		= line_flush_buffer,
 	.flush_chars 		= line_flush_chars,
 	.set_termios 		= line_set_termios,
-	.ioctl 	 		= line_ioctl,
+	.ioctl			= line_ioctl,
 	.throttle 		= line_throttle,
 	.unthrottle 		= line_unthrottle,
+	.install		= ssl_install,
+	.cleanup		= line_cleanup,
+	.hangup			= line_hangup,
 #if 0
 	.stop 	 		= ssl_stop,
 	.start 	 		= ssl_start,
diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
index 088776f..4c78c78 100644
--- a/arch/um/drivers/stdio_console.c
+++ b/arch/um/drivers/stdio_console.c
@@ -97,7 +97,7 @@ static int con_remove(int n, char **error_out)

 static int con_open(struct tty_struct *tty, struct file *filp)
 {
-	int err = line_open(vts, tty);
+	int err = line_open(tty, filp);
 	if (err)
 		printk(KERN_ERR "Failed to open console %d, err = %d\n",
 		       tty->index, err);
@@ -105,11 +105,17 @@ static int con_open(struct tty_struct *tty, struct file *filp)
 	return err;
 }

+static int con_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	return line_install(driver, tty, &vts[tty->index]);
+}
+
 /* Set in an initcall, checked in an exitcall */
 static int con_init_done = 0;

 static const struct tty_operations console_ops = {
 	.open 	 		= con_open,
+	.install		= con_install,
 	.close 	 		= line_close,
 	.write 	 		= line_write,
 	.put_char 		= line_put_char,
@@ -121,6 +127,8 @@ static const struct tty_operations console_ops = {
 	.ioctl 	 		= line_ioctl,
 	.throttle 		= line_throttle,
 	.unthrottle 		= line_unthrottle,
+	.cleanup		= line_cleanup,
+	.hangup			= line_hangup,
 };

 static void uml_console_write(struct console *console, const char *string,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: TTY: tty_port questions

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2012 11:26 PM, Richard Weinberger wrote:
> Hi!
> 
> While moving UML's console driver to tty_port some strange things
> happened. So, I have a few questions. :-)
> 
> The original driver did not implement tty_operations->hangup(). If
> I implement it and call tty_port_hangup(), as Alan suggested, the
> login fails on all TTYs except tty0. It fails because the opened
> TTY returns EIO upon read()/write() after /bin/login called
> vhangup().
> 
> The call chain is: vhangup() -> tty_vhangup_self() -> tty_vhangup()
> -> __tty_hangup()
> 
> Within __tty_hangup() something happens that I don't fully
> understand:
> 
> if (cons_filp) { if (tty->ops->close) for (n = 0; n < closecount;
> n++) tty->ops->close(tty, cons_filp); } else if (tty->ops->hangup) 
> (tty->ops->hangup)(tty);
> 
> Login on tty0 works because cons_filp is not NULL and
> tty->ops->close() is called. On the other hand login fails on every
> other TTY because cons_filp remains NULL and the TTY hangs up.
> 
> Is there something missing in my hangup function?
> 
> If I omit tty_operations->hangup() and leave it, like the old
> driver, NULL non-tty0 TTYs cannot be opened. (getty terminates
> immediately because it cannot open any TTY.) open() retuns -EIO
> because the TTY_CLOSING is set in tty->flags.
> 
> How can this be?

Hmm, it looks like some process is sitting on a TTY which was hung.
And the system offers this hung TTY to others on further opens. Could
you check that there is no process with open TTY after the vhangup?

regards,
- -- 
js
suse labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.18 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPW9sIAAoJEL0lsQQGtHBJlgMP/1Raic2yKfoGxQto7i4wcs6q
B++AgVPfYktAdODL5hem4n6Unf5n2QoLqs7ITGaH9UkN6dWWo9el0RO8zIUl3wpg
OFkyD2CwBpa3HZ2VmIDTtEatL64Xh+lE3PZyJv+ACLN8qJYCOy+9JpuqPltYg64j
fihMX9bvOkwhnRHS/G78S3M4RnqjGcR8V7vIdm8/FfnbHlIzRFUERk9sa0d1V7my
782nTTPVFsczLHL3wWIA1txKnF98LU6Dab0/EqTzen23Z1bD30hhPsahdGIj04YW
IoM1v5ps3aie1QTyAHbi3dR/ncFI3G6ux4YIJJEAnNprnjxy4FGSqfen5sgmtiZm
ugengHeajJlqBYWPzjc7XNENC9rJUCBAMyPWGA3LEc/jfWp3wdkL1/kQ6znYKD5V
hAEWs8KABCdFhOK2OPx47QMs4kEENVTMZniYL/U7PCiwYjmCXI7PUUHwi/qrtk+F
rU9e0bYpSFYOvDqQQiemZHpNCtPz8TQieEV/LvMJ4f/bKrxZQdVadoFf0yv7iDE5
9v3SnrRMm/BxjSJhim8UOgjNj+oX8LYh7vCqbbq+dxxlNV1zIzNPoCVnMHJpazio
Z6xJAu2w4GsIB99IzrUl8/q2EmHtPktRWL1JNqlr8R5+g2SeIMz8ha4o2ymDE6CE
NGGVl9p5SE7r0kkttiNi
=Mhvv
-----END PGP SIGNATURE-----

Re: TTY: tty_port questions

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

Am 10.03.2012 23:51, schrieb Jiri Slaby:
> On 03/10/2012 11:26 PM, Richard Weinberger wrote:
>> Hi!
> 
>> While moving UML's console driver to tty_port some strange things
>> happened. So, I have a few questions. :-)
> 
>> The original driver did not implement tty_operations->hangup(). If
>> I implement it and call tty_port_hangup(), as Alan suggested, the
>> login fails on all TTYs except tty0. It fails because the opened
>> TTY returns EIO upon read()/write() after /bin/login called
>> vhangup().
> 
>> The call chain is: vhangup() -> tty_vhangup_self() -> tty_vhangup()
>> -> __tty_hangup()
> 
>> Within __tty_hangup() something happens that I don't fully
>> understand:
> 
>> if (cons_filp) { if (tty->ops->close) for (n = 0; n < closecount;
>> n++) tty->ops->close(tty, cons_filp); } else if (tty->ops->hangup) 
>> (tty->ops->hangup)(tty);
> 
>> Login on tty0 works because cons_filp is not NULL and
>> tty->ops->close() is called. On the other hand login fails on every
>> other TTY because cons_filp remains NULL and the TTY hangs up.
> 
>> Is there something missing in my hangup function?
> 
>> If I omit tty_operations->hangup() and leave it, like the old
>> driver, NULL non-tty0 TTYs cannot be opened. (getty terminates
>> immediately because it cannot open any TTY.) open() retuns -EIO
>> because the TTY_CLOSING is set in tty->flags.
> 
>> How can this be?
> 
> Hmm, it looks like some process is sitting on a TTY which was hung.
> And the system offers this hung TTY to others on further opens. Could
> you check that there is no process with open TTY after the vhangup?
> 

"lsof | grep tty" does not show anything else than tty0. :-\

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: TTY: tty_port questions

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

Am 11.03.2012 00:21, schrieb Richard Weinberger:
> Am 10.03.2012 23:51, schrieb Jiri Slaby:
>> On 03/10/2012 11:26 PM, Richard Weinberger wrote:
>>> Hi!
>>
>>> While moving UML's console driver to tty_port some strange things
>>> happened. So, I have a few questions. :-)
>>
>>> The original driver did not implement tty_operations->hangup(). If
>>> I implement it and call tty_port_hangup(), as Alan suggested, the
>>> login fails on all TTYs except tty0. It fails because the opened
>>> TTY returns EIO upon read()/write() after /bin/login called
>>> vhangup().
>>
>>> The call chain is: vhangup() -> tty_vhangup_self() -> tty_vhangup()
>>> -> __tty_hangup()
>>
>>> Within __tty_hangup() something happens that I don't fully
>>> understand:
>>
>>> if (cons_filp) { if (tty->ops->close) for (n = 0; n < closecount;
>>> n++) tty->ops->close(tty, cons_filp); } else if (tty->ops->hangup) 
>>> (tty->ops->hangup)(tty);
>>
>>> Login on tty0 works because cons_filp is not NULL and
>>> tty->ops->close() is called. On the other hand login fails on every
>>> other TTY because cons_filp remains NULL and the TTY hangs up.
>>
>>> Is there something missing in my hangup function?
>>
>>> If I omit tty_operations->hangup() and leave it, like the old
>>> driver, NULL non-tty0 TTYs cannot be opened. (getty terminates
>>> immediately because it cannot open any TTY.) open() retuns -EIO
>>> because the TTY_CLOSING is set in tty->flags.
>>
>>> How can this be?
>>
>> Hmm, it looks like some process is sitting on a TTY which was hung.
>> And the system offers this hung TTY to others on further opens. Could
>> you check that there is no process with open TTY after the vhangup?
>>
> 
> "lsof | grep tty" does not show anything else than tty0. :-\
> 

BTW: When I start the kernel with /bin/sh as init, opening and writing to any TTY works fine.
It looks like Fedora 16's userspace does something that makes the TTYs unhappy.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: TTY: tty_port questions

[Richard Weinberger]

On 11.03.2012 12:01, Richard Weinberger wrote:
> Am 11.03.2012 00:21, schrieb Richard Weinberger:
>> Am 10.03.2012 23:51, schrieb Jiri Slaby:
>>> On 03/10/2012 11:26 PM, Richard Weinberger wrote:
>>>> Hi!
>>>
>>>> While moving UML's console driver to tty_port some strange things
>>>> happened. So, I have a few questions. :-)
>>>
>>>> The original driver did not implement tty_operations->hangup(). If
>>>> I implement it and call tty_port_hangup(), as Alan suggested, the
>>>> login fails on all TTYs except tty0. It fails because the opened
>>>> TTY returns EIO upon read()/write() after /bin/login called
>>>> vhangup().
>>>
>>>> The call chain is: vhangup() ->  tty_vhangup_self() ->  tty_vhangup()
>>>> ->  __tty_hangup()
>>>
>>>> Within __tty_hangup() something happens that I don't fully
>>>> understand:
>>>
>>>> if (cons_filp) { if (tty->ops->close) for (n = 0; n<  closecount;
>>>> n++) tty->ops->close(tty, cons_filp); } else if (tty->ops->hangup)
>>>> (tty->ops->hangup)(tty);
>>>
>>>> Login on tty0 works because cons_filp is not NULL and
>>>> tty->ops->close() is called. On the other hand login fails on every
>>>> other TTY because cons_filp remains NULL and the TTY hangs up.
>>>
>>>> Is there something missing in my hangup function?
>>>
>>>> If I omit tty_operations->hangup() and leave it, like the old
>>>> driver, NULL non-tty0 TTYs cannot be opened. (getty terminates
>>>> immediately because it cannot open any TTY.) open() retuns -EIO
>>>> because the TTY_CLOSING is set in tty->flags.
>>>
>>>> How can this be?
>>>
>>> Hmm, it looks like some process is sitting on a TTY which was hung.
>>> And the system offers this hung TTY to others on further opens. Could
>>> you check that there is no process with open TTY after the vhangup?
>>>
>>
>> "lsof | grep tty" does not show anything else than tty0. :-\
>>
>
> BTW: When I start the kernel with /bin/sh as init, opening and writing to any TTY works fine.
> It looks like Fedora 16's userspace does something that makes the TTYs unhappy.
>
> Thanks,
> //richard

Another question, what is the purpose of tty_port->console?
It seems to have no user.

Is tty_port really the right thing for a console driver like in the UML 
case?

Thanks,
//richard

Re: TTY: tty_port questions

[Alan Cox]

> Another question, what is the purpose of tty_port->console?
> It seems to have no user.

So the tty_port layer can in future know about whether it is a console
without a tty necessarily being present or referenceable.

> Is tty_port really the right thing for a console driver like in the UML 
> case?

It will be. In order to fix the tty locking mess we need to shove a lot
of stuff whose lifetime is the lifetime of the physical port somewhere
else - the tty_port is that structure.

Alan

Re: TTY: tty_port questions

[Richard Weinberger]

On 12.03.2012 11:53, Alan Cox wrote:
>> Another question, what is the purpose of tty_port->console?
>> It seems to have no user.
>
> So the tty_port layer can in future know about whether it is a console
> without a tty necessarily being present or referenceable.
>
>> Is tty_port really the right thing for a console driver like in the UML
>> case?
>
> It will be. In order to fix the tty locking mess we need to shove a lot
> of stuff whose lifetime is the lifetime of the physical port somewhere
> else - the tty_port is that structure.
>

"It will be" in terms of "not now"? ;-)

Thanks,
//richard

Re: TTY: tty_port questions

[Alan Cox]

On Mon, 12 Mar 2012 12:15:32 +0100
Richard Weinberger <richard@nod.at> wrote:

> On 12.03.2012 11:53, Alan Cox wrote:
> >> Another question, what is the purpose of tty_port->console?
> >> It seems to have no user.
> >
> > So the tty_port layer can in future know about whether it is a console
> > without a tty necessarily being present or referenceable.
> >
> >> Is tty_port really the right thing for a console driver like in the UML
> >> case?
> >
> > It will be. In order to fix the tty locking mess we need to shove a lot
> > of stuff whose lifetime is the lifetime of the physical port somewhere
> > else - the tty_port is that structure.
> >
> 
> "It will be" in terms of "not now"? ;-)

As in, it's the very next step on.

Alan

Re: TTY: tty_port questions

[Al Viro]

On Mon, Mar 12, 2012 at 11:48:32AM +0000, Alan Cox wrote:

> > > It will be. In order to fix the tty locking mess we need to shove a lot
> > > of stuff whose lifetime is the lifetime of the physical port somewhere
> > > else - the tty_port is that structure.
> > >
> > 
> > "It will be" in terms of "not now"? ;-)
> 
> As in, it's the very next step on.

FWIW, uml console in default config is basically "start xterm for each VC".
What do you suggest to do on vhangup() on one of those?

Re: TTY: tty_port questions

[Alan Cox]

On Sat, 24 Mar 2012 23:20:01 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Mon, Mar 12, 2012 at 11:48:32AM +0000, Alan Cox wrote:
> 
> > > > It will be. In order to fix the tty locking mess we need to shove a lot
> > > > of stuff whose lifetime is the lifetime of the physical port somewhere
> > > > else - the tty_port is that structure.
> > > >
> > > 
> > > "It will be" in terms of "not now"? ;-)
> > 
> > As in, it's the very next step on.
> 
> FWIW, uml console in default config is basically "start xterm for each VC".
> What do you suggest to do on vhangup() on one of those?

What posix says must happen. Which is that the running processes get a
hangup. So a vhangup() would ensure there were no old apps on the UML
guess talking to the xterm (eg stealing login credentials, or abusing
TIOCSTI etc).

The fact it's an xterm isn't really relevant. That's just the physical
interface and vhangup is about breaking the logical link. The xterm would
continue, no reason for it to do otherwise I can see ?

Alan

Re: TTY: tty_port questions

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

Am 25.03.2012 16:51, schrieb Alan Cox:
> On Sat, 24 Mar 2012 23:20:01 +0000
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
>> On Mon, Mar 12, 2012 at 11:48:32AM +0000, Alan Cox wrote:
>>
>>>>> It will be. In order to fix the tty locking mess we need to shove a lot
>>>>> of stuff whose lifetime is the lifetime of the physical port somewhere
>>>>> else - the tty_port is that structure.
>>>>>
>>>>
>>>> "It will be" in terms of "not now"? ;-)
>>>
>>> As in, it's the very next step on.
>>
>> FWIW, uml console in default config is basically "start xterm for each VC".
>> What do you suggest to do on vhangup() on one of those?
> 
> What posix says must happen. Which is that the running processes get a
> hangup. So a vhangup() would ensure there were no old apps on the UML
> guess talking to the xterm (eg stealing login credentials, or abusing
> TIOCSTI etc).

Looks like Debian's /bin/login is violating POSIX. AFACT it does not
call vhangup() at all.

> The fact it's an xterm isn't really relevant. That's just the physical
> interface and vhangup is about breaking the logical link. The xterm would
> continue, no reason for it to do otherwise I can see ?
> 

As I wrote in my very first mail, if I implement tty_operations->hangup()
a vhangup() closes the current TTY and the shiny new login shell dies because
read/write() returns EIO.

So, the question is whether tty_port is not suitable for consoles or my driver
(see first mail in thread) is broken.

Thanks,
//richard



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: TTY: tty_port questions

[Al Viro]

On Sun, Mar 25, 2012 at 05:14:37PM +0200, Richard Weinberger wrote:
> >> FWIW, uml console in default config is basically "start xterm for each VC".
> >> What do you suggest to do on vhangup() on one of those?
> > 
> > What posix says must happen. Which is that the running processes get a
> > hangup. So a vhangup() would ensure there were no old apps on the UML
> > guess talking to the xterm (eg stealing login credentials, or abusing
> > TIOCSTI etc).

IIRC, vhangup(2) is Linux-specific, so I would be surprised if POSIX had
anything on it...

> Looks like Debian's /bin/login is violating POSIX. AFACT it does not
> call vhangup() at all.

... and Alan said nothing about /bin/login being required to call it,
be it by POSIX or by anything else.

login(1) from util-linux does vhangup(); login(1) from shadow doesn't.

> > The fact it's an xterm isn't really relevant. That's just the physical
> > interface and vhangup is about breaking the logical link. The xterm would
> > continue, no reason for it to do otherwise I can see ?
> > 
> 
> As I wrote in my very first mail, if I implement tty_operations->hangup()
> a vhangup() closes the current TTY and the shiny new login shell dies because
> read/write() returns EIO.

Hm?  If login(1) does vhangup(), it has to reopen tty - and util-linux
one does just that.  open() *after* vhangup() is supposed to work - it's
pre-existing openers that get screwed.  Which is why the sequence is
	fchown/fchmod to prevent new unpriveleged openers
	ignore SIGHUP
	vhangup to bugger all old openers (including ourselves - at that
point our stdin becomes unusable)
	open tty (BTW, I'd probably open /proc/self/0 instead of using
ttyname(3) result as util-linux is doing)
	replace stdin/stdout/stderr with it
And yes, it's far too convoluted...

	What makes me more concerned about the tty_port model is the
mechanism uml got for reconfiguring its ttys.  There's a control channel,
independent from the regular tty driver.  You can run mconsole(1) on host
to connect to it and issue commands; e.g. "config con2=pts" will remove
whatever you have as /dev/tty2 and associate it with pty pair on host.

The thing is, we don't want to do that when port is in use.  And we definitely
don't want somebody to open the damn thing when it's halfway through getting
set up.  I don't see any natural way to do that exclusion with tty_port -
port->{count,block_open} is protected only by a spinlock and port setup
we need to do is blocking...

Re: TTY: tty_port questions

[Al Viro]

How is tty_port supposed to work wrt hotplug?  I.e. are those guys (OK,
the structures they are embedded into) supposed to live as long as
tty_driver lives?  AFAICS, for serial we have an extra layer atop of
those guys (uart_port) and that's where removals seem to act, but there
seems to be more to it...

Suppose we handle uml reconfig requests as port removal + port addition;
what's needed to make sure that port is out of use and we can play with
it without stepping on anyone's toes?  Something along the lines of what
uart_remove_one_port() is doing?  I.e. tty_unregister_device() + tty_vhangup()?
But serial_core seems to be open-coding tty_port_open() for some reason
and _there_ we have port->count updates under port->mutex, so the
situation might be different...

Is there any FMtoR/thread/search terms that would give the description of
the situation?

Re: TTY: tty_port questions

[Alan Cox]

On Sun, 25 Mar 2012 19:31:14 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> How is tty_port supposed to work wrt hotplug?  I.e. are those guys (OK,
> the structures they are embedded into) supposed to live as long as
> tty_driver lives?  AFAICS, for serial we have an extra layer atop of
> those guys (uart_port) and that's where removals seem to act, but there
> seems to be more to it...

Serial had pre-exising gunge not all of which has been cleaned, just as
it still has its own buffers that want to be using kfifo.

Best examples are probably USB serial and neatest may well be the sdio
serial card driver.

> Suppose we handle uml reconfig requests as port removal + port addition;
> what's needed to make sure that port is out of use and we can play with
> it without stepping on anyone's toes?  Something along the lines of what
> uart_remove_one_port() is doing?  I.e. tty_unregister_device() + tty_vhangup()?
> But serial_core seems to be open-coding tty_port_open() for some reason
> and _there_ we have port->count updates under port->mutex, so the
> situation might be different...

If you are creating/removing physical ports (or fake physical ports) you
probably need to be refcounting them too.

Alan

Re: TTY: tty_port questions

[Alan Cox]

On Sun, 25 Mar 2012 18:20:18 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sun, Mar 25, 2012 at 05:14:37PM +0200, Richard Weinberger wrote:
> > >> FWIW, uml console in default config is basically "start xterm for each VC".
> > >> What do you suggest to do on vhangup() on one of those?
> > > 
> > > What posix says must happen. Which is that the running processes get a
> > > hangup. So a vhangup() would ensure there were no old apps on the UML
> > > guess talking to the xterm (eg stealing login credentials, or abusing
> > > TIOCSTI etc).
> 
> IIRC, vhangup(2) is Linux-specific, so I would be surprised if POSIX had
> anything on it...

vhangup causes a carrier drop event equivalent. The rest of the behavior
is POSIX/SUSv3.

> login(1) from util-linux does vhangup(); login(1) from shadow doesn't.

Shadow assumes the getty cleans the channel I believe.

> The thing is, we don't want to do that when port is in use.  And we definitely
> don't want somebody to open the damn thing when it's halfway through getting
> set up.  I don't see any natural way to do that exclusion with tty_port -
> port->{count,block_open} is protected only by a spinlock and port setup
> we need to do is blocking...

How does this differ from a hardware hotplug ?

Alan

Re: TTY: tty_port questions

[Al Viro]

On Sun, Mar 25, 2012 at 10:06:10PM +0100, Alan Cox wrote:
> Serial had pre-exising gunge not all of which has been cleaned, just as
> it still has its own buffers that want to be using kfifo.
> 
> Best examples are probably USB serial and neatest may well be the sdio
> serial card driver.

Umm...  So we have
	* tty_port created for each physical device, index assigned to it
and tty_register_device() done to create sysfs junk/kick udev/etc.
	* TTY layer allocates tty on demand (open() time) and feeds them
to ->install(), which is where we associate the suckers with tty_port,
grabbing a reference to the latter and shoving it into ->driver_data (OK,
it or that to struct it's embedded into - all the same)
	* ->cleanup() is called when TTY layer decided to free tty; drops
a reference to port.  Nobody will see that tty_struct anymore.
	* port outlives tty; over the lifetime of the latter associated
port remains the same.
	* ->open()/->close()/->hungup() simply call tty_port_...()
[BTW, is there any reason why you do not set ->driver_data to port and
use container_of() in the places that want other parts of containing
struct?  That way you wouldn't need those wrappers at all and while
->write() is certainly called more often, there's no extra cost - compiler
is able to figure out that local variable remains equal to argument-constant
and turn accesses relative to it into ones relative to argument]
	* guts of opening the damn thing go into ->activate(), guts of
stopping - into ->shutdown().
	* removal does tty_unregister_device() + prevents ->install() from
finding it + (under port->mutex) does tty_hangup() on associated tty (if any).
BTW, I really don't like the look of that place - tty_hangup() is async
(otherwise it'd deadlock instantly), so what the devil is protecting tty
from being freed before __tty_hangup() is done with it?  And when should
the actual channel be killed?  It appears to be done right after tty_hangup()
returns, but since the actual work done by it is async... why is that safe?
	* ->activate() plays strange games with TTY_IO_ERROR; why do we
bother, seeing that it's under port->mutex and anybody trying to open the
same tty will wait anyway?

Re: TTY: tty_port questions

[Alan Cox]

> 	* TTY layer allocates tty on demand (open() time) and feeds them
> to ->install(), which is where we associate the suckers with tty_port,
> grabbing a reference to the latter and shoving it into ->driver_data (OK,
> it or that to struct it's embedded into - all the same)

Yep - actually we want to get a tty->port pointer so we can clean up some
of the indirection and allow the core code to get at the port directly

> 	* ->open()/->close()/->hungup() simply call tty_port_...()
> [BTW, is there any reason why you do not set ->driver_data to port and
> use container_of() in the places that want other parts of containing

See aboe comment.. that's also the way I've been thinking.

> 	* removal does tty_unregister_device() + prevents ->install() from
> finding it + (under port->mutex) does tty_hangup() on associated tty (if any).
> BTW, I really don't like the look of that place - tty_hangup() is async
> (otherwise it'd deadlock instantly), so what the devil is protecting tty
> from being freed before __tty_hangup() is done with it?  And when should

Nothing. However the locking is unfixable in this area until we've
removed the big tty mutex. It's a known problem. I've killed the big tty
mutex in the console layer this -next so we are inching in the right
direction. Once the BTM has gone we can actually fix the unplug race.

> 	* ->activate() plays strange games with TTY_IO_ERROR; why do we
> bother, seeing that it's under port->mutex and anybody trying to open the
> same tty will wait anyway?

The historic code used to do this and some of our drivers are not fully
converted over so still expect that pattern of behaviour in a few spots.

Alan