Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1305471 > unrolled thread

[PATCH v2 0/7] More n_tty fixes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/7] More n_tty fixes Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 2/7] tty, n_tty: Remove fasync() ldisc notification Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 5/7] n_tty: Fix stuck write wakeup Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 3/7] tty: Add fasync() hung up file operation Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 6/7] n_tty: Remove tty count checks from unthrottle Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 4/7] tty: Fix ioctl(FIOASYNC) on hungup file Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100
    [PATCH v2 7/7] tty: n_tty: fix SIGIO for output Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 06:50 +0100

#1305471 — [PATCH v2 0/7] More n_tty fixes

Changes from v2:
	added patch 7 from Johannes Stezenbach which fixes the
	TTY_DO_WRITE_WAKEUP logic to prevent missed SIGIO and reduce
	excessive SIGIO

original message follows:

Hi Greg,

This series collects up several fixes for N_TTY.

The first patch fixes read() when VMIN > 0 & VTIME = 0 (so called "record mode").
By ripping out a marginal optimization, the overall code is greatly simplified
and "record mode" read()s won't hang :)

Patch 2 removes the pointless fasync() notification to line disciplines.
Patches 3,4 & 5 fix hangup races with enabling/disabling signal-driven i/o.
Patch 6 is a minor cleanup for a condition test that can never be true.

Peter Hurley (7):
  n_tty: Always wake up read()/poll() if new input
  tty, n_tty: Remove fasync() ldisc notification
  tty: Add fasync() hung up file operation
  tty: Fix ioctl(FIOASYNC) on hungup file
  n_tty: Fix stuck write wakeup
  n_tty: Remove tty count checks from unthrottle
  tty: n_tty: fix SIGIO for output

 drivers/tty/n_tty.c       | 49 +++++------------------------------------------
 drivers/tty/tty_io.c      | 19 +++++++++---------
 include/linux/tty_ldisc.h |  6 ------
 3 files changed, 14 insertions(+), 60 deletions(-)

-- 
2.7.0

[toc] | [next] | [standalone]

#1305472 — [PATCH v2 2/7] tty, n_tty: Remove fasync() ldisc notification

Only the N_TTY line discipline implements the signal-driven i/o
notification enabled/disabled by fcntl(F_SETFL, O_ASYNC). The ldisc
fasync() notification is sent to the ldisc when the enable state has
changed (the tty core is notified via the fasync() VFS file operation).

The N_TTY line discipline used the enable state to change the wakeup
condition (minimum_to_wake = 1) for notifying the signal handler i/o is
available. However, just the presence of data is sufficient and necessary
to signal i/o is available, so changing minimum_to_wake is unnecessary
(and creates a race condition with read() and poll() which may be
concurrently updating minimum_to_wake).

Furthermore, since the kill_fasync() VFS helper performs no action if
the fasync list is empty, calling unconditionally is preferred; if
signal driven i/o just has been disabled, no signal will be sent by
kill_fasync() anyway so notification of the change via the ldisc
fasync() method is superfluous.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c       | 5 -----
 drivers/tty/tty_io.c      | 8 --------
 include/linux/tty_ldisc.h | 6 ------
 3 files changed, 19 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d04f398..a93e4c7 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2448,10 +2448,6 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
-static void n_tty_fasync(struct tty_struct *tty, int on)
-{
-}
-
 static struct tty_ldisc_ops n_tty_ops = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",
@@ -2465,7 +2461,6 @@ static struct tty_ldisc_ops n_tty_ops = {
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
 	.write_wakeup    = n_tty_write_wakeup,
-	.fasync		 = n_tty_fasync,
 	.receive_buf2	 = n_tty_receive_buf2,
 };
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 48281a5..e8b431e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2219,7 +2219,6 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;
 
@@ -2230,13 +2229,6 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
-	ldisc = tty_ldisc_ref(tty);
-	if (ldisc) {
-		if (ldisc->ops->fasync)
-			ldisc->ops->fasync(tty, on);
-		tty_ldisc_deref(ldisc);
-	}
-
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 6101ab8..3971cf0 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -98,11 +98,6 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
- * void (*fasync)(struct tty_struct *, int on)
- *
- *	Notify line discipline when signal-driven I/O is enabled or
- *	disabled.
- *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.
@@ -202,7 +197,6 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
-	void	(*fasync)(struct tty_struct *tty, int on);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				char *fp, int count);
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1305473 — [PATCH v2 5/7] n_tty: Fix stuck write wakeup

If signal-driven i/o is disabled while write wakeup is pending (ie.,
n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
will cause tty_wakeup() to always call n_tty_write_wakeup.

Unconditionally clear the write wakeup, and since kill_fasync()
already checks if the fasync ptr is null, call kill_fasync()
unconditionally as well.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index a93e4c7..086fd99 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -228,8 +228,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
 
 static void n_tty_check_throttle(struct tty_struct *tty)
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1305474 — [PATCH v2 3/7] tty: Add fasync() hung up file operation

VFS uses a two-stage check-and-call method for invoking file_operations
methods, without explicitly snapshotting either the file_operations ptr
or the function ptr. Since the tty core is one of the few VFS users that
changes the f_op file_operations ptr of the file descriptor (when the
tty has been hung up), and since the likelihood of the compiler generating
a reload of either f_op or the function ptr is basically nil, just define
a hung up fasync() file operation that returns an error.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e8b431e..05b0a9c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -468,6 +468,11 @@ static long hung_up_tty_compat_ioctl(struct file *file,
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
 
+static int hung_up_tty_fasync(int fd, struct file *file, int on)
+{
+	return -ENOTTY;
+}
+
 static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read		= tty_read,
@@ -500,6 +505,7 @@ static const struct file_operations hung_up_tty_fops = {
 	.unlocked_ioctl	= hung_up_tty_ioctl,
 	.compat_ioctl	= hung_up_tty_compat_ioctl,
 	.release	= tty_release,
+	.fasync		= hung_up_tty_fasync,
 };
 
 static DEFINE_SPINLOCK(redirect_lock);
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1305475 — [PATCH v2 6/7] n_tty: Remove tty count checks from unthrottle

Since n_tty_check_unthrottle() is only called from n_tty_read()
which only originates from a userspace read(), the tty count cannot
be 0; the read() guarantees the file descriptor has not yet been
released.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 086fd99..5ae661c 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -261,8 +261,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			return;
-		if (!tty->count)
-			return;
 		n_tty_kick_worker(tty);
 		tty_wakeup(tty->link);
 		return;
@@ -281,8 +279,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			break;
-		if (!tty->count)
-			break;
 		n_tty_kick_worker(tty);
 		unthrottled = tty_unthrottle_safe(tty);
 		if (!unthrottled)
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1305476 — [PATCH v2 4/7] tty: Fix ioctl(FIOASYNC) on hungup file

A small race window exists which allows signal-driven async i/o to be
enabled for the tty when the file ptr has already been hungup and
signal-driven i/o has been disabled:

CPU 0                                CPU 1
-----                                ------
ioctl_fioasync(on)
  filp->f_op->fasync(on)             __tty_hangup()
    tty_fasync(on)                     tty_lock()
      tty_lock()                       ...
        .                              filp->f_op = &hung_up_tty_fops;
      (waiting)                       __tty_fasync(off)
        .                              tty_unlock()
      /* gets tty lock  */
      /* enables FASYNC */

Check the tty has not been hungup while holding tty_lock.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 05b0a9c..7113bfa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2260,10 +2260,11 @@ out:
 static int tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
-	int retval;
+	int retval = -ENOTTY;
 
 	tty_lock(tty);
-	retval = __tty_fasync(fd, filp, on);
+	if (!tty_hung_up_p(filp))
+		retval = __tty_fasync(fd, filp, on);
 	tty_unlock(tty);
 
 	return retval;
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1305477 — [PATCH v2 7/7] tty: n_tty: fix SIGIO for output

According to fcntl(2), "a SIGIO signal is sent whenever input
or output becomes possible on that file descriptor", i.e.
after the output buffer was full and now has space for new data.
But in fact SIGIO is sent after every write.

n_tty_write() should set TTY_DO_WRITE_WAKEUP only when
not all data could be written to the buffer.

[pjh: Also fixes missed SIGIO if amt written just happens to be
[     amount still to write

Signed-off-by: Johannes Stezenbach <js@sig21.net>
[pjh: minor patch edits and re-submit]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 5ae661c..fad365a 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2361,7 +2361,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 	}
 break_out:
 	remove_wait_queue(&tty->write_wait, &wait);
-	if (b - buf != nr && tty->fasync)
+	if (nr && tty->fasync)
 		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	up_read(&tty->termios_rwsem);
 	return (b - buf) ? b - buf : retval;
-- 
2.7.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web