[RFC PATCH 0/5] patches for a network-based hvc backend (s390)

[RFC PATCH 0/5] patches for a network-based hvc backend (s390)

[Hendrik Brueckner]

Hello,

I work on a network-based hvc console backend for s390 that allows
to get "full-screen" terminal access to z/VM guest machines.
The solution consists of a HVC backend that provides the terminal interface;
and a tool to connect to the terminal via a z/VM specific communication
protocol.

The network-based backend differs in a few aspects from the notifier-based
model of the HVC console; because it has to deal with (dis)connects and must
take care of virtual tty hangups. Therefore, I would like to introduce a third
notifier for hangups (see patch 1).

Further I found out that if put_char() returns 0 in particular cases, the hvc
console starts to loop. I tried to address this problem with patch 3 and I hope
that it will work for all backends.

Finally, I would like to add a function that allows to resize the terminal
window of a HVC terminal (patch 4).


Here is an overview about the complete patch series:
 Patch 1 adds a hangup notifier
 Patch 2 adds tty driver flag TTY_DRIVER_RESET_TERMIOS
 Patch 3 fixes a loop if put_char() returns 0
 Patch 4 adds a function to resize the tty window
 Patch 5 removes __devexit of hvc_remove() for use in __(dev)init sections


Any review feedback would be greatly appreciated.
Thank you in advance.

Regards,
Hendrik
-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC PATCH 1/5] hvc_console: Adding hangup notifier

[Hendrik Brueckner]

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

I have added a hangup notifier that can be used by hvc console
backends to handle a tty hangup. The default irq hangup notifier
calls the notifier_del_irq() for compatibility.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c    |    4 ++--
 drivers/char/hvc_console.h    |    4 +++-
 drivers/char/hvc_irq.c        |    5 +++++
 drivers/char/hvc_iseries.c    |    1 +
 drivers/char/hvc_vio.c        |    1 +
 drivers/char/hvc_xen.c        |    1 +
 drivers/char/virtio_console.c |    1 +
 7 files changed, 14 insertions(+), 3 deletions(-)

--- a/drivers/char/hvc_console.c	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_console.c	2008-10-09 10:30:13.000000000 +0200
@@ -416,8 +416,8 @@ static void hvc_hangup(struct tty_struct
 	hp->n_outbuf = 0;
 	hp->tty = NULL;
 
-	if (hp->ops->notifier_del)
-			hp->ops->notifier_del(hp, hp->data);
+	if (hp->ops->notifier_hangup)
+			hp->ops->notifier_hangup(hp, hp->data);
 
 	spin_unlock_irqrestore(&hp->lock, flags);
 
--- a/drivers/char/hvc_console.h	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_console.h	2008-10-09 10:30:13.000000000 +0200
@@ -65,9 +65,10 @@ struct hv_ops {
 	int (*get_chars)(uint32_t vtermno, char *buf, int count);
 	int (*put_chars)(uint32_t vtermno, const char *buf, int count);
 
-	/* Callbacks for notification. Called in open and close */
+	/* Callbacks for notification. Called in open, close and hangup */
 	int (*notifier_add)(struct hvc_struct *hp, int irq);
 	void (*notifier_del)(struct hvc_struct *hp, int irq);
+	void (*notifier_hangup)(struct hvc_struct *hp, int irq);
 };
 
 /* Register a vterm and a slot index for use as a console (console_init) */
@@ -86,6 +87,7 @@ void hvc_kick(void);
 /* default notifier for irq based notification */
 extern int notifier_add_irq(struct hvc_struct *hp, int data);
 extern void notifier_del_irq(struct hvc_struct *hp, int data);
+extern void notifier_hangup_irq(struct hvc_struct *hp, int data);
 
 
 #if defined(CONFIG_XMON) && defined(CONFIG_SMP)
--- a/drivers/char/hvc_irq.c	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_irq.c	2008-10-09 10:30:13.000000000 +0200
@@ -42,3 +42,8 @@ void notifier_del_irq(struct hvc_struct 
 	free_irq(irq, hp);
 	hp->irq_requested = 0;
 }
+
+void notifier_hangup_irq(struct hvc_struct *hp, int irq)
+{
+	notifier_del_irq(hp, irq);
+}
--- a/drivers/char/hvc_iseries.c	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_iseries.c	2008-10-09 10:30:13.000000000 +0200
@@ -202,6 +202,7 @@ static struct hv_ops hvc_get_put_ops = {
 	.put_chars = put_chars,
 	.notifier_add = notifier_add_irq,
 	.notifier_del = notifier_del_irq,
+	.notifier_hangup = notifier_hangup_irq,
 };
 
 static int __devinit hvc_vio_probe(struct vio_dev *vdev,
--- a/drivers/char/hvc_xen.c	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_xen.c	2008-10-09 10:30:13.000000000 +0200
@@ -102,6 +102,7 @@ static struct hv_ops hvc_ops = {
 	.put_chars = write_console,
 	.notifier_add = notifier_add_irq,
 	.notifier_del = notifier_del_irq,
+	.notifier_hangup = notifier_hangup_irq,
 };
 
 static int __init xen_init(void)
--- a/drivers/char/hvc_vio.c	2008-10-08 16:08:40.000000000 +0200
+++ b/drivers/char/hvc_vio.c	2008-10-09 10:30:13.000000000 +0200
@@ -82,6 +82,7 @@ static struct hv_ops hvc_get_put_ops = {
 	.put_chars = hvc_put_chars,
 	.notifier_add = notifier_add_irq,
 	.notifier_del = notifier_del_irq,
+	.notifier_hangup = notifier_hangup_irq,
 };
 
 static int __devinit hvc_vio_probe(struct vio_dev *vdev,
--- a/drivers/char/virtio_console.c	2008-10-09 10:29:41.000000000 +0200
+++ b/drivers/char/virtio_console.c	2008-10-09 10:30:23.000000000 +0200
@@ -198,6 +198,7 @@ static int __devinit virtcons_probe(stru
 	virtio_cons.put_chars = put_chars;
 	virtio_cons.notifier_add = notifier_add_vio;
 	virtio_cons.notifier_del = notifier_del_vio;
+	virtio_cons.notifier_hangup = notifier_del_vio;
 
 	/* The first argument of hvc_alloc() is the virtual console number, so
 	 * we use zero.  The second argument is the parameter for the

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC PATCH 2/5] hvc_console: Add tty driver flag TTY_DRIVER_RESET_TERMIOS

[Hendrik Brueckner]

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

After a tty hangup() or close() operation, processes might not reset the
termio settings to a sane state. In order to reset the termios to its
default settings the tty driver flag TTY_DRIVER_RESET_TERMIOS has been added.

TTY driver flag description from include/linux/tty_driver.h:
	TTY_DRIVER_RESET_TERMIOS --- requests the tty layer to reset the
	  termios setting when the last process has closed the device.
	  Used for PTY's, in particular.


Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -796,7 +796,7 @@ static int hvc_init(void)
 	drv->minor_start = HVC_MINOR;
 	drv->type = TTY_DRIVER_TYPE_SYSTEM;
 	drv->init_termios = tty_std_termios;
-	drv->flags = TTY_DRIVER_REAL_RAW;
+	drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_RESET_TERMIOS;
 	tty_set_operations(drv, &hvc_ops);
 
 	/* Always start the kthread because there can be hotplug vty adapters

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RFC PATCH 2/5] hvc_console: Add tty driver flag TTY_DRIVER_RESET_TERMIOS

[Alan Cox]

On Tue, 14 Oct 2008 11:12:49 +0200
Hendrik Brueckner <brueckner@linux.vnet.ibm.com> wrote:

> From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> 
> After a tty hangup() or close() operation, processes might not reset the
> termio settings to a sane state.

That is the job of the getty task normally. pty is special as the reissue
of the same pty is done as a new device (with new state).

Setting this on the hvc would parallel the PC vt console behaviour but
differ from most other ports.

Anyway its a policy question for PPC64 so if thats how you want it to work

Acked-by: Alan Cox <alan@redhat.com>

Re: [RFC PATCH 2/5] hvc_console: Add tty driver flag TTY_DRIVER_RESET_TERMIOS

[Hendrik Brueckner]

Hello,

for the sake of completion, here are few more details why I have suggest to
add the TTY_DRIVER_RESET_TERMIOS flag:

On Tue, Oct 14, 2008 at 10:40:25AM +0100, Alan Cox wrote:
> On Tue, 14 Oct 2008 11:12:49 +0200
> Hendrik Brueckner <brueckner@linux.vnet.ibm.com> wrote:
> 
> > After a tty hangup() or close() operation, processes might not reset the
> > termio settings to a sane state.
> That is the job of the getty task normally. pty is special as the reissue
> of the same pty is done as a new device (with new state).

During some testing, I have experienced that the bash alters few termios
settings before showing the bash prompt:
-
ioctl(0, TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(0, TCSETSW, {B38400 opost isig -icanon -echo ...}) = 0
write(2, "root@t6345050:~# ", 17) = 17
-
If the tty gets a hangup, the bash gets terminated but the settings still
remains after init has respawned the getty process.
For my network-based hvc backend, a tty_hangup() is caused by a disconnect.

I looked into the tty_io.c source and found out that the termios settings
are stored in an array of the tty driver struct and they remains unchanged
if a tty device is released and initialized again.
At tty device initialization, the tty_init_termios() set tty->termios to the
tty->driver->termios[tty->index].

The idea is to ensure that when a tty is initialized it has the
default (initial) termio settings; and that is actually done if
TTY_DRIVER_RESET_TERMIOS is set.

Anyhow the other possibility might be to always set the initial termios
when the tty is initialized (see patch below).
But I am not sure if that case is the general behavior of ttys.
Please let me know if there is a reason not to re-initialize the termios
of a new tty.

Thanks.

Best regards
Hendrik

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/tty_io.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/char/tty_io.c	2008-10-16 09:48:00.000000000 +0200
+++ b/drivers/char/tty_io.c	2008-10-16 12:55:52.000000000 +0200
@@ -1243,10 +1243,9 @@ int tty_init_termios(struct tty_struct *
 		tp = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
 		if (tp == NULL)
 			return -ENOMEM;
-		memcpy(tp, &tty->driver->init_termios,
-						sizeof(struct ktermios));
 		tty->driver->termios[idx] = tp;
 	}
+	memcpy(tp, &tty->driver->init_termios, sizeof(struct ktermios));
 	tty->termios = tp;
 	tty->termios_locked = tp + 1;
 
-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RFC PATCH 2/5] hvc_console: Add tty driver flag TTY_DRIVER_RESET_TERMIOS

[Alan Cox]

> For my network-based hvc backend, a tty_hangup() is caused by a disconnect.

If it is network backed then you probably do want
TTY_DRIVER_TERMIOS_RESET. For a normal serial type port getty is supposed
to sort the terminal settings out. So in your case it sounds like
resetting it is the right thing to do.

[RFC PATCH 3/5] hvc_console: Fix loop if put_char() returns 0

[Hendrik Brueckner]

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

If put_char() routine of a hvc console backend returns 0, then the
hvc console starts looping in the following scenarios:

1. hvc_console_print()
	If put_char() returns 0 then the while loop may loop forever.
	I have added the missing check for 0 to throw away console messages.

2. khvcd may loop:
	The thread calls hvc_poll() --> hvc_push()... if there are still
	buffered data then the HVC_POLL_WRITE bit is set and causes the
	khvcd thread to loop (if yield() returns immediately).

	However, instead of looping, the khvcd thread could sleep for
	MIN_TIMEOUT (doing the same as for get_chars()).
	The MIN_TIMEOUT is set if hvc_push() was not able to write
	data to the backend. If data has been written, the timeout is
	set to 0 to immediately re-schedule hvc_poll().

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com> (virtio_console)
Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -161,7 +161,7 @@ static void hvc_console_print(struct con
 			}
 		} else {
 			r = cons_ops[index]->put_chars(vtermnos[index], c, i);
-			if (r < 0) {
+			if (r <= 0) {
 				/* throw away chars on error */
 				i = 0;
 			} else if (r > 0) {
@@ -431,7 +431,7 @@ static void hvc_hangup(struct tty_struct
  * Push buffered characters whether they were just recently buffered or waiting
  * on a blocked hypervisor.  Call this function with hp->lock held.
  */
-static void hvc_push(struct hvc_struct *hp)
+static int hvc_push(struct hvc_struct *hp)
 {
 	int n;
 
@@ -439,7 +439,7 @@ static void hvc_push(struct hvc_struct *
 	if (n <= 0) {
 		if (n == 0) {
 			hp->do_wakeup = 1;
-			return;
+			return 0;
 		}
 		/* throw away output on error; this happens when
 		   there is no session connected to the vterm. */
@@ -450,6 +450,8 @@ static void hvc_push(struct hvc_struct *
 		memmove(hp->outbuf, hp->outbuf + n, hp->n_outbuf);
 	else
 		hp->do_wakeup = 1;
+
+	return n;
 }
 
 static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -538,16 +540,20 @@ int hvc_poll(struct hvc_struct *hp)
 	char buf[N_INBUF] __ALIGNED__;
 	unsigned long flags;
 	int read_total = 0;
+	int written_total = 0;
 
 	spin_lock_irqsave(&hp->lock, flags);
 
 	/* Push pending writes */
 	if (hp->n_outbuf > 0)
-		hvc_push(hp);
+		written_total = hvc_push(hp);
 
 	/* Reschedule us if still some write pending */
-	if (hp->n_outbuf > 0)
+	if (hp->n_outbuf > 0) {
 		poll_mask |= HVC_POLL_WRITE;
+		/* If hvc_push() was not able to write, sleep a few msecs */
+		timeout = (written_total) ? 0 : MIN_TIMEOUT;
+	}
 
 	/* No tty attached, just skip */
 	tty = hp->tty;
@@ -659,10 +665,6 @@ static int khvcd(void *unused)
 			poll_mask |= HVC_POLL_READ;
 		if (hvc_kicked)
 			continue;
-		if (poll_mask & HVC_POLL_WRITE) {
-			yield();
-			continue;
-		}
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (!hvc_kicked) {
 			if (poll_mask == 0)

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC PATCH 4/5] hvc_console: Add tty window resizing

[Hendrik Brueckner]

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

The patch provides the hvc_resize() function to update the terminal
window dimensions (struct winsize) for a specified hvc console.
The function stores the new window size and schedules a function
that finally updates the tty winsize and signals the change to
user space (SIGWINCH).
Because the winsize update must acquire a mutex and might sleep,
the function is scheduled instead of being called from hvc_poll()
or khvcd.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |   61 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/char/hvc_console.h |    6 ++++
 2 files changed, 67 insertions(+)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -494,6 +494,48 @@ static int hvc_write(struct tty_struct *
 	return written;
 }
 
+/**
+ * hvc_set_winsz() - Resize the hvc tty terminal window.
+ * @work:	work structure.
+ *
+ * The routine shall not be called within an atomic context because it
+ * acquires the tty termios mutex and might sleep.
+ *
+ * Locking:	hp->lock, tty->termios_mutex and tty->ctrl_lock
+ */
+static void hvc_set_winsz(struct work_struct *work)
+{
+	struct hvc_struct *hp;
+	struct pid *pgrp;
+	unsigned long hvc_flags;
+	unsigned long ctrl_flags;
+
+	hp = container_of(work, struct hvc_struct, tty_resize);
+	if (!hp || !hp->tty)
+		return;
+
+	mutex_lock(&hp->tty->termios_mutex);
+	spin_lock_irqsave(&hp->lock, hvc_flags);
+
+	if ((hp->ws.ws_row == hp->tty->winsize.ws_row) &&
+	    (hp->ws.ws_col == hp->tty->winsize.ws_col))
+		goto out_mutex_unlock;
+
+	spin_lock_irqsave(&hp->tty->ctrl_lock, ctrl_flags);
+	pgrp = get_pid(hp->tty->pgrp);
+	spin_unlock_irqrestore(&hp->tty->ctrl_lock, ctrl_flags);
+
+	if (pgrp)
+		kill_pgrp(pgrp, SIGWINCH, 1);
+	put_pid(pgrp);
+
+	hp->tty->winsize = hp->ws;
+
+out_mutex_unlock:
+	spin_unlock_irqrestore(&hp->lock, hvc_flags);
+	mutex_unlock(&hp->tty->termios_mutex);
+}
+
 /*
  * This is actually a contract between the driver and the tty layer outlining
  * how much write room the driver can guarantee will be sent OR BUFFERED.  This
@@ -638,6 +680,24 @@ int hvc_poll(struct hvc_struct *hp)
 }
 EXPORT_SYMBOL_GPL(hvc_poll);
 
+/**
+ * hvc_resize() - Update terminal window size information.
+ * @hp:		HVC console pointer
+ * @ws:		Terminal window size structure
+ *
+ * Stores the specified window size information in the hvc structure of @hp.
+ * The function schedule the tty resize update.
+ *
+ * Locking:	Locking free; the function MUST be called holding hp->lock
+ */
+void hvc_resize(struct hvc_struct *hp, struct winsize ws)
+{
+	if ((hp->ws.ws_row != ws.ws_row) || (hp->ws.ws_col != ws.ws_col)) {
+		hp->ws = ws;
+		schedule_work(&hp->tty_resize);
+	}
+}
+
 /*
  * This kthread is either polling or interrupt driven.  This is determined by
  * calling hvc_poll() who determines whether a console adapter support
@@ -720,6 +780,7 @@ struct hvc_struct __devinit *hvc_alloc(u
 
 	kref_init(&hp->kref);
 
+	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
 	spin_lock_init(&hp->lock);
 	spin_lock(&hvc_structs_lock);
 
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -27,6 +27,7 @@
 #ifndef HVC_CONSOLE_H
 #define HVC_CONSOLE_H
 #include <linux/kref.h>
+#include <linux/tty.h>
 
 /*
  * This is the max number of console adapters that can/will be found as
@@ -56,6 +57,8 @@ struct hvc_struct {
 	struct hv_ops *ops;
 	int irq_requested;
 	int data;
+	struct winsize ws;
+	struct work_struct tty_resize;
 	struct list_head next;
 	struct kref kref; /* ref count & hvc_struct lifetime */
 };
@@ -84,6 +87,9 @@ extern int __devexit hvc_remove(struct h
 int hvc_poll(struct hvc_struct *hp);
 void hvc_kick(void);
 
+/* Resize hvc tty terminal window */
+extern void hvc_resize(struct hvc_struct *hp, struct winsize ws);
+
 /* default notifier for irq based notification */
 extern int notifier_add_irq(struct hvc_struct *hp, int data);
 extern void notifier_del_irq(struct hvc_struct *hp, int data);

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RFC PATCH 4/5] hvc_console: Add tty window resizing

[Alan Cox]

> +	hp = container_of(work, struct hvc_struct, tty_resize);
> +	if (!hp || !hp->tty)
> +		return;

What locks hp->tty here, it can go NULL after the test on a hangup it
seems ?

> + * hvc_resize() - Update terminal window size information.
> + * @hp:		HVC console pointer
> + * @ws:		Terminal window size structure

See tty_do_resize() for all of this stuff in the latest git. If you can't
use tty_do_resize from your work queue then please let me know why as I'd
like everyone to be using one abstraction.

We also now have a "resize" operation for devices that want to react to a
resize from TIOCSWINSZ

Re: [RFC PATCH 4/5] hvc_console: Add tty window resizing

[Hendrik Brueckner]

On Tue, Oct 14, 2008 at 10:44:28AM +0100, Alan Cox wrote:
> > +	hp = container_of(work, struct hvc_struct, tty_resize);
> > +	if (!hp || !hp->tty)
> > +		return;
> What locks hp->tty here, it can go NULL after the test on a hangup it
> seems ?
You are right. Thanks.

> See tty_do_resize() for all of this stuff in the latest git. If you can't
> use tty_do_resize from your work queue then please let me know why as I'd
> like everyone to be using one abstraction.
I have looked at it  and I have corrected my patch to use tty_do_resize().
Since tty_do_resize() cannot be called holding the hp spinlock; the code uses
now tty_kref_get/put to keep track of the tty object. I am not sure if the
use of the kref's is correct here, so please let me know if there is a better
solution.

Thanks.

Regards,
Hendrik

[RFC PATCH 4/5 v2] hvc_console: Add tty window resizing using tty_do_resize()

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

The patch provides the hvc_resize() function to update the terminal
window dimensions (struct winsize) for a specified hvc console.
The function stores the new window size and schedules a function
that finally updates the tty winsize and signals the change to
user space (SIGWINCH).
Because the winsize update must acquire a mutex and might sleep,
the function is scheduled instead of being called from hvc_poll()
or khvcd.

This patch uses the tty_do_resize() routine from the tty layer.
A pending resize work is canceled in hvc_close() and hvc_hangup().

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |   58 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/char/hvc_console.h |    6 ++++
 2 files changed, 64 insertions(+)

--- a/drivers/char/hvc_console.c	2008-10-14 16:13:12.000000000 +0200
+++ b/drivers/char/hvc_console.c	2008-10-14 16:43:03.000000000 +0200
@@ -374,6 +374,9 @@ static void hvc_close(struct tty_struct 
 		hp->tty = NULL;
 		spin_unlock_irqrestore(&hp->lock, flags);
 
+		/* cancel pending tty resize work */
+		cancel_work_sync(&hp->tty_resize);
+
 		/*
 		 * Chain calls chars_in_buffer() and returns immediately if
 		 * there is no buffered data otherwise sleeps on a wait queue
@@ -399,6 +402,9 @@ static void hvc_hangup(struct tty_struct
 	if (!hp)
 		return;
 
+	/* cancel pending tty resize work */
+	cancel_work_sync(&hp->tty_resize);
+
 	spin_lock_irqsave(&hp->lock, flags);
 
 	/*
@@ -494,6 +500,39 @@ static int hvc_write(struct tty_struct *
 	return written;
 }
 
+/**
+ * hvc_set_winsz() - Resize the hvc tty terminal window.
+ * @work:	work structure.
+ *
+ * The routine shall not be called within an atomic context because it
+ * might sleep.
+ *
+ * Locking:	hp->lock
+ */
+static void hvc_set_winsz(struct work_struct *work)
+{
+	struct hvc_struct *hp;
+	unsigned long hvc_flags;
+	struct tty_struct *tty;
+	struct winsize ws;
+
+	hp = container_of(work, struct hvc_struct, tty_resize);
+	if (!hp)
+		return;
+
+	spin_lock_irqsave(&hp->lock, hvc_flags);
+	if (!hp->tty) {
+		spin_unlock_irqrestore(&hp->lock, hvc_flags);
+		return;
+	}
+	ws  = hp->ws;
+	tty = tty_kref_get(hp->tty);
+	spin_unlock_irqrestore(&hp->lock, hvc_flags);
+
+	tty_do_resize(tty, tty, &ws);
+	tty_kref_put(tty);
+}
+
 /*
  * This is actually a contract between the driver and the tty layer outlining
  * how much write room the driver can guarantee will be sent OR BUFFERED.  This
@@ -638,6 +677,24 @@ int hvc_poll(struct hvc_struct *hp)
 }
 EXPORT_SYMBOL_GPL(hvc_poll);
 
+/**
+ * hvc_resize() - Update terminal window size information.
+ * @hp:		HVC console pointer
+ * @ws:		Terminal window size structure
+ *
+ * Stores the specified window size information in the hvc structure of @hp.
+ * The function schedule the tty resize update.
+ *
+ * Locking:	Locking free; the function MUST be called holding hp->lock
+ */
+void hvc_resize(struct hvc_struct *hp, struct winsize ws)
+{
+	if ((hp->ws.ws_row != ws.ws_row) || (hp->ws.ws_col != ws.ws_col)) {
+		hp->ws = ws;
+		schedule_work(&hp->tty_resize);
+	}
+}
+
 /*
  * This kthread is either polling or interrupt driven.  This is determined by
  * calling hvc_poll() who determines whether a console adapter support
@@ -720,6 +777,7 @@ struct hvc_struct __devinit *hvc_alloc(u
 
 	kref_init(&hp->kref);
 
+	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
 	spin_lock_init(&hp->lock);
 	spin_lock(&hvc_structs_lock);
 
--- a/drivers/char/hvc_console.h	2008-10-14 16:13:11.000000000 +0200
+++ b/drivers/char/hvc_console.h	2008-10-14 16:13:13.000000000 +0200
@@ -27,6 +27,7 @@
 #ifndef HVC_CONSOLE_H
 #define HVC_CONSOLE_H
 #include <linux/kref.h>
+#include <linux/tty.h>
 
 /*
  * This is the max number of console adapters that can/will be found as
@@ -56,6 +57,8 @@ struct hvc_struct {
 	struct hv_ops *ops;
 	int irq_requested;
 	int data;
+	struct winsize ws;
+	struct work_struct tty_resize;
 	struct list_head next;
 	struct kref kref; /* ref count & hvc_struct lifetime */
 };
@@ -84,6 +87,9 @@ extern int __devexit hvc_remove(struct h
 int hvc_poll(struct hvc_struct *hp);
 void hvc_kick(void);
 
+/* Resize hvc tty terminal window */
+extern void hvc_resize(struct hvc_struct *hp, struct winsize ws);
+
 /* default notifier for irq based notification */
 extern int notifier_add_irq(struct hvc_struct *hp, int data);
 extern void notifier_del_irq(struct hvc_struct *hp, int data);
-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RFC PATCH 4/5] hvc_console: Add tty window resizing

[Alan Cox]

> Since tty_do_resize() cannot be called holding the hp spinlock; the code uses
> now tty_kref_get/put to keep track of the tty object. I am not sure if the
> use of the kref's is correct here, so please let me know if there is a better
> solution.

That looks right to me, hp->tty can go NULL but the tty object itself
will still have a reference even if the asynchronous events kick off late

Re: [RFC PATCH 4/5] hvc_console: Add tty window resizing

[Rusty Russell]

On Tuesday 14 October 2008 20:12:51 Hendrik Brueckner wrote:
> From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
>
> The patch provides the hvc_resize() function to update the terminal
> window dimensions (struct winsize) for a specified hvc console.
> The function stores the new window size and schedules a function
> that finally updates the tty winsize and signals the change to
> user space (SIGWINCH).
> Because the winsize update must acquire a mutex and might sleep,
> the function is scheduled instead of being called from hvc_poll()
> or khvcd.

I want this functionality for lguest, too.  But I don't see anything which 
uses this call yet?   Are we going to use the config_change notifier from 
virtio?  (Which IIRC neither S/390 nor lguest have plumbed in).

Cheers,
Rusty.

Re: [RFC PATCH 4/5] hvc_console: Add tty window resizing

[Christian Borntraeger]

Am Donnerstag, 16. Oktober 2008 schrieb Rusty Russell:
> > The patch provides the hvc_resize() function to update the terminal
> > window dimensions (struct winsize) for a specified hvc console.
> > The function stores the new window size and schedules a function
> > that finally updates the tty winsize and signals the change to
> > user space (SIGWINCH).
> > Because the winsize update must acquire a mutex and might sleep,
> > the function is scheduled instead of being called from hvc_poll()
> > or khvcd.
> 
> I want this functionality for lguest, too.  But I don't see anything which 
> uses this call yet?   Are we going to use the config_change notifier from 
> virtio?  (Which IIRC neither S/390 nor lguest have plumbed in).

I have some prototype patches for virtio_console, but did not find the time to 
finish them.

Christian

[RFC PATCH 5/5] hvc_console: Remove __devexit annotation of hvc_remove()

[Hendrik Brueckner]

From: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

Removed __devexit annotation of hvc_remove() to avoid a section mismatch
if the backend initialization fails and hvc_remove() must be used to
clean up allocated hvc structs (called in section __init or __devinit).

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |    2 +-
 drivers/char/hvc_console.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -806,7 +806,7 @@ struct hvc_struct __devinit *hvc_alloc(u
 }
 EXPORT_SYMBOL_GPL(hvc_alloc);
 
-int __devexit hvc_remove(struct hvc_struct *hp)
+int hvc_remove(struct hvc_struct *hp)
 {
 	unsigned long flags;
 	struct tty_struct *tty;
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -81,7 +81,7 @@ extern int hvc_instantiate(uint32_t vter
 extern struct hvc_struct * __devinit hvc_alloc(uint32_t vtermno, int data,
 				struct hv_ops *ops, int outbuf_size);
 /* remove a vterm from hvc tty operation (module_exit or hotplug remove) */
-extern int __devexit hvc_remove(struct hvc_struct *hp);
+extern int hvc_remove(struct hvc_struct *hp);
 
 /* data available */
 int hvc_poll(struct hvc_struct *hp);

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RFC PATCH 0/5] patches for a network-based hvc backend (s390)

[Benjamin Herrenschmidt]

On Tue, 2008-10-14 at 11:12 +0200, Hendrik Brueckner wrote:
> Hello,
> 
> I work on a network-based hvc console backend for s390 that allows
> to get "full-screen" terminal access to z/VM guest machines.
> The solution consists of a HVC backend that provides the terminal interface;
> and a tool to connect to the terminal via a z/VM specific communication
> protocol.

 .../...

Hi !

What's the status with that patch serie ? Is this 2.6.28 material ?
Alan, are you ok with those ? Hendrik, have there been any respin other
than v2 of patch 4/5 ?

I can merge it via powerpc but it's getting late in the merge window...

Cheers,
Ben.

Re: [RFC PATCH 0/5] patches for a network-based hvc backend (s390)

[Hendrik Brueckner]

On Tue, Oct 21, 2008 at 10:23:37AM +1100, Benjamin Herrenschmidt wrote:
> What's the status with that patch serie ? Is this 2.6.28 material ?
It is 2.6.28 material.
> Alan, are you ok with those ? Hendrik, have there been any respin other
> than v2 of patch 4/5 ?
The other patches have not been changed.

fyi: patch 1/5 (hangup notifier) applies successfully with an offset
of 2 lines due to commit eef2622a9fcfa964073333ea72c7c9cd20ad45e6
(hvc_console: Fix free_irq in spinlocked section).


> I can merge it via powerpc but it's getting late in the merge window...
Thanks... that would be really great!

Regards
Hendrik

-- 
Hendrik Brueckner
D/3303 Linux on System z Development
Tel: +49 7031 16-1073
Fax: +49 7031 16-3456
eMail: brueckner@linux.vnet.ibm.com
IBM Deutschland Research & Development GmbH, Schoenaicher Str. 220, 71032 Boeblingen

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294