Replace tty->closing

PATCH Superseded Peter Hurley 3 participants 14 messages Nov 8, 2015 – Dec 14, 2015

Subsystems (7)

Networking Network drivers Staging s390 +3 more subsystems

lore

Share & tools

lore

Peter Hurley

author

2015-11-08 22:02 UTC #0 lore raw

cover Replace tty->closing

Hi Greg,

This series cleans up a messy and poorly documented mechanism required at tty final close to prevent drivers from crashing after h/w shutdown.

Without special handling, N_TTY echoing will cause driver i/o requests _after_ h/w shutdown, which typically crashes the driver. Currently, the tty_struct::closing flag triggers this special handling. However, this mechanism is error-prone and subject to driver misuse.

This series replaces tty->closing with a ldisc-specific interface, tty_ldisc_closing(), and implements this interface for N_TTY.
For tty drivers which use tty_port_close_start(), this change eliminates the last vestige of direct driver<->ldisc interaction. The few tty drivers which open-code the close() method still use [1] tty_ldisc_closing() directly.

The tty driver is aware final close for the tty is commencing because the tty->count == 1 in the close() method. On final close, the following is also true:
1. port->count == 1. tty drivers which ref count the port, use the --port->count == 0 as a substitute condition for final close.
2. final close is occurring as a result of the last in-use file descriptor release. Consequently, there will be no read/poll/ioctl in-progress.
3. the line discipline instance will be stopped and destroyed immediately after the tty driver completes the close() method
4. the tty itself will be unrefed immediately after the line discipline instance is destroyed.

Thus, the ldisc and tty buffers need only be flushed once, as any data received by the tty driver after the flush but before h/w shutdown will be deleted when the line discipline instance is destroyed. No new echoes will occur after the ldisc flush because the echo buffer is also flushed and new input (which otherwise might generate echoes) is ignored while closing. This series removes the extra tty_ldisc_flush() being performed by most drivers after h/w shutdown.

Additionally, the ldisc closing state need not be reset since the line discipline instance is being destroyed, so no interface is provided to reset closing.

[1] tty drivers which open-code the close() method drivers/staging/dgnc/dgnc_tty.c drivers/staging/dgap/dgap.c drivers/tty/hvc/hvsi.c drivers/tty/serial/68328serial.c drivers/tty/serial/crisv10.c drivers/isdn/i4l/isdn_tty.c drivers/s390/char/con3215.c

Regards,

Peter Hurley (4):
tty: rocket: Remove private close_wait
n_tty: Ignore all read data when closing
tty: Abstract and encapsulate tty->closing behavior
tty: Remove drivers' extra tty_ldisc_flush()

 drivers/char/pcmcia/synclink_cs.c |  3 ---
 drivers/isdn/i4l/isdn_tty.c       |  2 +-
 drivers/s390/char/con3215.c       |  3 +--
 drivers/staging/dgap/dgap.c       |  4 +---
 drivers/staging/dgnc/dgnc_tty.c   |  4 +---
 drivers/tty/amiserial.c           |  2 --
 drivers/tty/hvc/hvsi.c            |  2 +-
 drivers/tty/ipwireless/tty.c      |  1 -
 drivers/tty/n_tty.c               | 15 +++++++++++----
 drivers/tty/rocket.c              |  5 -----
 drivers/tty/rocket_int.h          |  1 -
 drivers/tty/serial/68328serial.c  |  3 +--
 drivers/tty/serial/crisv10.c      |  3 +--
 drivers/tty/serial/serial_core.c  |  3 ---
 drivers/tty/synclink.c            |  1 -
 drivers/tty/synclink_gt.c         |  1 -
 drivers/tty/synclinkmp.c          |  1 -
 drivers/tty/tty_ldisc.c           | 19 +++++++++++++++++++
 drivers/tty/tty_port.c            |  5 +----
 include/linux/tty.h               |  2 +-
 include/linux/tty_ldisc.h         |  9 +++++++++
 21 files changed, 48 insertions(+), 41 deletions(-)

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-08 22:02 UTC #1 lore raw

PATCH 1/4 tty: rocket: Remove private close_wait

This driver's private completion variable, close_wait, is no longer used for wait since "tty: Remove ASYNC_CLOSING checks in open()/hangup"; remove.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/rocket.c     | 2 --
 drivers/tty/rocket_int.h | 1 -
 2 files changed, 3 deletions(-)

drivers/tty/rocket.c +0 -2

index 802eac7..c5179f8 100644
@@ -643,7 +643,6 @@ static void init_r_port(int board, int aiop, int chan, struct pci_dev *pci_dev)
 	info->chan = chan;
 	tty_port_init(&info->port);
 	info->port.ops = &rocket_port_ops;
-	init_completion(&info->close_wait);
 	info->flags &= ~ROCKET_MODE_MASK;
 	switch (pc104[board][line]) {
 	case 422:

@@ -1049,7 +1048,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 	mutex_unlock(&port->mutex);
 	tty_port_tty_set(port, NULL);
 
-	complete_all(&info->close_wait);
 	atomic_dec(&rp_num_ports_open);
 
 #ifdef ROCKET_DEBUG_OPEN

drivers/tty/rocket_int.h +0 -1

index 67e0f1e..ef1e1be 100644
@@ -1144,7 +1144,6 @@ struct r_port {
 	int read_status_mask;
 	int cps;
 
-	struct completion close_wait;	/* Not yet matching the core */
 	spinlock_t slock;
 	struct mutex write_mtx;
 };

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-08 22:02 UTC #2 lore raw

PATCH 2/4 n_tty: Ignore all read data when closing

In reply to Peter Hurley

On final port close (and thus final tty close), only output flow control requests in the input data should be processed. Ignore all other input data, including parity errors, overruns and breaks.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 2 --
 1 file changed, 2 deletions(-)

drivers/tty/n_tty.c +0 -2

index bc613b8..2de0283 100644
@@ -1564,8 +1564,6 @@ n_tty_receive_buf_closing(struct tty_struct *tty, const unsigned char *cp,
 			flag = *fp++;
 		if (likely(flag == TTY_NORMAL))
 			n_tty_receive_char_closing(tty, *cp++);
-		else
-			n_tty_receive_char_flagged(tty, *cp++, flag);
 	}
 }

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-08 22:02 UTC #3 lore raw

PATCH 3/4 tty: Abstract and encapsulate tty->closing behavior

In reply to Peter Hurley

tty->closing is exclusively used to cause the N_TTY line discipline to drop further input on final tty close (except XON/XOFF output flow control changes). In turn, this prevents the line discipline from generating new tty driver i/o requests (eg., when echoing) after the tty driver has performed h/w shutdown.

Abstract this notification with new ldisc api function, tty_ldisc_closing(), which invokes the new line discipline method, ops->closing(). Define this method for N_TTY line discipline and localize closing state to n_tty private data. Remove tty->closing.

Note that resetting tty->closing to 0 (and thus allowing the line discipline to resume normal input processing) is unnecessary and undesirable: since the tty is in final close, both the line discipline instance and the tty are being destroyed, so resuming normal input processing after h/w shutdown is counter-productive.

NB: ipwireless_tty_free() is completely bogus; freeing the tty (?!) with open, in-use file descriptors is laughable.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/isdn/i4l/isdn_tty.c      |  2 +-
 drivers/s390/char/con3215.c      |  3 +--
 drivers/staging/dgap/dgap.c      |  4 +---
 drivers/staging/dgnc/dgnc_tty.c  |  4 +---
 drivers/tty/hvc/hvsi.c           |  2 +-
 drivers/tty/ipwireless/tty.c     |  1 -
 drivers/tty/n_tty.c              | 13 +++++++++++--
 drivers/tty/rocket.c             |  1 -
 drivers/tty/serial/68328serial.c |  3 +--
 drivers/tty/serial/crisv10.c     |  3 +--
 drivers/tty/serial/serial_core.c |  1 -
 drivers/tty/tty_ldisc.c          | 19 +++++++++++++++++++
 drivers/tty/tty_port.c           |  3 +--
 include/linux/tty.h              |  2 +-
 include/linux/tty_ldisc.h        |  9 +++++++++
 15 files changed, 48 insertions(+), 22 deletions(-)

drivers/isdn/i4l/isdn_tty.c +1 -1

index 2175225..cddba25 100644
@@ -1574,7 +1574,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
 	}
 	port->flags |= ASYNC_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	/*
 	 * At this point we stop accepting input.  To do this, we
 	 * disable the receive line status interrupts, and tell the

drivers/s390/char/con3215.c +1 -2

index 0fc3fe5..715251d 100644
@@ -1006,11 +1006,10 @@ static void tty3215_close(struct tty_struct *tty, struct file * filp)
 	raw = (struct raw3215_info *) tty->driver_data;
 	if (raw == NULL || tty->count > 1)
 		return;
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	/* Shutdown the terminal */
 	raw3215_shutdown(raw);
 	tasklet_kill(&raw->tlet);
-	tty->closing = 0;
 	tty_port_tty_set(&raw->port, NULL);
 }

drivers/staging/dgap/dgap.c +1 -3

index 9112dd2..0456e28 100644
@@ -4622,7 +4622,7 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
 
 	un->un_flags |= UN_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 	/*
 	 * Only officially close channel if count is 0 and

@@ -4645,8 +4645,6 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
 
 		spin_lock_irqsave(&ch->ch_lock, lock_flags);
 
-		tty->closing = 0;
-
 		/*
 		 * If we have HUPCL set, lower DTR and RTS
 		 */

drivers/staging/dgnc/dgnc_tty.c +1 -3

index fbfe79a..96960d8 100644
@@ -1410,7 +1410,7 @@ static void dgnc_tty_close(struct tty_struct *tty, struct file *file)
 	/* OK, its the last close on the unit */
 	un->un_flags |= UN_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 
 	/*

@@ -1441,8 +1441,6 @@ static void dgnc_tty_close(struct tty_struct *tty, struct file *file)
 
 		spin_lock_irqsave(&ch->ch_lock, flags);
 
-		tty->closing = 0;
-
 		/*
 		 * If we have HUPCL set, lower DTR and RTS
 		 */

drivers/tty/hvc/hvsi.c +1 -1

index a75146f..fbaa6ab 100644
@@ -796,7 +796,7 @@ static void hvsi_close(struct tty_struct *tty, struct file *filp)
 			 * any data delivered to the tty layer after this will be
 			 * discarded (except for XON/XOFF)
 			 */
-			tty->closing = 1;
+			tty_ldisc_closing(tty);
 
 			spin_unlock_irqrestore(&hp->lock, flags);

drivers/tty/ipwireless/tty.c +0 -1

index 345cebb..2ed8831 100644
@@ -536,7 +536,6 @@ void ipwireless_tty_free(struct ipw_tty *tty)
 				printk(KERN_INFO IPWIRELESS_PCCARD_NAME
 				       ": deregistering %s device ttyIPWp%d\n",
 				       tty_type_name(ttyj->tty_type), j);
-			ttyj->closing = 1;
 			if (ttyj->port.tty != NULL) {
 				mutex_unlock(&ttyj->ipw_tty_mutex);
 				tty_vhangup(ttyj->port.tty);

drivers/tty/n_tty.c +11 -2

index 2de0283..6342b37 100644
@@ -114,6 +114,7 @@ struct n_tty_data {
 	unsigned char echo_buf[N_TTY_BUF_SIZE];
 
 	int minimum_to_wake;
+	int closing;
 
 	/* consumer-published */
 	size_t read_tail;

@@ -1637,7 +1638,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 		n_tty_receive_buf_real_raw(tty, cp, fp, count);
 	else if (ldata->raw || (L_EXTPROC(tty) && !preops))
 		n_tty_receive_buf_raw(tty, cp, fp, count);
-	else if (tty->closing && !L_EXTPROC(tty))
+	else if (ldata->closing && !L_EXTPROC(tty))
 		n_tty_receive_buf_closing(tty, cp, fp, count);
 	else {
 		if (ldata->lnext) {

@@ -1942,7 +1943,7 @@ static int n_tty_open(struct tty_struct *tty)
 	ldata->num_overrun = 0;
 	ldata->no_room = 0;
 	ldata->lnext = 0;
-	tty->closing = 0;
+	ldata->closing = 0;
 	/* indicate buffer work may resume */
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);
 	n_tty_set_termios(tty, NULL);

@@ -2525,6 +2526,13 @@ static void n_tty_fasync(struct tty_struct *tty, int on)
 	}
 }
 
+static void n_tty_closing(struct tty_struct *tty)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	ldata->closing = 1;
+}
+
 struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2541,6 +2549,7 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.write_wakeup    = n_tty_write_wakeup,
 	.fasync		 = n_tty_fasync,
 	.receive_buf2	 = n_tty_receive_buf2,
+	.closing	 = n_tty_closing,
 };
 
 /**

drivers/tty/rocket.c +0 -1

index c5179f8..a6b5ce0 100644
@@ -1043,7 +1043,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 	}
 	spin_lock_irq(&port->lock);
 	info->port.flags &= ~(ASYNC_INITIALIZED | ASYNC_CLOSING | ASYNC_NORMAL_ACTIVE);
-	tty->closing = 0;
 	spin_unlock_irq(&port->lock);
 	mutex_unlock(&port->mutex);
 	tty_port_tty_set(port, NULL);

drivers/tty/serial/68328serial.c +1 -2

index 0140ba4..9a6aaf0 100644
@@ -1035,7 +1035,7 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	 * Now we wait for the transmit buffer to clear; and we notify 
 	 * the line discipline to only process XON/XOFF characters.
 	 */
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent(tty, port->closing_wait);
 	/*

@@ -1052,7 +1052,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	rs_flush_buffer(tty);
 		
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 	tty_port_tty_set(&info->tport, NULL);
 #warning "This is not and has never been valid so fix it"	
 #if 0

drivers/tty/serial/crisv10.c +1 -2

index f13f2eb..0100410 100644
@@ -3620,7 +3620,7 @@ rs_close(struct tty_struct *tty, struct file * filp)
 	 * Now we wait for the transmit buffer to clear; and we notify
 	 * the line discipline to only process XON/XOFF characters.
 	 */
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	if (info->port.closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent(tty, info->port.closing_wait);
 	/*

@@ -3646,7 +3646,6 @@ rs_close(struct tty_struct *tty, struct file * filp)
 	shutdown(info);
 	rs_flush_buffer(tty);
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 	info->event = 0;
 	info->port.tty = NULL;
 	if (info->port.blocked_open) {

drivers/tty/serial/serial_core.c +0 -1

index def5199..db27a40 100644
@@ -1441,7 +1441,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	mutex_unlock(&port->mutex);
 
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)

drivers/tty/tty_ldisc.c +19 -0

index b776f2e..ef6af5f 100644
@@ -407,6 +407,25 @@ void tty_ldisc_flush(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_ldisc_flush);
 
 /**
+ *	tty_ldisc_closing	-	notify line discipline of final close
+ *	@tty: tty
+ *
+ *	Notify the line discipline the driver has begun final close of the tty.
+ *	The N_TTY line discipline uses this notification to ignore new input
+ */
+
+void tty_ldisc_closing(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+
+	if (ld->ops->closing)
+		ld->ops->closing(tty);
+	if (ld)
+		tty_ldisc_deref(ld);
+}
+EXPORT_SYMBOL_GPL(tty_ldisc_closing);
+
+/**
  *	tty_set_termios_ldisc		-	set ldisc field
  *	@tty: tty structure
  *	@num: line discipline number

drivers/tty/tty_port.c +1 -2

index a76aec2..ecb6435 100644
@@ -479,7 +479,7 @@ int tty_port_close_start(struct tty_port *port,
 	set_bit(ASYNCB_CLOSING, &port->flags);
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
 		/* Don't block on a stalled port, just pull the chain */

@@ -504,7 +504,6 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
 	unsigned long flags;
 
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 
 	spin_lock_irqsave(&port->lock, flags);

include/linux/tty.h +1 -1

index 4b6c62e..70f3a9c1 100644
@@ -306,7 +306,6 @@ struct tty_struct {
 
 #define N_TTY_BUF_SIZE 4096
 
-	int closing;
 	unsigned char *write_buf;
 	int write_cnt;
 	/* If the tty has a pending do_SAK, queue it here - akpm */

@@ -498,6 +497,7 @@ extern const struct file_operations tty_ldiscs_proc_fops;
 
 extern void tty_wakeup(struct tty_struct *tty);
 extern void tty_ldisc_flush(struct tty_struct *tty);
+extern void tty_ldisc_closing(struct tty_struct *tty);
 
 extern long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 extern int tty_mode_ioctl(struct tty_struct *tty, struct file *file,

include/linux/tty_ldisc.h +9 -0

index 00c9d68..0f3b19f 100644
@@ -125,6 +125,14 @@
  *	received with a parity error, etc. <fp> may be NULL to indicate
  *	all data received is TTY_NORMAL.
  *	If assigned, prefer this function for automatic flow control.
+ *
+ * void	(*closing)(struct tty_struct *);
+ *
+ *	This function notifies the line discipline the driver is
+ *	starting final close for this tty; tty and ldisc destruction
+ *	is imminent. The N_TTY line discipline uses this notification
+ *	to ignore new input other than IXON flow control changes.
+ *	This notification is not received for ptys or vts.
  */
 
 #include <linux/fs.h>

@@ -212,6 +220,7 @@ struct tty_ldisc_ops {
 	void	(*fasync)(struct tty_struct *tty, int on);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				char *fp, int count);
+	void	(*closing)(struct tty_struct *tty);
 
 	struct  module *owner;

Signature · 1 line

2.6.3

Johannes Stezenbach

2015-11-09 09:12 UTC #4 lore raw

Re: [PATCH 3/4] tty: Abstract and encapsulate tty->closing behavior

drivers/tty/tty_ldisc.c void tty_ldisc_flush(struct tty_struct *tty) · line 425

Quoted 9 lines · Peter Hurley

drivers/tty/tty_ldisc.c +9 -0

+void tty_ldisc_closing(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+
+	if (ld->ops->closing)
+		ld->ops->closing(tty);
+	if (ld)
+		tty_ldisc_deref(ld);
+}

That looks strange. Do you need to check ld _before_ dereferencing?

Johannes

Peter Hurley

author

2015-11-09 11:58 UTC #5 lore raw

Re: [PATCH 3/4] tty: Abstract and encapsulate tty->closing behavior

Quoted 12 lines · Johannes Stezenbach

drivers/tty/tty_ldisc.c void tty_ldisc_flush(struct tty_struct *tty) · line 425
Quoted 9 lines · Peter Hurley
drivers/tty/tty_ldisc.c +9 -0
+void tty_ldisc_closing(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+
+	if (ld->ops->closing)
+		ld->ops->closing(tty);
+	if (ld)
+		tty_ldisc_deref(ld);
+}
That looks strange. Do you need to check ld _before_ dereferencing?

Yes. Thanks for noticing.

Regards,
Peter Hurley

Peter Hurley

author

2015-11-08 22:02 UTC #6 lore raw

PATCH 4/4 tty: Remove drivers' extra tty_ldisc_flush()

In reply to Peter Hurley

The tty_port_close_start() helper already flushes the tty and ldisc buffers on final close; tty drivers which use this helper need not repeat tty_ldisc_flush().

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/char/pcmcia/synclink_cs.c | 3 ---
 drivers/tty/amiserial.c           | 2 --
 drivers/tty/rocket.c              | 2 --
 drivers/tty/serial/serial_core.c  | 2 --
 drivers/tty/synclink.c            | 1 -
 drivers/tty/synclink_gt.c         | 1 -
 drivers/tty/synclinkmp.c          | 1 -
 drivers/tty/tty_port.c            | 2 --
 8 files changed, 14 deletions(-)

drivers/char/pcmcia/synclink_cs.c +0 -3

index 45df4bf..3f74677 100644
@@ -2354,10 +2354,7 @@ static void mgslpc_close(struct tty_struct *tty, struct file * filp)
 		mgslpc_wait_until_sent(tty, info->timeout);
 
 	mgslpc_flush_buffer(tty);
-
-	tty_ldisc_flush(tty);
 	shutdown(info, tty);
-	
 	tty_port_close_end(port, tty);
 	tty_port_tty_set(port, NULL);
 cleanup:

drivers/tty/amiserial.c +0 -2

index 2caaf5a..bffc0a4 100644
@@ -1420,8 +1420,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	}
 	shutdown(tty, state);
 	rs_flush_buffer(tty);
-		
-	tty_ldisc_flush(tty);
 	port->tty = NULL;
 
 	tty_port_close_end(port, tty);

drivers/tty/rocket.c +0 -2

index a6b5ce0..5905200 100644
@@ -1022,8 +1022,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 		sClrDTR(cp);
 
 	rp_flush_buffer(tty);
-		
-	tty_ldisc_flush(tty);
 
 	clear_bit((info->aiop * 8) + info->chan, (void *) &xmit_flags[info->board]);

drivers/tty/serial/serial_core.c +0 -2

index db27a40..418587f 100644
@@ -1439,8 +1439,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	wake_up_interruptible(&port->open_wait);
 
 	mutex_unlock(&port->mutex);
-
-	tty_ldisc_flush(tty);
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)

drivers/tty/synclink.c +0 -1

index 6188059..1334498 100644
@@ -3099,7 +3099,6 @@ static void mgsl_close(struct tty_struct *tty, struct file * filp)
  	if (info->port.flags & ASYNC_INITIALIZED)
  		mgsl_wait_until_sent(tty, info->timeout);
 	mgsl_flush_buffer(tty);
-	tty_ldisc_flush(tty);
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/synclink_gt.c +0 -1

index 5505ea8..1987fb4 100644
@@ -729,7 +729,6 @@ static void close(struct tty_struct *tty, struct file *filp)
  	if (info->port.flags & ASYNC_INITIALIZED)
  		wait_until_sent(tty, info->timeout);
 	flush_buffer(tty);
-	tty_ldisc_flush(tty);
 
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/synclinkmp.c +0 -1

index fb00a06..fb17dac 100644
@@ -816,7 +816,6 @@ static void close(struct tty_struct *tty, struct file *filp)
  		wait_until_sent(tty, info->timeout);
 
 	flush_buffer(tty);
-	tty_ldisc_flush(tty);
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/tty_port.c +0 -2

index ecb6435..c30525a 100644
@@ -503,8 +503,6 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
 {
 	unsigned long flags;
 
-	tty_ldisc_flush(tty);
-
 	spin_lock_irqsave(&port->lock, flags);
 
 	if (port->blocked_open) {

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-09 12:15 UTC #7 lore raw

v2 cover Replace tty->closing

In reply to Peter Hurley

Hi Greg,

This series cleans up a messy and poorly documented mechanism required at tty final close to prevent drivers from crashing after h/w shutdown.

The tty driver is aware final close for the tty has commenced because the tty->count == 1 in the close() method. On final close, the following is also true:
1. port->count == 1. tty drivers which ref count the port, use the --port->count == 0 as a substitute condition for final close.
2. final close is occurring as a result of the last in-use file descriptor release. Consequently, there will be no read/poll/ioctl in-progress.
3. the line discipline instance will be stopped and destroyed immediately after the tty driver completes the close() method
4. the tty itself will be unrefed immediately after the line discipline instance is destroyed.

Additionally, the ldisc closing state need not be reset since the line discipline instance is being destroyed, so no interface is provided to reset closing.

Changes to v2:
* Fixed tty_ldisc_closing() ld use found by Johannes Stezenbach <[email protected]>

Regards,

 drivers/char/pcmcia/synclink_cs.c |  3 ---
 drivers/isdn/i4l/isdn_tty.c       |  2 +-
 drivers/s390/char/con3215.c       |  3 +--
 drivers/staging/dgap/dgap.c       |  4 +---
 drivers/staging/dgnc/dgnc_tty.c   |  4 +---
 drivers/tty/amiserial.c           |  2 --
 drivers/tty/hvc/hvsi.c            |  2 +-
 drivers/tty/ipwireless/tty.c      |  1 -
 drivers/tty/n_tty.c               | 15 +++++++++++----
 drivers/tty/rocket.c              |  5 -----
 drivers/tty/rocket_int.h          |  1 -
 drivers/tty/serial/68328serial.c  |  3 +--
 drivers/tty/serial/crisv10.c      |  3 +--
 drivers/tty/serial/serial_core.c  |  3 ---
 drivers/tty/synclink.c            |  1 -
 drivers/tty/synclink_gt.c         |  1 -
 drivers/tty/synclinkmp.c          |  1 -
 drivers/tty/tty_ldisc.c           | 20 ++++++++++++++++++++
 drivers/tty/tty_port.c            |  5 +----
 include/linux/tty.h               |  2 +-
 include/linux/tty_ldisc.h         |  9 +++++++++
 21 files changed, 49 insertions(+), 41 deletions(-)

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-09 12:15 UTC #8 lore raw

PATCH v2 1/4 tty: rocket: Remove private close_wait

This driver's private completion variable, close_wait, is no longer used for wait since "tty: Remove ASYNC_CLOSING checks in open()/hangup"; remove.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/rocket.c     | 2 --
 drivers/tty/rocket_int.h | 1 -
 2 files changed, 3 deletions(-)

drivers/tty/rocket.c +0 -2

index 802eac7..c5179f8 100644
@@ -643,7 +643,6 @@ static void init_r_port(int board, int aiop, int chan, struct pci_dev *pci_dev)
 	info->chan = chan;
 	tty_port_init(&info->port);
 	info->port.ops = &rocket_port_ops;
-	init_completion(&info->close_wait);
 	info->flags &= ~ROCKET_MODE_MASK;
 	switch (pc104[board][line]) {
 	case 422:

@@ -1049,7 +1048,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 	mutex_unlock(&port->mutex);
 	tty_port_tty_set(port, NULL);
 
-	complete_all(&info->close_wait);
 	atomic_dec(&rp_num_ports_open);
 
 #ifdef ROCKET_DEBUG_OPEN

drivers/tty/rocket_int.h +0 -1

index 67e0f1e..ef1e1be 100644
@@ -1144,7 +1144,6 @@ struct r_port {
 	int read_status_mask;
 	int cps;
 
-	struct completion close_wait;	/* Not yet matching the core */
 	spinlock_t slock;
 	struct mutex write_mtx;
 };

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-09 12:15 UTC #9 lore raw

PATCH v2 2/4 n_tty: Ignore all read data when closing

In reply to Peter Hurley

On final port close (and thus final tty close), only output flow control requests in the input data should be processed. Ignore all other input data, including parity errors, overruns and breaks.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 2 --
 1 file changed, 2 deletions(-)

drivers/tty/n_tty.c +0 -2

index bc613b8..2de0283 100644
@@ -1564,8 +1564,6 @@ n_tty_receive_buf_closing(struct tty_struct *tty, const unsigned char *cp,
 			flag = *fp++;
 		if (likely(flag == TTY_NORMAL))
 			n_tty_receive_char_closing(tty, *cp++);
-		else
-			n_tty_receive_char_flagged(tty, *cp++, flag);
 	}
 }

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-09 12:15 UTC #10 lore raw

PATCH v2 3/4 tty: Abstract and encapsulate tty->closing behavior

In reply to Peter Hurley

NB: ipwireless_tty_free() is completely bogus; freeing the tty (?!) with open, in-use file descriptors is laughable.

Signed-off-by Peter Hurley <[email protected]>

---

v2: Fixed tty_ldisc_closing() ld use found by Johannes Stezenbach <[email protected]>

 drivers/isdn/i4l/isdn_tty.c      |  2 +-
 drivers/s390/char/con3215.c      |  3 +--
 drivers/staging/dgap/dgap.c      |  4 +---
 drivers/staging/dgnc/dgnc_tty.c  |  4 +---
 drivers/tty/hvc/hvsi.c           |  2 +-
 drivers/tty/ipwireless/tty.c     |  1 -
 drivers/tty/n_tty.c              | 13 +++++++++++--
 drivers/tty/rocket.c             |  1 -
 drivers/tty/serial/68328serial.c |  3 +--
 drivers/tty/serial/crisv10.c     |  3 +--
 drivers/tty/serial/serial_core.c |  1 -
 drivers/tty/tty_ldisc.c          | 20 ++++++++++++++++++++
 drivers/tty/tty_port.c           |  3 +--
 include/linux/tty.h              |  2 +-
 include/linux/tty_ldisc.h        |  9 +++++++++
 15 files changed, 49 insertions(+), 22 deletions(-)

drivers/isdn/i4l/isdn_tty.c +1 -1

index 2175225..cddba25 100644
@@ -1574,7 +1574,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
 	}
 	port->flags |= ASYNC_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	/*
 	 * At this point we stop accepting input.  To do this, we
 	 * disable the receive line status interrupts, and tell the

drivers/s390/char/con3215.c +1 -2

index 0fc3fe5..715251d 100644
@@ -1006,11 +1006,10 @@ static void tty3215_close(struct tty_struct *tty, struct file * filp)
 	raw = (struct raw3215_info *) tty->driver_data;
 	if (raw == NULL || tty->count > 1)
 		return;
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	/* Shutdown the terminal */
 	raw3215_shutdown(raw);
 	tasklet_kill(&raw->tlet);
-	tty->closing = 0;
 	tty_port_tty_set(&raw->port, NULL);
 }

drivers/staging/dgap/dgap.c +1 -3

index 9112dd2..0456e28 100644
@@ -4622,7 +4622,7 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
 
 	un->un_flags |= UN_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 	/*
 	 * Only officially close channel if count is 0 and

@@ -4645,8 +4645,6 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
 
 		spin_lock_irqsave(&ch->ch_lock, lock_flags);
 
-		tty->closing = 0;
-
 		/*
 		 * If we have HUPCL set, lower DTR and RTS
 		 */

drivers/staging/dgnc/dgnc_tty.c +1 -3

index fbfe79a..96960d8 100644
@@ -1410,7 +1410,7 @@ static void dgnc_tty_close(struct tty_struct *tty, struct file *file)
 	/* OK, its the last close on the unit */
 	un->un_flags |= UN_CLOSING;
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 
 	/*

@@ -1441,8 +1441,6 @@ static void dgnc_tty_close(struct tty_struct *tty, struct file *file)
 
 		spin_lock_irqsave(&ch->ch_lock, flags);
 
-		tty->closing = 0;
-
 		/*
 		 * If we have HUPCL set, lower DTR and RTS
 		 */

drivers/tty/hvc/hvsi.c +1 -1

index a75146f..fbaa6ab 100644
@@ -796,7 +796,7 @@ static void hvsi_close(struct tty_struct *tty, struct file *filp)
 			 * any data delivered to the tty layer after this will be
 			 * discarded (except for XON/XOFF)
 			 */
-			tty->closing = 1;
+			tty_ldisc_closing(tty);
 
 			spin_unlock_irqrestore(&hp->lock, flags);

drivers/tty/ipwireless/tty.c +0 -1

index 345cebb..2ed8831 100644
@@ -536,7 +536,6 @@ void ipwireless_tty_free(struct ipw_tty *tty)
 				printk(KERN_INFO IPWIRELESS_PCCARD_NAME
 				       ": deregistering %s device ttyIPWp%d\n",
 				       tty_type_name(ttyj->tty_type), j);
-			ttyj->closing = 1;
 			if (ttyj->port.tty != NULL) {
 				mutex_unlock(&ttyj->ipw_tty_mutex);
 				tty_vhangup(ttyj->port.tty);

drivers/tty/n_tty.c +11 -2

index 2de0283..6342b37 100644
@@ -114,6 +114,7 @@ struct n_tty_data {
 	unsigned char echo_buf[N_TTY_BUF_SIZE];
 
 	int minimum_to_wake;
+	int closing;
 
 	/* consumer-published */
 	size_t read_tail;

@@ -1637,7 +1638,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 		n_tty_receive_buf_real_raw(tty, cp, fp, count);
 	else if (ldata->raw || (L_EXTPROC(tty) && !preops))
 		n_tty_receive_buf_raw(tty, cp, fp, count);
-	else if (tty->closing && !L_EXTPROC(tty))
+	else if (ldata->closing && !L_EXTPROC(tty))
 		n_tty_receive_buf_closing(tty, cp, fp, count);
 	else {
 		if (ldata->lnext) {

@@ -1942,7 +1943,7 @@ static int n_tty_open(struct tty_struct *tty)
 	ldata->num_overrun = 0;
 	ldata->no_room = 0;
 	ldata->lnext = 0;
-	tty->closing = 0;
+	ldata->closing = 0;
 	/* indicate buffer work may resume */
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);
 	n_tty_set_termios(tty, NULL);

@@ -2525,6 +2526,13 @@ static void n_tty_fasync(struct tty_struct *tty, int on)
 	}
 }
 
+static void n_tty_closing(struct tty_struct *tty)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	ldata->closing = 1;
+}
+
 struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2541,6 +2549,7 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.write_wakeup    = n_tty_write_wakeup,
 	.fasync		 = n_tty_fasync,
 	.receive_buf2	 = n_tty_receive_buf2,
+	.closing	 = n_tty_closing,
 };
 
 /**

drivers/tty/rocket.c +0 -1

index c5179f8..a6b5ce0 100644
@@ -1043,7 +1043,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 	}
 	spin_lock_irq(&port->lock);
 	info->port.flags &= ~(ASYNC_INITIALIZED | ASYNC_CLOSING | ASYNC_NORMAL_ACTIVE);
-	tty->closing = 0;
 	spin_unlock_irq(&port->lock);
 	mutex_unlock(&port->mutex);
 	tty_port_tty_set(port, NULL);

drivers/tty/serial/68328serial.c +1 -2

index 0140ba4..9a6aaf0 100644
@@ -1035,7 +1035,7 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	 * Now we wait for the transmit buffer to clear; and we notify 
 	 * the line discipline to only process XON/XOFF characters.
 	 */
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent(tty, port->closing_wait);
 	/*

@@ -1052,7 +1052,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	rs_flush_buffer(tty);
 		
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 	tty_port_tty_set(&info->tport, NULL);
 #warning "This is not and has never been valid so fix it"	
 #if 0

drivers/tty/serial/crisv10.c +1 -2

index f13f2eb..0100410 100644
@@ -3620,7 +3620,7 @@ rs_close(struct tty_struct *tty, struct file * filp)
 	 * Now we wait for the transmit buffer to clear; and we notify
 	 * the line discipline to only process XON/XOFF characters.
 	 */
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 	if (info->port.closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent(tty, info->port.closing_wait);
 	/*

@@ -3646,7 +3646,6 @@ rs_close(struct tty_struct *tty, struct file * filp)
 	shutdown(info);
 	rs_flush_buffer(tty);
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 	info->event = 0;
 	info->port.tty = NULL;
 	if (info->port.blocked_open) {

drivers/tty/serial/serial_core.c +0 -1

index def5199..db27a40 100644
@@ -1441,7 +1441,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	mutex_unlock(&port->mutex);
 
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)

drivers/tty/tty_ldisc.c +20 -0

index b776f2e..ab0f559 100644
@@ -407,6 +407,26 @@ void tty_ldisc_flush(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_ldisc_flush);
 
 /**
+ *	tty_ldisc_closing	-	notify line discipline of final close
+ *	@tty: tty
+ *
+ *	Notify the line discipline the driver has begun final close of the tty.
+ *	The N_TTY line discipline uses this notification to ignore new input
+ */
+
+void tty_ldisc_closing(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+
+	if (ld) {
+		if (ld->ops->closing)
+			ld->ops->closing(tty);
+		tty_ldisc_deref(ld);
+	}
+}
+EXPORT_SYMBOL_GPL(tty_ldisc_closing);
+
+/**
  *	tty_set_termios_ldisc		-	set ldisc field
  *	@tty: tty structure
  *	@num: line discipline number

drivers/tty/tty_port.c +1 -2

index a76aec2..ecb6435 100644
@@ -479,7 +479,7 @@ int tty_port_close_start(struct tty_port *port,
 	set_bit(ASYNCB_CLOSING, &port->flags);
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	tty->closing = 1;
+	tty_ldisc_closing(tty);
 
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
 		/* Don't block on a stalled port, just pull the chain */

@@ -504,7 +504,6 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
 	unsigned long flags;
 
 	tty_ldisc_flush(tty);
-	tty->closing = 0;
 
 	spin_lock_irqsave(&port->lock, flags);

include/linux/tty.h +1 -1

index 4b6c62e..70f3a9c1 100644
@@ -306,7 +306,6 @@ struct tty_struct {
 
 #define N_TTY_BUF_SIZE 4096
 
-	int closing;
 	unsigned char *write_buf;
 	int write_cnt;
 	/* If the tty has a pending do_SAK, queue it here - akpm */

@@ -498,6 +497,7 @@ extern const struct file_operations tty_ldiscs_proc_fops;
 
 extern void tty_wakeup(struct tty_struct *tty);
 extern void tty_ldisc_flush(struct tty_struct *tty);
+extern void tty_ldisc_closing(struct tty_struct *tty);
 
 extern long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 extern int tty_mode_ioctl(struct tty_struct *tty, struct file *file,

include/linux/tty_ldisc.h +9 -0

index 00c9d68..0f3b19f 100644
@@ -125,6 +125,14 @@
  *	received with a parity error, etc. <fp> may be NULL to indicate
  *	all data received is TTY_NORMAL.
  *	If assigned, prefer this function for automatic flow control.
+ *
+ * void	(*closing)(struct tty_struct *);
+ *
+ *	This function notifies the line discipline the driver is
+ *	starting final close for this tty; tty and ldisc destruction
+ *	is imminent. The N_TTY line discipline uses this notification
+ *	to ignore new input other than IXON flow control changes.
+ *	This notification is not received for ptys or vts.
  */
 
 #include <linux/fs.h>

@@ -212,6 +220,7 @@ struct tty_ldisc_ops {
 	void	(*fasync)(struct tty_struct *tty, int on);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				char *fp, int count);
+	void	(*closing)(struct tty_struct *tty);
 
 	struct  module *owner;

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-09 12:15 UTC #11 lore raw

PATCH v2 4/4 tty: Remove drivers' extra tty_ldisc_flush()

In reply to Peter Hurley

The tty_port_close_start() helper already flushes the tty and ldisc buffers on final close; tty drivers which use this helper need not repeat tty_ldisc_flush().

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/char/pcmcia/synclink_cs.c | 3 ---
 drivers/tty/amiserial.c           | 2 --
 drivers/tty/rocket.c              | 2 --
 drivers/tty/serial/serial_core.c  | 2 --
 drivers/tty/synclink.c            | 1 -
 drivers/tty/synclink_gt.c         | 1 -
 drivers/tty/synclinkmp.c          | 1 -
 drivers/tty/tty_port.c            | 2 --
 8 files changed, 14 deletions(-)

drivers/char/pcmcia/synclink_cs.c +0 -3

index 45df4bf..3f74677 100644
@@ -2354,10 +2354,7 @@ static void mgslpc_close(struct tty_struct *tty, struct file * filp)
 		mgslpc_wait_until_sent(tty, info->timeout);
 
 	mgslpc_flush_buffer(tty);
-
-	tty_ldisc_flush(tty);
 	shutdown(info, tty);
-	
 	tty_port_close_end(port, tty);
 	tty_port_tty_set(port, NULL);
 cleanup:

drivers/tty/amiserial.c +0 -2

index 2caaf5a..bffc0a4 100644
@@ -1420,8 +1420,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 	}
 	shutdown(tty, state);
 	rs_flush_buffer(tty);
-		
-	tty_ldisc_flush(tty);
 	port->tty = NULL;
 
 	tty_port_close_end(port, tty);

drivers/tty/rocket.c +0 -2

index a6b5ce0..5905200 100644
@@ -1022,8 +1022,6 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
 		sClrDTR(cp);
 
 	rp_flush_buffer(tty);
-		
-	tty_ldisc_flush(tty);
 
 	clear_bit((info->aiop * 8) + info->chan, (void *) &xmit_flags[info->board]);

drivers/tty/serial/serial_core.c +0 -2

index db27a40..418587f 100644
@@ -1439,8 +1439,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	wake_up_interruptible(&port->open_wait);
 
 	mutex_unlock(&port->mutex);
-
-	tty_ldisc_flush(tty);
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)

drivers/tty/synclink.c +0 -1

index 6188059..1334498 100644
@@ -3099,7 +3099,6 @@ static void mgsl_close(struct tty_struct *tty, struct file * filp)
  	if (info->port.flags & ASYNC_INITIALIZED)
  		mgsl_wait_until_sent(tty, info->timeout);
 	mgsl_flush_buffer(tty);
-	tty_ldisc_flush(tty);
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/synclink_gt.c +0 -1

index 5505ea8..1987fb4 100644
@@ -729,7 +729,6 @@ static void close(struct tty_struct *tty, struct file *filp)
  	if (info->port.flags & ASYNC_INITIALIZED)
  		wait_until_sent(tty, info->timeout);
 	flush_buffer(tty);
-	tty_ldisc_flush(tty);
 
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/synclinkmp.c +0 -1

index fb00a06..fb17dac 100644
@@ -816,7 +816,6 @@ static void close(struct tty_struct *tty, struct file *filp)
  		wait_until_sent(tty, info->timeout);
 
 	flush_buffer(tty);
-	tty_ldisc_flush(tty);
 	shutdown(info);
 	mutex_unlock(&info->port.mutex);

drivers/tty/tty_port.c +0 -2

index ecb6435..c30525a 100644
@@ -503,8 +503,6 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
 {
 	unsigned long flags;
 
-	tty_ldisc_flush(tty);
-
 	spin_lock_irqsave(&port->lock, flags);
 
 	if (port->blocked_open) {

Signature · 1 line

2.6.3

Peter Hurley

author

2015-12-14 00:16 UTC #12 lore raw

In reply to Peter Hurley

Greg,

Would you drop these 4 patches from tty-testing please?

Quoted 16 lines · Peter Hurley

Hi Greg,
This series cleans up a messy and poorly documented mechanism required at tty final close to prevent drivers from crashing after h/w shutdown.
Without special handling, N_TTY echoing will cause driver i/o requests _after_ h/w shutdown, which typically crashes the driver. Currently, the tty_struct::closing flag triggers this special handling. However, this mechanism is error-prone and subject to driver misuse.
This series replaces tty->closing with a ldisc-specific interface, tty_ldisc_closing(), and implements this interface for N_TTY.
For tty drivers which use tty_port_close_start(), this change eliminates the last vestige of direct driver<->ldisc interaction. The few tty drivers which open-code the close() method [1] still use tty_ldisc_closing() directly.

Quoted 5 lines

The tty driver is aware final close for the tty has commenced because the tty->count == 1 in the close() method. On final close, the following is also true:
1. port->count == 1. tty drivers which ref count the port, use the --port->count == 0 as a substitute condition for final close.

I overlooked the implications of a blocked open here.

Since a blocked open drops the port count while blocking, a port count of 1 is not equivalent to a tty count of 1 at tty_release() time. So even though the port is shutting down, the extra tty count held by the blocking open will keep the ldisc instance from being destructed; iow, a port count of 1 does _not_ imply final tty close.

For the blocked open itself, this would not be a problem because the open will error out anyway, drop the tty count and cause final close. (And, oddly, trigger a second port shutdown which I need to consider the implications of.)

However, there is a very small race window where a third process might be able to successfully reopen the tty, but now would have a dead-stick ldisc instance (because this series does not reset the ldisc instance back to the not closing state).

Regards,
Peter Hurley

Quoted 64 lines

2. final close is occurring as a result of the last in-use file descriptor release. Consequently, there will be no read/poll/ioctl in-progress.
3. the line discipline instance will be stopped and destroyed immediately after the tty driver completes the close() method
4. the tty itself will be unrefed immediately after the line discipline instance is destroyed.
Thus, the ldisc and tty buffers need only be flushed once, as any data received by the tty driver after the flush but before h/w shutdown will be deleted when the line discipline instance is destroyed. No new echoes will occur after the ldisc flush because the echo buffer is also flushed and new input (which otherwise might generate echoes) is ignored while closing. This series removes the extra tty_ldisc_flush() being performed by most drivers after h/w shutdown.
Additionally, the ldisc closing state need not be reset since the line discipline instance is being destroyed, so no interface is provided to reset closing.
[1] tty drivers which open-code the close() method drivers/staging/dgnc/dgnc_tty.c drivers/staging/dgap/dgap.c drivers/tty/hvc/hvsi.c drivers/tty/serial/68328serial.c drivers/tty/serial/crisv10.c drivers/isdn/i4l/isdn_tty.c drivers/s390/char/con3215.c
Changes to v2:
* Fixed tty_ldisc_closing() ld use found by Johannes Stezenbach <[email protected]>
Regards,
Peter Hurley (4):
tty: rocket: Remove private close_wait
n_tty: Ignore all read data when closing
tty: Abstract and encapsulate tty->closing behavior
tty: Remove drivers' extra tty_ldisc_flush()
 drivers/char/pcmcia/synclink_cs.c |  3 ---
 drivers/isdn/i4l/isdn_tty.c       |  2 +-
 drivers/s390/char/con3215.c       |  3 +--
 drivers/staging/dgap/dgap.c       |  4 +---
 drivers/staging/dgnc/dgnc_tty.c   |  4 +---
 drivers/tty/amiserial.c           |  2 --
 drivers/tty/hvc/hvsi.c            |  2 +-
 drivers/tty/ipwireless/tty.c      |  1 -
 drivers/tty/n_tty.c               | 15 +++++++++++----
 drivers/tty/rocket.c              |  5 -----
 drivers/tty/rocket_int.h          |  1 -
 drivers/tty/serial/68328serial.c  |  3 +--
 drivers/tty/serial/crisv10.c      |  3 +--
 drivers/tty/serial/serial_core.c  |  3 ---
 drivers/tty/synclink.c            |  1 -
 drivers/tty/synclink_gt.c         |  1 -
 drivers/tty/synclinkmp.c          |  1 -
 drivers/tty/tty_ldisc.c           | 20 ++++++++++++++++++++
 drivers/tty/tty_port.c            |  5 +----
 include/linux/tty.h               |  2 +-
 include/linux/tty_ldisc.h         |  9 +++++++++
 21 files changed, 49 insertions(+), 41 deletions(-)

Greg Kroah-Hartman

maintainer

2015-12-14 04:02 UTC #13 lore raw

Quoted 3 lines · Peter Hurley

Greg,
Would you drop these 4 patches from tty-testing please?

Now dropped.