| Message ID | 1372777635-10423-2-git-send-email-brueckner@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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) */
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
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.
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
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.
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
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.
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
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) */
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(-)