Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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 +, 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 xx_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
> > To: Greg KH
> > Cc: Alan Stern , linux-usb@vger.kernel.org,
> > linux-ser...@vger.kernel.org, Peter Hurley ,
> > Johan Hovold
> > 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 be
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
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
> To: Greg KH
> Cc: Alan Stern , linux-usb@vger.kernel.org,
> linux-ser...@vger.kernel.org, Peter Hurley ,
> Johan Hovold
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
