[PATCH 0/2] hvc_console: Add DTR/RTS callbacks to handle HUPCL conditions

[PATCH 0/2] hvc_console: Add DTR/RTS callbacks to handle HUPCL conditions

[Hendrik Brueckner]

Hi folks,

this series resolves an issue for hvc back-ends that transfer
terminal data over an established communication path.

The current implementation of the hvc_console layer notifies its
back-ends for tty open, close, and hangups.  However, there are
conditions where the hangup-on-close (HUPCL) termios flag must be
considered, for example, when doing a vhangup().

For the hvc_iucv back-end, users perceives disconnects at their
login which are triggered by a modified vhangup() invocation as
described in https://lkml.org/lkml/2012/6/5/145.
(The kernel change implied also a change in the login program).

However, this also necessitates to inform back-ends about changes
in the DTR/RTS control lines which actually depend on the setting
of the HUPCL termios flag.  Like in the old days for modems,
network-based back-ends need to know when to hang-up and drop off
an established communication path.  Without this new notification,
back-ends can only use the tty open, close, hangup notifiers to
decide whether to disconnect.  This is not sufficient because the
HUPCL flag can be cleared (i.e. not to hang-up a connection) when
last tty file descriptor is closed.

This series adds the dtr_rts() callback to the hvc_console layer
and modifies the hvc_iucv device driver to disconnect an established
IUCV connection only when the DTR/RTS is lowered.


Feedback is very welcome.  Thanks in advance!

Kind regards,
  Hendrik

[PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Hendrik Brueckner]

Introduce a new callback to explicitly handle the HUPCL termios control flag.
This prepares for a follow-up commit for the hvc_iucv device driver to
improve handling when to drop an established network connection.

The callback naming is based on the recently added tty_port interface to
facilitate a potential refactoring of the hvc_console to use tty_port
functions.

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvc_console.c |   11 ++++++++++-
 drivers/tty/hvc/hvc_console.h |    3 +++
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index eb255e8..9eba119 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -361,7 +361,12 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 		tty->driver_data = NULL;
 		tty_port_put(&hp->port);
 		printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
-	}
+	} else
+		/* We are ready... raise DTR/RTS */
+		if (C_BAUD(tty))
+			if (hp->ops->dtr_rts)
+				hp->ops->dtr_rts(hp, 1);
+
 	/* Force wakeup of the polling thread */
 	hvc_kick();
 
@@ -393,6 +398,10 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		/* We are done with the tty pointer now. */
 		tty_port_tty_set(&hp->port, NULL);
 
+		if (C_HUPCL(tty))
+			if (hp->ops->dtr_rts)
+				hp->ops->dtr_rts(hp, 0);
+
 		if (hp->ops->notifier_del)
 			hp->ops->notifier_del(hp, hp->data);
 
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 674d23c..9131019 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -75,6 +75,9 @@ struct hv_ops {
 	/* tiocmget/set implementation */
 	int (*tiocmget)(struct hvc_struct *hp);
 	int (*tiocmset)(struct hvc_struct *hp, unsigned int set, unsigned int clear);
+
+	/* Callbacks to handle tty ports */
+	void (*dtr_rts)(struct hvc_struct *hp, int raise);
 };
 
 /* Register a vterm and a slot index for use as a console (console_init) */
-- 
1.7.5.4

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Benjamin Herrenschmidt]

On Tue, 2013-07-02 at 17:07 +0200, Hendrik Brueckner wrote:
> Introduce a new callback to explicitly handle the HUPCL termios control flag.
> This prepares for a follow-up commit for the hvc_iucv device driver to
> improve handling when to drop an established network connection.
> 
> The callback naming is based on the recently added tty_port interface to
> facilitate a potential refactoring of the hvc_console to use tty_port
> functions.

I only just noticed that ... oops. Why add those dtr_rts() calls ? We
already have tiocmset in there which is used to set DTR on HVSI consoles
such as hvc_opal when using hvsi_lib...

Any reason why a separate callback was needed ?

Cheers,
Ben.

> Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> ---
>  drivers/tty/hvc/hvc_console.c |   11 ++++++++++-
>  drivers/tty/hvc/hvc_console.h |    3 +++
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index eb255e8..9eba119 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -361,7 +361,12 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
>  		tty->driver_data = NULL;
>  		tty_port_put(&hp->port);
>  		printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
> -	}
> +	} else
> +		/* We are ready... raise DTR/RTS */
> +		if (C_BAUD(tty))
> +			if (hp->ops->dtr_rts)
> +				hp->ops->dtr_rts(hp, 1);
> +
>  	/* Force wakeup of the polling thread */
>  	hvc_kick();
>  
> @@ -393,6 +398,10 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
>  		/* We are done with the tty pointer now. */
>  		tty_port_tty_set(&hp->port, NULL);
>  
> +		if (C_HUPCL(tty))
> +			if (hp->ops->dtr_rts)
> +				hp->ops->dtr_rts(hp, 0);
> +
>  		if (hp->ops->notifier_del)
>  			hp->ops->notifier_del(hp, hp->data);
>  
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index 674d23c..9131019 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -75,6 +75,9 @@ struct hv_ops {
>  	/* tiocmget/set implementation */
>  	int (*tiocmget)(struct hvc_struct *hp);
>  	int (*tiocmset)(struct hvc_struct *hp, unsigned int set, unsigned int clear);
> +
> +	/* Callbacks to handle tty ports */
> +	void (*dtr_rts)(struct hvc_struct *hp, int raise);
>  };
>  
>  /* Register a vterm and a slot index for use as a console (console_init) */

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Hendrik Brueckner]

Hi Benjamin,

On Fri, Oct 11, 2013 at 06:15:11PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2013-07-02 at 17:07 +0200, Hendrik Brueckner wrote:
> > Introduce a new callback to explicitly handle the HUPCL termios control flag.
> > This prepares for a follow-up commit for the hvc_iucv device driver to
> > improve handling when to drop an established network connection.
> > 
> > The callback naming is based on the recently added tty_port interface to
> > facilitate a potential refactoring of the hvc_console to use tty_port
> > functions.
> 
> I only just noticed that ... oops. Why add those dtr_rts() calls ? We
> already have tiocmset in there which is used to set DTR on HVSI consoles
> such as hvc_opal when using hvsi_lib...
> 
> Any reason why a separate callback was needed ?

The tiocmget/tiocmset callbacks are used to set and get modem status and
triggered through an tty ioctl.

The dtr_rts() callback is different and it is used for DTS/RTS handshaking
between the hvc_console (or any other tty_port) and the tty layer.  The tty
port layer uses this callback to signal the hvc_console whether to raise or
lower the DTR/RTS lines.  This is different than the ioctl interface to
controls the modem status.

Thanks and kind regards,
  Hendrik

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Benjamin Herrenschmidt]

On Fri, 2013-10-11 at 14:47 +0200, Hendrik Brueckner wrote:
> The tiocmget/tiocmset callbacks are used to set and get modem status and
> triggered through an tty ioctl.
> 
> The dtr_rts() callback is different and it is used for DTS/RTS handshaking
> between the hvc_console (or any other tty_port) and the tty layer.  The tty
> port layer uses this callback to signal the hvc_console whether to raise or
> lower the DTR/RTS lines.  This is different than the ioctl interface to
> controls the modem status.

Well, DTR at least is the same via both callbacks... Also normal handshaking
is normally RTS/CTS, only some HW setups "hijacks" DTR for RTS (old Macs come
to mind).

Cheers,
Ben.

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Hendrik Brueckner]

Hi Benjamin,

On Sat, Oct 12, 2013 at 07:43:24AM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2013-10-11 at 14:47 +0200, Hendrik Brueckner wrote:
> > The tiocmget/tiocmset callbacks are used to set and get modem status and
> > triggered through an tty ioctl.
> > 
> > The dtr_rts() callback is different and it is used for DTS/RTS handshaking
> > between the hvc_console (or any other tty_port) and the tty layer.  The tty
> > port layer uses this callback to signal the hvc_console whether to raise or
> > lower the DTR/RTS lines.  This is different than the ioctl interface to
> > controls the modem status.
> 
> Well, DTR at least is the same via both callbacks... Also normal handshaking
> is normally RTS/CTS, only some HW setups "hijacks" DTR for RTS (old Macs come
> to mind).

Yep. DTR is changed in both callbacks but from different layers.  The
tiocmget/tiocmset are triggered through the ioctl.  The dtr_rts() callback is
called in hvc_close() to properly handle HUPCL to lower modem control lines
after last process closes the device (hang up).

This is also done in the hvsilib_close() in hvsi_lib.c:

	/* Clear our own DTR */
	if (!pv->tty || (pv->tty->termios.c_cflag & HUPCL))
		hvsilib_write_mctrl(pv, 0); 

This is actually what the dtr_rts() callback should trigger and I wonder
whether it would be worth to introduce the dtr_rts() callback to encapsulate
the "hvsilib_write_mctrl(pv, 0);" call from above.

On the other hand, the dtr_rts() callback is a good encapsulation to not
directly access the hp->tty to potentially prevent a layering violation. At
least for the hvc_iucv() I do not want to deal with the "underlying" tty layer
and introduce additional reference accounting.

I hope this helps you to understand my rational for introducing the dtr_rts()
callback.

Thanks and kind regards,
  Hendrik

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Benjamin Herrenschmidt]

On Tue, 2013-10-15 at 17:36 +0200, Hendrik Brueckner wrote:
> Hi Benjamin,
> 
> On Sat, Oct 12, 2013 at 07:43:24AM +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2013-10-11 at 14:47 +0200, Hendrik Brueckner wrote:
> > > The tiocmget/tiocmset callbacks are used to set and get modem status and
> > > triggered through an tty ioctl.
> > > 
> > > The dtr_rts() callback is different and it is used for DTS/RTS handshaking
> > > between the hvc_console (or any other tty_port) and the tty layer.  The tty
> > > port layer uses this callback to signal the hvc_console whether to raise or
> > > lower the DTR/RTS lines.  This is different than the ioctl interface to
> > > controls the modem status.
> > 
> > Well, DTR at least is the same via both callbacks... Also normal handshaking
> > is normally RTS/CTS, only some HW setups "hijacks" DTR for RTS (old Macs come
> > to mind).
> 
> Yep. DTR is changed in both callbacks but from different layers.  The
> tiocmget/tiocmset are triggered through the ioctl.  The dtr_rts() callback is
> called in hvc_close() to properly handle HUPCL to lower modem control lines
> after last process closes the device (hang up).
> 
> This is also done in the hvsilib_close() in hvsi_lib.c:
> 
> 	/* Clear our own DTR */
> 	if (!pv->tty || (pv->tty->termios.c_cflag & HUPCL))
> 		hvsilib_write_mctrl(pv, 0); 
> 
> This is actually what the dtr_rts() callback should trigger and I wonder
> whether it would be worth to introduce the dtr_rts() callback to encapsulate
> the "hvsilib_write_mctrl(pv, 0);" call from above.
> 
> On the other hand, the dtr_rts() callback is a good encapsulation to not
> directly access the hp->tty to potentially prevent a layering violation. At
> least for the hvc_iucv() I do not want to deal with the "underlying" tty layer
> and introduce additional reference accounting.
> 
> I hope this helps you to understand my rational for introducing the dtr_rts()
> callback.

I'm not sure :) We still end up basically with 2 callbacks to do the
same thing ... change the DTR line. It's odd at best, I still don't
quite see why hvc_console couldn't just use mctrl...

Cheers,
Ben.

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Hendrik Brueckner]

On Tue, Oct 15, 2013 at 03:47:50PM -0500, Benjamin Herrenschmidt wrote:
> On Tue, 2013-10-15 at 17:36 +0200, Hendrik Brueckner wrote:
> > On Sat, Oct 12, 2013 at 07:43:24AM +1100, Benjamin Herrenschmidt wrote:
> > > On Fri, 2013-10-11 at 14:47 +0200, Hendrik Brueckner wrote:
> > > > The tiocmget/tiocmset callbacks are used to set and get modem status and
> > > > triggered through an tty ioctl.
> > > > 
> > > > The dtr_rts() callback is different and it is used for DTS/RTS handshaking
> > > > between the hvc_console (or any other tty_port) and the tty layer.  The tty
> > > > port layer uses this callback to signal the hvc_console whether to raise or
> > > > lower the DTR/RTS lines.  This is different than the ioctl interface to
> > > > controls the modem status.
> > > 
> > > Well, DTR at least is the same via both callbacks... Also normal handshaking
> > > is normally RTS/CTS, only some HW setups "hijacks" DTR for RTS (old Macs come
> > > to mind).
> > 
> > Yep. DTR is changed in both callbacks but from different layers.  The
> > tiocmget/tiocmset are triggered through the ioctl.  The dtr_rts() callback is
> > called in hvc_close() to properly handle HUPCL to lower modem control lines
> > after last process closes the device (hang up).
> > 
> > This is also done in the hvsilib_close() in hvsi_lib.c:
> > 
> > 	/* Clear our own DTR */
> > 	if (!pv->tty || (pv->tty->termios.c_cflag & HUPCL))
> > 		hvsilib_write_mctrl(pv, 0); 
> > 
> > This is actually what the dtr_rts() callback should trigger and I wonder
> > whether it would be worth to introduce the dtr_rts() callback to encapsulate
> > the "hvsilib_write_mctrl(pv, 0);" call from above.
> > 
> > On the other hand, the dtr_rts() callback is a good encapsulation to not
> > directly access the hp->tty to potentially prevent a layering violation. At
> > least for the hvc_iucv() I do not want to deal with the "underlying" tty layer
> > and introduce additional reference accounting.
> > 
> > I hope this helps you to understand my rational for introducing the dtr_rts()
> > callback.
> 
> I'm not sure :) We still end up basically with 2 callbacks to do the
> same thing ... change the DTR line. It's odd at best, I still don't
> quite see why hvc_console couldn't just use mctrl...
> 
Indeed, two callbacks change the DTR line.  The main difference is that
tiocmget/tiocmset can be called from user space by ioctl.  That's not the case
for the dtr_cts callback.  Also, tiocmget/tiocmset provide more flags that can
be changed (ST, SR, CTS, CD, RNG, RI,  ...)

Assume we would like to unify them have a single callback to change DTR, then
we have to take care of these differences.  So the question to you now is
whether you plan for a) other modem flags to be changed and b) if changing the
DTR line (or other control flags) through an ioctl?

Depending on your results, I could work on sth that helps us both and reduces
the callbacks.

Thanks and kind regards,
  Hendrik

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Benjamin Herrenschmidt]

On Wed, 2013-10-16 at 11:04 +0200, Hendrik Brueckner wrote:
> Indeed, two callbacks change the DTR line.  The main difference is that
> tiocmget/tiocmset can be called from user space by ioctl.  That's not the case
> for the dtr_cts callback.  Also, tiocmget/tiocmset provide more flags that can
> be changed (ST, SR, CTS, CD, RNG, RI,  ...)
> 
> Assume we would like to unify them have a single callback to change DTR, then
> we have to take care of these differences.  So the question to you now is
> whether you plan for a) other modem flags to be changed and b) if changing the
> DTR line (or other control flags) through an ioctl?
> 
> Depending on your results, I could work on sth that helps us both and reduces
> the callbacks.

Can we not just have the users of dtr_cts just call the backend's tiocmset ?

If they need to filter or clamp bits, we could handle all that in hvc_console
by caching the user intended value and passing a cooked value down to the backend..

None of that is urgent or anything, it's just odd and would be nice to cleanup.

Cheers,
Ben.

Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control

[Hendrik Brueckner]

On Wed, Oct 16, 2013 at 06:21:12PM -0500, Benjamin Herrenschmidt wrote:
> On Wed, 2013-10-16 at 11:04 +0200, Hendrik Brueckner wrote:
> > Indeed, two callbacks change the DTR line.  The main difference is that
> > tiocmget/tiocmset can be called from user space by ioctl.  That's not the case
> > for the dtr_cts callback.  Also, tiocmget/tiocmset provide more flags that can
> > be changed (ST, SR, CTS, CD, RNG, RI,  ...)
> > 
> > Assume we would like to unify them have a single callback to change DTR, then
> > we have to take care of these differences.  So the question to you now is
> > whether you plan for a) other modem flags to be changed and b) if changing the
> > DTR line (or other control flags) through an ioctl?
> > 
> > Depending on your results, I could work on sth that helps us both and reduces
> > the callbacks.
> 
> Can we not just have the users of dtr_cts just call the backend's tiocmset ?

That's possible.  The only concern is that the tiocmset() callback could be
triggered from within the hvc_console() layer as well as from user space via
ioctl.  For the hvc_iucv driver, I do not want to introduce this ioctl.

One option would be to add parameter to the hvc_callbacks that indicate the
origin so that a backend could filter.

> If they need to filter or clamp bits, we could handle all that in hvc_console
> by caching the user intended value and passing a cooked value down to the backend..

Sure the hvc_console layer could do as much as possible.

> 
> None of that is urgent or anything, it's just odd and would be nice to cleanup.

Thanks and kind regards,
  Hendrik

[PATCH 2/2] tty/hvc_iucv: Disconnect IUCV connection when lowering DTR

[Hendrik Brueckner]

Implement the dtr_rts() hvc console callback to improve control when to
disconnect the IUCV connection.  Previously, the IUCV connection was
disconnected during the notifier_del() callback, i.e., when the last file
descriptor to the hvc terminal device was closed.

Recent changes in login programs caused undesired disconnects during the
login phase.  To prevent these kind of disconnects, implement the dtr_rts
callback to implicitly handle the HUPCL termios control via the hvc_console
driver.

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvc_iucv.c |   64 +++++++++++++++++++++++++++++++++++---------
 1 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/hvc/hvc_iucv.c b/drivers/tty/hvc/hvc_iucv.c
index b6f7d52..bb08676 100644
--- a/drivers/tty/hvc/hvc_iucv.c
+++ b/drivers/tty/hvc/hvc_iucv.c
@@ -656,21 +656,64 @@ static void hvc_iucv_notifier_hangup(struct hvc_struct *hp, int id)
 }
 
 /**
+ * hvc_iucv_dtr_rts() - HVC notifier for handling DTR/RTS
+ * @hp:		Pointer the HVC device (struct hvc_struct)
+ * @raise:	Non-zero to raise or zero to lower DTR/RTS lines
+ *
+ * This routine notifies the HVC back-end to raise or lower DTR/RTS
+ * lines.  Raising DTR/RTS is ignored.  Lowering DTR/RTS indicates to
+ * drop the IUCV connection (similar to hang up the modem).
+ */
+static void hvc_iucv_dtr_rts(struct hvc_struct *hp, int raise)
+{
+	struct hvc_iucv_private *priv;
+	struct iucv_path        *path;
+
+	/* Raising the DTR/RTS is ignored as IUCV connections can be
+	 * established at any times.
+	 */
+	if (raise)
+		return;
+
+	priv = hvc_iucv_get_private(hp->vtermno);
+	if (!priv)
+		return;
+
+	/* Lowering the DTR/RTS lines disconnects an established IUCV
+	 * connection.
+	 */
+	flush_sndbuf_sync(priv);
+
+	spin_lock_bh(&priv->lock);
+	path = priv->path;		/* save reference to IUCV path */
+	priv->path = NULL;
+	priv->iucv_state = IUCV_DISCONN;
+	spin_unlock_bh(&priv->lock);
+
+	/* Sever IUCV path outside of priv->lock due to lock ordering of:
+	 * priv->lock <--> iucv_table_lock */
+	if (path) {
+		iucv_path_sever(path, NULL);
+		iucv_path_free(path);
+	}
+}
+
+/**
  * hvc_iucv_notifier_del() - HVC notifier for closing a TTY for the last time.
  * @hp:		Pointer to the HVC device (struct hvc_struct)
  * @id:		Additional data (originally passed to hvc_alloc):
  *		the index of an struct hvc_iucv_private instance.
  *
  * This routine notifies the HVC back-end that the last tty device fd has been
- * closed.  The function calls hvc_iucv_cleanup() to clean up the struct
- * hvc_iucv_private instance.
+ * closed.  The function cleans up tty resources.  The clean-up of the IUCV
+ * connection is done in hvc_iucv_dtr_rts() and depends on the HUPCL termios
+ * control setting.
  *
  * Locking:	struct hvc_iucv_private->lock
  */
 static void hvc_iucv_notifier_del(struct hvc_struct *hp, int id)
 {
 	struct hvc_iucv_private *priv;
-	struct iucv_path	*path;
 
 	priv = hvc_iucv_get_private(id);
 	if (!priv)
@@ -679,17 +722,11 @@ static void hvc_iucv_notifier_del(struct hvc_struct *hp, int id)
 	flush_sndbuf_sync(priv);
 
 	spin_lock_bh(&priv->lock);
-	path = priv->path;		/* save reference to IUCV path */
-	priv->path = NULL;
-	hvc_iucv_cleanup(priv);
+	destroy_tty_buffer_list(&priv->tty_outqueue);
+	destroy_tty_buffer_list(&priv->tty_inqueue);
+	priv->tty_state = TTY_CLOSED;
+	priv->sndbuf_len = 0;
 	spin_unlock_bh(&priv->lock);
-
-	/* sever IUCV path outside of priv->lock due to lock ordering of:
-	 * priv->lock <--> iucv_table_lock */
-	if (path) {
-		iucv_path_sever(path, NULL);
-		iucv_path_free(path);
-	}
 }
 
 /**
@@ -931,6 +968,7 @@ static const struct hv_ops hvc_iucv_ops = {
 	.notifier_add = hvc_iucv_notifier_add,
 	.notifier_del = hvc_iucv_notifier_del,
 	.notifier_hangup = hvc_iucv_notifier_hangup,
+	.dtr_rts = hvc_iucv_dtr_rts,
 };
 
 /* Suspend / resume device operations */
-- 
1.7.5.4