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

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

[Jiri Slaby]

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

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

[Peter Hurley]

On Tue, 2013-03-05 at 17:02 +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?

Alan asked to be dropped.

On Wed, 2013-02-13 at 17:36 +0000, Alan Cox wrote:
> [ Note that this closing of an uninitialised port seems to be a bug in
> >   itself, which these patches aim to fix. ]
> 
> You don't want to be cc'ing me on these - not my problem any more.

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

This is the existing behavior of the serial core.

uart_hangup() -> uart_shutdown()

static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
{
	struct uart_port *uport = state->uart_port;
	struct tty_port *port = &state->port;

	/*
	 * Set the TTY IO error marker
	 */
	if (tty)
		set_bit(TTY_IO_ERROR, &tty->flags);

	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
		/*
		 * Turn off DTR and RTS early.
		 */
		if (!tty || (tty->termios.c_cflag & HUPCL))
====>			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);

		uart_port_shutdown(port);
	}


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

[See my comment below]

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

Nothing. That's why it's tested in tty_port_shutdown() above.

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

I did.

It makes sense to wake blocked opens only _after_ the port has been
shutdown. This way any blocked opens wake up, discover the port has been
shutdown and exit their wait loops in xxxxxx_block_til_ready().

Similarly for delta_msr_wait. Although I think the delta_msr_wait loop
is already unsafe (or at least not robust).


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

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

[Jiri Slaby]

On 03/05/2013 06:06 PM, Peter Hurley wrote:
>>> @@ -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. That's why it's tested in tty_port_shutdown() above.

I know :). But the question is rather don't we want to pass the real
refcounted port->tty (take a snapshot inside the lock) instead?

thanks,
-- 
js
suse labs

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

[Peter Hurley]

On Tue, 2013-03-05 at 22:56 +0100, Jiri Slaby wrote:
> On 03/05/2013 06:06 PM, Peter Hurley wrote:
> >>> @@ -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. That's why it's tested in tty_port_shutdown() above.
> 
> I know :).

Sorry :)

> But the question is rather don't we want to pass the real
> refcounted port->tty (take a snapshot inside the lock) instead?

I think that's why he moved the kref release to after the shutdown (via
tty_port_set_tty()) -- but I'm tired and maybe I'm missing something
here?

Regards,
Peter Hurley

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

[Jiri Slaby]

On 03/05/2013 11:02 PM, Peter Hurley wrote:
> On Tue, 2013-03-05 at 22:56 +0100, Jiri Slaby wrote:
>> On 03/05/2013 06:06 PM, Peter Hurley wrote:
>>>>> @@ -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. That's why it's tested in tty_port_shutdown() above.
>>
>> I know :).
> 
> Sorry :)
> 
>> But the question is rather don't we want to pass the real
>> refcounted port->tty (take a snapshot inside the lock) instead?
> 
> I think that's why he moved the kref release to after the shutdown (via
> tty_port_set_tty()) -- but I'm tired and maybe I'm missing something
> here?

port->tty can be changed right after the unlock. So I'm thinking about
something like this:

if (port->tty)
   set_bit(TTY_IO_ERROR, &port->tty->flags);
tty = port->tty; <=== take a snapshot
spin_unlock_irqrestore(&port->lock, flags);
tty_port_shutdown(port, tty); <=== use the snapshot
set_tty_port(port, NULL); <=== put kref on that tty

-- 
js
suse labs

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

[Peter Hurley]

On Tue, 2013-03-05 at 23:10 +0100, Jiri Slaby wrote:
> On 03/05/2013 11:02 PM, Peter Hurley wrote:
> > On Tue, 2013-03-05 at 22:56 +0100, Jiri Slaby wrote:
> >> On 03/05/2013 06:06 PM, Peter Hurley wrote:
> >>>>> @@ -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. That's why it's tested in tty_port_shutdown() above.
> >>
> >> I know :).
> > 
> > Sorry :)
> > 
> >> But the question is rather don't we want to pass the real
> >> refcounted port->tty (take a snapshot inside the lock) instead?
> > 
> > I think that's why he moved the kref release to after the shutdown (via
> > tty_port_set_tty()) -- but I'm tired and maybe I'm missing something
> > here?
> 
> port->tty can be changed right after the unlock.

Right. My bad. Thanks for catching this.

>  So I'm thinking about
> something like this:
> 
> if (port->tty)
>    set_bit(TTY_IO_ERROR, &port->tty->flags);
> tty = port->tty; <=== take a snapshot
> spin_unlock_irqrestore(&port->lock, flags);
> tty_port_shutdown(port, tty); <=== use the snapshot
> set_tty_port(port, NULL); <=== put kref on that tty

Yeah, that's better.

Regards,
Peter Hurley

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

[Jiri Slaby]

On 03/05/2013 11:32 PM, Peter Hurley wrote:
>>  So I'm thinking about
>> something like this:
>>
>> if (port->tty)
>>    set_bit(TTY_IO_ERROR, &port->tty->flags);
>> tty = port->tty; <=== take a snapshot
>> spin_unlock_irqrestore(&port->lock, flags);
>> tty_port_shutdown(port, tty); <=== use the snapshot
>> set_tty_port(port, NULL); <=== put kref on that tty
> 
> Yeah, that's better.

But still not correct. The tty can be invalid (freed) at the time
tty_port_shutdown is called. We should take a real reference inside the
lock and put the reference explicitly after the call.

-- 
js
suse labs

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

[Johan Hovold]

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

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

[Peter Hurley]

On Wed, 2013-03-06 at 17:52 +0100, Johan Hovold wrote:

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

Don't do that. Steal the tty and put the kref after like this:

void tty_port_hangup(struct tty_port *port)
{
	struct tty_struct *tty;
	unsigned long flags;

	spin_lock_irqsave(&port->lock, flags);
	port->count = 0;
	port->flags &= ~ASYNC_NORMAL_ACTIVE;
	tty = port->tty;
	port->tty = NULL;
	if (tty)
		set_bit(TTY_IO_ERROR, &tty->flags);
	spin_unlock_irqrestore(&port->lock, flags);
	tty_port_shutdown(port, tty);
	tty_kref_put(tty);
	wake_up_interruptible(&port->open_wait);
	wake_up_interruptible(&port->delta_msr_wait);
}



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

Only if the open() was ever successful, otherwise the filp won't be in
the tty->tty_files list. That's why the blocking opens also check
ASYNC_INITIALIZED (or ASYNCB_INITIALIZED depending on which they use).

Which is why I said it was actually better to shutdown() first, then
wake up the blocked opens.

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

uart_wait_modem_status() is what I was referring to and should be fixed.

Patches always welcome.

Regards,
Peter Hurley

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

[Johan Hovold]

On Wed, Mar 06, 2013 at 02:14:56PM -0500, Peter Hurley wrote:
> On Wed, 2013-03-06 at 17:52 +0100, Johan Hovold wrote:
> 
> > > > @@ -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.
> 
> Don't do that. Steal the tty and put the kref after like this:
 
Allright.

> > 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.
> 
> Only if the open() was ever successful, otherwise the filp won't be in
> the tty->tty_files list. That's why the blocking opens also check
> ASYNC_INITIALIZED (or ASYNCB_INITIALIZED depending on which they use).
> Which is why I said it was actually better to shutdown() first, then
> wake up the blocked opens.

ASYNC_INITIALIZED have also been cleared when the blocked opens are
being woken up from tty_port_close_end.

And the filp is added to tty_files before open() is called:

===>    tty_add_file(tty, filp);

	check_tty_count(tty, __func__);
	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
	    tty->driver->subtype == PTY_TYPE_MASTER)
		noctty = 1;
#ifdef TTY_DEBUG_HANGUP
	printk(KERN_DEBUG "%s: opening %s...\n", __func__, tty->name);
#endif
	if (tty->ops->open)
===>		retval = tty->ops->open(tty, filp);

so a blocked open must have hung_up_tty_fops when woken up from hangup,
right?

Either way, postponing wake-up somewhat in tty_port_hangup should be
fine.

Thanks,
Johan

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

[Peter Hurley]

On Thu, 2013-03-07 at 10:43 +0100, Johan Hovold wrote:
> On Wed, Mar 06, 2013 at 02:14:56PM -0500, Peter Hurley wrote:
> > On Wed, 2013-03-06 at 17:52 +0100, Johan Hovold wrote:
> > > 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.
> > 
> > Only if the open() was ever successful, otherwise the filp won't be in
> > the tty->tty_files list. That's why the blocking opens also check
> > ASYNC_INITIALIZED (or ASYNCB_INITIALIZED depending on which they use).
> > Which is why I said it was actually better to shutdown() first, then
> > wake up the blocked opens.
> 
> ASYNC_INITIALIZED have also been cleared when the blocked opens are
> being woken up from tty_port_close_end.
> 
> And the filp is added to tty_files before open() is called:
> 
> ===>    tty_add_file(tty, filp);
> 
> 	check_tty_count(tty, __func__);
> 	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> 	    tty->driver->subtype == PTY_TYPE_MASTER)
> 		noctty = 1;
> #ifdef TTY_DEBUG_HANGUP
> 	printk(KERN_DEBUG "%s: opening %s...\n", __func__, tty->name);
> #endif
> 	if (tty->ops->open)
> ===>		retval = tty->ops->open(tty, filp);
> 
> so a blocked open must have hung_up_tty_fops when woken up from hangup,
> right?

You're right, my mistake.

> Either way, postponing wake-up somewhat in tty_port_hangup should be
> fine.

Yep.

Regards,
Peter Hurley

[PATCH v3 0/6] TTY: port hangup and close fixes

[Johan Hovold]

These patches against 3.9-rc1 fix a few issues with tty-port hangup and
close.

The first and fifth patch are essentially clean ups.

The second and third patch fix the fact that DTR could get raised
(rather than dropped) at hangup if there are blocked opens. [ Note that
the second patch has been separated into its own patch and that the
third patch is new in v3 of this series. ]

The fourth patch makes sure DTR is dropped also on hangup and that DTR
is only dropped for initialised ports (where it could have been raised
in the first place).

The sixth and final patch, makes 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


v3: 
 - amend series with fix of DTR sometimes being raised on hang-up
 - do not lower DTR on hangup if port tty is gone
 - make sure tty in call to shutdown is refcounted
 - use cflag-macros throughout

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 (6):
  TTY: clean up port shutdown
  TTY: wake up processes last at hangup
  TTY: fix DTR being raised on hang up
  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 | 77 ++++++++++++++++++++++++++++++--------------------
 3 files changed, 54 insertions(+), 31 deletions(-)

-- 
1.8.1.1

[PATCH v3 1/6] TTY: clean up port shutdown

[Johan Hovold]

Untangle port-shutdown logic and make sure the initialised flag is
always cleared for non-console ports.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index b7ff59d..57a061e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -199,9 +199,14 @@ EXPORT_SYMBOL(tty_port_tty_set);
 static void tty_port_shutdown(struct tty_port *port)
 {
 	mutex_lock(&port->mutex);
-	if (port->ops->shutdown && !port->console &&
-		test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags))
+	if (port->console)
+		goto out;
+
+	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		if (port->ops->shutdown)
 			port->ops->shutdown(port);
+	}
+out:
 	mutex_unlock(&port->mutex);
 }
 
-- 
1.8.1.1

[PATCH v3 2/6] TTY: wake up processes last at hangup

[Johan Hovold]

Move wake up of processes on blocked-open and modem-status wait queues
to after port shutdown at hangup.

This way the woken up processes can use the ASYNC_INITIALIZED flag to
detect port shutdown.

Note that this is the order currently used by serial-core.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 57a061e..3de5918 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -231,9 +231,9 @@ void tty_port_hangup(struct tty_port *port)
 	}
 	port->tty = NULL;
 	spin_unlock_irqrestore(&port->lock, flags);
+	tty_port_shutdown(port);
 	wake_up_interruptible(&port->open_wait);
 	wake_up_interruptible(&port->delta_msr_wait);
-	tty_port_shutdown(port);
 }
 EXPORT_SYMBOL(tty_port_hangup);
 
-- 
1.8.1.1

[PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Johan Hovold]

Make sure to check ASYNC_INITIALISED before raising DTR when waking up
from blocked open in tty_port_block_til_ready.

Currently DTR could get raised at hang up as a blocked process would
raise DTR unconditionally before checking for hang up and returning.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 3de5918..52f1066 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
 
 	while (1) {
 		/* Indicate we are open */
-		if (tty->termios.c_cflag & CBAUD)
+		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
 			tty_port_raise_dtr_rts(port);
 
 		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
-- 
1.8.1.1

Re: [PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Peter Hurley]

On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> from blocked open in tty_port_block_til_ready.
> 
> Currently DTR could get raised at hang up as a blocked process would
> raise DTR unconditionally before checking for hang up and returning.
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>  drivers/tty/tty_port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 3de5918..52f1066 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
>  
>  	while (1) {
>  		/* Indicate we are open */
> -		if (tty->termios.c_cflag & CBAUD)
> +		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
>  			tty_port_raise_dtr_rts(port);
>  
>  		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);

This is ok, but there are 6 other *_block_til_ready() functions:

This one doesn't use DTR/RTS so can be ignored:
./drivers/staging/sb105x/sb_pci_mp.c:static int mp_block_til_ready(struct file *filp, struct sb_uart_state *state)

This one is so scary you should probably leave it alone:
./drivers/tty/serial/crisv10.c:block_til_ready(struct tty_struct *tty, struct file * filp,

These will need the same change (although be careful: some use
ASYNC_INITIALIZED rather than ASYNCB_INITIALIZED).
./drivers/tty/synclinkmp.c:static int block_til_ready(struct tty_struct *tty, struct file *filp,
./drivers/tty/synclink_gt.c:static int block_til_ready(struct tty_struct *tty, struct file *filp,
./drivers/tty/synclink.c:static int block_til_ready(struct tty_struct *tty, struct file * filp,
./net/irda/ircomm/ircomm_tty.c:static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,

Please be sure to Cc: David Miller <davem@davemloft.net> on changes to
net/irda.

Regards,
Peter Hurley

Re: [PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Johan Hovold]

On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > from blocked open in tty_port_block_til_ready.
> > 
> > Currently DTR could get raised at hang up as a blocked process would
> > raise DTR unconditionally before checking for hang up and returning.
> > 
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > ---
> >  drivers/tty/tty_port.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 3de5918..52f1066 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> >  
> >  	while (1) {
> >  		/* Indicate we are open */
> > -		if (tty->termios.c_cflag & CBAUD)
> > +		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> >  			tty_port_raise_dtr_rts(port);
> >  
> >  		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
> 
> This is ok, but there are 6 other *_block_til_ready() functions:

Yes, but that's not really a comment on this patch, is it?

The purpose of this series is to fix the tty-port implementation, and
I've only touched individual drivers when I had to in order not to break
anything due to changed assumptions.

There's a ton of buggy and odd behaviour to be found once you start
turning the stones. Drivers like the ones below really ought to be
using tty ports and it's helpers.

Thanks,
Johan

> This one doesn't use DTR/RTS so can be ignored:
> ./drivers/staging/sb105x/sb_pci_mp.c:static int mp_block_til_ready(struct file *filp, struct sb_uart_state *state)
> 
> This one is so scary you should probably leave it alone:
> ./drivers/tty/serial/crisv10.c:block_til_ready(struct tty_struct *tty, struct file * filp,
> 
> These will need the same change (although be careful: some use
> ASYNC_INITIALIZED rather than ASYNCB_INITIALIZED).
> ./drivers/tty/synclinkmp.c:static int block_til_ready(struct tty_struct *tty, struct file *filp,
> ./drivers/tty/synclink_gt.c:static int block_til_ready(struct tty_struct *tty, struct file *filp,
> ./drivers/tty/synclink.c:static int block_til_ready(struct tty_struct *tty, struct file * filp,
> ./net/irda/ircomm/ircomm_tty.c:static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,

Re: [PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Peter Hurley]

On Fri, 2013-03-15 at 10:24 +0100, Johan Hovold wrote:
> On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> > On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > > from blocked open in tty_port_block_til_ready.
> > > 
> > > Currently DTR could get raised at hang up as a blocked process would
> > > raise DTR unconditionally before checking for hang up and returning.
> > > 
> > > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > > ---
> > >  drivers/tty/tty_port.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > index 3de5918..52f1066 100644
> > > --- a/drivers/tty/tty_port.c
> > > +++ b/drivers/tty/tty_port.c
> > > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> > >  
> > >  	while (1) {
> > >  		/* Indicate we are open */
> > > -		if (tty->termios.c_cflag & CBAUD)
> > > +		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> > >  			tty_port_raise_dtr_rts(port);
> > >  
> > >  		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
> > 
> > This is ok, but there are 6 other *_block_til_ready() functions:
     ^^^^^^
Comment on patch

> Yes, but that's not really a comment on this patch, is it?
> 
> The purpose of this series is to fix the tty-port implementation, and
> I've only touched individual drivers when I had to in order not to break
> anything due to changed assumptions.
> 
> There's a ton of buggy and odd behaviour to be found once you start
> turning the stones. Drivers like the ones below really ought to be
> using tty ports and it's helpers.

Sure, I understand.

OTOH, tty_port and these drivers stem from the same ancestor and it's
partly because of localized bug fixes like these that the drivers have
buggy and odd behavior (because tty_port gets fixed and these do not).

As you can verify from the changelogs of these drivers, it's traditional
to continue to maintain the common aspects, despite the desire to
abandon them.

That said, I'm not the maintainer so feel free to disagree with my
point-of-view.

Regards,
Peter Hurley

Re: [PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Johan Hovold]

On Fri, Mar 15, 2013 at 07:03:08AM -0400, Peter Hurley wrote:
> On Fri, 2013-03-15 at 10:24 +0100, Johan Hovold wrote:
> > On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> > > On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > > > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > > > from blocked open in tty_port_block_til_ready.
> > > > 
> > > > Currently DTR could get raised at hang up as a blocked process would
> > > > raise DTR unconditionally before checking for hang up and returning.
> > > > 
> > > > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > > > ---
> > > >  drivers/tty/tty_port.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > > index 3de5918..52f1066 100644
> > > > --- a/drivers/tty/tty_port.c
> > > > +++ b/drivers/tty/tty_port.c
> > > > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> > > >  
> > > >  	while (1) {
> > > >  		/* Indicate we are open */
> > > > -		if (tty->termios.c_cflag & CBAUD)
> > > > +		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> > > >  			tty_port_raise_dtr_rts(port);
> > > >  
> > > >  		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
> > > 
> > > This is ok, but there are 6 other *_block_til_ready() functions:
>      ^^^^^^
> Comment on patch

I saw that, but just wanted to stress that those comments shouldn't
block the series.

> > Yes, but that's not really a comment on this patch, is it?
> > 
> > The purpose of this series is to fix the tty-port implementation, and
> > I've only touched individual drivers when I had to in order not to break
> > anything due to changed assumptions.
> > 
> > There's a ton of buggy and odd behaviour to be found once you start
> > turning the stones. Drivers like the ones below really ought to be
> > using tty ports and it's helpers.
> 
> Sure, I understand.
> 
> OTOH, tty_port and these drivers stem from the same ancestor and it's
> partly because of localized bug fixes like these that the drivers have
> buggy and odd behavior (because tty_port gets fixed and these do not).

Arguably, fixing the core isn't really a localised bug fix. Some of
those drivers you mentioned have custom open, close, hangup which are
quite different from the tty port implementation, and surely would have
a lot to gain from being ported to tty ports if someone could find the
time to do so.

> As you can verify from the changelogs of these drivers, it's traditional
> to continue to maintain the common aspects, despite the desire to
> abandon them.

Most entries I see have to do with changed interfaces.

> That said, I'm not the maintainer so feel free to disagree with my
> point-of-view.

You do have a point, and I will try to find the time for a follow-up
series fixing at least a few of those five-or-so custom block_til_ready
you pointed to.

Thanks,
Johan

Re: [PATCH v3 3/6] TTY: fix DTR being raised on hang up

[Peter Hurley]

On Fri, 2013-03-15 at 12:30 +0100, Johan Hovold wrote:
> On Fri, Mar 15, 2013 at 07:03:08AM -0400, Peter Hurley wrote:
> > On Fri, 2013-03-15 at 10:24 +0100, Johan Hovold wrote:
> > > On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> > > > On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > > > > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > > > > from blocked open in tty_port_block_til_ready.
> > > > > 
> > > > > Currently DTR could get raised at hang up as a blocked process would
> > > > > raise DTR unconditionally before checking for hang up and returning.
> > > > > 
> > > > > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > > > > ---
> > > > >  drivers/tty/tty_port.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > > > index 3de5918..52f1066 100644
> > > > > --- a/drivers/tty/tty_port.c
> > > > > +++ b/drivers/tty/tty_port.c
> > > > > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> > > > >  
> > > > >  	while (1) {
> > > > >  		/* Indicate we are open */
> > > > > -		if (tty->termios.c_cflag & CBAUD)
> > > > > +		if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> > > > >  			tty_port_raise_dtr_rts(port);
> > > > >  
> > > > >  		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
> > > > 
> > > > This is ok, but there are 6 other *_block_til_ready() functions:
> >      ^^^^^^
> > Comment on patch
> 
> I saw that, but just wanted to stress that those comments shouldn't
> block the series.

I completely agree. In fact, I should have said as much in the initial
review. Sorry.

> > > Yes, but that's not really a comment on this patch, is it?
> > > 
> > > The purpose of this series is to fix the tty-port implementation, and
> > > I've only touched individual drivers when I had to in order not to break
> > > anything due to changed assumptions.
> > > 
> > > There's a ton of buggy and odd behaviour to be found once you start
> > > turning the stones. Drivers like the ones below really ought to be
> > > using tty ports and it's helpers.
> > 
> > Sure, I understand.
> > 
> > OTOH, tty_port and these drivers stem from the same ancestor and it's
> > partly because of localized bug fixes like these that the drivers have
> > buggy and odd behavior (because tty_port gets fixed and these do not).
> 
> Arguably, fixing the core isn't really a localised bug fix. Some of
> those drivers you mentioned have custom open, close, hangup which are
> quite different from the tty port implementation, and surely would have
> a lot to gain from being ported to tty ports if someone could find the
> time to do so.

I think the reluctance to do a full port is partly due to lack of
testable hardware.

> > As you can verify from the changelogs of these drivers, it's traditional
> > to continue to maintain the common aspects, despite the desire to
> > abandon them.
> 
> Most entries I see have to do with changed interfaces.
> 
> > That said, I'm not the maintainer so feel free to disagree with my
> > point-of-view.
> 
> You do have a point, and I will try to find the time for a follow-up
> series fixing at least a few of those five-or-so custom block_til_ready
> you pointed to.

Thanks.

Regards,
Peter Hurley

[PATCH v3 4/6] TTY: fix DTR not being dropped on hang up

[Johan Hovold]

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

Also do not try to drop DTR for uninitialised ports where it has never
been raised (e.g. after a failed open).

Note that this is also the current behaviour of serial-core.

Nine drivers currently call tty_port_close_start directly (rather than
through tty_port_close) and seven of them lower DTR as part of their
close (if the port has been initialised). Fixup the remaining two
drivers so that it continues to be lowered also on normal (non-HUP)
close. [ Note that most of those other seven drivers did not expect DTR
to have been dropped by tty_port_close_start in the first place. ]

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/mxser.c    |  4 ++++
 drivers/tty/n_gsm.c    |  4 ++++
 drivers/tty/tty_port.c | 27 +++++++++++++++------------
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 484b6a3..d996038 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -1084,6 +1084,10 @@ static void mxser_close(struct tty_struct *tty, struct file *filp)
 	mutex_lock(&port->mutex);
 	mxser_close_port(port);
 	mxser_flush_buffer(tty);
+	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		if (C_HUPCL(tty))
+			tty_port_lower_dtr_rts(port);
+	}
 	mxser_shutdown_port(port);
 	clear_bit(ASYNCB_INITIALIZED, &port->flags);
 	mutex_unlock(&port->mutex);
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4a43ef5d7..a43b01f 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2968,6 +2968,10 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
 		goto out;
 	gsm_dlci_begin_close(dlci);
+	if (test_bit(ASYNCB_INITIALIZED, &dlci->port.flags)) {
+		if (C_HUPCL(tty))
+			tty_port_lower_dtr_rts(&dlci->port);
+	}
 	tty_port_close_end(&dlci->port, tty);
 	tty_port_tty_set(&dlci->port, NULL);
 out:
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 52f1066..cd65f7e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -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 && C_HUPCL(tty))
+			tty_port_lower_dtr_rts(port);
+
 		if (port->ops->shutdown)
 			port->ops->shutdown(port);
 	}
@@ -220,18 +227,19 @@ out:
 
 void tty_port_hangup(struct tty_port *port)
 {
+	struct tty_struct *tty;
 	unsigned long flags;
 
 	spin_lock_irqsave(&port->lock, flags);
 	port->count = 0;
 	port->flags &= ~ASYNC_NORMAL_ACTIVE;
-	if (port->tty) {
-		set_bit(TTY_IO_ERROR, &port->tty->flags);
-		tty_kref_put(port->tty);
-	}
+	tty = port->tty;
+	if (tty)
+		set_bit(TTY_IO_ERROR, &tty->flags);
 	port->tty = NULL;
 	spin_unlock_irqrestore(&port->lock, flags);
-	tty_port_shutdown(port);
+	tty_port_shutdown(port, tty);
+	tty_kref_put(tty);
 	wake_up_interruptible(&port->open_wait);
 	wake_up_interruptible(&port->delta_msr_wait);
 }
@@ -452,11 +460,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 */
@@ -491,7 +494,7 @@ void tty_port_close(struct tty_port *port, struct tty_struct *tty,
 {
 	if (tty_port_close_start(port, tty, filp) == 0)
 		return;
-	tty_port_shutdown(port);
+	tty_port_shutdown(port, tty);
 	set_bit(TTY_IO_ERROR, &tty->flags);
 	tty_port_close_end(port, tty);
 	tty_port_tty_set(port, NULL);
-- 
1.8.1.1

[PATCH v3 5/6] TTY: clean up port drain-delay handling

[Johan Hovold]

Move port drain-delay handling to a separate function.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index cd65f7e..048cc85 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -408,6 +408,20 @@ int tty_port_block_til_ready(struct tty_port *port,
 }
 EXPORT_SYMBOL(tty_port_block_til_ready);
 
+static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
+{
+	unsigned int bps = tty_get_baud_rate(tty);
+	long timeout;
+
+	if (bps > 1200) {
+		timeout = (HZ * 10 * port->drain_delay) / bps;
+		timeout = max_t(long, timeout, HZ / 10);
+	} else {
+		timeout = 2 * HZ;
+	}
+	schedule_timeout_interruptible(timeout);
+}
+
 int tty_port_close_start(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
@@ -446,17 +460,8 @@ int tty_port_close_start(struct tty_port *port,
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
 			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	if (port->drain_delay) {
-		unsigned int bps = tty_get_baud_rate(tty);
-		long timeout;
-
-		if (bps > 1200)
-			timeout = max_t(long,
-				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
-		else
-			timeout = 2 * HZ;
-		schedule_timeout_interruptible(timeout);
-	}
+	if (port->drain_delay)
+		tty_port_drain_delay(port, tty);
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

[PATCH v3 6/6] TTY: fix close of uninitialised ports

[Johan Hovold]

Make sure we do not make tty-driver callbacks or wait for port to drain
on uninitialised ports (e.g. when open failed) in
tty_port_close_start().

No callback, such as flush_buffer or wait_until_sent, needs to be made
on a port that has never been opened. Neither does it make much sense to
add drain delay for an uninitialised port.

Currently a drain delay of up to two seconds could be added when a tty
fails to open.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 048cc85..d028df3 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -454,14 +454,16 @@ int tty_port_close_start(struct tty_port *port,
 	set_bit(ASYNCB_CLOSING, &port->flags);
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);
-	/* Don't block on a stalled port, just pull the chain */
-	if (tty->flow_stopped)
-		tty_driver_flush_buffer(tty);
-	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
-			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	if (port->drain_delay)
-		tty_port_drain_delay(port, tty);
+
+	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/* Don't block on a stalled port, just pull the chain */
+		if (tty->flow_stopped)
+			tty_driver_flush_buffer(tty);
+		if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
+			tty_wait_until_sent_from_close(tty, port->closing_wait);
+		if (port->drain_delay)
+			tty_port_drain_delay(port, tty);
+	}
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

Re: [PATCH v3 0/6] TTY: port hangup and close fixes

[Peter Hurley]

On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> These patches against 3.9-rc1 fix a few issues with tty-port hangup and
> close.
> 
> The first and fifth patch are essentially clean ups.
> 
> The second and third patch fix the fact that DTR could get raised
> (rather than dropped) at hangup if there are blocked opens. [ Note that
> the second patch has been separated into its own patch and that the
> third patch is new in v3 of this series. ]
> 
> The fourth patch makes sure DTR is dropped also on hangup and that DTR
> is only dropped for initialised ports (where it could have been raised
> in the first place).
> 
> The sixth and final patch, makes 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
> 
> 
> v3: 
>  - amend series with fix of DTR sometimes being raised on hang-up
>  - do not lower DTR on hangup if port tty is gone
>  - make sure tty in call to shutdown is refcounted
>  - use cflag-macros throughout

Other than the comments for patch 3/6, this series looks good. Thanks
again for your work on this.

Please cc: me on the USB serial core changes as well, if you don't mind.

Regards,
Peter Hurley

Re: [PATCH v3 0/6] TTY: port hangup and close fixes

[Johan Hovold]

On Wed, Mar 13, 2013 at 03:50:32PM -0400, Peter Hurley wrote:
> On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > These patches against 3.9-rc1 fix a few issues with tty-port hangup and
> > close.
> > 
> > The first and fifth patch are essentially clean ups.
> > 
> > The second and third patch fix the fact that DTR could get raised
> > (rather than dropped) at hangup if there are blocked opens. [ Note that
> > the second patch has been separated into its own patch and that the
> > third patch is new in v3 of this series. ]
> > 
> > The fourth patch makes sure DTR is dropped also on hangup and that DTR
> > is only dropped for initialised ports (where it could have been raised
> > in the first place).
> > 
> > The sixth and final patch, makes 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
> > 
> > 
> > v3: 
> >  - amend series with fix of DTR sometimes being raised on hang-up
> >  - do not lower DTR on hangup if port tty is gone
> >  - make sure tty in call to shutdown is refcounted
> >  - use cflag-macros throughout
> 
> Other than the comments for patch 3/6, this series looks good. Thanks
> again for your work on this.

As I mentioned in my reply to 3/6, fixing bugs in other drivers not
using the tty-port implementation is all good but not the purpose of
this series.

Thanks,
Johan

Re: [PATCH v3 0/6] TTY: port hangup and close fixes

[Greg Kroah-Hartman]

On Thu, Mar 07, 2013 at 03:55:47PM +0100, Johan Hovold wrote:
> These patches against 3.9-rc1 fix a few issues with tty-port hangup and
> close.

Are these for 3.9-final?

thanks,

greg k-h

Re: [PATCH v3 0/6] TTY: port hangup and close fixes

[Johan Hovold]

On Fri, Mar 15, 2013 at 12:05:58PM -0700, Greg KH wrote:
> On Thu, Mar 07, 2013 at 03:55:47PM +0100, Johan Hovold wrote:
> > These patches against 3.9-rc1 fix a few issues with tty-port hangup and
> > close.
> 
> Are these for 3.9-final?

I'd say it can wait for 3.10 as it fixes long-standing issues without
any really severe consequences: DTR is being raised or dropped when it
shouldn't, some possibly annoying delays when open fails, and the
callbacks after disconnect that we get in usb-serial that we are already
work around.

Johan

Re: [PATCH v3 0/6] TTY: port hangup and close fixes

[Greg Kroah-Hartman]

On Thu, Mar 07, 2013 at 03:55:47PM +0100, Johan Hovold wrote:
> These patches against 3.9-rc1 fix a few issues with tty-port hangup and
> close.

all now applied, thanks.

greg k-h