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

login
register
mail settings
Submitter Hendrik Brueckner
Date July 2, 2013, 3:07 p.m.
Message ID <1372777635-10423-2-git-send-email-brueckner@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/256446/
State Not Applicable
Headers show

Comments

Hendrik Brueckner - July 2, 2013, 3:07 p.m.
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(-)
Benjamin Herrenschmidt - Oct. 11, 2013, 7:15 a.m.
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) */
Hendrik Brueckner - Oct. 11, 2013, 12:47 p.m.
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
Benjamin Herrenschmidt - Oct. 11, 2013, 8:43 p.m.
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.
Hendrik Brueckner - Oct. 15, 2013, 3:36 p.m.
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
Benjamin Herrenschmidt - Oct. 15, 2013, 8:47 p.m.
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.
Hendrik Brueckner - Oct. 16, 2013, 9:04 a.m.
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
Benjamin Herrenschmidt - Oct. 16, 2013, 11:21 p.m.
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.
Hendrik Brueckner - Oct. 17, 2013, 8:16 a.m.
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

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) */