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

[Johan Hovold <jhovold@gmail.com>]

Hi Jiri,

On Tue, Mar 05, 2013 at 05:02:44PM +0100, Jiri Slaby wrote:
> 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?

Sorry about that. Thought it was a bit odd I didn't hear anything from
you actually. ;) Everything was posted to linux-serial (and Alan
initially), but I'll remember to CC you in the future as well.

> 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?

As Peter also mentioned, this is how serial_core (and another seven tty
drivers) work today.

There are currently seven drivers (counting usb-serial as one) that
manipulate DTR at open/close but do not drop DTR on hangup, and five of
those (including usb-serial) don't do it because they use the tty_port
implementation.

> > @@ -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?

You have a point in that it might be better to do it the other way round
and not touch DTR unless we know for sure it was requested. [ But see my
answer to you next question as well. ]

Several drivers (including serial_core) had a similar construct in their
shutdown() but tty is never NULL when called from hangup in those cases.

> > @@ -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?

Nothing, I'll get a new reference within the port lock section as you
just suggested elsewhere in this thread.

But this should never be the case when using both tty_port_close and
tty_port_hangup, as then port->tty will only be NULL if the port has
already been shut down, right?

> > +       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...

Yes, I did. First, the order should not matter for blocked opens as they
will exit their wait loops based on tty_hung_up_p(filp) either way.

As for delta_msr_wait the changed order is actually preferred as it
allows the waiting process to return based on ASYNC_INITIALIZED. This is
also the order used by serial_core. Note however that the current
serial_core TIOCMIWAIT is broken in that it doesn't return on hangups at
all.

Perhaps I should separate this to a patch of its own, and send a fix
for serial_core TIOCMIWAIT as well.

Thanks,
Johan

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