Messages in this thread | | | Date | Wed, 6 Mar 2013 17:52:11 +0100 | From | Johan Hovold <> | Subject | Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes] |
| |
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 */
| |