[PATCH 1/5] TTY: serial, use ASYNCB_CLOSING in uart_close

Subsystems: the rest, tty layer and serial drivers

STALE5476d

6 messages, 2 authors, 2011-08-25 · open the first message on its own page

[PATCH 1/5] TTY: serial, use ASYNCB_CLOSING in uart_close

From: Jiri Slaby <hidden>
Date: 2011-08-25 13:12:25

We need to move port->mutex locking after wait_until_sent in
uart_close (for rationale see next patches). But if we did it now, we
would introduce a race between close and open. This is exactly why
port->mutex is locked at the top of uart_close.

To avoid the race, we add ASYNCB_CLOSING to uart_close. Like every
other sane TTY driver. Thanks to tty_port_block_til_ready used in
uart_open we will have this for free. Then we can move the port->mutex
lock.

Also note that this will make the conversion to tty_port helpers
easier. They are currently handling ASYNC_CLOSING flag correctly.

Signed-off-by: Jiri Slaby <redacted>
---
 drivers/tty/serial/serial_core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 781d566..cd7a0bb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c

@@ -1293,6 +1293,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	 * the line discipline to only process XON/XOFF characters by
 	 * setting tty->closing.
 	 */
+	set_bit(ASYNCB_CLOSING, &port->flags);
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);

@@ -1340,8 +1341,10 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	 * Wake up anyone trying to open this port.
 	 */
 	clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
+	clear_bit(ASYNCB_CLOSING, &port->flags);
 	spin_unlock_irqrestore(&port->lock, flags);
 	wake_up_interruptible(&port->open_wait);
+	wake_up_interruptible(&port->close_wait);
 
 done:
 	mutex_unlock(&port->mutex);

-- 
1.7.6

[PATCH 4/5] TTY: use tty_wait_until_sent_from_close in tty_port_close_start

From: Jiri Slaby <hidden>
Date: 2011-08-25 13:12:19

Let's use the newly added helper to avoid stalls in drivers which are
already ported to tty_port helpers.

We have to ensure here, that there is no user of tty_port_close_start
and tty_port_close which holds port->mutex (or other) lock over them.
And sure, there is none.

Signed-off-by: Jiri Slaby <redacted>
---
 drivers/tty/tty_port.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 33d37d2..ef9dd62 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c

@@ -350,7 +350,7 @@ int tty_port_close_start(struct tty_port *port,
 		tty_driver_flush_buffer(tty);
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
 			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, port->closing_wait);
+		tty_wait_until_sent_from_close(tty, port->closing_wait);
 	if (port->drain_delay) {
 		unsigned int bps = tty_get_baud_rate(tty);
 		long timeout;

-- 
1.7.6

[PATCH 3/5] TTY: define tty_wait_until_sent_from_close

From: Jiri Slaby <hidden>
Date: 2011-08-25 13:12:23

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 <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/tty.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index a7720a4..a0bf319 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h

@@ -604,6 +604,24 @@ extern void __lockfunc tty_lock(void) __acquires(tty_lock);
 extern void __lockfunc tty_unlock(void) __releases(tty_lock);
 
 /*
+ * 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->ops->close holds the BTM, drop it while waiting */
+	tty_wait_until_sent(tty, timeout);
+	tty_lock();
+}
+
+/*
  * 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

-- 
1.7.6

[PATCH 2/5] TTY: serial, move locking in uart_close

From: Jiri Slaby <hidden>
Date: 2011-08-25 13:12:28

So now, when we handle CLOSING flag, there is no point to hold
port->mutex over the start of uart_close.

Yes, there are still several things to reason about:
* port->count etc is and always was protected by a spinlock
* ->stop_rx is protected by a spinlock. Otherwise it would
  race with interrupts.
* uart_wait_until_sent -- that one is already called without
  port->mutex from set_termios and tty_set_ldisc. Should anything
  be protected there, it would be tx_empty. And by a spinlock.
  8250 does this internally...

This step is needed to fix system stalls. To not create an AB-BA lock
dependency (see next patches).

Signed-off-by: Jiri Slaby <redacted>
---
 drivers/tty/serial/serial_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index cd7a0bb..e1bff1b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c

@@ -1258,7 +1258,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 
 	pr_debug("uart_close(%d) called\n", uport->line);
 
-	mutex_lock(&port->mutex);
 	spin_lock_irqsave(&port->lock, flags);
 
 	if (tty_hung_up_p(filp)) {

@@ -1317,6 +1316,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 		uart_wait_until_sent(tty, uport->timeout);
 	}
 
+	mutex_lock(&port->mutex);
 	uart_shutdown(tty, state);
 	uart_flush_buffer(tty);

-- 
1.7.6

[PATCH 5/5] TTY: use tty_wait_until_sent_from_close in other drivers

From: Jiri Slaby <hidden>
Date: 2011-08-25 13:13:16

Let's use the newly added helper to avoid stalls in drivers which are
not yet ported to tty_port helpers.

Those which are broken (call tty_wait_until_sent with irqs disabled)
are left untouched. They are in a deeper trouble than we are trying to
solve here. This includes amiserial, 68328serial, 68360serial and
crisv10.

Signed-off-by: Jiri Slaby <redacted>
---
 drivers/isdn/i4l/isdn_tty.c      |    2 +-
 drivers/tty/hvc/hvc_console.c    |    2 +-
 drivers/tty/hvc/hvcs.c           |    2 +-
 drivers/tty/serial/serial_core.c |    3 ++-
 net/irda/ircomm/ircomm_tty.c     |    2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index d850427..e5546cb 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c

@@ -1693,7 +1693,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
 	 * line status register.
 	 */
 	if (info->flags & ISDN_ASYNC_INITIALIZED) {
-		tty_wait_until_sent(tty, 3000);	/* 30 seconds timeout */
+		tty_wait_until_sent_from_close(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 e1aaf4f..b6b2d18 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c

@@ -388,7 +388,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(tty, HVC_CLOSE_WAIT);
+		tty_wait_until_sent_from_close(tty, HVC_CLOSE_WAIT);
 	} else {
 		if (hp->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 4c8b665..e523773 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c

@@ -1237,7 +1237,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(tty, HVCS_CLOSE_WAIT);
+		tty_wait_until_sent_from_close(tty, HVCS_CLOSE_WAIT);
 
 		/*
 		 * This line is important because it tells hvcs_open that this

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e1bff1b..5c04cb9 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c

@@ -1297,7 +1297,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, msecs_to_jiffies(port->closing_wait));
+		tty_wait_until_sent_from_close(tty,
+				msecs_to_jiffies(port->closing_wait));
 
 	/*
 	 * At this point, we stop accepting input.  To do this, we

diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index b3cc8b3..253695d 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c

@@ -551,7 +551,7 @@ static void ircomm_tty_close(struct tty_struct *tty, struct file *filp)
 	 */
 	tty->closing = 1;
 	if (self->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, self->closing_wait);
+		tty_wait_until_sent_from_close(tty, self->closing_wait);
 
 	ircomm_tty_shutdown(self);

-- 
1.7.6

Re: [PATCH 1/5] TTY: serial, use ASYNCB_CLOSING in uart_close

From: Arnd Bergmann <arnd@arndb.de>
Date: 2011-08-25 15:15:40

On Thursday 25 August 2011, Jiri Slaby wrote:

We need to move port->mutex locking after wait_until_sent in
uart_close (for rationale see next patches). But if we did it now, we
would introduce a race between close and open. This is exactly why
port->mutex is locked at the top of uart_close.

To avoid the race, we add ASYNCB_CLOSING to uart_close. Like every
other sane TTY driver. Thanks to tty_port_block_til_ready used in
uart_open we will have this for free. Then we can move the port->mutex
lock.

Also note that this will make the conversion to tty_port helpers
easier. They are currently handling ASYNC_CLOSING flag correctly.

Signed-off-by: Jiri Slaby <redacted>

The series looks good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

The only thing left to note is probably that there are a bunch
of drivers that implement their own *_wait_until_sent function
and call that from their *_close function instead of calling
tty_wait_until_sent. These drivers continue to suffer from the
same stalls that you are fixing the other ones.

	Arnd

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help