[PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).

However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.

Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.

CC: Karsten Keil <isdn@linux-pingi.de>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/isdn/i4l/isdn_tty.c   |  2 +-
 drivers/tty/hvc/hvc_console.c |  2 +-
 drivers/tty/hvc/hvcs.c        |  2 +-
 drivers/tty/tty_port.c        | 11 ++---------
 include/linux/tty.h           | 18 ------------------
 5 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 3c5f249..732f68a 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1587,7 +1587,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
 	 * line status register.
 	 */
 	if (port->flags & ASYNC_INITIALIZED) {
-		tty_wait_until_sent_from_close(tty, 3000);	/* 30 seconds timeout */
+		tty_wait_until_sent(tty, 3000);	/* 30 seconds timeout */
 		/*
 		 * Before we drop DTR, make sure the UART transmitter
 		 * has completely drained; this is especially
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 0ff7fda..2297dc7 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -417,7 +417,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		 * there is no buffered data otherwise sleeps on a wait queue
 		 * waking periodically to check chars_in_buffer().
 		 */
-		tty_wait_until_sent_from_close(tty, HVC_CLOSE_WAIT);
+		tty_wait_until_sent(tty, HVC_CLOSE_WAIT);
 	} else {
 		if (hp->port.count < 0)
 			printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 81e939e..236302d 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1230,7 +1230,7 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
 		irq = hvcsd->vdev->irq;
 		spin_unlock_irqrestore(&hvcsd->lock, flags);
 
-		tty_wait_until_sent_from_close(tty, HVCS_CLOSE_WAIT);
+		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
 
 		/*
 		 * This line is important because it tells hvcs_open that this
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1b93357..6b6214b 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -464,10 +464,7 @@ static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
 	schedule_timeout_interruptible(timeout);
 }
 
-/* Caller holds tty lock.
- * NB: may drop and reacquire tty lock (in tty_wait_until_sent_from_close())
- * so tty and tty port may have changed state (but not hung up or reopened).
- */
+/* Caller holds tty lock. */
 int tty_port_close_start(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
@@ -505,7 +502,7 @@ int tty_port_close_start(struct tty_port *port,
 		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);
+			tty_wait_until_sent(tty, port->closing_wait);
 		if (port->drain_delay)
 			tty_port_drain_delay(port, tty);
 	}
@@ -545,10 +542,6 @@ EXPORT_SYMBOL(tty_port_close_end);
  * tty_port_close
  *
  * Caller holds tty lock
- *
- * NB: may drop and reacquire tty lock (in tty_port_close_start()->
- * tty_wait_until_sent_from_close()) so tty and tty_port may have changed
- * state (but not hung up or reopened).
  */
 void tty_port_close(struct tty_port *port, struct tty_struct *tty,
 							struct file *filp)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..f3eb70d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -644,24 +644,6 @@ extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,
 				struct tty_struct *tty2);
 
 /*
- * this shall be called only from where BTM is held (like close)
- *
- * We need this to ensure nobody waits for us to finish while we are waiting.
- * Without this we were encountering system stalls.
- *
- * This should be indeed removed with BTM removal later.
- *
- * Locking: BTM required. Nobody is allowed to hold port->mutex.
- */
-static inline void tty_wait_until_sent_from_close(struct tty_struct *tty,
-		long timeout)
-{
-	tty_unlock(tty); /* tty->ops->close holds the BTM, drop it while waiting */
-	tty_wait_until_sent(tty, timeout);
-	tty_lock(tty);
-}
-
-/*
  * wait_event_interruptible_tty -- wait for a condition with the tty lock held
  *
  * The condition we are waiting for might take a long time to
-- 
2.0.0

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Arnd Bergmann]

On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
> tty_wait_until_sent_from_close() drops the tty lock while waiting
> for the tty driver to finish sending previously accepted data (ie.,
> data remaining in its write buffer and transmit fifo).
> 
> However, dropping the tty lock is a hold-over from when the tty
> lock was system-wide; ie., one lock for all ttys.
> 
> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
> 'tty: localise the lock', dropping the tty lock has not been necessary.
> 
> CC: Karsten Keil <isdn@linux-pingi.de>
> CC: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.

	Arnd

RE: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[David Laight]

RnJvbTogQXJuZCBCZXJnbWFubg0KPiBPbiBNb25kYXkgMTYgSnVuZSAyMDE0IDA5OjE3OjExIFBl
dGVyIEh1cmxleSB3cm90ZToNCj4gPiB0dHlfd2FpdF91bnRpbF9zZW50X2Zyb21fY2xvc2UoKSBk
cm9wcyB0aGUgdHR5IGxvY2sgd2hpbGUgd2FpdGluZw0KPiA+IGZvciB0aGUgdHR5IGRyaXZlciB0
byBmaW5pc2ggc2VuZGluZyBwcmV2aW91c2x5IGFjY2VwdGVkIGRhdGEgKGllLiwNCj4gPiBkYXRh
IHJlbWFpbmluZyBpbiBpdHMgd3JpdGUgYnVmZmVyIGFuZCB0cmFuc21pdCBmaWZvKS4NCj4gPg0K
PiA+IEhvd2V2ZXIsIGRyb3BwaW5nIHRoZSB0dHkgbG9jayBpcyBhIGhvbGQtb3ZlciBmcm9tIHdo
ZW4gdGhlIHR0eQ0KPiA+IGxvY2sgd2FzIHN5c3RlbS13aWRlOyBpZS4sIG9uZSBsb2NrIGZvciBh
bGwgdHR5cy4NCj4gPg0KPiA+IFNpbmNlIGNvbW1pdCA4OWM4ZDkxZTMxZjI2NzcwM2UzNjU1OTNm
NmJmZWJiOWY2ZDJhZDAxLA0KPiA+ICd0dHk6IGxvY2FsaXNlIHRoZSBsb2NrJywgZHJvcHBpbmcg
dGhlIHR0eSBsb2NrIGhhcyBub3QgYmVlbiBuZWNlc3NhcnkuDQo+ID4NCj4gPiBDQzogS2Fyc3Rl
biBLZWlsIDxpc2RuQGxpbnV4LXBpbmdpLmRlPg0KPiA+IENDOiBsaW51eHBwYy1kZXZAbGlzdHMu
b3psYWJzLm9yZw0KPiA+IFNpZ25lZC1vZmYtYnk6IFBldGVyIEh1cmxleSA8cGV0ZXJAaHVybGV5
c29mdHdhcmUuY29tPg0KPiANCj4gSSBkb24ndCB1bmRlcnN0YW5kIHRoZSBzZWNvbmQgaGFsZiBv
ZiB0aGUgY2hhbmdlbG9nLCBpdCBkb2Vzbid0IHNlZW0NCj4gdG8gZml0IGhlcmU6IHRoZXJlIGRl
YWRsb2NrIHRoYXQgd2UgYXJlIHRyeWluZyB0byBhdm9pZCBoZXJlIGhhcHBlbnMNCj4gd2hlbiB0
aGUgKnNhbWUqIHR0eSBuZWVkcyB0aGUgbG9jayB0byBjb21wbGV0ZSB0aGUgZnVuY3Rpb24gdGhh
dA0KPiBzZW5kcyB0aGUgcGVuZGluZyBkYXRhLiBJIGRvbid0IHRoaW5rIHdlIGRvIHN0aWxsIGRv
IHRoYXQgYW55IG1vcmUsDQo+IGJ1dCBpdCBkb2Vzbid0IHNlZW0gcmVsYXRlZCB0byB0aGUgdHR5
IGxvY2sgYmVpbmcgc3lzdGVtLXdpZGUgb3Igbm90Lg0KDQpXaGlsZSBJJ3ZlIG5vdCBsb29rZWQg
YXQgdGhlIGNvZGUgaW4gcXVlc3Rpb247IG15IHRob3VnaHRzIHdlcmUgdGhhdA0KaG9sZGluZyBh
bnkgbG9jayB3aGlsZSB3YWl0aW5nIGZvciBvdXRwdXQgdG8gZHJhaW4gKG9yIGFueXRoaW5nIGVs
c2UNCnJlYWxseSkgcHJvYmFibHkgaXNuJ3QgYSBnb29kIGlkZWEuDQpZb3UgbWlnaHQgZmluZCB0
aGF0IHNvbWV0aGluZyBlbHNlIG5lZWRzIHRoZSBsb2NrIC0gZXZlbiBpZiBvbmx5DQpzb21lIGtp
bmQgb2Ygc3RhdHVzIHJlcXVlc3QuDQoNCglEYXZpZA0KDQo=

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

On 06/17/2014 04:00 AM, Arnd Bergmann wrote:
> On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
>> tty_wait_until_sent_from_close() drops the tty lock while waiting
>> for the tty driver to finish sending previously accepted data (ie.,
>> data remaining in its write buffer and transmit fifo).
>>
>> However, dropping the tty lock is a hold-over from when the tty
>> lock was system-wide; ie., one lock for all ttys.
>>
>> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
>> 'tty: localise the lock', dropping the tty lock has not been necessary.
>>
>> CC: Karsten Keil <isdn@linux-pingi.de>
>> CC: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>
> I don't understand the second half of the changelog, it doesn't seem
> to fit here: there deadlock that we are trying to avoid here happens
> when the *same* tty needs the lock to complete the function that
> sends the pending data. I don't think we do still do that any more,
> but it doesn't seem related to the tty lock being system-wide or not.

The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().

The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).

Regards,
Peter Hurley


[1]
commit a57a7bf3fc7eff00f07eb9c805774d911a3f2472
Author: Jiri Slaby <jslaby@suse.cz>
Date:   Thu Aug 25 15:12:06 2011 +0200

     TTY: define tty_wait_until_sent_from_close

     We need this helper to fix system stalls. The issue is that the rest
     of the system TTYs wait for us to finish waiting. This wasn't an issue
     with BKL. BKL used to unlock implicitly.

     This is based on the Arnd suggestion.

     Signed-off-by: Jiri Slaby <jslaby@suse.cz>
     Acked-by: Arnd Bergmann <arnd@arndb.de>
     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

RE: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[David Laight]

RnJvbTogUGV0ZXIgSHVybGV5DQouLi4NCj4gPiBJIGRvbid0IHVuZGVyc3RhbmQgdGhlIHNlY29u
ZCBoYWxmIG9mIHRoZSBjaGFuZ2Vsb2csIGl0IGRvZXNuJ3Qgc2VlbQ0KPiA+IHRvIGZpdCBoZXJl
OiB0aGVyZSBkZWFkbG9jayB0aGF0IHdlIGFyZSB0cnlpbmcgdG8gYXZvaWQgaGVyZSBoYXBwZW5z
DQo+ID4gd2hlbiB0aGUgKnNhbWUqIHR0eSBuZWVkcyB0aGUgbG9jayB0byBjb21wbGV0ZSB0aGUg
ZnVuY3Rpb24gdGhhdA0KPiA+IHNlbmRzIHRoZSBwZW5kaW5nIGRhdGEuIEkgZG9uJ3QgdGhpbmsg
d2UgZG8gc3RpbGwgZG8gdGhhdCBhbnkgbW9yZSwNCj4gPiBidXQgaXQgZG9lc24ndCBzZWVtIHJl
bGF0ZWQgdG8gdGhlIHR0eSBsb2NrIGJlaW5nIHN5c3RlbS13aWRlIG9yIG5vdC4NCj4gDQo+IFRo
ZSB0dHkgbG9jayBpcyBub3QgdXNlZCBpbiB0aGUgaS9vIHBhdGg7IGl0J3MgcHVycG9zZSBpcyB0
bw0KPiBtdXR1YWxseSBleGNsdWRlIHN0YXRlIGNoYW5nZXMgaW4gb3BlbigpLCBjbG9zZSgpIGFu
ZCBoYW5ndXAoKS4NCj4gDQo+IFRoZSBjb21taXQgdGhhdCBhZGRlZCB0aGlzIFsxXSBjb21tZW50
cyB0aGF0IF9vdGhlcl8gdHR5cyBtYXkgd2FpdA0KPiBmb3IgdGhpcyB0dHkgdG8gY29tcGxldGUs
IGFuZCBjb21tZW50cyBpbiB0aGUgY29kZSBub3RlIHRoYXQgdGhpcw0KPiBmdW5jdGlvbiBzaG91
bGQgYmUgcmVtb3ZlZCB3aGVuIHRoZSBzeXN0ZW0td2lkZSB0dHkgbXV0ZXggd2FzIHJlbW92ZWQN
Cj4gKHdoaWNoIGhhcHBlbmVkIHdpdGggdGhlIGNvbW1pdCBub3RlZCBpbiB0aGUgY2hhbmdlbG9n
KS4NCg0KV2hhdCBoYXBwZW5zIGlmIGFub3RoZXIgcHJvY2VzcyB0cmllcyB0byBkbyBhIG5vbi1i
bG9ja2luZyBvcGVuDQp3aGlsZSB5b3UgYXJlIHNsZWVwaW5nIGluIGNsb3NlIHdhaXRpbmcgZm9y
IG91dHB1dCB0byBkcmFpbj8NCg0KSG9wZWZ1bGx5IHRoaXMgcmV0dXJucyBiZWZvcmUgdGhhdCBk
YXRhIGhhcyBkcmFpbmVkLg0KDQoJRGF2aWQNCg0K

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Arnd Bergmann]

On Tuesday 17 June 2014 11:03:50 David Laight wrote:
> From: Peter Hurley
> ...
> > > I don't understand the second half of the changelog, it doesn't seem
> > > to fit here: there deadlock that we are trying to avoid here happens
> > > when the *same* tty needs the lock to complete the function that
> > > sends the pending data. I don't think we do still do that any more,
> > > but it doesn't seem related to the tty lock being system-wide or not.
> > 
> > The tty lock is not used in the i/o path; it's purpose is to
> > mutually exclude state changes in open(), close() and hangup().
> > 
> > The commit that added this [1] comments that _other_ ttys may wait
> > for this tty to complete, and comments in the code note that this
> > function should be removed when the system-wide tty mutex was removed
> > (which happened with the commit noted in the changelog).
> 
> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
> 
> Hopefully this returns before that data has drained.

Before the patch, I believe tty_reopen() would return -EIO because
the TTY_CLOSING flag is set. After the patch, tty_open() blocks
on tty_lock() before calling tty_reopen(). AFAICT, this is independent
of O_NONBLOCK.

	Arnd

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

On 06/17/2014 07:03 AM, David Laight wrote:
> From: Peter Hurley
> ...
>>> I don't understand the second half of the changelog, it doesn't seem
>>> to fit here: there deadlock that we are trying to avoid here happens
>>> when the *same* tty needs the lock to complete the function that
>>> sends the pending data. I don't think we do still do that any more,
>>> but it doesn't seem related to the tty lock being system-wide or not.
>>
>> The tty lock is not used in the i/o path; it's purpose is to
>> mutually exclude state changes in open(), close() and hangup().
>>
>> The commit that added this [1] comments that _other_ ttys may wait
>> for this tty to complete, and comments in the code note that this
>> function should be removed when the system-wide tty mutex was removed
>> (which happened with the commit noted in the changelog).
>
> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
>
> Hopefully this returns before that data has drained.

Good point.

tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.

Regards,
Peter Hurley

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[One Thousand Gnomes]

> Before the patch, I believe tty_reopen() would return -EIO because
> the TTY_CLOSING flag is set. After the patch, tty_open() blocks
> on tty_lock() before calling tty_reopen(). AFAICT, this is independent
> of O_NONBLOCK.

That would be a bug then. Returning -EIO is fine (if unfriendly). The
O_NONBLOCK can't block in this case though because the port could take a
long time to give up trying to dribble its bits (up to 30 seconds or so)

Alan

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

On 06/17/2014 07:32 AM, Peter Hurley wrote:
> On 06/17/2014 07:03 AM, David Laight wrote:
>> From: Peter Hurley
>> ...
>>>> I don't understand the second half of the changelog, it doesn't seem
>>>> to fit here: there deadlock that we are trying to avoid here happens
>>>> when the *same* tty needs the lock to complete the function that
>>>> sends the pending data. I don't think we do still do that any more,
>>>> but it doesn't seem related to the tty lock being system-wide or not.
>>>
>>> The tty lock is not used in the i/o path; it's purpose is to
>>> mutually exclude state changes in open(), close() and hangup().
>>>
>>> The commit that added this [1] comments that _other_ ttys may wait
>>> for this tty to complete, and comments in the code note that this
>>> function should be removed when the system-wide tty mutex was removed
>>> (which happened with the commit noted in the changelog).
>>
>> What happens if another process tries to do a non-blocking open
>> while you are sleeping in close waiting for output to drain?
>>
>> Hopefully this returns before that data has drained.
>
> Good point.
>
> tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.

Further, the tty lock should not be nested within the tty_mutex lock
in a reopen, regardless of O_NONBLOCK.

AFAICT, the tty_mutex in the reopen scenario is only protecting the
tty count bump of the linked tty (if the tty is a pty).

I think with some refactoring and returning with a tty reference held
from both tty_open_current_tty() and tty_driver_lookup_tty(), the tty
lock in tty_open() can be attempted without nesting in the tty_mutex.

Regardless, I'll be splitting this series and I'll be sure to cc
you all when I resubmit these changes (after testing).

Regards,
Peter Hurley

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Greg Kroah-Hartman]

On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
> tty_wait_until_sent_from_close() drops the tty lock while waiting
> for the tty driver to finish sending previously accepted data (ie.,
> data remaining in its write buffer and transmit fifo).
> 
> However, dropping the tty lock is a hold-over from when the tty
> lock was system-wide; ie., one lock for all ttys.
> 
> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
> 'tty: localise the lock', dropping the tty lock has not been necessary.
> 
> CC: Karsten Keil <isdn@linux-pingi.de>
> CC: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/isdn/i4l/isdn_tty.c   |  2 +-
>  drivers/tty/hvc/hvc_console.c |  2 +-
>  drivers/tty/hvc/hvcs.c        |  2 +-
>  drivers/tty/tty_port.c        | 11 ++---------
>  include/linux/tty.h           | 18 ------------------
>  5 files changed, 5 insertions(+), 30 deletions(-)

I've applied the first 13 patches in this series, as it looks like you
were going to split things up from here, right?  Can you refresh these
and resend when you have that done?

thanks,

greg k-h

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

On 07/10/2014 07:09 PM, Greg Kroah-Hartman wrote:
> On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
>> tty_wait_until_sent_from_close() drops the tty lock while waiting
>> for the tty driver to finish sending previously accepted data (ie.,
>> data remaining in its write buffer and transmit fifo).
>>
>> However, dropping the tty lock is a hold-over from when the tty
>> lock was system-wide; ie., one lock for all ttys.
>>
>> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
>> 'tty: localise the lock', dropping the tty lock has not been necessary.
>>
>> CC: Karsten Keil <isdn@linux-pingi.de>
>> CC: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/isdn/i4l/isdn_tty.c   |  2 +-
>>   drivers/tty/hvc/hvc_console.c |  2 +-
>>   drivers/tty/hvc/hvcs.c        |  2 +-
>>   drivers/tty/tty_port.c        | 11 ++---------
>>   include/linux/tty.h           | 18 ------------------
>>   5 files changed, 5 insertions(+), 30 deletions(-)
>
> I've applied the first 13 patches in this series, as it looks like you
> were going to split things up from here, right?

Yes, thanks for doing that.

> Can you refresh these and resend when you have that done?

Unfortunately, that probably won't be until after the 3.17 merge window,
for 3.18. The tty_open() rework is not trivial and there is an issue
with the ldisc flush removal patch.

I'm hoping to include the tty flow control fixes with that stuff as well.

Regards,
Peter Hurley

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[Peter Hurley]

On 06/17/2014 07:03 AM, David Laight wrote:
> From: Peter Hurley
> ...
>>> I don't understand the second half of the changelog, it doesn't seem
>>> to fit here: there deadlock that we are trying to avoid here happens
>>> when the *same* tty needs the lock to complete the function that
>>> sends the pending data. I don't think we do still do that any more,
>>> but it doesn't seem related to the tty lock being system-wide or not.
>>
>> The tty lock is not used in the i/o path; it's purpose is to
>> mutually exclude state changes in open(), close() and hangup().
>>
>> The commit that added this [1] comments that _other_ ttys may wait
>> for this tty to complete, and comments in the code note that this
>> function should be removed when the system-wide tty mutex was removed
>> (which happened with the commit noted in the changelog).

I just wanted to revisit this discussion briefly so I can clarify the
situation regarding holding the tty lock while closing, and how that
affects parallel opens.

I've unnested the tty lock from the tty mutex (which I'm still testing)
but will be submitting after the merge window re-opens for 3.19. So this
is more relevant now.

The original patch that led to this thread is here:
https://lkml.org/lkml/2014/6/16/306


> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
> 
> Hopefully this returns before that data has drained.

Current mainline blocks on _any_ racing re-open while this lock is
dropped in tty_wait_until_sent_from_close(); blocking while
ASYNC_CLOSING has been in mainline since at least 2.6.29 and that just
merged existing code together. See tty_port_block_til_ready(); note
the test for O_NONBLOCK is after the wait while ASYNC_CLOSING.

IOW, currently a non-blocking open will sleep for the _entire_ duration
of a parallel hardware shutdown, and when it wakes, the error return will
cause a release of its tty, and it will restart with a fresh attempt
to open. Same with a blocking open that is already waiting; when its
woken the hardware shutdown has already completed so ASYNC_INITIALIZED
is cleared, which forces a release and restart too.

The point being that holding the tty lock across the _entire_ close
is equivalent to the current outcome, regardless of O_NONBLOCK.

I'm reluctant to start returning EGAIN for non-blocking tty opens
because no tty driver does that now, and I don't think userspace will
deal well with new return codes from tty opens.

Regards,
Peter Hurley

Re: [PATCH tty-next 14/22] tty: Remove tty_wait_until_sent_from_close()

[One Thousand Gnomes]

> The point being that holding the tty lock across the _entire_ close
> is equivalent to the current outcome, regardless of O_NONBLOCK.
> 
> I'm reluctant to start returning EGAIN for non-blocking tty opens
> because no tty driver does that now, and I don't think userspace will
> deal well with new return codes from tty opens.

I do not know about the non blocking case mattering. The blocking open
does need to wait, when I broke that case before I broke the console
login drivers (mingetty).

Returning EAGAIN would also only work if poll/select did the right thing.
Currently Linux can't support a System5 style ttymon process because of
this limitation, which means, for example, that systemd can't implement a
single thread to manage all console prompts/setup

Alan