[PATCH -next 00/10] serial core fixes

[PATCH -next 00/10] serial core fixes

[Peter Hurley]

Hi Greg,

The first 3 patches are picked out from the closing series that was
split up; the first 2 patches lay the groundwork for the fixing
the locking for the ->set_termios() method.

The 2 UPF_*/ASYNC* patches are to bind the definitions together
since userspace only gets one set of definitions, but the code
operates as if the definitions are synch'd.

The remainder except the last patch are cleanups; the last patch
fixes port count mismatch in uart_open()/uart_close(), which
Geert and I discussed a while back. So one less thing on my todo
list.

Still on the todo list:
1. Some tty_port flags usage is non-atomic.
2. uart_remove_one_port will happily remove an in-use port since
   there's no reference counting.
3. fixing autoRTS for omap (so it clears RTS when tiocmset drops
   RTS)

Regards,

Peter Hurley (10):
  serial: Refactor uart_flush_buffer() from uart_close()
  serial: core: Flush ldisc after dropping port mutex in uart_close()
  serial: Fix locking for uart driver set_termios() method
  tty,serial: Unify UPF_* and ASYNC_* flag definitions
  tty: Document defunct ASYNC_* bits in uapi header
  serial: core: Unwrap >80 char line in uart_close()
  serial: core: Remove redundant timeout assignments
  serial: core: Colocate crucial structure linkage
  serial: core: Remove extra locking in uart_write()
  serial: core: Fix port count when uart_open() errors

 Documentation/serial/driver       |  6 +++--
 drivers/tty/serial/amba-pl011.c   |  1 +
 drivers/tty/serial/atmel_serial.c | 28 +++++++++++----------
 drivers/tty/serial/serial-tegra.c | 30 +++++++++++-----------
 drivers/tty/serial/serial_core.c  | 52 ++++++++++++++++++++++-----------------
 drivers/tty/serial/timbuart.c     |  2 ++
 include/linux/serial_core.h       | 47 ++++++++++++++++++++++++-----------
 include/uapi/linux/tty_flags.h    | 15 +++++++----
 8 files changed, 110 insertions(+), 71 deletions(-)

-- 
2.1.1

[PATCH -next 01/10] serial: Refactor uart_flush_buffer() from uart_close()

[Peter Hurley]

In the context of the final tty & port close, flushing the tx
ring buffer after the hardware has already been shutdown and
the ring buffer freed is neither required nor desirable.

uart_flush_buffer() performs 3 operations:
1. Resets tx ring buffer indices, but the tx ring buffer has
   already been freed and the indices are reset if the port is
   re-opened.
2. Calls uart driver's flush_buffer() method
   5 in-tree uart drivers define flush_buffer() methods:
     amba-pl011, atmel-serial, imx, serial-tegra, timbuart
   These have been refactored into the shutdown() method, if
   required.
3. Kicks the ldisc for more writing, but this is undesirable.
   The file handle is being released; any waiting writer will
   will be kicked out by tty_release() with a warning. Further,
   the N_TTY ldisc may generate SIGIO for a file handle which
   is no longer valid.

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/amba-pl011.c   |  1 +
 drivers/tty/serial/atmel_serial.c | 28 +++++++++++++++-------------
 drivers/tty/serial/serial-tegra.c | 30 ++++++++++++++++--------------
 drivers/tty/serial/serial_core.c  |  1 -
 drivers/tty/serial/timbuart.c     |  2 ++
 5 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8572f2a..5b82d7a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1674,6 +1674,7 @@ static void pl011_shutdown(struct uart_port *port)
 			plat->exit();
 	}
 
+	pl011_dma_flush_buffer(port);
 }
 
 static void
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index d7d4584..8b13d4d 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1801,6 +1801,20 @@ free_irq:
 }
 
 /*
+ * Flush any TX data submitted for DMA. Called when the TX circular
+ * buffer is reset.
+ */
+static void atmel_flush_buffer(struct uart_port *port)
+{
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
+	if (atmel_use_pdc_tx(port)) {
+		UART_PUT_TCR(port, 0);
+		atmel_port->pdc_tx.ofs = 0;
+	}
+}
+
+/*
  * Disable the port
  */
 static void atmel_shutdown(struct uart_port *port)
@@ -1851,20 +1865,8 @@ static void atmel_shutdown(struct uart_port *port)
 	atmel_free_gpio_irq(port);
 
 	atmel_port->ms_irq_enabled = false;
-}
 
-/*
- * Flush any TX data submitted for DMA. Called when the TX circular
- * buffer is reset.
- */
-static void atmel_flush_buffer(struct uart_port *port)
-{
-	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-
-	if (atmel_use_pdc_tx(port)) {
-		UART_PUT_TCR(port, 0);
-		atmel_port->pdc_tx.ofs = 0;
-	}
+	atmel_flush_buffer(port);
 }
 
 /*
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 53d7c31..78a5cf6 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1034,6 +1034,20 @@ fail_rx_dma:
 	return ret;
 }
 
+/*
+ * Flush any TX data submitted for DMA and PIO. Called when the
+ * TX circular buffer is reset.
+ */
+static void tegra_uart_flush_buffer(struct uart_port *u)
+{
+	struct tegra_uart_port *tup = to_tegra_uport(u);
+
+	tup->tx_bytes = 0;
+	if (tup->tx_dma_chan)
+		dmaengine_terminate_all(tup->tx_dma_chan);
+	return;
+}
+
 static void tegra_uart_shutdown(struct uart_port *u)
 {
 	struct tegra_uart_port *tup = to_tegra_uport(u);
@@ -1046,6 +1060,8 @@ static void tegra_uart_shutdown(struct uart_port *u)
 	tegra_uart_dma_channel_free(tup, true);
 	tegra_uart_dma_channel_free(tup, false);
 	free_irq(u->irq, tup);
+
+	tegra_uart_flush_buffer(u);
 }
 
 static void tegra_uart_enable_ms(struct uart_port *u)
@@ -1174,20 +1190,6 @@ static void tegra_uart_set_termios(struct uart_port *u,
 	return;
 }
 
-/*
- * Flush any TX data submitted for DMA and PIO. Called when the
- * TX circular buffer is reset.
- */
-static void tegra_uart_flush_buffer(struct uart_port *u)
-{
-	struct tegra_uart_port *tup = to_tegra_uport(u);
-
-	tup->tx_bytes = 0;
-	if (tup->tx_dma_chan)
-		dmaengine_terminate_all(tup->tx_dma_chan);
-	return;
-}
-
 static const char *tegra_uart_type(struct uart_port *u)
 {
 	return TEGRA_UART_TYPE;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 221ca6f..ce0e762 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1360,7 +1360,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 
 	mutex_lock(&port->mutex);
 	uart_shutdown(tty, state);
-	uart_flush_buffer(tty);
 
 	tty_ldisc_flush(tty);
 
diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
index 0d11d50..e9e2523 100644
--- a/drivers/tty/serial/timbuart.c
+++ b/drivers/tty/serial/timbuart.c
@@ -273,6 +273,8 @@ static void timbuart_shutdown(struct uart_port *port)
 	dev_dbg(port->dev, "%s\n", __func__);
 	free_irq(port->irq, uart);
 	iowrite32(0, port->membase + TIMBUART_IER);
+
+	timbuart_flush_buffer(port);
 }
 
 static int get_bindex(int baud)
-- 
2.1.1

[PATCH -next 02/10] serial: core: Flush ldisc after dropping port mutex in uart_close()

[Peter Hurley]

The tty buffers (and any line discipline buffers) must be flushed after
the UART hardware has shutdown; otherwise, a racing open on the same
tty may receive data from the previous session, which is a security
hazard. However, holding the port mutex while flushing the line
discipline buffers creates a lock inversion if the set_termios()
handler takes the port mutex (as it does in the followup patch,
'serial: Fix locking for uart driver set_termios method'.

Flush the ldisc buffers after dropping the port mutex; the tty lock
is still held which prevents a concurrent open() from advancing while
flushing. Since no new rx data is possible after uart_shutdown() until
a new open reinitializes the port, the later flush has no impact on
what data is being discarded.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ce0e762..6203c6c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1360,9 +1360,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 
 	mutex_lock(&port->mutex);
 	uart_shutdown(tty, state);
-
-	tty_ldisc_flush(tty);
-
 	tty_port_tty_set(port, NULL);
 	tty->closing = 0;
 	spin_lock_irqsave(&port->lock, flags);
@@ -1389,6 +1386,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	wake_up_interruptible(&port->close_wait);
 
 	mutex_unlock(&port->mutex);
+
+	tty_ldisc_flush(tty);
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
-- 
2.1.1

[PATCH -next 03/10] serial: Fix locking for uart driver set_termios() method

[Peter Hurley]

The low-level uart driver may modify termios settings to override
settings that are not compatible with the uart, such as CRTSCTS.
Thus, callers of the low-level uart driver's set_termios() method must
hold termios_rwsem write lock to prevent concurrent access to termios,
in case such override occurs.

The termios_rwsem lock requirement does not extend to console setup
(ie., uart_set_options), as console setup cannot race with tty
operations. Nor does this lock requirement extend to functions which
cannot be concurrent with tty ioctls (ie., uart_port_startup() and
uart_resume_port()).

Further, always claim the port mutex to protect hardware
re-reprogramming in the set_termios() uart driver method. Note this
is unnecessary for console initialization in uart_set_options()
which cannot be concurrent with other uart operations.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 Documentation/serial/driver      | 6 ++++--
 drivers/tty/serial/serial_core.c | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index ba64e4b..c415b0e 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -59,7 +59,9 @@ The core driver uses the info->tmpbuf_sem lock to prevent multi-threaded
 access to the info->tmpbuf bouncebuffer used for port writes.
 
 The port_sem semaphore is used to protect against ports being added/
-removed or reconfigured at inappropriate times.
+removed or reconfigured at inappropriate times. Since v2.6.27, this
+semaphore has been the 'mutex' member of the tty_port struct, and
+commonly referred to as the port mutex (or port->mutex).
 
 
 uart_ops
@@ -248,7 +250,7 @@ hardware.
 	Other flags may be used (eg, xon/xoff characters) if your
 	hardware supports hardware "soft" flow control.
 
-	Locking: none.
+	Locking: caller holds port->mutex
 	Interrupts: caller dependent.
 	This call must not sleep
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6203c6c..6ebe78c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -436,7 +436,7 @@ uart_get_divisor(struct uart_port *port, unsigned int baud)
 
 EXPORT_SYMBOL(uart_get_divisor);
 
-/* FIXME: Consistent locking policy */
+/* Caller holds port mutex */
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios)
 {
@@ -1172,11 +1172,15 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
 		break;
 
 	case TIOCSSERIAL:
+		down_write(&tty->termios_rwsem);
 		ret = uart_set_info_user(tty, state, uarg);
+		up_write(&tty->termios_rwsem);
 		break;
 
 	case TIOCSERCONFIG:
+		down_write(&tty->termios_rwsem);
 		ret = uart_do_autoconfig(tty, state);
+		up_write(&tty->termios_rwsem);
 		break;
 
 	case TIOCSERGWILD: /* obsolete */
@@ -1277,7 +1281,9 @@ static void uart_set_termios(struct tty_struct *tty,
 		return;
 	}
 
+	mutex_lock(&state->port.mutex);
 	uart_change_speed(tty, state, old_termios);
+	mutex_unlock(&state->port.mutex);
 	/* reload cflag from termios; port driver may have overriden flags */
 	cflag = tty->termios.c_cflag;
 
-- 
2.1.1

[PATCH -next 04/10] tty,serial: Unify UPF_* and ASYNC_* flag definitions

[Peter Hurley]

The userspace-defined ASYNC_* flags in include/uapi/linux/tty_flags.h
are the authoritative bit definitions for the serial_struct flags,
and thus for any derivative values or fields.

Although the serial core provides the TIOCSSERIAL and TIOCGSERIAL
ioctls to set and retrieve these flags from userspace, it defines these
bits independently, as UPF_* macros.

Define the UPF_* macros which are userspace-modifiable directly from
the ASYNC_* symbolic constants. Add compile-time test to ensure the
bits changeable by TIOCSSERIAL match the defined range in the uapi
header.

Add ASYNCB_MAGIC_MULTIPLIER to the uapi header since this bit is
programmable by userspace.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/linux/serial_core.h    | 47 ++++++++++++++++++++++++++++--------------
 include/uapi/linux/tty_flags.h |  6 +++++-
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index bb828a5..ab7180e 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -156,21 +156,33 @@ struct uart_port {
 	/* flags must be updated while holding port mutex */
 	upf_t			flags;
 
-#define UPF_FOURPORT		((__force upf_t) (1 << 1))
-#define UPF_SAK			((__force upf_t) (1 << 2))
-#define UPF_SPD_MASK		((__force upf_t) (0x1030))
-#define UPF_SPD_HI		((__force upf_t) (0x0010))
-#define UPF_SPD_VHI		((__force upf_t) (0x0020))
-#define UPF_SPD_CUST		((__force upf_t) (0x0030))
-#define UPF_SPD_SHI		((__force upf_t) (0x1000))
-#define UPF_SPD_WARP		((__force upf_t) (0x1010))
-#define UPF_SKIP_TEST		((__force upf_t) (1 << 6))
-#define UPF_AUTO_IRQ		((__force upf_t) (1 << 7))
-#define UPF_HARDPPS_CD		((__force upf_t) (1 << 11))
-#define UPF_LOW_LATENCY		((__force upf_t) (1 << 13))
-#define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
+	/*
+	 * These flags must be equivalent to the flags defined in
+	 * include/uapi/linux/tty_flags.h which are the userspace definitions
+	 * assigned from the serial_struct flags in uart_set_info()
+	 * [for bit definitions in the UPF_CHANGE_MASK]
+	 *
+	 * Bits [0..UPF_LAST_USER] are userspace defined/visible/changeable
+	 * except bit 15 (UPF_NO_TXEN_TEST) which is masked off.
+	 * The remaining bits are serial-core specific and not modifiable by
+	 * userspace.
+	 */
+#define UPF_FOURPORT		((__force upf_t) ASYNC_FOURPORT       /* 1  */ )
+#define UPF_SAK			((__force upf_t) ASYNC_SAK            /* 2  */ )
+#define UPF_SPD_HI		((__force upf_t) ASYNC_SPD_HI         /* 4  */ )
+#define UPF_SPD_VHI		((__force upf_t) ASYNC_SPD_VHI        /* 5  */ )
+#define UPF_SPD_CUST		((__force upf_t) ASYNC_SPD_CUST   /* 0x0030 */ )
+#define UPF_SPD_WARP		((__force upf_t) ASYNC_SPD_WARP   /* 0x1010 */ )
+#define UPF_SPD_MASK		((__force upf_t) ASYNC_SPD_MASK   /* 0x1030 */ )
+#define UPF_SKIP_TEST		((__force upf_t) ASYNC_SKIP_TEST      /* 6  */ )
+#define UPF_AUTO_IRQ		((__force upf_t) ASYNC_AUTO_IRQ       /* 7  */ )
+#define UPF_HARDPPS_CD		((__force upf_t) ASYNC_HARDPPS_CD     /* 11 */ )
+#define UPF_SPD_SHI		((__force upf_t) ASYNC_SPD_SHI        /* 12 */ )
+#define UPF_LOW_LATENCY		((__force upf_t) ASYNC_LOW_LATENCY    /* 13 */ )
+#define UPF_BUGGY_UART		((__force upf_t) ASYNC_BUGGY_UART     /* 14 */ )
 #define UPF_NO_TXEN_TEST	((__force upf_t) (1 << 15))
-#define UPF_MAGIC_MULTIPLIER	((__force upf_t) (1 << 16))
+#define UPF_MAGIC_MULTIPLIER	((__force upf_t) ASYNC_MAGIC_MULTIPLIER /* 16 */ )
+
 /* Port has hardware-assisted h/w flow control (iow, auto-RTS *not* auto-CTS) */
 #define UPF_HARD_FLOW		((__force upf_t) (1 << 21))
 /* Port has hardware-assisted s/w flow control */
@@ -186,9 +198,14 @@ struct uart_port {
 #define UPF_DEAD		((__force upf_t) (1 << 30))
 #define UPF_IOREMAP		((__force upf_t) (1 << 31))
 
-#define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
+#define __UPF_CHANGE_MASK	0x17fff
+#define UPF_CHANGE_MASK		((__force upf_t) __UPF_CHANGE_MASK)
 #define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
 
+#if __UPF_CHANGE_MASK > ASYNC_FLAGS
+#error Change mask not equivalent to userspace-visible bit defines
+#endif
+
 	/* status must be updated while holding port lock */
 	upstat_t		status;
 
diff --git a/include/uapi/linux/tty_flags.h b/include/uapi/linux/tty_flags.h
index eefcb48..7b516f7 100644
--- a/include/uapi/linux/tty_flags.h
+++ b/include/uapi/linux/tty_flags.h
@@ -6,6 +6,8 @@
  * shared by the tty_port flags structures.
  *
  * Define ASYNCB_* for convenient use with {test,set,clear}_bit.
+ *
+ * Bits [0..ASYNCB_LAST_USER] are userspace defined/visible/changeable
  */
 #define ASYNCB_HUP_NOTIFY	 0 /* Notify getty on hangups and closes
 				    * on the callout port */
@@ -26,7 +28,8 @@
 #define ASYNCB_BUGGY_UART	14 /* This is a buggy UART, skip some safety
 				    * checks.  Note: can be dangerous! */
 #define ASYNCB_AUTOPROBE	15 /* Port was autoprobed by PCI or PNP code */
-#define ASYNCB_LAST_USER	15
+#define ASYNCB_MAGIC_MULTIPLIER	16 /* Use special CLK or divisor */
+#define ASYNCB_LAST_USER	16
 
 /* Internal flags used only by kernel */
 #define ASYNCB_INITIALIZED	31 /* Serial port was initialized */
@@ -57,6 +60,7 @@
 #define ASYNC_LOW_LATENCY	(1U << ASYNCB_LOW_LATENCY)
 #define ASYNC_BUGGY_UART	(1U << ASYNCB_BUGGY_UART)
 #define ASYNC_AUTOPROBE		(1U << ASYNCB_AUTOPROBE)
+#define ASYNC_MAGIC_MULTIPLIER	(1U << ASYNCB_MAGIC_MULTIPLIER)
 
 #define ASYNC_FLAGS		((1U << (ASYNCB_LAST_USER + 1)) - 1)
 #define ASYNC_USR_MASK		(ASYNC_SPD_MASK|ASYNC_CALLOUT_NOHUP| \
-- 
2.1.1

[PATCH -next 05/10] tty: Document defunct ASYNC_* bits in uapi header

[Peter Hurley]

Note the serial_struct flags for which the kernel ignores and performs
no action. The flags cannot be removed since they form part of the
userspace interface via the TIOCSSERIAL/TIOCGSERIAL ioctls.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/uapi/linux/tty_flags.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/tty_flags.h b/include/uapi/linux/tty_flags.h
index 7b516f7..4e021e3 100644
--- a/include/uapi/linux/tty_flags.h
+++ b/include/uapi/linux/tty_flags.h
@@ -8,6 +8,7 @@
  * Define ASYNCB_* for convenient use with {test,set,clear}_bit.
  *
  * Bits [0..ASYNCB_LAST_USER] are userspace defined/visible/changeable
+ * [x] in the bit comments indicates the flag is defunct and no longer used.
  */
 #define ASYNCB_HUP_NOTIFY	 0 /* Notify getty on hangups and closes
 				    * on the callout port */
@@ -19,15 +20,15 @@
 #define ASYNCB_SKIP_TEST	 6 /* Skip UART test during autoconfiguration */
 #define ASYNCB_AUTO_IRQ		 7 /* Do automatic IRQ during
 				    * autoconfiguration */
-#define ASYNCB_SESSION_LOCKOUT	 8 /* Lock out cua opens based on session */
-#define ASYNCB_PGRP_LOCKOUT	 9 /* Lock out cua opens based on pgrp */
-#define ASYNCB_CALLOUT_NOHUP	10 /* Don't do hangups for cua device */
+#define ASYNCB_SESSION_LOCKOUT	 8 /* [x] Lock out cua opens based on session */
+#define ASYNCB_PGRP_LOCKOUT	 9 /* [x] Lock out cua opens based on pgrp */
+#define ASYNCB_CALLOUT_NOHUP	10 /* [x] Don't do hangups for cua device */
 #define ASYNCB_HARDPPS_CD	11 /* Call hardpps when CD goes high  */
 #define ASYNCB_SPD_SHI		12 /* Use 230400 instead of 38400 bps */
 #define ASYNCB_LOW_LATENCY	13 /* Request low latency behaviour */
 #define ASYNCB_BUGGY_UART	14 /* This is a buggy UART, skip some safety
 				    * checks.  Note: can be dangerous! */
-#define ASYNCB_AUTOPROBE	15 /* Port was autoprobed by PCI or PNP code */
+#define ASYNCB_AUTOPROBE	15 /* [x] Port was autoprobed by PCI/PNP code */
 #define ASYNCB_MAGIC_MULTIPLIER	16 /* Use special CLK or divisor */
 #define ASYNCB_LAST_USER	16
 
-- 
2.1.1

Re: [PATCH -next 05/10] tty: Document defunct ASYNC_* bits in uapi header

[Jiri Slaby]

On 10/16/2014, 10:54 PM, Peter Hurley wrote:
> Note the serial_struct flags for which the kernel ignores and performs
> no action. The flags cannot be removed since they form part of the
> userspace interface via the TIOCSSERIAL/TIOCGSERIAL ioctls.

Hello,

would it make sense to mark them deprecated somehow? At build time, or
at least warn in the serial core that "current->comm is using a
deprecated flag"_ratelimited()?

thanks,
-- 
js
suse labs

Re: [PATCH -next 05/10] tty: Document defunct ASYNC_* bits in uapi header

[Peter Hurley]

Hi Jiri,

On 10/17/2014 04:46 AM, Jiri Slaby wrote:
> On 10/16/2014, 10:54 PM, Peter Hurley wrote:
>> Note the serial_struct flags for which the kernel ignores and performs
>> no action. The flags cannot be removed since they form part of the
>> userspace interface via the TIOCSSERIAL/TIOCGSERIAL ioctls.
> 
> Hello,
> 
> would it make sense to mark them deprecated somehow? At build time

A build warning when the macro is expanded would be best, but my
c-preprocessor-fu is terrible, so I have no idea how to make that work.

> or
> at least warn in the serial core that "current->comm is using a
> deprecated flag"_ratelimited()?

If we just print the message at TIOCSSERIAL if any of the deprecated
bits are set, that would be ok. Probably the only issue would be that
setserial could cause this message at will, so log flooding would be
a concern.

Regards,
Peter Hurley

Re: [PATCH -next 05/10] tty: Document defunct ASYNC_* bits in uapi header

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On 10/17/2014, 02:44 PM, Peter Hurley wrote:
> Hi Jiri,
> 
> On 10/17/2014 04:46 AM, Jiri Slaby wrote:
>> On 10/16/2014, 10:54 PM, Peter Hurley wrote:
>>> Note the serial_struct flags for which the kernel ignores and performs
>>> no action. The flags cannot be removed since they form part of the
>>> userspace interface via the TIOCSSERIAL/TIOCGSERIAL ioctls.
>>
>> Hello,
>>
>> would it make sense to mark them deprecated somehow? At build time
> 
> A build warning when the macro is expanded would be best, but my
> c-preprocessor-fu is terrible, so I have no idea how to make that work.

We can define a deprecated type like:

typedef __u32 __attribute__((deprecated)) depr_tty_t

and then do define:

#define ASYNCB_FLAG_1 (depr_tty_t)1
#define ASYNCB_FLAG_2 (depr_tty_t)2

Etc.

However, I do not know if it is desirable AND whether all compilers out
there used in userspace support that attribute.

>> or
>> at least warn in the serial core that "current->comm is using a
>> deprecated flag"_ratelimited()?
> 
> If we just print the message at TIOCSSERIAL if any of the deprecated
> bits are set, that would be ok. Probably the only issue would be that
> setserial could cause this message at will, so log flooding would be
> a concern.

That's why _ratelimited(). So something like the patch attached. But it
needs to be put to a separate function, out of the line.

/me currently builds a kernel to see who, if anybody, uses the flags on
a up-to-date system...

thanks,
-- 
js
suse labs

[-- Attachment #2: 0001-tty-warn-on-deprecated-flags.patch --]
[-- Type: application/mbox, Size: 1757 bytes --]

Re: [PATCH -next 05/10] tty: Document defunct ASYNC_* bits in uapi header

[Greg Kroah-Hartman]

On Fri, Oct 17, 2014 at 03:31:28PM +0200, Jiri Slaby wrote:
> On 10/17/2014, 02:44 PM, Peter Hurley wrote:
> > Hi Jiri,
> > 
> > On 10/17/2014 04:46 AM, Jiri Slaby wrote:
> >> On 10/16/2014, 10:54 PM, Peter Hurley wrote:
> >>> Note the serial_struct flags for which the kernel ignores and performs
> >>> no action. The flags cannot be removed since they form part of the
> >>> userspace interface via the TIOCSSERIAL/TIOCGSERIAL ioctls.
> >>
> >> Hello,
> >>
> >> would it make sense to mark them deprecated somehow? At build time
> > 
> > A build warning when the macro is expanded would be best, but my
> > c-preprocessor-fu is terrible, so I have no idea how to make that work.
> 
> We can define a deprecated type like:
> 
> typedef __u32 __attribute__((deprecated)) depr_tty_t
> 
> and then do define:
> 
> #define ASYNCB_FLAG_1 (depr_tty_t)1
> #define ASYNCB_FLAG_2 (depr_tty_t)2
> 
> Etc.
> 
> However, I do not know if it is desirable AND whether all compilers out
> there used in userspace support that attribute.
> 
> >> or
> >> at least warn in the serial core that "current->comm is using a
> >> deprecated flag"_ratelimited()?
> > 
> > If we just print the message at TIOCSSERIAL if any of the deprecated
> > bits are set, that would be ok. Probably the only issue would be that
> > setserial could cause this message at will, so log flooding would be
> > a concern.
> 
> That's why _ratelimited(). So something like the patch attached. But it
> needs to be put to a separate function, out of the line.
> 
> /me currently builds a kernel to see who, if anybody, uses the flags on
> a up-to-date system...

What ever happened with this test?  Care to resend it as a "real" patch
so we can annoy userspace?  :)

thanks,

greg k-h

[PATCH -next 06/10] serial: core: Unwrap >80 char line in uart_close()

[Peter Hurley]

The wrapped line looks wrong and out-of-place; leave it as
>80 char line.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6ebe78c..be58916 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1373,8 +1373,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	if (port->blocked_open) {
 		spin_unlock_irqrestore(&port->lock, flags);
 		if (port->close_delay)
-			msleep_interruptible(
-					jiffies_to_msecs(port->close_delay));
+			msleep_interruptible(jiffies_to_msecs(port->close_delay));
 		spin_lock_irqsave(&port->lock, flags);
 	} else if (!uart_console(uport)) {
 		spin_unlock_irqrestore(&port->lock, flags);
-- 
2.1.1

[PATCH -next 07/10] serial: core: Remove redundant timeout assignments

[Peter Hurley]

tty_port_init() initializes close_delay and closing_wait to these
same values; remove.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index be58916..676c03a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2346,8 +2346,6 @@ int uart_register_driver(struct uart_driver *drv)
 
 		tty_port_init(port);
 		port->ops = &uart_port_ops;
-		port->close_delay     = HZ / 2;	/* .5 seconds */
-		port->closing_wait    = 30 * HZ;/* 30 seconds */
 	}
 
 	retval = tty_register_driver(normal);
-- 
2.1.1

[PATCH -next 08/10] serial: core: Colocate crucial structure linkage

[Peter Hurley]

The key function of uart_add_one_port() is to cross-reference the
UART driver's port structure with the serial core's state table;
keep the assignments together and document this crucial association.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 676c03a..e04ae58 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2594,11 +2594,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 
+	/* Link the port to the driver state table and vice versa */
 	state->uart_port = uport;
-	state->pm_state = UART_PM_STATE_UNDEFINED;
+	uport->state = state;
 
+	state->pm_state = UART_PM_STATE_UNDEFINED;
 	uport->cons = drv->cons;
-	uport->state = state;
 
 	/*
 	 * If this port is a console, then the spinlock is already
-- 
2.1.1

[PATCH -next 09/10] serial: core: Remove extra locking in uart_write()

[Peter Hurley]

uart_start() only claims the port->lock to call __uart_start(),
which does the actual processing. Eliminate the extra acquire/release
in uart_write(); call __uart_start() directly with port->lock already
held.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e04ae58..6a3571e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -537,9 +537,10 @@ static int uart_write(struct tty_struct *tty,
 		count -= c;
 		ret += c;
 	}
+
+	__uart_start(tty);
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	uart_start(tty);
 	return ret;
 }
 
-- 
2.1.1

[PATCH -next 10/10] serial: core: Fix port count when uart_open() errors

[Peter Hurley]

A port count mismatch occurs if mutex_lock_interruptible()
exits uart_open() and the port has already been opened. This may
prematurely close a port on an open tty. Since uart_close() is _always_
called if uart_open() fails, the port count must be corrected if errors
occur.

Always increment the port count in uart_open(), regardless of errors;
always decrement the port count in uart_close(). Note that
tty_port_close_start() decrements the port count when uart_open()
was successful.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/serial_core.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6a3571e..3f02c2b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1337,8 +1337,16 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	struct uart_port *uport;
 	unsigned long flags;
 
-	if (!state)
+	if (!state) {
+		struct uart_driver *drv = tty->driver->driver_state;
+
+		state = drv->state + tty->index;
+		port = &state->port;
+		spin_lock_irq(&port->lock);
+		--port->count;
+		spin_unlock_irq(&port->lock);
 		return;
+	}
 
 	uport = state->uart_port;
 	port = &state->port;
@@ -1555,6 +1563,10 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 
 	pr_debug("uart_open(%d) called\n", line);
 
+	spin_lock_irq(&port->lock);
+	++port->count;
+	spin_unlock_irq(&port->lock);
+
 	/*
 	 * We take the semaphore here to guarantee that we won't be re-entered
 	 * while allocating the state structure, or while we request any IRQs
@@ -1567,17 +1579,11 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 		goto end;
 	}
 
-	port->count++;
 	if (!state->uart_port || state->uart_port->flags & UPF_DEAD) {
 		retval = -ENXIO;
-		goto err_dec_count;
+		goto err_unlock;
 	}
 
-	/*
-	 * Once we set tty->driver_data here, we are guaranteed that
-	 * uart_close() will decrement the driver module use count.
-	 * Any failures from here onwards should not touch the count.
-	 */
 	tty->driver_data = state;
 	state->uart_port->state = state;
 	state->port.low_latency =
@@ -1598,8 +1604,7 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 
 end:
 	return retval;
-err_dec_count:
-	port->count--;
+err_unlock:
 	mutex_unlock(&port->mutex);
 	goto end;
 }
-- 
2.1.1