Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]

[Jiri Slaby <jslaby@suse.cz>]

On 02/28/2013 09:57 PM, Peter Hurley wrote:
> Hi Jiri,
> 
> Just wanted to make sure you saw this series.

Hi, thanks for letting me know. Johan, care to CC Alan Cox and me (or at
least LKML) when you're changing the TTY core next time?

I have a couple of questions for 2/4:

> Move HUPCL handling to port shutdown so that DTR is dropped also on
> hang up (tty_port_close is a noop for hung-up ports).

It makes sense, but I'm not sure -- is this expected, i.e. does this
conform to standards and/or BSDs?

> @@ -196,13 +196,20 @@ void tty_port_tty_set(struct tty_port *port, struct
> tty_struct *tty)
>  }
>  EXPORT_SYMBOL(tty_port_tty_set);

-static void tty_port_shutdown(struct tty_port *port)
+static void tty_port_shutdown(struct tty_port *port, struct tty_struct
*tty)
 {
        mutex_lock(&port->mutex);
        if (port->console)
                goto out;

        if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+               /*
+                * Drop DTR/RTS if HUPCL is set. This causes any attached
+                * modem to hang up the line.
+                */
+               if (!tty || tty->termios.c_cflag & HUPCL)
> +                       tty_port_lower_dtr_rts(port);
> +

So you drop the line even thought the user didn't necessarily want to,
in case the tty is gone already?

> @@ -225,15 +232,13 @@ void tty_port_hangup(struct tty_port *port)
        spin_lock_irqsave(&port->lock, flags);
        port->count = 0;
        port->flags &= ~ASYNC_NORMAL_ACTIVE;
-       if (port->tty) {
+       if (port->tty)
                set_bit(TTY_IO_ERROR, &port->tty->flags);
-               tty_kref_put(port->tty);
-       }
-       port->tty = NULL;
        spin_unlock_irqrestore(&port->lock, flags);
> +       tty_port_shutdown(port, port->tty);

What prevents port->tty to be NULL here already?

> +       tty_port_tty_set(port, NULL);
>         wake_up_interruptible(&port->open_wait);
>         wake_up_interruptible(&port->delta_msr_wait);
> -       tty_port_shutdown(port);

Did you investigate if the order matters here? I don't know, just curious...

> @@ -452,11 +457,6 @@ int tty_port_close_start(struct tty_port *port,
        /* Flush the ldisc buffering */
        tty_ldisc_flush(tty);

-       /* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
-          hang up the line */
-       if (tty->termios.c_cflag & HUPCL)
-               tty_port_lower_dtr_rts(port);
-
        /* Don't call port->drop for the last reference. Callers will want
           to drop the last active reference in ->shutdown() or the tty
>            shutdown path */

> -------- Forwarded Message --------
> From: Johan Hovold <jhovold@gmail.com>
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>, linux-usb@vger.kernel.org,
> linux-serial@vger.kernel.org, Peter Hurley <peter@hurleysoftware.com>,
> Johan Hovold <jhovold@gmail.com>
> Subject: [PATCH v2 0/4] TTY: port hangup and close fixes
> Date: Tue, 26 Feb 2013 12:14:28 +0100
> 
> These patches against tty-next fix a few issues with tty-port hangup and
> close.
> 
> The first and third patch are essentially clean ups.
> 
> The second patch makes sure DTR is dropped also on hangup and that DTR
> is only dropped for initialised ports (where is could have been raised
> in the first place).
> 
> The fourth and final patch, make sure no tty callbacks are made from
> tty_port_close_start when the port has not been initialised (successfully
> opened). This was previously only done for wait_until_sent but there's
> no reason to call flush_buffer or to honour port drain delay either.
> The latter could cause a failed open to stall for up to two seconds.
> 
> As a side-effect, these patches also fix an issue in USB-serial where we could
> get tty-port callbacks on an uninitialised port after having hung up and
> unregistered a device after disconnect.
> 
> Johan
> 
> 
> v2:
>  - reuse tty reference from hangup and close in shutdown. Both call sites
>    guarantee tty is either NULL or has a kref.
> 
> Changes since RFC-series:
>  - fix up the two driver relying on tty_port_close_start directly but
>    that did not manage DTR themselves
> 
> 
> Johan Hovold (4):
>   TTY: clean up port shutdown
>   TTY: fix DTR not being dropped on hang up
>   TTY: clean up port drain-delay handling
>   TTY: fix close of uninitialised ports
> 
>  drivers/tty/mxser.c    |  4 +++
>  drivers/tty/n_gsm.c    |  4 +++
>  drivers/tty/tty_port.c | 72 +++++++++++++++++++++++++++++---------------------
>  3 files changed, 50 insertions(+), 30 deletions(-)
> 

thanks,
-- 
js
suse labs