[PATCH 00/26] tty/serial flow control fixes

[PATCH 00/26] tty/serial flow control fixes

[Peter Hurley]

Hi Greg,

This patch series reworks the internals of tty and serial flow control to
fix multiple races in both START/STOP flow control and RTS/CTS flow control.

The main changes in this series are:
Patch 1
   Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
   for mainline and -next

Patches 3-7
   Fixes to x_char handling (ie., sending START/STOP) both in the serial
   core and to several UART drivers

Patches 10-15
   Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
   a private field for the serial core, and add helper functions to test
   those statuses for UART drivers.

   This is a necessary step toward making tty port->flags SMP-safe, without
   introducing a new lock.

Patches 14, 17 and 21
   Make CTS flow control state SMP-safe, by moving hw_stopped into a private
   field of the serial core.

Patches 22-24
   Make tty flow control changes mutually exclusive.

   Protecting tty->stopped and tty->flow_stopped with a new spin lock (instead
   of the ->ctrl_lock) is required to fix multiple races with controlling
   terminal changes, coming in a follow-on patch series.

Patches 25 and 26
   Cleanup tcflow(TCIxxx)

Regards,

Peter Hurley (26):
  Revert "serial: uart: add hw flow control support configuration"
  serial: Style fix
  serial: imx: Fix x_char handling and tx flow control
  serial: core: Fix x_char race
  serial: core: Remove unsafe x_char optimization
  serial: Fix send_xchar() handlers
  serial: mpc52xx: Use default serial core x_char handler
  serial: sunsab: Don't enable tx if tx stopped
  serial: blackfin: Fix missing gpio.h
  serial: core: Document lock requirement for UPF_* flags updates
  serial: 8250: Document serial8250_modem_status() locking
  serial: core: Unwrap tertiary assignment in uart_handle_dcd_change()
  locking: Add non-fatal spin lock assert
  serial: core: Document and assert lock requirements for irq helpers
  serial: core: Privatize modem status enable flags
  isdn: i4l: Remove ASYNC_CTS_FLOW
  serial: core: Privatize tty->hw_stopped
  usb: serial: Remove unused tty->hw_stopped
  serial: bfin-uart: Fix auto CTS
  serial: core: Use spin_lock_irq() in uart_set_termios()
  tty: Convert tty_struct bitfield to bools
  tty: Serialize tty flow control changes with flow_lock
  tty: Move packet mode flow control notifications to pty driver
  tty: Serialize tcflow() with other tty flow control changes
  tty: Move and rename send_prio_char() as tty_send_xchar()
  tty: Hold termios_rwsem for tcflow(TCIxxx)

 .../devicetree/bindings/serial/of-serial.txt       |   1 -
 Documentation/serial/driver                        |   2 +
 drivers/isdn/i4l/isdn_tty.c                        |   5 -
 drivers/tty/pty.c                                  |  41 ++++++++
 drivers/tty/serial/8250/8250_core.c                |   7 +-
 drivers/tty/serial/bfin_sport_uart.c               |   1 +
 drivers/tty/serial/bfin_uart.c                     |  15 +--
 drivers/tty/serial/imx.c                           |  39 +++-----
 drivers/tty/serial/mpc52xx_uart.c                  |  17 ----
 drivers/tty/serial/mxs-auart.c                     |   2 +-
 drivers/tty/serial/of_serial.c                     |   4 -
 drivers/tty/serial/serial_core.c                   | 109 +++++++++++----------
 drivers/tty/serial/sunhv.c                         |   3 +
 drivers/tty/serial/sunsab.c                        |   5 +-
 drivers/tty/tty_io.c                               |  93 +++++++++++-------
 drivers/tty/tty_ioctl.c                            |  47 +++------
 drivers/usb/serial/digi_acceleport.c               |   7 +-
 drivers/usb/serial/io_ti.c                         |   7 +-
 drivers/usb/serial/ti_usb_3410_5052.c              |   7 +-
 include/linux/serial_core.h                        |  16 ++-
 include/linux/spinlock.h                           |   1 +
 include/linux/spinlock_api_smp.h                   |   1 +
 include/linux/spinlock_api_up.h                    |   1 +
 include/linux/tty.h                                |  11 ++-
 include/linux/tty_driver.h                         |   4 +
 25 files changed, 238 insertions(+), 208 deletions(-)

-- 
2.1.0

[PATCH 01/26] Revert "serial: uart: add hw flow control support configuration"

[Peter Hurley]

This reverts commit 06aa82e498c144c7784a6f3d3b55458b272d6146.
This commit purports to enable auto CTS flow control for the 8250
UART driver. However, the 8250 UART driver already supports auto
CTS flow control via UART_CAP_AFE and UART_CAP_EFR. Indeed, this
patch introduces another DT attribute for which an existing firmware
flag already exists ("auto-flow-control"). Furthermore, the use of
UPF_HARD_FLOW requires the UART driver to define .throttle and
.unthrottle methods, neither of which are defined for the 8250 UART
driver (which will result in a NULL ptr dereference). Finally, this patch
supposes to fix existing bugs in the serial core for auto CTS-enabled
hardware, but does not include the class of hardware for which these
bugs exist.

CC: Murali Karicheri <m-karicheri2@ti.com>
CC: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 Documentation/devicetree/bindings/serial/of-serial.txt |  1 -
 drivers/tty/serial/8250/8250_core.c                    |  6 ++----
 drivers/tty/serial/of_serial.c                         |  4 ----
 drivers/tty/serial/serial_core.c                       | 12 +++---------
 4 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
index 7705477..1928a3e 100644
--- a/Documentation/devicetree/bindings/serial/of-serial.txt
+++ b/Documentation/devicetree/bindings/serial/of-serial.txt
@@ -37,7 +37,6 @@ Optional properties:
 - auto-flow-control: one way to enable automatic flow control support. The
   driver is allowed to detect support for the capability even without this
   property.
-- has-hw-flow-control: the hardware has flow control capability.
 
 Example:
 
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 7a91c6d..109da60 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2333,11 +2333,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * the trigger, or the MCR RTS bit is cleared.  In the case where
 	 * the remote UART is not using CTS auto flow control, we must
 	 * have sufficient FIFO entries for the latency of the remote
-	 * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
-	 * AFE if hw flow control is supported
+	 * UART to respond.  IOW, at least 32 bytes of FIFO.
 	 */
-	if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32)) ||
-	    (port->flags & UPF_HARD_FLOW)) {
+	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
 		up->mcr &= ~UART_MCR_AFE;
 		if (termios->c_cflag & CRTSCTS)
 			up->mcr |= UART_MCR_AFE;
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 68d4455..27981e2 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -183,10 +183,6 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
 					  "auto-flow-control"))
 			port8250.capabilities |= UART_CAP_AFE;
 
-		if (of_property_read_bool(ofdev->dev.of_node,
-					  "has-hw-flow-control"))
-			port8250.port.flags |= UPF_HARD_FLOW;
-
 		ret = serial8250_register_8250_port(&port8250);
 		break;
 	}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c2f90ec..261e49e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -174,12 +174,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 			if (tty->termios.c_cflag & CBAUD)
 				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
 		}
-		/*
-		 * if hw support flow control without software intervention,
-		 * then skip the below check
-		 */
-		if (tty_port_cts_enabled(port) &&
-		    !(uport->flags & UPF_HARD_FLOW)) {
+
+		if (tty_port_cts_enabled(port)) {
 			spin_lock_irq(&uport->lock);
 			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
 				tty->hw_stopped = 1;
@@ -2777,9 +2773,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.cts++;
 
-	/* skip below code if the hw flow control is supported */
-	if (tty_port_cts_enabled(port) &&
-	    !(uport->flags & UPF_HARD_FLOW)) {
+	if (tty_port_cts_enabled(port)) {
 		if (tty->hw_stopped) {
 			if (status) {
 				tty->hw_stopped = 0;
-- 
2.1.0

Re: [PATCH 01/26] Revert "serial: uart: add hw flow control support configuration"

[Murali Karicheri]

Peter,

On 09/02/2014 05:39 PM, Peter Hurley wrote:
>  Finally, this patch
> supposes to fix existing bugs in the serial core for auto CTS-enabled
> hardware, but does not include the class of hardware for which these
> bugs exist.

What is this? What class of hardware are you referring to?

As discussed, I will send an updated patch for this as discussed..

Regards,

Murali
>
> CC: Murali Karicheri<m-karicheri2@ti.com>
> CC: Rob Herring<robh+dt@kernel.org>
> Signed-off-by: Peter Hurley<peter@hurleysoftware.com>
> ---
>   Documentation/devicetree/bindings/serial/of-serial.txt |  1 -
>   drivers/tty/serial/8250/8250_core.c                    |  6 ++----
>   drivers/tty/serial/of_serial.c                         |  4 ----
>   drivers/tty/serial/serial_core.c                       | 12 +++---------
>   4 files changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 7705477..1928a3e 100644
> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
> @@ -37,7 +37,6 @@ Optional properties:
>   - auto-flow-control: one way to enable automatic flow control support. The
>     driver is allowed to detect support for the capability even without this
>     property.
> -- has-hw-flow-control: the hardware has flow control capability.
>
>   Example:
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 7a91c6d..109da60 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -2333,11 +2333,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>   	 * the trigger, or the MCR RTS bit is cleared.  In the case where
>   	 * the remote UART is not using CTS auto flow control, we must
>   	 * have sufficient FIFO entries for the latency of the remote
> -	 * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
> -	 * AFE if hw flow control is supported
> +	 * UART to respond.  IOW, at least 32 bytes of FIFO.
>   	 */
> -	if ((up->capabilities&  UART_CAP_AFE&&  (port->fifosize>= 32)) ||
> -	    (port->flags&  UPF_HARD_FLOW)) {
> +	if (up->capabilities&  UART_CAP_AFE&&  port->fifosize>= 32) {
>   		up->mcr&= ~UART_MCR_AFE;
>   		if (termios->c_cflag&  CRTSCTS)
>   			up->mcr |= UART_MCR_AFE;
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index 68d4455..27981e2 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -183,10 +183,6 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>   					  "auto-flow-control"))
>   			port8250.capabilities |= UART_CAP_AFE;
>
> -		if (of_property_read_bool(ofdev->dev.of_node,
> -					  "has-hw-flow-control"))
> -			port8250.port.flags |= UPF_HARD_FLOW;
> -
>   		ret = serial8250_register_8250_port(&port8250);
>   		break;
>   	}
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c2f90ec..261e49e 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -174,12 +174,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>   			if (tty->termios.c_cflag&  CBAUD)
>   				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>   		}
> -		/*
> -		 * if hw support flow control without software intervention,
> -		 * then skip the below check
> -		 */
> -		if (tty_port_cts_enabled(port)&&
> -		    !(uport->flags&  UPF_HARD_FLOW)) {
> +
> +		if (tty_port_cts_enabled(port)) {
>   			spin_lock_irq(&uport->lock);
>   			if (!(uport->ops->get_mctrl(uport)&  TIOCM_CTS))
>   				tty->hw_stopped = 1;
> @@ -2777,9 +2773,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>
>   	uport->icount.cts++;
>
> -	/* skip below code if the hw flow control is supported */
> -	if (tty_port_cts_enabled(port)&&
> -	    !(uport->flags&  UPF_HARD_FLOW)) {
> +	if (tty_port_cts_enabled(port)) {
>   		if (tty->hw_stopped) {
>   			if (status) {
>   				tty->hw_stopped = 0;

Re: [PATCH 01/26] Revert "serial: uart: add hw flow control support configuration"

[Peter Hurley]

On 09/02/2014 05:50 PM, Murali Karicheri wrote:
> Peter,
> 
> On 09/02/2014 05:39 PM, Peter Hurley wrote:
>>  Finally, this patch
>> supposes to fix existing bugs in the serial core for auto CTS-enabled
>> hardware, but does not include the class of hardware for which these
>> bugs exist.
> 
> What is this? What class of hardware are you referring to?

UART_CAP_AFE and UART_CAP_EFR hardware.

> As discussed, I will send an updated patch for this as discussed..

Great.

And since that patch will touch every single line this reverts (and more)
with a different solution, I see no reason not to back this out.

Plus, as you can see, you're holding up progress.

Regards,
Peter Hurley

Re: [PATCH 01/26] Revert "serial: uart: add hw flow control support configuration"

[Murali Karicheri]

On 09/02/2014 05:58 PM, Peter Hurley wrote:
> On 09/02/2014 05:50 PM, Murali Karicheri wrote:
>> Peter,
>>
>> On 09/02/2014 05:39 PM, Peter Hurley wrote:
>>>   Finally, this patch
>>> supposes to fix existing bugs in the serial core for auto CTS-enabled
>>> hardware, but does not include the class of hardware for which these
>>> bugs exist.
>>
>> What is this? What class of hardware are you referring to?
>
> UART_CAP_AFE and UART_CAP_EFR hardware.
>
>> As discussed, I will send an updated patch for this as discussed..
>
> Great.
>
> And since that patch will touch every single line this reverts (and more)
> with a different solution, I see no reason not to back this out.
>

No Issues and I am fine with the revert.

> Plus, as you can see, you're holding up progress.

I need to make progress on the hardware that I deal with for which this 
patch was submitted :) Unfortunately this patch didn't get proper review 
before merge causing the issue. Just want to give right context.

Regards,
Murali
>
> Regards,
> Peter Hurley
>

[PATCH 02/26] serial: Style fix

[Peter Hurley]

Unwrap if() conditional; no functional change.

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 261e49e..87cde4c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1283,8 +1283,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	/* Handle transition away from B0 status */
 	else if (!(old_termios->c_cflag & CBAUD) && (cflag & CBAUD)) {
 		unsigned int mask = TIOCM_DTR;
-		if (!(cflag & CRTSCTS) ||
-		    !test_bit(TTY_THROTTLED, &tty->flags))
+		if (!(cflag & CRTSCTS) || !test_bit(TTY_THROTTLED, &tty->flags))
 			mask |= TIOCM_RTS;
 		uart_set_mctrl(uport, mask);
 	}
-- 
2.1.0

[PATCH 03/26] serial: imx: Fix x_char handling and tx flow control

[Peter Hurley]

The serial core expects the UART driver to transmit x_char
(START/STOP chars) even if tx is stopped and before data already
in the tx ring buffer if possible. Also, sending x_char must
not cause additional data in the tx ring buffer to transmit
if tx is stopped.

Cause x_char to be transmitted before any other data is sent.
Auto-stop tx if the tx ring buffer is empty or tx should be stopped.
Only perform one write wakeup if tx ring buffer space is below
threshold.

x_char handling in DMA mode is still broken; add FIXME.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/imx.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 044e86d..c86f153 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -464,9 +464,19 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
 {
 	struct circ_buf *xmit = &sport->port.state->xmit;
 
+	if (sport->port.x_char) {
+		/* Send next char */
+		writel(sport->port.x_char, sport->port.membase + URTX0);
+		return;
+	}
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
+		imx_stop_tx(&sport->port);
+		return;
+	}
+
 	while (!uart_circ_empty(xmit) &&
-			!(readl(sport->port.membase + uts_reg(sport))
-				& UTS_TXFULL)) {
+	       !(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
 		/* send xmit->buf[xmit->tail]
 		 * out the port here */
 		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
@@ -567,9 +577,6 @@ static void imx_start_tx(struct uart_port *port)
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long temp;
 
-	if (uart_circ_empty(&port->state->xmit))
-		return;
-
 	if (USE_IRDA(sport)) {
 		/* half duplex in IrDA mode; have to disable receive mode */
 		temp = readl(sport->port.membase + UCR4);
@@ -604,7 +611,10 @@ static void imx_start_tx(struct uart_port *port)
 	}
 
 	if (sport->dma_is_enabled) {
-		imx_dma_tx(sport);
+		/* FIXME: port->x_char must be transmitted if != 0 */
+		if (!uart_circ_empty(&port->state->xmit) &&
+		    !uart_tx_stopped(port))
+			imx_dma_tx(sport);
 		return;
 	}
 
@@ -632,27 +642,10 @@ static irqreturn_t imx_rtsint(int irq, void *dev_id)
 static irqreturn_t imx_txint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	struct circ_buf *xmit = &sport->port.state->xmit;
 	unsigned long flags;
 
 	spin_lock_irqsave(&sport->port.lock, flags);
-	if (sport->port.x_char) {
-		/* Send next char */
-		writel(sport->port.x_char, sport->port.membase + URTX0);
-		goto out;
-	}
-
-	if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
-		imx_stop_tx(&sport->port);
-		goto out;
-	}
-
 	imx_transmit_buffer(sport);
-
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-		uart_write_wakeup(&sport->port);
-
-out:
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 	return IRQ_HANDLED;
 }
-- 
2.1.0

[PATCH 04/26] serial: core: Fix x_char race

[Peter Hurley]

The UART driver is expected to clear port->x_char after
transmission while holding the port->lock. However, the serial
core fails to take the port->lock before assigning port->xchar.
This allows for the following race

CPU 0                         |  CPU 1
                              |
                              | serial8250_handle_irq
                              |   ...
                              |   serial8250_tx_chars
                              |     if (port->x_char)
                              |       serial_out(up, UART_TX, port->x_char)
uart_send_xchar               |
  port->x_char = ch           |
                              |       port->x_char = 0
  port->ops->start_tx()       |
                              |

The x_char on CPU 0 will never be sent.

Take the port->lock in uart_send_xchar() before assigning port->x_char.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 87cde4c..a68bff0 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -596,12 +596,11 @@ static void uart_send_xchar(struct tty_struct *tty, char ch)
 	if (port->ops->send_xchar)
 		port->ops->send_xchar(port, ch);
 	else {
+		spin_lock_irqsave(&port->lock, flags);
 		port->x_char = ch;
-		if (ch) {
-			spin_lock_irqsave(&port->lock, flags);
+		if (ch)
 			port->ops->start_tx(port);
-			spin_unlock_irqrestore(&port->lock, flags);
-		}
+		spin_unlock_irqrestore(&port->lock, flags);
 	}
 }
 
-- 
2.1.0

[PATCH 05/26] serial: core: Remove unsafe x_char optimization

[Peter Hurley]

uart_unthrottle() attempts to avoid sending START and the previous
x_char if the previous x_char has not yet been sent. However, this
optimization could leave the sender in a throttled state; for example,
if the sender is throttled and this unthrottle coincides with a manual
tcflow(TCION) from user-space, then neither START would be sent.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a68bff0..11c8fe6 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -643,12 +643,8 @@ static void uart_unthrottle(struct tty_struct *tty)
 		mask &= ~port->flags;
 	}
 
-	if (mask & UPF_SOFT_FLOW) {
-		if (port->x_char)
-			port->x_char = 0;
-		else
-			uart_send_xchar(tty, START_CHAR(tty));
-	}
+	if (mask & UPF_SOFT_FLOW)
+		uart_send_xchar(tty, START_CHAR(tty));
 
 	if (mask & UPF_HARD_FLOW)
 		uart_set_mctrl(port, TIOCM_RTS);
-- 
2.1.0

[PATCH 06/26] serial: Fix send_xchar() handlers

[Peter Hurley]

START_CHAR() & STOP_CHAR() can be disabled if set to '\0'
(__DISABLED_CHAR).  UART drivers which define a send_xchar()
handler must not transmit __DISABLED_CHAR.

Document requirement.

Affected drivers:
sunsab
sunhv

cc: David S. Miller <davem@davemloft.net>
cc: <sparclinux@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 Documentation/serial/driver | 2 ++
 drivers/tty/serial/sunhv.c  | 3 +++
 drivers/tty/serial/sunsab.c | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index 3bba1ae..ba64e4b 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -140,6 +140,8 @@ hardware.
 	will append the character to the circular buffer and then call
 	start_tx() / stop_tx() to flush the data out.
 
+	Do not transmit if ch == '\0' (__DISABLED_CHAR).
+
 	Locking: none.
 	Interrupts: caller dependent.
 
diff --git a/drivers/tty/serial/sunhv.c b/drivers/tty/serial/sunhv.c
index 20521db..25d43ce 100644
--- a/drivers/tty/serial/sunhv.c
+++ b/drivers/tty/serial/sunhv.c
@@ -268,6 +268,9 @@ static void sunhv_send_xchar(struct uart_port *port, char ch)
 	unsigned long flags;
 	int limit = 10000;
 
+	if (ch == __DISABLED_CHAR)
+		return;
+
 	spin_lock_irqsave(&port->lock, flags);
 
 	while (limit-- > 0) {
diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index bb09920..c83b1c6 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -459,6 +459,9 @@ static void sunsab_send_xchar(struct uart_port *port, char ch)
 	struct uart_sunsab_port *up = (struct uart_sunsab_port *) port;
 	unsigned long flags;
 
+	if (ch == __DISABLED_CHAR)
+		return;
+
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	sunsab_tec_wait(up);
-- 
2.1.0

[PATCH 07/26] serial: mpc52xx: Use default serial core x_char handler

[Peter Hurley]

mpc52xx_uart_send_xchar() is _identical_ to the default serial core
x_char handling behavior in uart_send_xchar().

Remove mpc52xx_uart_send_xchar().

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

diff --git a/drivers/tty/serial/mpc52xx_uart.c b/drivers/tty/serial/mpc52xx_uart.c
index 97888f4..a5f4e36 100644
--- a/drivers/tty/serial/mpc52xx_uart.c
+++ b/drivers/tty/serial/mpc52xx_uart.c
@@ -1087,22 +1087,6 @@ mpc52xx_uart_start_tx(struct uart_port *port)
 }
 
 static void
-mpc52xx_uart_send_xchar(struct uart_port *port, char ch)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&port->lock, flags);
-
-	port->x_char = ch;
-	if (ch) {
-		/* Make sure tx interrupts are on */
-		/* Truly necessary ??? They should be anyway */
-		psc_ops->start_tx(port);
-	}
-
-	spin_unlock_irqrestore(&port->lock, flags);
-}
-
-static void
 mpc52xx_uart_stop_rx(struct uart_port *port)
 {
 	/* port->lock taken by caller */
@@ -1361,7 +1345,6 @@ static struct uart_ops mpc52xx_uart_ops = {
 	.get_mctrl	= mpc52xx_uart_get_mctrl,
 	.stop_tx	= mpc52xx_uart_stop_tx,
 	.start_tx	= mpc52xx_uart_start_tx,
-	.send_xchar	= mpc52xx_uart_send_xchar,
 	.stop_rx	= mpc52xx_uart_stop_rx,
 	.enable_ms	= mpc52xx_uart_enable_ms,
 	.break_ctl	= mpc52xx_uart_break_ctl,
-- 
2.1.0

[PATCH 08/26] serial: sunsab: Don't enable tx if tx stopped

[Peter Hurley]

The serial core may call the UART driver's start_tx() even if
tx is stopped; the UART driver must verify tx should be enabled
before transmitting.

Reported-by: Sam Ravnborg <sam@ravnborg.org>
cc: David S. Miller <davem@davemloft.net>
cc: <sparclinux@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/sunsab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index c83b1c6..448a93b 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -427,7 +427,7 @@ static void sunsab_start_tx(struct uart_port *port)
 	struct circ_buf *xmit = &up->port.state->xmit;
 	int i;
 
-	if (uart_circ_empty(xmit))
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
 		return;
 
 	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
-- 
2.1.0

[PATCH 09/26] serial: blackfin: Fix missing gpio.h

[Peter Hurley]

If CONFIG_SERIAL_BFIN_CTSRTS is set, compile fails because of missing
declarations for the gpio_* api. Include necessary header.

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

diff --git a/drivers/tty/serial/bfin_sport_uart.c b/drivers/tty/serial/bfin_sport_uart.c
index 7810aa2..d62d8da 100644
--- a/drivers/tty/serial/bfin_sport_uart.c
+++ b/drivers/tty/serial/bfin_sport_uart.c
@@ -33,6 +33,7 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/serial_core.h>
+#include <linux/gpio.h>
 
 #include <asm/bfin_sport.h>
 #include <asm/delay.h>
-- 
2.1.0

[PATCH 10/26] serial: core: Document lock requirement for UPF_* flags updates

[Peter Hurley]

The flags field of struct uart_port can only be safely modified
if the port mutex is held; no other lock prevents concurrent
changes from corrupting the field.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/linux/serial_core.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5bbb809..4a43292 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -152,6 +152,7 @@ struct uart_port {
 	unsigned long		sysrq;			/* sysrq timeout */
 #endif
 
+	/* flags must be updated while holding port mutex */
 	upf_t			flags;
 
 #define UPF_FOURPORT		((__force upf_t) (1 << 1))
-- 
2.1.0

[PATCH 11/26] serial: 8250: Document serial8250_modem_status() locking

[Peter Hurley]

Existing callers of serial8250_modem_status() [1] hold the uart port
lock; document.

[1] In-tree callers of serial8250_modem_status()

drivers/tty/serial/8250/8250_fsl.c
  fsl8250_handle_irq()

drivers/tty/serial/8250/8250_core.c
  serial8250_handle_irq()
  serial8250_console_write()
  serial8250_get_mctrl() *

* Call graphs for callers of serial8250_get_mctrl() from the function
  which acquires the uart port lock

drivers/tty/serial/serial_core.c
  uart_port_startup()
  uart_tiocmget()
  uart_set_termios()
  uart_carrier_raised()
    ops->get_mctrl() ---> serial8250_get_mctrl()

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

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 109da60..d2615e3b 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1467,6 +1467,7 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_tx_chars);
 
+/* Caller holds uart port lock */
 unsigned int serial8250_modem_status(struct uart_8250_port *up)
 {
 	struct uart_port *port = &up->port;
-- 
2.1.0

[PATCH 12/26] serial: core: Unwrap tertiary assignment in uart_handle_dcd_change()

[Peter Hurley]

Prepare for spin lock assertion; move non-trivial assignment into
function body.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 11c8fe6..732d386 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2736,12 +2736,15 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 {
 	struct tty_port *port = &uport->state->port;
 	struct tty_struct *tty = port->tty;
-	struct tty_ldisc *ld = tty ? tty_ldisc_ref(tty) : NULL;
+	struct tty_ldisc *ld;
 
-	if (ld) {
-		if (ld->ops->dcd_change)
-			ld->ops->dcd_change(tty, status);
-		tty_ldisc_deref(ld);
+	if (tty) {
+		ld = tty_ldisc_ref(tty);
+		if (ld) {
+			if (ld->ops->dcd_change)
+				ld->ops->dcd_change(tty, status);
+			tty_ldisc_deref(ld);
+		}
 	}
 
 	uport->icount.dcd++;
-- 
2.1.0

[PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Hurley]

Provide method for non-essential or non-critical code to warn of
invariant errors.

CC: Ingo Molnar <mingo@kernel.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/linux/spinlock.h         | 1 +
 include/linux/spinlock_api_smp.h | 1 +
 include/linux/spinlock_api_up.h  | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 3f2867f..8a9aaf1 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -394,6 +394,7 @@ static inline int spin_can_lock(spinlock_t *lock)
 }
 
 #define assert_spin_locked(lock)	assert_raw_spin_locked(&(lock)->rlock)
+#define warn_not_spin_locked(lock)	warn_not_raw_spin_locked(&(lock)->rlock)
 
 /*
  * Pull the atomic_t declaration:
diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
index 42dfab8..0ddd499 100644
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -18,6 +18,7 @@
 int in_lock_functions(unsigned long addr);
 
 #define assert_raw_spin_locked(x)	BUG_ON(!raw_spin_is_locked(x))
+#define warn_not_raw_spin_locked(x)	WARN_ON_ONCE(!raw_spin_is_locked(x))
 
 void __lockfunc _raw_spin_lock(raw_spinlock_t *lock)		__acquires(lock);
 void __lockfunc _raw_spin_lock_nested(raw_spinlock_t *lock, int subclass)
diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
index d0d1888..f890f12 100644
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -17,6 +17,7 @@
 #define in_lock_functions(ADDR)		0
 
 #define assert_raw_spin_locked(lock)	do { (void)(lock); } while (0)
+#define warn_not_raw_spin_locked(lock)	do { (void)(lock); } while (0)
 
 /*
  * In the UP-nondebug case there's no real locking going on, so the
-- 
2.1.0

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Zijlstra]

On Tue, Sep 02, 2014 at 05:39:22PM -0400, Peter Hurley wrote:
> Provide method for non-essential or non-critical code to warn of
> invariant errors.
> 
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  include/linux/spinlock.h         | 1 +
>  include/linux/spinlock_api_smp.h | 1 +
>  include/linux/spinlock_api_up.h  | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 3f2867f..8a9aaf1 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -394,6 +394,7 @@ static inline int spin_can_lock(spinlock_t *lock)
>  }
>  
>  #define assert_spin_locked(lock)	assert_raw_spin_locked(&(lock)->rlock)
> +#define warn_not_spin_locked(lock)	warn_not_raw_spin_locked(&(lock)->rlock)
>  
>  /*
>   * Pull the atomic_t declaration:
> diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
> index 42dfab8..0ddd499 100644
> --- a/include/linux/spinlock_api_smp.h
> +++ b/include/linux/spinlock_api_smp.h
> @@ -18,6 +18,7 @@
>  int in_lock_functions(unsigned long addr);
>  
>  #define assert_raw_spin_locked(x)	BUG_ON(!raw_spin_is_locked(x))
> +#define warn_not_raw_spin_locked(x)	WARN_ON_ONCE(!raw_spin_is_locked(x))

No we should remove assert_spin_locked() not add to it. Use
lockdep_assert_held() instead.

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Hurley]

Hi Peter,

On 09/03/2014 05:27 AM, Peter Zijlstra wrote:
> On Tue, Sep 02, 2014 at 05:39:22PM -0400, Peter Hurley wrote:
>> Provide method for non-essential or non-critical code to warn of
>> invariant errors.
>>
>> CC: Ingo Molnar <mingo@kernel.org>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  include/linux/spinlock.h         | 1 +
>>  include/linux/spinlock_api_smp.h | 1 +
>>  include/linux/spinlock_api_up.h  | 1 +
>>  3 files changed, 3 insertions(+)
>>
>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
>> index 3f2867f..8a9aaf1 100644
>> --- a/include/linux/spinlock.h
>> +++ b/include/linux/spinlock.h
>> @@ -394,6 +394,7 @@ static inline int spin_can_lock(spinlock_t *lock)
>>  }
>>  
>>  #define assert_spin_locked(lock)	assert_raw_spin_locked(&(lock)->rlock)
>> +#define warn_not_spin_locked(lock)	warn_not_raw_spin_locked(&(lock)->rlock)
>>  
>>  /*
>>   * Pull the atomic_t declaration:
>> diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
>> index 42dfab8..0ddd499 100644
>> --- a/include/linux/spinlock_api_smp.h
>> +++ b/include/linux/spinlock_api_smp.h
>> @@ -18,6 +18,7 @@
>>  int in_lock_functions(unsigned long addr);
>>  
>>  #define assert_raw_spin_locked(x)	BUG_ON(!raw_spin_is_locked(x))
>> +#define warn_not_raw_spin_locked(x)	WARN_ON_ONCE(!raw_spin_is_locked(x))
> 
> No we should remove assert_spin_locked() not add to it. Use
> lockdep_assert_held() instead.

I probably should have been more descriptive in the changelog: this
is not for a test configuration, but rather, an assertion in an
exported api.

Regards,
Peter Hurley

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Zijlstra]

On Wed, Sep 03, 2014 at 07:20:04AM -0400, Peter Hurley wrote:
> Hi Peter,
> 
> On 09/03/2014 05:27 AM, Peter Zijlstra wrote:
> > On Tue, Sep 02, 2014 at 05:39:22PM -0400, Peter Hurley wrote:
> >> Provide method for non-essential or non-critical code to warn of
> >> invariant errors.
> >>
> >> CC: Ingo Molnar <mingo@kernel.org>
> >> CC: Peter Zijlstra <peterz@infradead.org>
> >> CC: Thomas Gleixner <tglx@linutronix.de>
> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> >> ---
> >>  include/linux/spinlock.h         | 1 +
> >>  include/linux/spinlock_api_smp.h | 1 +
> >>  include/linux/spinlock_api_up.h  | 1 +
> >>  3 files changed, 3 insertions(+)
> >>
> >> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> >> index 3f2867f..8a9aaf1 100644
> >> --- a/include/linux/spinlock.h
> >> +++ b/include/linux/spinlock.h
> >> @@ -394,6 +394,7 @@ static inline int spin_can_lock(spinlock_t *lock)
> >>  }
> >>  
> >>  #define assert_spin_locked(lock)	assert_raw_spin_locked(&(lock)->rlock)
> >> +#define warn_not_spin_locked(lock)	warn_not_raw_spin_locked(&(lock)->rlock)
> >>  
> >>  /*
> >>   * Pull the atomic_t declaration:
> >> diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
> >> index 42dfab8..0ddd499 100644
> >> --- a/include/linux/spinlock_api_smp.h
> >> +++ b/include/linux/spinlock_api_smp.h
> >> @@ -18,6 +18,7 @@
> >>  int in_lock_functions(unsigned long addr);
> >>  
> >>  #define assert_raw_spin_locked(x)	BUG_ON(!raw_spin_is_locked(x))
> >> +#define warn_not_raw_spin_locked(x)	WARN_ON_ONCE(!raw_spin_is_locked(x))
> > 
> > No we should remove assert_spin_locked() not add to it. Use
> > lockdep_assert_held() instead.
> 
> I probably should have been more descriptive in the changelog: this
> is not for a test configuration, but rather, an assertion in an
> exported api.

So ?

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Hurley]

On 09/03/2014 10:40 AM, Peter Zijlstra wrote:
> On Wed, Sep 03, 2014 at 07:20:04AM -0400, Peter Hurley wrote:
>> Hi Peter,
>>
>> On 09/03/2014 05:27 AM, Peter Zijlstra wrote:
>>> On Tue, Sep 02, 2014 at 05:39:22PM -0400, Peter Hurley wrote:
>>>> Provide method for non-essential or non-critical code to warn of
>>>> invariant errors.
>>>>
>>>> CC: Ingo Molnar <mingo@kernel.org>
>>>> CC: Peter Zijlstra <peterz@infradead.org>
>>>> CC: Thomas Gleixner <tglx@linutronix.de>
>>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>>> ---
>>>>  include/linux/spinlock.h         | 1 +
>>>>  include/linux/spinlock_api_smp.h | 1 +
>>>>  include/linux/spinlock_api_up.h  | 1 +
>>>>  3 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
>>>> index 3f2867f..8a9aaf1 100644
>>>> --- a/include/linux/spinlock.h
>>>> +++ b/include/linux/spinlock.h
>>>> @@ -394,6 +394,7 @@ static inline int spin_can_lock(spinlock_t *lock)
>>>>  }
>>>>  
>>>>  #define assert_spin_locked(lock)	assert_raw_spin_locked(&(lock)->rlock)
>>>> +#define warn_not_spin_locked(lock)	warn_not_raw_spin_locked(&(lock)->rlock)
>>>>  
>>>>  /*
>>>>   * Pull the atomic_t declaration:
>>>> diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
>>>> index 42dfab8..0ddd499 100644
>>>> --- a/include/linux/spinlock_api_smp.h
>>>> +++ b/include/linux/spinlock_api_smp.h
>>>> @@ -18,6 +18,7 @@
>>>>  int in_lock_functions(unsigned long addr);
>>>>  
>>>>  #define assert_raw_spin_locked(x)	BUG_ON(!raw_spin_is_locked(x))
>>>> +#define warn_not_raw_spin_locked(x)	WARN_ON_ONCE(!raw_spin_is_locked(x))
>>>
>>> No we should remove assert_spin_locked() not add to it. Use
>>> lockdep_assert_held() instead.
>>
>> I probably should have been more descriptive in the changelog: this
>> is not for a test configuration, but rather, an assertion in an
>> exported api.
> 
> So ?

So a lockdep-only assert is unlikely to draw attention to existing bugs,
especially in established drivers.

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Zijlstra]

On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
> So a lockdep-only assert is unlikely to draw attention to existing bugs,
> especially in established drivers.

By the same logic lockdep will not find locking errors in established
drivers.

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
> > So a lockdep-only assert is unlikely to draw attention to existing bugs,
> > especially in established drivers.
> 
> By the same logic lockdep will not find locking errors in established
> drivers.

Indeed, this patch is ill-advised in several ways:

  - it extends an API variant that we want to phase

  - emits a warning even if say lockdep has already emitted a
    warning and locking state is not guaranteed to be consistent. 

  - makes the kernel more expensive once fully debugged, in that
    non-fatal checks are unconditional.

Also please submit locking related patches as standalone series 
to the locking subsystem, not embedded in an unrelated series.

Thanks,

	Ingo

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Hurley]

On 09/04/2014 01:14 AM, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
>>> So a lockdep-only assert is unlikely to draw attention to existing bugs,
>>> especially in established drivers.
>>
>> By the same logic lockdep will not find locking errors in established
>> drivers.
> 
> Indeed, this patch is ill-advised in several ways:
> 
>   - it extends an API variant that we want to phase
> 
>   - emits a warning even if say lockdep has already emitted a
>     warning and locking state is not guaranteed to be consistent. 
> 
>   - makes the kernel more expensive once fully debugged, in that
>     non-fatal checks are unconditional.

Ok.

One thing: I'm not seeing how lockdep_assert_held() switches off once
the warning has been emitted? Is the caller expected to construct their
own _ONCE tags?

Regards,
Peter Hurley

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Zijlstra]

On Wed, Sep 10, 2014 at 07:02:10AM -0400, Peter Hurley wrote:
> On 09/04/2014 01:14 AM, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> >> On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
> >>> So a lockdep-only assert is unlikely to draw attention to existing bugs,
> >>> especially in established drivers.
> >>
> >> By the same logic lockdep will not find locking errors in established
> >> drivers.
> > 
> > Indeed, this patch is ill-advised in several ways:
> > 
> >   - it extends an API variant that we want to phase
> > 
> >   - emits a warning even if say lockdep has already emitted a
> >     warning and locking state is not guaranteed to be consistent. 
> > 
> >   - makes the kernel more expensive once fully debugged, in that
> >     non-fatal checks are unconditional.
> 
> Ok.
> 
> One thing: I'm not seeing how lockdep_assert_held() switches off once
> the warning has been emitted? Is the caller expected to construct their
> own _ONCE tags?

Indeed, it does not do that. I suppose you can add
lockdep_assert_held_once() or somesuch if you think the once thing is
important.

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Peter Hurley]

On 09/10/2014 09:08 AM, Peter Zijlstra wrote:
> On Wed, Sep 10, 2014 at 07:02:10AM -0400, Peter Hurley wrote:
>> On 09/04/2014 01:14 AM, Ingo Molnar wrote:
>>>
>>> * Peter Zijlstra <peterz@infradead.org> wrote:
>>>
>>>> On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
>>>>> So a lockdep-only assert is unlikely to draw attention to existing bugs,
>>>>> especially in established drivers.
>>>>
>>>> By the same logic lockdep will not find locking errors in established
>>>> drivers.
>>>
>>> Indeed, this patch is ill-advised in several ways:
>>>
>>>   - it extends an API variant that we want to phase
>>>
>>>   - emits a warning even if say lockdep has already emitted a
>>>     warning and locking state is not guaranteed to be consistent. 
>>>
>>>   - makes the kernel more expensive once fully debugged, in that
>>>     non-fatal checks are unconditional.
>>
>> Ok.
>>
>> One thing: I'm not seeing how lockdep_assert_held() switches off once
>> the warning has been emitted? Is the caller expected to construct their
>> own _ONCE tags?
> 
> Indeed, it does not do that. I suppose you can add
> lockdep_assert_held_once() or somesuch if you think the once thing is
> important.

Ok, will do.

On 09/04/2014 01:14 AM, Ingo Molnar wrote:
> Also please submit locking related patches as standalone series 
> to the locking subsystem, not embedded in an unrelated series.

Ok, but how will Greg know when to take the series that depends on
this change, if the locking change is submitted separately?

Regards,
Peter Hurley

Re: [PATCH 13/26] locking: Add non-fatal spin lock assert

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 10:45:33AM -0400, Peter Hurley wrote:
> On 09/10/2014 09:08 AM, Peter Zijlstra wrote:
> > On Wed, Sep 10, 2014 at 07:02:10AM -0400, Peter Hurley wrote:
> >> On 09/04/2014 01:14 AM, Ingo Molnar wrote:
> >>>
> >>> * Peter Zijlstra <peterz@infradead.org> wrote:
> >>>
> >>>> On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
> >>>>> So a lockdep-only assert is unlikely to draw attention to existing bugs,
> >>>>> especially in established drivers.
> >>>>
> >>>> By the same logic lockdep will not find locking errors in established
> >>>> drivers.
> >>>
> >>> Indeed, this patch is ill-advised in several ways:
> >>>
> >>>   - it extends an API variant that we want to phase
> >>>
> >>>   - emits a warning even if say lockdep has already emitted a
> >>>     warning and locking state is not guaranteed to be consistent. 
> >>>
> >>>   - makes the kernel more expensive once fully debugged, in that
> >>>     non-fatal checks are unconditional.
> >>
> >> Ok.
> >>
> >> One thing: I'm not seeing how lockdep_assert_held() switches off once
> >> the warning has been emitted? Is the caller expected to construct their
> >> own _ONCE tags?
> > 
> > Indeed, it does not do that. I suppose you can add
> > lockdep_assert_held_once() or somesuch if you think the once thing is
> > important.
> 
> Ok, will do.
> 
> On 09/04/2014 01:14 AM, Ingo Molnar wrote:
> > Also please submit locking related patches as standalone series 
> > to the locking subsystem, not embedded in an unrelated series.
> 
> Ok, but how will Greg know when to take the series that depends on
> this change, if the locking change is submitted separately?

 Cc: me on those changes, and I can track what happens with the tip tree
 where these would show up.

Or wait a release cycle, that also is an easy way.

thanks,

greg k-h

[PATCH 14/26] serial: core: Document and assert lock requirements for irq helpers

[Peter Hurley]

The serial core provides two helper functions, uart_handle_dcd_change()
and uart_handle_cts_change(), for UART drivers to use at interrupt
time. The serial core expects the UART driver to hold the uart port lock
when calling these helpers to prevent state corruption.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 732d386..b50050d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2731,6 +2731,8 @@ EXPORT_SYMBOL(uart_match_port);
  *	uart_handle_dcd_change - handle a change of carrier detect state
  *	@uport: uart_port structure for the open port
  *	@status: new carrier detect status, nonzero if active
+ *
+ *	Caller must hold uport->lock
  */
 void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 {
@@ -2738,6 +2740,8 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 	struct tty_struct *tty = port->tty;
 	struct tty_ldisc *ld;
 
+	warn_not_spin_locked(&uport->lock);
+
 	if (tty) {
 		ld = tty_ldisc_ref(tty);
 		if (ld) {
@@ -2762,12 +2766,16 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
  *	uart_handle_cts_change - handle a change of clear-to-send state
  *	@uport: uart_port structure for the open port
  *	@status: new clear to send status, nonzero if active
+ *
+ *	Caller must hold uport->lock
  */
 void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 {
 	struct tty_port *port = &uport->state->port;
 	struct tty_struct *tty = port->tty;
 
+	warn_not_spin_locked(&uport->lock);
+
 	uport->icount.cts++;
 
 	if (tty_port_cts_enabled(port)) {
-- 
2.1.0

[PATCH 15/26] serial: core: Privatize modem status enable flags

[Peter Hurley]

The serial core uses the tty port flags, ASYNC_CTS_FLOW and
ASYNC_CD_CHECK, to track whether CTS and DCD changes should be
ignored or handled. However, the tty port flags are not safe for
atomic bit operations and no lock provides serialized updates.

Introduce the struct uart_port status field to track CTS and DCD
enable states, and serialize access with uart port lock. Substitute
uart_cts_enabled() helper for tty_port_cts_enabled().

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/mxs-auart.c   |  2 +-
 drivers/tty/serial/serial_core.c | 29 +++++++++++++++++------------
 include/linux/serial_core.h      | 12 ++++++++++++
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index b5c3292..10c2933 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -408,7 +408,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
 	if (mctrl & TIOCM_RTS) {
-		if (tty_port_cts_enabled(&u->state->port))
+		if (uart_cts_enabled(u))
 			ctrl |= AUART_CTRL2_RTSEN;
 		else
 			ctrl |= AUART_CTRL2_RTS;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b50050d..288dc2e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -58,6 +58,11 @@ static void uart_change_pm(struct uart_state *state,
 
 static void uart_port_shutdown(struct tty_port *port);
 
+static int uart_dcd_enabled(struct uart_port *uport)
+{
+	return uport->status & UPSTAT_DCD_ENABLE;
+}
+
 /*
  * This routine is used by the interrupt handler to schedule processing in
  * the software interrupt portion of the driver.
@@ -129,7 +134,6 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		int init_hw)
 {
 	struct uart_port *uport = state->uart_port;
-	struct tty_port *port = &state->port;
 	unsigned long page;
 	int retval = 0;
 
@@ -175,12 +179,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
 		}
 
-		if (tty_port_cts_enabled(port)) {
-			spin_lock_irq(&uport->lock);
+		spin_lock_irq(&uport->lock);
+		if (uart_cts_enabled(uport)) {
 			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
 				tty->hw_stopped = 1;
-			spin_unlock_irq(&uport->lock);
 		}
+		spin_unlock_irq(&uport->lock);
 	}
 
 	/*
@@ -431,7 +435,6 @@ EXPORT_SYMBOL(uart_get_divisor);
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios)
 {
-	struct tty_port *port = &state->port;
 	struct uart_port *uport = state->uart_port;
 	struct ktermios *termios;
 
@@ -446,17 +449,19 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 	uport->ops->set_termios(uport, termios, old_termios);
 
 	/*
-	 * Set flags based on termios cflag
+	 * Set modem status enables based on termios cflag
 	 */
+	spin_lock_irq(&uport->lock);
 	if (termios->c_cflag & CRTSCTS)
-		set_bit(ASYNCB_CTS_FLOW, &port->flags);
+		uport->status |= UPSTAT_CTS_ENABLE;
 	else
-		clear_bit(ASYNCB_CTS_FLOW, &port->flags);
+		uport->status &= ~UPSTAT_CTS_ENABLE;
 
 	if (termios->c_cflag & CLOCAL)
-		clear_bit(ASYNCB_CHECK_CD, &port->flags);
+		uport->status &= ~UPSTAT_DCD_ENABLE;
 	else
-		set_bit(ASYNCB_CHECK_CD, &port->flags);
+		uport->status |= UPSTAT_DCD_ENABLE;
+	spin_unlock_irq(&uport->lock);
 }
 
 static inline int __uart_put_char(struct uart_port *port,
@@ -2753,7 +2758,7 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.dcd++;
 
-	if (port->flags & ASYNC_CHECK_CD) {
+	if (uart_dcd_enabled(uport)) {
 		if (status)
 			wake_up_interruptible(&port->open_wait);
 		else if (tty)
@@ -2778,7 +2783,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.cts++;
 
-	if (tty_port_cts_enabled(port)) {
+	if (uart_cts_enabled(uport)) {
 		if (tty->hw_stopped) {
 			if (status) {
 				tty->hw_stopped = 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4a43292..bc70048 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -112,6 +112,7 @@ struct uart_icount {
 };
 
 typedef unsigned int __bitwise__ upf_t;
+typedef unsigned int __bitwise__ upstat_t;
 
 struct uart_port {
 	spinlock_t		lock;			/* port lock */
@@ -188,6 +189,12 @@ struct uart_port {
 #define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
 #define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
 
+	/* status must be updated while holding port lock */
+	upstat_t		status;
+
+#define UPSTAT_CTS_ENABLE	((__force upstat_t) (1 << 0))
+#define UPSTAT_DCD_ENABLE	((__force upstat_t) (1 << 1))
+
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */
 	unsigned int		type;			/* port type */
@@ -351,6 +358,11 @@ static inline int uart_tx_stopped(struct uart_port *port)
 	return 0;
 }
 
+static inline bool uart_cts_enabled(struct uart_port *uport)
+{
+	return uport->status & UPSTAT_CTS_ENABLE;
+}
+
 /*
  * The following are helper functions for the low level drivers.
  */
-- 
2.1.0

[PATCH 16/26] isdn: i4l: Remove ASYNC_CTS_FLOW

[Peter Hurley]

ISDN4Linux always enables CTS flow control and does not use the
tty_port_cts_enabled() helper function; remove ASYNC_CTS_FLOW
state enable/disable.

cc: Karsten Keil <isdn@linux-pingi.de>
cc: <netdev@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/isdn/i4l/isdn_tty.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 3c5f249..bc91261 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1043,11 +1043,6 @@ isdn_tty_change_speed(modem_info *info)
 	if (!(cflag & PARODD))
 		cval |= UART_LCR_EPAR;
 
-	/* CTS flow control flag and modem status interrupts */
-	if (cflag & CRTSCTS) {
-		port->flags |= ASYNC_CTS_FLOW;
-	} else
-		port->flags &= ~ASYNC_CTS_FLOW;
 	if (cflag & CLOCAL)
 		port->flags &= ~ASYNC_CHECK_CD;
 	else {
-- 
2.1.0

[PATCH 17/26] serial: core: Privatize tty->hw_stopped

[Peter Hurley]

tty->hw_stopped is not used by the tty core and is thread-unsafe;
hw_stopped is a member of a bitfield whose fields are updated
non-atomically and no lock is suitable for serializing updates.

Replace serial core usage of tty->hw_stopped with uport->hw_stopped.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/bfin_uart.c   | 14 +++++++-------
 drivers/tty/serial/serial_core.c | 28 +++++++++++++---------------
 include/linux/serial_core.h      |  3 ++-
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index dec0fd7..fc9fbf3 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -108,22 +108,22 @@ static void bfin_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 static irqreturn_t bfin_serial_mctrl_cts_int(int irq, void *dev_id)
 {
 	struct bfin_serial_port *uart = dev_id;
-	unsigned int status = bfin_serial_get_mctrl(&uart->port);
+	struct uart_port *uport = &uart->port;
+	unsigned int status = bfin_serial_get_mctrl(uport);
 #ifdef CONFIG_SERIAL_BFIN_HARD_CTSRTS
-	struct tty_struct *tty = uart->port.state->port.tty;
 
 	UART_CLEAR_SCTS(uart);
-	if (tty->hw_stopped) {
+	if (uport->hw_stopped) {
 		if (status) {
-			tty->hw_stopped = 0;
-			uart_write_wakeup(&uart->port);
+			uport->hw_stopped = 0;
+			uart_write_wakeup(uport);
 		}
 	} else {
 		if (!status)
-			tty->hw_stopped = 1;
+			uport->hw_stopped = 1;
 	}
 #endif
-	uart_handle_cts_change(&uart->port, status & TIOCM_CTS);
+	uart_handle_cts_change(uport, status & TIOCM_CTS);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 288dc2e..95277a2 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -94,7 +94,7 @@ static void __uart_start(struct tty_struct *tty)
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *port = state->uart_port;
 
-	if (!tty->stopped && !tty->hw_stopped)
+	if (!uart_tx_stopped(port))
 		port->ops->start_tx(port);
 }
 
@@ -180,10 +180,11 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		}
 
 		spin_lock_irq(&uport->lock);
-		if (uart_cts_enabled(uport)) {
-			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
-				tty->hw_stopped = 1;
-		}
+		if (uart_cts_enabled(uport) &&
+		    !(uport->ops->get_mctrl(uport) & TIOCM_CTS))
+			uport->hw_stopped = 1;
+		else
+			uport->hw_stopped = 0;
 		spin_unlock_irq(&uport->lock);
 	}
 
@@ -944,7 +945,7 @@ static int uart_get_lsr_info(struct tty_struct *tty,
 	 */
 	if (uport->x_char ||
 	    ((uart_circ_chars_pending(&state->xmit) > 0) &&
-	     !tty->stopped && !tty->hw_stopped))
+	     !uart_tx_stopped(uport)))
 		result &= ~TIOCSER_TEMT;
 
 	return put_user(result, value);
@@ -1290,7 +1291,7 @@ static void uart_set_termios(struct tty_struct *tty,
 
 	/*
 	 * If the port is doing h/w assisted flow control, do nothing.
-	 * We assume that tty->hw_stopped has never been set.
+	 * We assume that port->hw_stopped has never been set.
 	 */
 	if (uport->flags & UPF_HARD_FLOW)
 		return;
@@ -1298,7 +1299,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
 		spin_lock_irqsave(&uport->lock, flags);
-		tty->hw_stopped = 0;
+		uport->hw_stopped = 0;
 		__uart_start(tty);
 		spin_unlock_irqrestore(&uport->lock, flags);
 	}
@@ -1306,7 +1307,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
 		spin_lock_irqsave(&uport->lock, flags);
 		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
-			tty->hw_stopped = 1;
+			uport->hw_stopped = 1;
 			uport->ops->stop_tx(uport);
 		}
 		spin_unlock_irqrestore(&uport->lock, flags);
@@ -2776,23 +2777,20 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
  */
 void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 {
-	struct tty_port *port = &uport->state->port;
-	struct tty_struct *tty = port->tty;
-
 	warn_not_spin_locked(&uport->lock);
 
 	uport->icount.cts++;
 
 	if (uart_cts_enabled(uport)) {
-		if (tty->hw_stopped) {
+		if (uport->hw_stopped) {
 			if (status) {
-				tty->hw_stopped = 0;
+				uport->hw_stopped = 0;
 				uport->ops->start_tx(uport);
 				uart_write_wakeup(uport);
 			}
 		} else {
 			if (!status) {
-				tty->hw_stopped = 1;
+				uport->hw_stopped = 1;
 				uport->ops->stop_tx(uport);
 			}
 		}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index bc70048..c87aaf8 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -195,6 +195,7 @@ struct uart_port {
 #define UPSTAT_CTS_ENABLE	((__force upstat_t) (1 << 0))
 #define UPSTAT_DCD_ENABLE	((__force upstat_t) (1 << 1))
 
+	bool			hw_stopped;		/* sw-assisted CTS flow state */
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */
 	unsigned int		type;			/* port type */
@@ -353,7 +354,7 @@ int uart_resume_port(struct uart_driver *reg, struct uart_port *port);
 static inline int uart_tx_stopped(struct uart_port *port)
 {
 	struct tty_struct *tty = port->state->port.tty;
-	if(tty->stopped || tty->hw_stopped)
+	if (tty->stopped || port->hw_stopped)
 		return 1;
 	return 0;
 }
-- 
2.1.0

Re: [PATCH 17/26] serial: core: Privatize tty->hw_stopped

[Peter Hurley]

On 09/02/2014 05:39 PM, Peter Hurley wrote:
> tty->hw_stopped is not used by the tty core and is thread-unsafe;
> hw_stopped is a member of a bitfield whose fields are updated
> non-atomically and no lock is suitable for serializing updates.
> 
> Replace serial core usage of tty->hw_stopped with uport->hw_stopped.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/bfin_uart.c   | 14 +++++++-------
>  drivers/tty/serial/serial_core.c | 28 +++++++++++++---------------
>  include/linux/serial_core.h      |  3 ++-
>  3 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
> index dec0fd7..fc9fbf3 100644
> --- a/drivers/tty/serial/bfin_uart.c
> +++ b/drivers/tty/serial/bfin_uart.c
> @@ -108,22 +108,22 @@ static void bfin_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  static irqreturn_t bfin_serial_mctrl_cts_int(int irq, void *dev_id)
>  {
>  	struct bfin_serial_port *uart = dev_id;
> -	unsigned int status = bfin_serial_get_mctrl(&uart->port);
> +	struct uart_port *uport = &uart->port;
> +	unsigned int status = bfin_serial_get_mctrl(uport);
>  #ifdef CONFIG_SERIAL_BFIN_HARD_CTSRTS
> -	struct tty_struct *tty = uart->port.state->port.tty;
>  
>  	UART_CLEAR_SCTS(uart);
> -	if (tty->hw_stopped) {
> +	if (uport->hw_stopped) {
>  		if (status) {
> -			tty->hw_stopped = 0;
> -			uart_write_wakeup(&uart->port);
> +			uport->hw_stopped = 0;
> +			uart_write_wakeup(uport);
>  		}
>  	} else {
>  		if (!status)
> -			tty->hw_stopped = 1;
> +			uport->hw_stopped = 1;
>  	}
>  #endif
> -	uart_handle_cts_change(&uart->port, status & TIOCM_CTS);
> +	uart_handle_cts_change(uport, status & TIOCM_CTS);
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 288dc2e..95277a2 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -94,7 +94,7 @@ static void __uart_start(struct tty_struct *tty)
>  	struct uart_state *state = tty->driver_data;
>  	struct uart_port *port = state->uart_port;
>  
> -	if (!tty->stopped && !tty->hw_stopped)
> +	if (!uart_tx_stopped(port))
>  		port->ops->start_tx(port);
>  }
>  
> @@ -180,10 +180,11 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>  		}
>  
>  		spin_lock_irq(&uport->lock);
> -		if (uart_cts_enabled(uport)) {
> -			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
> -				tty->hw_stopped = 1;
> -		}
> +		if (uart_cts_enabled(uport) &&
> +		    !(uport->ops->get_mctrl(uport) & TIOCM_CTS))
> +			uport->hw_stopped = 1;
> +		else
> +			uport->hw_stopped = 0;
>  		spin_unlock_irq(&uport->lock);
>  	}
>  
> @@ -944,7 +945,7 @@ static int uart_get_lsr_info(struct tty_struct *tty,
>  	 */
>  	if (uport->x_char ||
>  	    ((uart_circ_chars_pending(&state->xmit) > 0) &&
> -	     !tty->stopped && !tty->hw_stopped))
> +	     !uart_tx_stopped(uport)))
>  		result &= ~TIOCSER_TEMT;
>  
>  	return put_user(result, value);
> @@ -1290,7 +1291,7 @@ static void uart_set_termios(struct tty_struct *tty,
>  
>  	/*
>  	 * If the port is doing h/w assisted flow control, do nothing.
> -	 * We assume that tty->hw_stopped has never been set.
> +	 * We assume that port->hw_stopped has never been set.
>  	 */
>  	if (uport->flags & UPF_HARD_FLOW)
>  		return;
> @@ -1298,7 +1299,7 @@ static void uart_set_termios(struct tty_struct *tty,
>  	/* Handle turning off CRTSCTS */
>  	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
>  		spin_lock_irqsave(&uport->lock, flags);
> -		tty->hw_stopped = 0;
> +		uport->hw_stopped = 0;
>  		__uart_start(tty);
>  		spin_unlock_irqrestore(&uport->lock, flags);
>  	}
> @@ -1306,7 +1307,7 @@ static void uart_set_termios(struct tty_struct *tty,
>  	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
>  		spin_lock_irqsave(&uport->lock, flags);
>  		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
> -			tty->hw_stopped = 1;
> +			uport->hw_stopped = 1;
>  			uport->ops->stop_tx(uport);
>  		}
>  		spin_unlock_irqrestore(&uport->lock, flags);
> @@ -2776,23 +2777,20 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
>   */
>  void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>  {
> -	struct tty_port *port = &uport->state->port;
> -	struct tty_struct *tty = port->tty;
> -
>  	warn_not_spin_locked(&uport->lock);
>  
>  	uport->icount.cts++;
>  
>  	if (uart_cts_enabled(uport)) {
> -		if (tty->hw_stopped) {
> +		if (uport->hw_stopped) {
>  			if (status) {
> -				tty->hw_stopped = 0;
> +				uport->hw_stopped = 0;
>  				uport->ops->start_tx(uport);
>  				uart_write_wakeup(uport);
>  			}
>  		} else {
>  			if (!status) {
> -				tty->hw_stopped = 1;
> +				uport->hw_stopped = 1;
>  				uport->ops->stop_tx(uport);
>  			}
>  		}
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index bc70048..c87aaf8 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -195,6 +195,7 @@ struct uart_port {
>  #define UPSTAT_CTS_ENABLE	((__force upstat_t) (1 << 0))
>  #define UPSTAT_DCD_ENABLE	((__force upstat_t) (1 << 1))
>  
> +	bool			hw_stopped;		/* sw-assisted CTS flow state */

This is fragile on Alpha as well (byte storage) in structure with
mixed cpu contexts.

Regards,
Peter Hurley

>  	unsigned int		mctrl;			/* current modem ctrl settings */
>  	unsigned int		timeout;		/* character-based timeout */
>  	unsigned int		type;			/* port type */
> @@ -353,7 +354,7 @@ int uart_resume_port(struct uart_driver *reg, struct uart_port *port);
>  static inline int uart_tx_stopped(struct uart_port *port)
>  {
>  	struct tty_struct *tty = port->state->port.tty;
> -	if(tty->stopped || tty->hw_stopped)
> +	if (tty->stopped || port->hw_stopped)
>  		return 1;
>  	return 0;
>  }
> 

[PATCH 18/26] usb: serial: Remove unused tty->hw_stopped

[Peter Hurley]

The tty core does not test tty->hw_stopped; remove from drivers
which don't test it themselves.

cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/usb/serial/digi_acceleport.c  | 7 +------
 drivers/usb/serial/io_ti.c            | 7 +------
 drivers/usb/serial/ti_usb_3410_5052.c | 7 +------
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 8a23c53..12b0e67 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -834,7 +834,6 @@ static void digi_set_termios(struct tty_struct *tty,
 			arg |= DIGI_OUTPUT_FLOW_CONTROL_CTS;
 		} else {
 			arg &= ~DIGI_OUTPUT_FLOW_CONTROL_CTS;
-			tty->hw_stopped = 0;
 		}
 
 		buf[i++] = DIGI_CMD_SET_OUTPUT_FLOW_CONTROL;
@@ -1500,15 +1499,11 @@ static int digi_read_oob_callback(struct urb *urb)
 			if (val & DIGI_READ_INPUT_SIGNALS_CTS) {
 				priv->dp_modem_signals |= TIOCM_CTS;
 				/* port must be open to use tty struct */
-				if (rts) {
-					tty->hw_stopped = 0;
+				if (rts)
 					tty_port_tty_wakeup(&port->port);
-				}
 			} else {
 				priv->dp_modem_signals &= ~TIOCM_CTS;
 				/* port must be open to use tty struct */
-				if (rts)
-					tty->hw_stopped = 1;
 			}
 			if (val & DIGI_READ_INPUT_SIGNALS_DSR)
 				priv->dp_modem_signals |= TIOCM_DSR;
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index c0a42e9..ddbb8fe 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1456,12 +1456,8 @@ static void handle_new_msr(struct edgeport_port *edge_port, __u8 msr)
 	tty = tty_port_tty_get(&edge_port->port->port);
 	/* handle CTS flow control */
 	if (tty && C_CRTSCTS(tty)) {
-		if (msr & EDGEPORT_MSR_CTS) {
-			tty->hw_stopped = 0;
+		if (msr & EDGEPORT_MSR_CTS)
 			tty_wakeup(tty);
-		} else {
-			tty->hw_stopped = 1;
-		}
 	}
 	tty_kref_put(tty);
 }
@@ -2177,7 +2173,6 @@ static void change_port_settings(struct tty_struct *tty,
 		dev_dbg(dev, "%s - RTS/CTS is enabled\n", __func__);
 	} else {
 		dev_dbg(dev, "%s - RTS/CTS is disabled\n", __func__);
-		tty->hw_stopped = 0;
 		restart_read(edge_port);
 	}
 
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 3dd3ff8..e9da41d 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -773,7 +773,6 @@ static void ti_set_termios(struct tty_struct *tty,
 			config->wFlags |= TI_UART_ENABLE_RTS_IN;
 		config->wFlags |= TI_UART_ENABLE_CTS_OUT;
 	} else {
-		tty->hw_stopped = 0;
 		ti_restart_read(tport, tty);
 	}
 
@@ -1291,12 +1290,8 @@ static void ti_handle_new_msr(struct ti_port *tport, __u8 msr)
 	/* handle CTS flow control */
 	tty = tty_port_tty_get(&tport->tp_port->port);
 	if (tty && C_CRTSCTS(tty)) {
-		if (msr & TI_MSR_CTS) {
-			tty->hw_stopped = 0;
+		if (msr & TI_MSR_CTS)
 			tty_wakeup(tty);
-		} else {
-			tty->hw_stopped = 1;
-		}
 	}
 	tty_kref_put(tty);
 }
-- 
2.1.0

[PATCH 19/26] serial: bfin-uart: Fix auto CTS

[Peter Hurley]

Commit 64851636d568ae9f167cd5d1dcdbfe17e6eef73c,
serial: bfin-uart: Remove ASYNC_CTS_FLOW flag for hardware automatic CTS,
open-codes uart_handle_cts_change() when CONFIG_SERIAL_BFIN_HARD_CTSRTS
to skip start and stop tx.

But the CTS interrupt handler _still_ calls uart_handle_cts_change();
only call uart_handle_cts_change() if !CONFIG_SERIAL_BFIN_HARD_CTSRTS.

cc: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/bfin_uart.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index fc9fbf3..7da9911 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -122,8 +122,9 @@ static irqreturn_t bfin_serial_mctrl_cts_int(int irq, void *dev_id)
 		if (!status)
 			uport->hw_stopped = 1;
 	}
-#endif
+#else
 	uart_handle_cts_change(uport, status & TIOCM_CTS);
+#endif
 
 	return IRQ_HANDLED;
 }
-- 
2.1.0

[PATCH 20/26] serial: core: Use spin_lock_irq() in uart_set_termios()

[Peter Hurley]

uart_set_termios() is called with interrupts enabled; no need to
save and restore the interrupt state when taking the uart port lock.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 95277a2..200fa8c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1244,7 +1244,6 @@ static void uart_set_termios(struct tty_struct *tty,
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *uport = state->uart_port;
-	unsigned long flags;
 	unsigned int cflag = tty->termios.c_cflag;
 	unsigned int iflag_mask = IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK;
 	bool sw_changed = false;
@@ -1298,19 +1297,19 @@ static void uart_set_termios(struct tty_struct *tty,
 
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
-		spin_lock_irqsave(&uport->lock, flags);
+		spin_lock_irq(&uport->lock);
 		uport->hw_stopped = 0;
 		__uart_start(tty);
-		spin_unlock_irqrestore(&uport->lock, flags);
+		spin_unlock_irq(&uport->lock);
 	}
 	/* Handle turning on CRTSCTS */
 	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
-		spin_lock_irqsave(&uport->lock, flags);
+		spin_lock_irq(&uport->lock);
 		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
 			uport->hw_stopped = 1;
 			uport->ops->stop_tx(uport);
 		}
-		spin_unlock_irqrestore(&uport->lock, flags);
+		spin_unlock_irq(&uport->lock);
 	}
 }
 
-- 
2.1.0

[PATCH 21/26] tty: Convert tty_struct bitfield to bools

[Peter Hurley]

The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe
and interrupt-unsafe. For example,

CPU 0                         | CPU 1
                              |
tty->flow_stopped = 1         | tty->hw_stopped = 0

One of these updates will be corrupted, as the bitwise operation
on the bitfield is non-atomic.

Ensure each flag has a separate memory location, so concurrent
updates do not corrupt orthogonal states.

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

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..7cf61cb 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -261,7 +261,10 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
+	bool stopped;
+	bool hw_stopped;
+	bool flow_stopped;
+	bool packet;
 	unsigned char ctrl_status;	/* ctrl_lock */
 	unsigned int receive_room;	/* Bytes free for queue */
 	int flow_change;
-- 
2.1.0

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[One Thousand Gnomes]

On Tue,  2 Sep 2014 17:39:30 -0400
Peter Hurley <peter@hurleysoftware.com> wrote:

> The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe
> and interrupt-unsafe. For example,
> 
> CPU 0                         | CPU 1
>                               |
> tty->flow_stopped = 1         | tty->hw_stopped = 0
> 
> One of these updates will be corrupted, as the bitwise operation
> on the bitfield is non-atomic.
> 
> Ensure each flag has a separate memory location, so concurrent
> updates do not corrupt orthogonal states.

Ouch....

Alas the fix is not sufficient on some platforms. gcc will happily use
32bit operations on those fields if it thinks its a performance win.

It needs to use set_bit and friends.

x86 is generally ok, but ia64 gcc will do things like load all four
bytes, mask and write the dword back. I believe ARM gcc may also
sometimes generate similar results.


> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  include/linux/tty.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 1c3316a..7cf61cb 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -261,7 +261,10 @@ struct tty_struct {
>  	unsigned long flags;
>  	int count;
>  	struct winsize winsize;		/* winsize_mutex */
> -	unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
> +	bool stopped;
> +	bool hw_stopped;
> +	bool flow_stopped;
> +	bool packet;
>  	unsigned char ctrl_status;	/* ctrl_lock */
>  	unsigned int receive_room;	/* Bytes free for queue */
>  	int flow_change;

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[Peter Hurley]

On 09/03/2014 06:58 AM, One Thousand Gnomes wrote:
> On Tue,  2 Sep 2014 17:39:30 -0400
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
>> The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe
>> and interrupt-unsafe. For example,
>>
>> CPU 0                         | CPU 1
>>                               |
>> tty->flow_stopped = 1         | tty->hw_stopped = 0
>>
>> One of these updates will be corrupted, as the bitwise operation
>> on the bitfield is non-atomic.
>>
>> Ensure each flag has a separate memory location, so concurrent
>> updates do not corrupt orthogonal states.
> 
> Ouch....
> 
> Alas the fix is not sufficient on some platforms. gcc will happily use
> 32bit operations on those fields if it thinks its a performance win.
> 
> It needs to use set_bit and friends.
> 
> x86 is generally ok, but ia64 gcc will do things like load all four
> bytes, mask and write the dword back. I believe ARM gcc may also
> sometimes generate similar results.

Ahh. Thanks for the insight, Alan.

But set_bit() et. al. will generate an incredible amount of churn;
what if I split the fields up to prevent false-sharing?

--- >% ---

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..36d3cf7 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -252,6 +252,11 @@ struct tty_struct {
 	struct rw_semaphore termios_rwsem;
 	struct mutex winsize_mutex;
 	spinlock_t ctrl_lock;
+	unsigned char ctrl_status;	/* ctrl_lock fields */
+	bool packet;
 	spinlock_t flow_lock;
+	unsigned char stopped:1,	/* flow_lock fields */
+		      flow_stopped:1;
 	/* Termios values are protected by the termios rwsem */
 	struct ktermios termios, termios_locked;
 	struct termiox *termiox;	/* May be NULL for unsupported */
@@ -261,8 +266,7 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
-	unsigned char ctrl_status;	/* ctrl_lock */
+	bool hw_stopped;
 	unsigned int receive_room;	/* Bytes free for queue */
 	int flow_change;


>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  include/linux/tty.h | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 1c3316a..7cf61cb 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
>> @@ -261,7 +261,10 @@ struct tty_struct {
>>  	unsigned long flags;
>>  	int count;
>>  	struct winsize winsize;		/* winsize_mutex */
>> -	unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
>> +	bool stopped;
>> +	bool hw_stopped;
>> +	bool flow_stopped;
>> +	bool packet;
>>  	unsigned char ctrl_status;	/* ctrl_lock */
>>  	unsigned int receive_room;	/* Bytes free for queue */
>>  	int flow_change;

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[One Thousand Gnomes]

> Ahh. Thanks for the insight, Alan.
> 
> But set_bit() et. al. will generate an incredible amount of churn;
> what if I split the fields up to prevent false-sharing?

Do you feel lucky ;-)

I'd rather set_bit and friends were used. They exist largely for this
kind of reason and they also have atomic test/set methods which may in
the longer term be very useful.

Yes it is churn can't argue with that.

Alan

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[Peter Hurley]

On 09/03/2014 08:19 AM, One Thousand Gnomes wrote:
>> Ahh. Thanks for the insight, Alan.
>>
>> But set_bit() et. al. will generate an incredible amount of churn;
>> what if I split the fields up to prevent false-sharing?
> 
> Do you feel lucky ;-)

Hahaha  :)

> I'd rather set_bit and friends were used. They exist largely for this
> kind of reason and they also have atomic test/set methods which may in
> the longer term be very useful.
> 
> Yes it is churn can't argue with that.

Yuck. There should be a better way. IXANY mode is suddenly going to
have a ton of unnecessary bus locks on x86.

Note the ctrl_status field is a byte as well, which can't be RMW'ed by
the bit-locked primitives, and definitely should not be aggregated with
any adjacent field.

Regards,
Peter Hurley

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[Greg Kroah-Hartman]

On Wed, Sep 03, 2014 at 11:10:45AM -0400, Peter Hurley wrote:
> On 09/03/2014 08:19 AM, One Thousand Gnomes wrote:
> >> Ahh. Thanks for the insight, Alan.
> >>
> >> But set_bit() et. al. will generate an incredible amount of churn;
> >> what if I split the fields up to prevent false-sharing?
> > 
> > Do you feel lucky ;-)
> 
> Hahaha  :)
> 
> > I'd rather set_bit and friends were used. They exist largely for this
> > kind of reason and they also have atomic test/set methods which may in
> > the longer term be very useful.
> > 
> > Yes it is churn can't argue with that.
> 
> Yuck. There should be a better way. IXANY mode is suddenly going to
> have a ton of unnecessary bus locks on x86.

True, but at least it will be correct, which I'm guessing today it isn't
:(

> Note the ctrl_status field is a byte as well, which can't be RMW'ed by
> the bit-locked primitives, and definitely should not be aggregated with
> any adjacent field.

Never trust what an ia64 compiler can, and will, do...

greg k-h

Re: [PATCH 21/26] tty: Convert tty_struct bitfield to bools

[Peter Hurley]

On 09/03/2014 01:56 PM, Greg Kroah-Hartman wrote:
> On Wed, Sep 03, 2014 at 11:10:45AM -0400, Peter Hurley wrote:
>> On 09/03/2014 08:19 AM, One Thousand Gnomes wrote:
>>>> Ahh. Thanks for the insight, Alan.
>>>>
>>>> But set_bit() et. al. will generate an incredible amount of churn;
>>>> what if I split the fields up to prevent false-sharing?
>>>
>>> Do you feel lucky ;-)
>>
>> Hahaha  :)
>>
>>> I'd rather set_bit and friends were used. They exist largely for this
>>> kind of reason and they also have atomic test/set methods which may in
>>> the longer term be very useful.
>>>
>>> Yes it is churn can't argue with that.
>>
>> Yuck. There should be a better way. IXANY mode is suddenly going to
>> have a ton of unnecessary bus locks on x86.
> 
> True, but at least it will be correct, which I'm guessing today it isn't
> :(
> 
>> Note the ctrl_status field is a byte as well, which can't be RMW'ed by
>> the bit-locked primitives, and definitely should not be aggregated with
>> any adjacent field.
> 
> Never trust what an ia64 compiler can, and will, do...

I could not get the ia64 compiler to merge adjacent reads or writes on
bools or chars, at all.

However, Alan's point applies anyway --
because the Alpha is not byte-addressable.

This means that smaller-than-int storage is _always_ read-modify-write.

So my plan is now to take this patch out of this series and submit
a new series on top of this one that will:
1) merge the ctrl_status field and packet field into an int size, which
   will guarantee atomicity to those fields on all arches when accessed
   holding the ->ctrl_lock.
2) make tty->hw_stopped an int. This way legacy drivers -- synclink,
   cyclades, nozomi, isdn4linux, et. al. -- will not need changes.
   UART drivers should use uport->hw_stopped (added in this series).
   New, non-UART drivers should use their own hw_stopped mechanism.
3) keep tty->stopped and tty->flow_stopped as bitfield members of the
   same bitfield, but size that bitfield to unsigned long [note 1 below].
   This should guarantee atomicity on all arches when accessed while
   holding the ->flow_lock, without requiring further code changes and
   without impacting other arches.

Regards,
Peter Hurley

[Note 1] gcc pre-4.7.2 corrupts data adjacent to a smaller-than-8-byte
bitfield on sparc64, ia64, ppc64 and alpha.

[PATCH 22/26] tty: Serialize tty flow control changes with flow_lock

[Peter Hurley]

Without serialization, the flow control state can become inverted
wrt. the actual hardware state. For example,

CPU 0                          | CPU 1
stop_tty()                     |
  lock ctrl_lock               |
  tty->stopped = 1             |
  unlock ctrl_lock             |
                               | start_tty()
                               |   lock ctrl_lock
                               |   tty->stopped = 0
                               |   unlock ctrl_lock
                               |   driver->start()
  driver->stop()               |

In this case, the flow control state now indicates the tty has
been started, but the actual hardware state has actually been stopped.

Introduce tty->flow_lock spinlock to serialize tty flow control changes.
Split out unlocked __start_tty()/__stop_tty() flavors for use by
ioctl(TCXONC) in follow-on patch.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c       | 39 ++++++++++++++++++++++++++++-----------
 include/linux/tty.h        |  5 ++++-
 include/linux/tty_driver.h |  4 ++++
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 714320b..b898e29 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -933,18 +933,18 @@ void no_tty(void)
  *	but not always.
  *
  *	Locking:
- *		Uses the tty control lock internally
+ *		ctrl_lock
+ *		flow_lock
  */
 
-void stop_tty(struct tty_struct *tty)
+void __stop_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->stopped) {
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (tty->stopped)
 		return;
-	}
 	tty->stopped = 1;
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link && tty->link->packet) {
 		tty->ctrl_status &= ~TIOCPKT_START;
 		tty->ctrl_status |= TIOCPKT_STOP;
@@ -955,6 +955,14 @@ void stop_tty(struct tty_struct *tty)
 		(tty->ops->stop)(tty);
 }
 
+void stop_tty(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->flow_lock, flags);
+	__stop_tty(tty);
+	spin_unlock_irqrestore(&tty->flow_lock, flags);
+}
 EXPORT_SYMBOL(stop_tty);
 
 /**
@@ -968,17 +976,17 @@ EXPORT_SYMBOL(stop_tty);
  *
  *	Locking:
  *		ctrl_lock
+ *		flow_lock
  */
 
-void start_tty(struct tty_struct *tty)
+void __start_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (!tty->stopped || tty->flow_stopped) {
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (!tty->stopped || tty->flow_stopped)
 		return;
-	}
 	tty->stopped = 0;
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link && tty->link->packet) {
 		tty->ctrl_status &= ~TIOCPKT_STOP;
 		tty->ctrl_status |= TIOCPKT_START;
@@ -991,6 +999,14 @@ void start_tty(struct tty_struct *tty)
 	tty_wakeup(tty);
 }
 
+void start_tty(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->flow_lock, flags);
+	__start_tty(tty);
+	spin_unlock_irqrestore(&tty->flow_lock, flags);
+}
 EXPORT_SYMBOL(start_tty);
 
 /* We limit tty time update visibility to every 8 seconds or so. */
@@ -3031,6 +3047,7 @@ void initialize_tty_struct(struct tty_struct *tty,
 	INIT_WORK(&tty->hangup_work, do_tty_hangup);
 	mutex_init(&tty->atomic_write_lock);
 	spin_lock_init(&tty->ctrl_lock);
+	spin_lock_init(&tty->flow_lock);
 	INIT_LIST_HEAD(&tty->tty_files);
 	INIT_WORK(&tty->SAK_work, do_SAK_work);
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 7cf61cb..19ce455 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -252,6 +252,7 @@ struct tty_struct {
 	struct rw_semaphore termios_rwsem;
 	struct mutex winsize_mutex;
 	spinlock_t ctrl_lock;
+	spinlock_t flow_lock;
 	/* Termios values are protected by the termios rwsem */
 	struct ktermios termios, termios_locked;
 	struct termiox *termiox;	/* May be NULL for unsupported */
@@ -261,7 +262,7 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	bool stopped;
+	bool stopped;			/* flow_lock */
 	bool hw_stopped;
 	bool flow_stopped;
 	bool packet;
@@ -400,7 +401,9 @@ extern int tty_paranoia_check(struct tty_struct *tty, struct inode *inode,
 extern char *tty_name(struct tty_struct *tty, char *buf);
 extern void tty_wait_until_sent(struct tty_struct *tty, long timeout);
 extern int tty_check_change(struct tty_struct *tty);
+extern void __stop_tty(struct tty_struct *tty);
 extern void stop_tty(struct tty_struct *tty);
+extern void __start_tty(struct tty_struct *tty);
 extern void start_tty(struct tty_struct *tty);
 extern int tty_register_driver(struct tty_driver *driver);
 extern int tty_unregister_driver(struct tty_driver *driver);
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index e48c608..92e337c 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -152,6 +152,8 @@
  * 	This routine notifies the tty driver that it should stop
  * 	outputting characters to the tty device.  
  *
+ *	Called with ->flow_lock held. Serialized with start() method.
+ *
  *	Optional:
  *
  *	Note: Call stop_tty not this method.
@@ -161,6 +163,8 @@
  * 	This routine notifies the tty driver that it resume sending
  *	characters to the tty device.
  *
+ *	Called with ->flow_lock held. Serialized with stop() method.
+ *
  *	Optional:
  *
  *	Note: Call start_tty not this method.
-- 
2.1.0

[PATCH 23/26] tty: Move packet mode flow control notifications to pty driver

[Peter Hurley]

When a master pty is set to packet mode, flow control changes to
the slave pty cause notifications to the master pty via reads and
polls. However, these tests are occurring for all ttys, not
just ptys.

Implement flow control packet mode notifications in the pty driver.
Only the slave side implements the flow control handlers since
packet mode is asymmetric; the master pty receives notifications
for slave-side changes, but not vice versa.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/tty/tty_io.c | 31 ++++---------------------------
 2 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 25c9bc7..c344de0 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -24,6 +24,7 @@
 #include <linux/devpts_fs.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
+#include <linux/poll.h>
 
 
 #ifdef CONFIG_UNIX98_PTYS
@@ -313,6 +314,42 @@ done:
 }
 
 /**
+ *	pty_start - start() handler
+ *	pty_stop  - stop() handler
+ *	@tty: tty being flow-controlled
+ *
+ *	Propagates the TIOCPKT status to the master pty.
+ *
+ *	NB: only the master pty can be in packet mode so only the slave
+ *	    needs start()/stop() handlers
+ */
+static void pty_start(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	if (tty->link && tty->link->packet) {
+		tty->ctrl_status &= ~TIOCPKT_STOP;
+		tty->ctrl_status |= TIOCPKT_START;
+		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
+	}
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+}
+
+static void pty_stop(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	if (tty->link && tty->link->packet) {
+		tty->ctrl_status &= ~TIOCPKT_START;
+		tty->ctrl_status |= TIOCPKT_STOP;
+		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
+	}
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+}
+
+/**
  *	pty_common_install		-	set up the pty pair
  *	@driver: the pty driver
  *	@tty: the tty being instantiated
@@ -472,6 +509,8 @@ static const struct tty_operations slave_pty_ops_bsd = {
 	.set_termios = pty_set_termios,
 	.cleanup = pty_cleanup,
 	.resize = pty_resize,
+	.start = pty_start,
+	.stop = pty_stop,
 	.remove = pty_remove
 };
 
@@ -647,6 +686,8 @@ static const struct tty_operations pty_unix98_ops = {
 	.chars_in_buffer = pty_chars_in_buffer,
 	.unthrottle = pty_unthrottle,
 	.set_termios = pty_set_termios,
+	.start = pty_start,
+	.stop = pty_stop,
 	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup,
 };
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b898e29..55b9103 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -922,8 +922,7 @@ void no_tty(void)
  *	stop_tty	-	propagate flow control
  *	@tty: tty to stop
  *
- *	Perform flow control to the driver. For PTY/TTY pairs we
- *	must also propagate the TIOCKPKT status. May be called
+ *	Perform flow control to the driver. May be called
  *	on an already stopped device and will not re-call the driver
  *	method.
  *
@@ -933,24 +932,14 @@ void no_tty(void)
  *	but not always.
  *
  *	Locking:
- *		ctrl_lock
  *		flow_lock
  */
 
 void __stop_tty(struct tty_struct *tty)
 {
-	unsigned long flags;
-
 	if (tty->stopped)
 		return;
 	tty->stopped = 1;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->link && tty->link->packet) {
-		tty->ctrl_status &= ~TIOCPKT_START;
-		tty->ctrl_status |= TIOCPKT_STOP;
-		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
-	}
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 	if (tty->ops->stop)
 		(tty->ops->stop)(tty);
 }
@@ -969,33 +958,21 @@ EXPORT_SYMBOL(stop_tty);
  *	start_tty	-	propagate flow control
  *	@tty: tty to start
  *
- *	Start a tty that has been stopped if at all possible. Perform
- *	any necessary wakeups and propagate the TIOCPKT status. If this
- *	is the tty was previous stopped and is being started then the
- *	driver start method is invoked and the line discipline woken.
+ *	Start a tty that has been stopped if at all possible. If this
+ *	tty was previous stopped and is now being started, the driver
+ *	start method is invoked and the line discipline woken.
  *
  *	Locking:
- *		ctrl_lock
  *		flow_lock
  */
 
 void __start_tty(struct tty_struct *tty)
 {
-	unsigned long flags;
-
 	if (!tty->stopped || tty->flow_stopped)
 		return;
 	tty->stopped = 0;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->link && tty->link->packet) {
-		tty->ctrl_status &= ~TIOCPKT_STOP;
-		tty->ctrl_status |= TIOCPKT_START;
-		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
-	}
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 	if (tty->ops->start)
 		(tty->ops->start)(tty);
-	/* If we have a running line discipline it may need kicking */
 	tty_wakeup(tty);
 }
 
-- 
2.1.0

[PATCH 24/26] tty: Serialize tcflow() with other tty flow control changes

[Peter Hurley]

Use newly-introduced tty->flow_lock to serialize updates to
tty->flow_stopped (via tcflow()) and with concurrent tty flow
control changes from other sources.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ioctl.c | 8 ++++++--
 include/linux/tty.h     | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 6fd60fe..ffed466 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1177,16 +1177,20 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			return retval;
 		switch (arg) {
 		case TCOOFF:
+			spin_lock_irq(&tty->flow_lock);
 			if (!tty->flow_stopped) {
 				tty->flow_stopped = 1;
-				stop_tty(tty);
+				__stop_tty(tty);
 			}
+			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCOON:
+			spin_lock_irq(&tty->flow_lock);
 			if (tty->flow_stopped) {
 				tty->flow_stopped = 0;
-				start_tty(tty);
+				__start_tty(tty);
 			}
+			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCIOFF:
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 19ce455..0a457c9 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -264,7 +264,7 @@ struct tty_struct {
 	struct winsize winsize;		/* winsize_mutex */
 	bool stopped;			/* flow_lock */
 	bool hw_stopped;
-	bool flow_stopped;
+	bool flow_stopped;		/* flow_lock */
 	bool packet;
 	unsigned char ctrl_status;	/* ctrl_lock */
 	unsigned int receive_room;	/* Bytes free for queue */
-- 
2.1.0

[PATCH 25/26] tty: Move and rename send_prio_char() as tty_send_xchar()

[Peter Hurley]

Relocate the file-scope function, send_prio_char(), as a global
helper tty_send_xchar(). Remove the global declarations for
tty_write_lock()/tty_write_unlock(), as these are file-scope only now.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 33 +++++++++++++++++++++++++++++++--
 drivers/tty/tty_ioctl.c | 33 ++-------------------------------
 include/linux/tty.h     |  3 +--
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 55b9103..54e359b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1037,14 +1037,14 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	return i;
 }
 
-void tty_write_unlock(struct tty_struct *tty)
+static void tty_write_unlock(struct tty_struct *tty)
 	__releases(&tty->atomic_write_lock)
 {
 	mutex_unlock(&tty->atomic_write_lock);
 	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
 }
 
-int tty_write_lock(struct tty_struct *tty, int ndelay)
+static int tty_write_lock(struct tty_struct *tty, int ndelay)
 	__acquires(&tty->atomic_write_lock)
 {
 	if (!mutex_trylock(&tty->atomic_write_lock)) {
@@ -1231,6 +1231,35 @@ ssize_t redirected_tty_write(struct file *file, const char __user *buf,
 	return tty_write(file, buf, count, ppos);
 }
 
+/**
+ *	tty_send_xchar	-	send priority character
+ *
+ *	Send a high priority character to the tty even if stopped
+ *
+ *	Locking: none for xchar method, write ordering for write method.
+ */
+
+int tty_send_xchar(struct tty_struct *tty, char ch)
+{
+	int	was_stopped = tty->stopped;
+
+	if (tty->ops->send_xchar) {
+		tty->ops->send_xchar(tty, ch);
+		return 0;
+	}
+
+	if (tty_write_lock(tty, 0) < 0)
+		return -ERESTARTSYS;
+
+	if (was_stopped)
+		start_tty(tty);
+	tty->ops->write(tty, &ch, 1);
+	if (was_stopped)
+		stop_tty(tty);
+	tty_write_unlock(tty);
+	return 0;
+}
+
 static char ptychar[] = "pqrstuvwxyzabcde";
 
 /**
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index ffed466..14d9002 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -912,35 +912,6 @@ static int set_ltchars(struct tty_struct *tty, struct ltchars __user *ltchars)
 #endif
 
 /**
- *	send_prio_char		-	send priority character
- *
- *	Send a high priority character to the tty even if stopped
- *
- *	Locking: none for xchar method, write ordering for write method.
- */
-
-static int send_prio_char(struct tty_struct *tty, char ch)
-{
-	int	was_stopped = tty->stopped;
-
-	if (tty->ops->send_xchar) {
-		tty->ops->send_xchar(tty, ch);
-		return 0;
-	}
-
-	if (tty_write_lock(tty, 0) < 0)
-		return -ERESTARTSYS;
-
-	if (was_stopped)
-		start_tty(tty);
-	tty->ops->write(tty, &ch, 1);
-	if (was_stopped)
-		stop_tty(tty);
-	tty_write_unlock(tty);
-	return 0;
-}
-
-/**
  *	tty_change_softcar	-	carrier change ioctl helper
  *	@tty: tty to update
  *	@arg: enable/disable CLOCAL
@@ -1194,11 +1165,11 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			break;
 		case TCIOFF:
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
-				return send_prio_char(tty, STOP_CHAR(tty));
+				return tty_send_xchar(tty, STOP_CHAR(tty));
 			break;
 		case TCION:
 			if (START_CHAR(tty) != __DISABLED_CHAR)
-				return send_prio_char(tty, START_CHAR(tty));
+				return tty_send_xchar(tty, START_CHAR(tty));
 			break;
 		default:
 			return -EINVAL;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 0a457c9..dea0f36 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -417,6 +417,7 @@ extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
 extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
 			     int buflen);
 extern void tty_write_message(struct tty_struct *tty, char *msg);
+extern int tty_send_xchar(struct tty_struct *tty, char ch);
 extern int tty_put_char(struct tty_struct *tty, unsigned char c);
 extern int tty_chars_in_buffer(struct tty_struct *tty);
 extern int tty_write_room(struct tty_struct *tty);
@@ -503,8 +504,6 @@ extern struct tty_struct *tty_pair_get_pty(struct tty_struct *tty);
 extern struct mutex tty_mutex;
 extern spinlock_t tty_files_lock;
 
-extern void tty_write_unlock(struct tty_struct *tty);
-extern int tty_write_lock(struct tty_struct *tty, int ndelay);
 #define tty_is_writelocked(tty)  (mutex_is_locked(&tty->atomic_write_lock))
 
 extern void tty_port_init(struct tty_port *port);
-- 
2.1.0

[PATCH 26/26] tty: Hold termios_rwsem for tcflow(TCIxxx)

[Peter Hurley]

While transmitting a START/STOP char for tcflow(TCION/TCIOFF), prevent
a termios change. Otherwise, a garbage in-band flow control char
may be sent, if the termios change overlaps the transmission setup.

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

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 14d9002..32cee97 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1164,17 +1164,21 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCIOFF:
+			down_read(&tty->termios_rwsem);
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
-				return tty_send_xchar(tty, STOP_CHAR(tty));
+				retval = tty_send_xchar(tty, STOP_CHAR(tty));
+			up_read(&tty->termios_rwsem);
 			break;
 		case TCION:
+			down_read(&tty->termios_rwsem);
 			if (START_CHAR(tty) != __DISABLED_CHAR)
-				return tty_send_xchar(tty, START_CHAR(tty));
+				retval = tty_send_xchar(tty, START_CHAR(tty));
+			up_read(&tty->termios_rwsem);
 			break;
 		default:
 			return -EINVAL;
 		}
-		return 0;
+		return retval;
 	case TCFLSH:
 		retval = tty_check_change(tty);
 		if (retval)
-- 
2.1.0

Re: [PATCH 00/26] tty/serial flow control fixes

[Greg Kroah-Hartman]

On Tue, Sep 02, 2014 at 05:39:09PM -0400, Peter Hurley wrote:
> Hi Greg,
> 
> This patch series reworks the internals of tty and serial flow control to
> fix multiple races in both START/STOP flow control and RTS/CTS flow control.
> 
> The main changes in this series are:
> Patch 1
>    Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
>    for mainline and -next
> 
> Patches 3-7
>    Fixes to x_char handling (ie., sending START/STOP) both in the serial
>    core and to several UART drivers
> 
> Patches 10-15
>    Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
>    a private field for the serial core, and add helper functions to test
>    those statuses for UART drivers.
> 
>    This is a necessary step toward making tty port->flags SMP-safe, without
>    introducing a new lock.

I stopped applying at patch 13, due to the objections to that patch.
Please redo the rest of the series and resend.

thanks,

greg k-h

Re: [PATCH 00/26] tty/serial flow control fixes

[Peter Hurley]

On 09/08/2014 07:24 PM, Greg Kroah-Hartman wrote:
> On Tue, Sep 02, 2014 at 05:39:09PM -0400, Peter Hurley wrote:
>> Hi Greg,
>>
>> This patch series reworks the internals of tty and serial flow control to
>> fix multiple races in both START/STOP flow control and RTS/CTS flow control.
>>
>> The main changes in this series are:
>> Patch 1
>>    Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
>>    for mainline and -next
>>
>> Patches 3-7
>>    Fixes to x_char handling (ie., sending START/STOP) both in the serial
>>    core and to several UART drivers
>>
>> Patches 10-15
>>    Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
>>    a private field for the serial core, and add helper functions to test
>>    those statuses for UART drivers.
>>
>>    This is a necessary step toward making tty port->flags SMP-safe, without
>>    introducing a new lock.
> 
> I stopped applying at patch 13, due to the objections to that patch.
> Please redo the rest of the series and resend.

I'm going to wait for the fallout from patch 21 to shake out first.

Regards,
Peter Hurley

Re: [PATCH 00/26] tty/serial flow control fixes

[Greg Kroah-Hartman]

On Mon, Sep 08, 2014 at 07:33:51PM -0400, Peter Hurley wrote:
> On 09/08/2014 07:24 PM, Greg Kroah-Hartman wrote:
> > On Tue, Sep 02, 2014 at 05:39:09PM -0400, Peter Hurley wrote:
> >> Hi Greg,
> >>
> >> This patch series reworks the internals of tty and serial flow control to
> >> fix multiple races in both START/STOP flow control and RTS/CTS flow control.
> >>
> >> The main changes in this series are:
> >> Patch 1
> >>    Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
> >>    for mainline and -next
> >>
> >> Patches 3-7
> >>    Fixes to x_char handling (ie., sending START/STOP) both in the serial
> >>    core and to several UART drivers
> >>
> >> Patches 10-15
> >>    Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
> >>    a private field for the serial core, and add helper functions to test
> >>    those statuses for UART drivers.
> >>
> >>    This is a necessary step toward making tty port->flags SMP-safe, without
> >>    introducing a new lock.
> > 
> > I stopped applying at patch 13, due to the objections to that patch.
> > Please redo the rest of the series and resend.
> 
> I'm going to wait for the fallout from patch 21 to shake out first.

Why, the earlier patches in this series should be ok to go, right?  They
don't depend on 21 to be "correct".

thanks,

greg k-h

Re: [PATCH 00/26] tty/serial flow control fixes

[Peter Hurley]

On 09/08/2014 07:43 PM, Greg Kroah-Hartman wrote:
> On Mon, Sep 08, 2014 at 07:33:51PM -0400, Peter Hurley wrote:
>> On 09/08/2014 07:24 PM, Greg Kroah-Hartman wrote:
>>> On Tue, Sep 02, 2014 at 05:39:09PM -0400, Peter Hurley wrote:
>>>> Hi Greg,
>>>>
>>>> This patch series reworks the internals of tty and serial flow control to
>>>> fix multiple races in both START/STOP flow control and RTS/CTS flow control.
>>>>
>>>> The main changes in this series are:
>>>> Patch 1
>>>>    Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
>>>>    for mainline and -next
>>>>
>>>> Patches 3-7
>>>>    Fixes to x_char handling (ie., sending START/STOP) both in the serial
>>>>    core and to several UART drivers
>>>>
>>>> Patches 10-15
>>>>    Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
>>>>    a private field for the serial core, and add helper functions to test
>>>>    those statuses for UART drivers.
>>>>
>>>>    This is a necessary step toward making tty port->flags SMP-safe, without
>>>>    introducing a new lock.
>>>
>>> I stopped applying at patch 13, due to the objections to that patch.
>>> Please redo the rest of the series and resend.
>>
>> I'm going to wait for the fallout from patch 21 to shake out first.
> 
> Why, the earlier patches in this series should be ok to go, right?  They
> don't depend on 21 to be "correct".

Oh, you want me to redo just 14 through 20, and resend that?

Regards,
Peter Hurley

Re: [PATCH 00/26] tty/serial flow control fixes

[Greg Kroah-Hartman]

On Mon, Sep 08, 2014 at 07:46:14PM -0400, Peter Hurley wrote:
> On 09/08/2014 07:43 PM, Greg Kroah-Hartman wrote:
> > On Mon, Sep 08, 2014 at 07:33:51PM -0400, Peter Hurley wrote:
> >> On 09/08/2014 07:24 PM, Greg Kroah-Hartman wrote:
> >>> On Tue, Sep 02, 2014 at 05:39:09PM -0400, Peter Hurley wrote:
> >>>> Hi Greg,
> >>>>
> >>>> This patch series reworks the internals of tty and serial flow control to
> >>>> fix multiple races in both START/STOP flow control and RTS/CTS flow control.
> >>>>
> >>>> The main changes in this series are:
> >>>> Patch 1
> >>>>    Backs out the UPF_HARD_FLOW kludge for 8250. This revert should be
> >>>>    for mainline and -next
> >>>>
> >>>> Patches 3-7
> >>>>    Fixes to x_char handling (ie., sending START/STOP) both in the serial
> >>>>    core and to several UART drivers
> >>>>
> >>>> Patches 10-15
> >>>>    Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
> >>>>    a private field for the serial core, and add helper functions to test
> >>>>    those statuses for UART drivers.
> >>>>
> >>>>    This is a necessary step toward making tty port->flags SMP-safe, without
> >>>>    introducing a new lock.
> >>>
> >>> I stopped applying at patch 13, due to the objections to that patch.
> >>> Please redo the rest of the series and resend.
> >>
> >> I'm going to wait for the fallout from patch 21 to shake out first.
> > 
> > Why, the earlier patches in this series should be ok to go, right?  They
> > don't depend on 21 to be "correct".
> 
> Oh, you want me to redo just 14 through 20, and resend that?

Sure, might as well get those out of the way :)

[PATCH v2 00/14] tty/serial flow control fixes

[Peter Hurley]

Hi Greg,

Changes since v1:
1. Omits v1 patches 1-12 which you already applied
2. Depends on 'locking: Add WARN_ON_ONCE lock assertion',
   which replaces v1 patch 13
3. Works around Alpha EV4/5 non-atomic byte/short storage
4. Works around gcc bug in versions < 4.7.2, where bitfield stores can
   corrupt adjacent storage on ia64, ppc64, sparc64 and 64-bit parisc arches
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52080

v1:

This patch series reworks the internals of tty and serial flow control to
fix multiple races in both START/STOP flow control and RTS/CTS flow control.

The main changes in this series are:
Patch 2 (v1 patch 15)
   Rename and move ASYNC_CTS_FLOW and ASYNC_CHECK_CD statuses into
   a private field for the serial core, and add helper functions to test
   those statuses for UART drivers.

   This is a necessary step toward making tty port->flags SMP-safe, without
   introducing a new lock.

Patches 1, 4 and 8 (v1 patches 14, 17 and 21)
   Make CTS flow control state SMP-safe, by moving hw_stopped into a private
   field of the serial core.

Patches 9-11 (v1 patches 22-24)
   Make tty flow control changes mutually exclusive.

   Protecting tty->stopped and tty->flow_stopped with a new spin lock (instead
   of the ->ctrl_lock) is required to fix multiple races with controlling
   terminal changes, coming in a follow-on patch series.

Patches 12 & 13 (v1 patches 25 & 26)
   Cleanup tcflow(TCIxxx)

Regards,

Peter Hurley (14):
  serial: core: Document and assert lock requirements for irq helpers
  serial: core: Privatize modem status enable flags
  isdn: i4l: Remove ASYNC_CTS_FLOW
  serial: core: Privatize tty->hw_stopped
  usb: serial: Remove unused tty->hw_stopped
  serial: bfin-uart: Fix auto CTS
  serial: core: Use spin_lock_irq() in uart_set_termios()
  tty: Convert tty_struct bitfield to ints
  tty: Serialize tty flow control changes with flow_lock
  tty: Move packet mode flow control notifications to pty driver
  tty: Serialize tcflow() with other tty flow control changes
  tty: Move and rename send_prio_char() as tty_send_xchar()
  tty: Hold termios_rwsem for tcflow(TCIxxx)
  tty: Workaround Alpha non-atomic byte storage in tty_struct

 drivers/isdn/i4l/isdn_tty.c           |  5 --
 drivers/tty/pty.c                     | 41 +++++++++++++++
 drivers/tty/serial/bfin_uart.c        | 15 +++---
 drivers/tty/serial/mxs-auart.c        |  2 +-
 drivers/tty/serial/serial_core.c      | 70 +++++++++++++++-----------
 drivers/tty/tty_io.c                  | 93 ++++++++++++++++++++++-------------
 drivers/tty/tty_ioctl.c               | 47 +++++-------------
 drivers/usb/serial/digi_acceleport.c  |  7 +--
 drivers/usb/serial/io_ti.c            |  7 +--
 drivers/usb/serial/ti_usb_3410_5052.c |  7 +--
 include/linux/serial_core.h           | 15 +++++-
 include/linux/tty.h                   | 15 ++++--
 include/linux/tty_driver.h            |  4 ++
 13 files changed, 193 insertions(+), 135 deletions(-)

-- 
2.1.0

[PATCH v2 01/14] serial: core: Document and assert lock requirements for irq helpers

[Peter Hurley]

The serial core provides two helper functions, uart_handle_dcd_change()
and uart_handle_cts_change(), for UART drivers to use at interrupt
time. The serial core expects the UART driver to hold the uart port lock
when calling these helpers to prevent state corruption.

If lockdep enabled, trigger a warning if the uart port lock is not held
when calling these helper functions.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 732d386..0567b65 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2731,6 +2731,8 @@ EXPORT_SYMBOL(uart_match_port);
  *	uart_handle_dcd_change - handle a change of carrier detect state
  *	@uport: uart_port structure for the open port
  *	@status: new carrier detect status, nonzero if active
+ *
+ *	Caller must hold uport->lock
  */
 void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 {
@@ -2738,6 +2740,8 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 	struct tty_struct *tty = port->tty;
 	struct tty_ldisc *ld;
 
+	lockdep_assert_held_once(&uport->lock);
+
 	if (tty) {
 		ld = tty_ldisc_ref(tty);
 		if (ld) {
@@ -2762,12 +2766,16 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
  *	uart_handle_cts_change - handle a change of clear-to-send state
  *	@uport: uart_port structure for the open port
  *	@status: new clear to send status, nonzero if active
+ *
+ *	Caller must hold uport->lock
  */
 void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 {
 	struct tty_port *port = &uport->state->port;
 	struct tty_struct *tty = port->tty;
 
+	lockdep_assert_held_once(&uport->lock);
+
 	uport->icount.cts++;
 
 	if (tty_port_cts_enabled(port)) {
-- 
2.1.0

[PATCH v2 02/14] serial: core: Privatize modem status enable flags

[Peter Hurley]

The serial core uses the tty port flags, ASYNC_CTS_FLOW and
ASYNC_CD_CHECK, to track whether CTS and DCD changes should be
ignored or handled. However, the tty port flags are not safe for
atomic bit operations and no lock provides serialized updates.

Introduce the struct uart_port status field to track CTS and DCD
enable states, and serialize access with uart port lock. Substitute
uart_cts_enabled() helper for tty_port_cts_enabled().

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/mxs-auart.c   |  2 +-
 drivers/tty/serial/serial_core.c | 29 +++++++++++++++++------------
 include/linux/serial_core.h      | 12 ++++++++++++
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index b5c3292..10c2933 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -408,7 +408,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
 	if (mctrl & TIOCM_RTS) {
-		if (tty_port_cts_enabled(&u->state->port))
+		if (uart_cts_enabled(u))
 			ctrl |= AUART_CTRL2_RTSEN;
 		else
 			ctrl |= AUART_CTRL2_RTS;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0567b65..3c62b3f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -58,6 +58,11 @@ static void uart_change_pm(struct uart_state *state,
 
 static void uart_port_shutdown(struct tty_port *port);
 
+static int uart_dcd_enabled(struct uart_port *uport)
+{
+	return uport->status & UPSTAT_DCD_ENABLE;
+}
+
 /*
  * This routine is used by the interrupt handler to schedule processing in
  * the software interrupt portion of the driver.
@@ -129,7 +134,6 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		int init_hw)
 {
 	struct uart_port *uport = state->uart_port;
-	struct tty_port *port = &state->port;
 	unsigned long page;
 	int retval = 0;
 
@@ -175,12 +179,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
 		}
 
-		if (tty_port_cts_enabled(port)) {
-			spin_lock_irq(&uport->lock);
+		spin_lock_irq(&uport->lock);
+		if (uart_cts_enabled(uport)) {
 			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
 				tty->hw_stopped = 1;
-			spin_unlock_irq(&uport->lock);
 		}
+		spin_unlock_irq(&uport->lock);
 	}
 
 	/*
@@ -431,7 +435,6 @@ EXPORT_SYMBOL(uart_get_divisor);
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios)
 {
-	struct tty_port *port = &state->port;
 	struct uart_port *uport = state->uart_port;
 	struct ktermios *termios;
 
@@ -446,17 +449,19 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 	uport->ops->set_termios(uport, termios, old_termios);
 
 	/*
-	 * Set flags based on termios cflag
+	 * Set modem status enables based on termios cflag
 	 */
+	spin_lock_irq(&uport->lock);
 	if (termios->c_cflag & CRTSCTS)
-		set_bit(ASYNCB_CTS_FLOW, &port->flags);
+		uport->status |= UPSTAT_CTS_ENABLE;
 	else
-		clear_bit(ASYNCB_CTS_FLOW, &port->flags);
+		uport->status &= ~UPSTAT_CTS_ENABLE;
 
 	if (termios->c_cflag & CLOCAL)
-		clear_bit(ASYNCB_CHECK_CD, &port->flags);
+		uport->status &= ~UPSTAT_DCD_ENABLE;
 	else
-		set_bit(ASYNCB_CHECK_CD, &port->flags);
+		uport->status |= UPSTAT_DCD_ENABLE;
+	spin_unlock_irq(&uport->lock);
 }
 
 static inline int __uart_put_char(struct uart_port *port,
@@ -2753,7 +2758,7 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.dcd++;
 
-	if (port->flags & ASYNC_CHECK_CD) {
+	if (uart_dcd_enabled(uport)) {
 		if (status)
 			wake_up_interruptible(&port->open_wait);
 		else if (tty)
@@ -2778,7 +2783,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.cts++;
 
-	if (tty_port_cts_enabled(port)) {
+	if (uart_cts_enabled(uport)) {
 		if (tty->hw_stopped) {
 			if (status) {
 				tty->hw_stopped = 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4a43292..bc70048 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -112,6 +112,7 @@ struct uart_icount {
 };
 
 typedef unsigned int __bitwise__ upf_t;
+typedef unsigned int __bitwise__ upstat_t;
 
 struct uart_port {
 	spinlock_t		lock;			/* port lock */
@@ -188,6 +189,12 @@ struct uart_port {
 #define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
 #define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
 
+	/* status must be updated while holding port lock */
+	upstat_t		status;
+
+#define UPSTAT_CTS_ENABLE	((__force upstat_t) (1 << 0))
+#define UPSTAT_DCD_ENABLE	((__force upstat_t) (1 << 1))
+
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */
 	unsigned int		type;			/* port type */
@@ -351,6 +358,11 @@ static inline int uart_tx_stopped(struct uart_port *port)
 	return 0;
 }
 
+static inline bool uart_cts_enabled(struct uart_port *uport)
+{
+	return uport->status & UPSTAT_CTS_ENABLE;
+}
+
 /*
  * The following are helper functions for the low level drivers.
  */
-- 
2.1.0

[PATCH v2 03/14] isdn: i4l: Remove ASYNC_CTS_FLOW

[Peter Hurley]

ISDN4Linux always enables CTS flow control and does not use the
tty_port_cts_enabled() helper function; remove ASYNC_CTS_FLOW
state enable/disable.

cc: Karsten Keil <isdn@linux-pingi.de>
cc: <netdev@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/isdn/i4l/isdn_tty.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 3c5f249..bc91261 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1043,11 +1043,6 @@ isdn_tty_change_speed(modem_info *info)
 	if (!(cflag & PARODD))
 		cval |= UART_LCR_EPAR;
 
-	/* CTS flow control flag and modem status interrupts */
-	if (cflag & CRTSCTS) {
-		port->flags |= ASYNC_CTS_FLOW;
-	} else
-		port->flags &= ~ASYNC_CTS_FLOW;
 	if (cflag & CLOCAL)
 		port->flags &= ~ASYNC_CHECK_CD;
 	else {
-- 
2.1.0

[PATCH v2 04/14] serial: core: Privatize tty->hw_stopped

[Peter Hurley]

tty->hw_stopped is not used by the tty core and is thread-unsafe;
hw_stopped is a member of a bitfield whose fields are updated
non-atomically and no lock is suitable for serializing updates.

Replace serial core usage of tty->hw_stopped with uport->hw_stopped.
Use int storage which works around Alpha EV4/5 non-atomic byte storage,
since uart_port uses different locks to protect certain fields within the
structure.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/bfin_uart.c   | 14 +++++++-------
 drivers/tty/serial/serial_core.c | 28 +++++++++++++---------------
 include/linux/serial_core.h      |  3 ++-
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index dec0fd7..fc9fbf3 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -108,22 +108,22 @@ static void bfin_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 static irqreturn_t bfin_serial_mctrl_cts_int(int irq, void *dev_id)
 {
 	struct bfin_serial_port *uart = dev_id;
-	unsigned int status = bfin_serial_get_mctrl(&uart->port);
+	struct uart_port *uport = &uart->port;
+	unsigned int status = bfin_serial_get_mctrl(uport);
 #ifdef CONFIG_SERIAL_BFIN_HARD_CTSRTS
-	struct tty_struct *tty = uart->port.state->port.tty;
 
 	UART_CLEAR_SCTS(uart);
-	if (tty->hw_stopped) {
+	if (uport->hw_stopped) {
 		if (status) {
-			tty->hw_stopped = 0;
-			uart_write_wakeup(&uart->port);
+			uport->hw_stopped = 0;
+			uart_write_wakeup(uport);
 		}
 	} else {
 		if (!status)
-			tty->hw_stopped = 1;
+			uport->hw_stopped = 1;
 	}
 #endif
-	uart_handle_cts_change(&uart->port, status & TIOCM_CTS);
+	uart_handle_cts_change(uport, status & TIOCM_CTS);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3c62b3f..1f52a7d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -94,7 +94,7 @@ static void __uart_start(struct tty_struct *tty)
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *port = state->uart_port;
 
-	if (!tty->stopped && !tty->hw_stopped)
+	if (!uart_tx_stopped(port))
 		port->ops->start_tx(port);
 }
 
@@ -180,10 +180,11 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 		}
 
 		spin_lock_irq(&uport->lock);
-		if (uart_cts_enabled(uport)) {
-			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
-				tty->hw_stopped = 1;
-		}
+		if (uart_cts_enabled(uport) &&
+		    !(uport->ops->get_mctrl(uport) & TIOCM_CTS))
+			uport->hw_stopped = 1;
+		else
+			uport->hw_stopped = 0;
 		spin_unlock_irq(&uport->lock);
 	}
 
@@ -944,7 +945,7 @@ static int uart_get_lsr_info(struct tty_struct *tty,
 	 */
 	if (uport->x_char ||
 	    ((uart_circ_chars_pending(&state->xmit) > 0) &&
-	     !tty->stopped && !tty->hw_stopped))
+	     !uart_tx_stopped(uport)))
 		result &= ~TIOCSER_TEMT;
 
 	return put_user(result, value);
@@ -1290,7 +1291,7 @@ static void uart_set_termios(struct tty_struct *tty,
 
 	/*
 	 * If the port is doing h/w assisted flow control, do nothing.
-	 * We assume that tty->hw_stopped has never been set.
+	 * We assume that port->hw_stopped has never been set.
 	 */
 	if (uport->flags & UPF_HARD_FLOW)
 		return;
@@ -1298,7 +1299,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
 		spin_lock_irqsave(&uport->lock, flags);
-		tty->hw_stopped = 0;
+		uport->hw_stopped = 0;
 		__uart_start(tty);
 		spin_unlock_irqrestore(&uport->lock, flags);
 	}
@@ -1306,7 +1307,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
 		spin_lock_irqsave(&uport->lock, flags);
 		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
-			tty->hw_stopped = 1;
+			uport->hw_stopped = 1;
 			uport->ops->stop_tx(uport);
 		}
 		spin_unlock_irqrestore(&uport->lock, flags);
@@ -2776,23 +2777,20 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
  */
 void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 {
-	struct tty_port *port = &uport->state->port;
-	struct tty_struct *tty = port->tty;
-
 	lockdep_assert_held_once(&uport->lock);
 
 	uport->icount.cts++;
 
 	if (uart_cts_enabled(uport)) {
-		if (tty->hw_stopped) {
+		if (uport->hw_stopped) {
 			if (status) {
-				tty->hw_stopped = 0;
+				uport->hw_stopped = 0;
 				uport->ops->start_tx(uport);
 				uart_write_wakeup(uport);
 			}
 		} else {
 			if (!status) {
-				tty->hw_stopped = 1;
+				uport->hw_stopped = 1;
 				uport->ops->stop_tx(uport);
 			}
 		}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index bc70048..677a97c 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -195,6 +195,7 @@ struct uart_port {
 #define UPSTAT_CTS_ENABLE	((__force upstat_t) (1 << 0))
 #define UPSTAT_DCD_ENABLE	((__force upstat_t) (1 << 1))
 
+	int			hw_stopped;		/* sw-assisted CTS flow state */
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */
 	unsigned int		type;			/* port type */
@@ -353,7 +354,7 @@ int uart_resume_port(struct uart_driver *reg, struct uart_port *port);
 static inline int uart_tx_stopped(struct uart_port *port)
 {
 	struct tty_struct *tty = port->state->port.tty;
-	if(tty->stopped || tty->hw_stopped)
+	if (tty->stopped || port->hw_stopped)
 		return 1;
 	return 0;
 }
-- 
2.1.0

[PATCH v2 05/14] usb: serial: Remove unused tty->hw_stopped

[Peter Hurley]

The tty core does not test tty->hw_stopped; remove from drivers
which don't test it themselves.

cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/usb/serial/digi_acceleport.c  | 7 +------
 drivers/usb/serial/io_ti.c            | 7 +------
 drivers/usb/serial/ti_usb_3410_5052.c | 7 +------
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 8a23c53..12b0e67 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -834,7 +834,6 @@ static void digi_set_termios(struct tty_struct *tty,
 			arg |= DIGI_OUTPUT_FLOW_CONTROL_CTS;
 		} else {
 			arg &= ~DIGI_OUTPUT_FLOW_CONTROL_CTS;
-			tty->hw_stopped = 0;
 		}
 
 		buf[i++] = DIGI_CMD_SET_OUTPUT_FLOW_CONTROL;
@@ -1500,15 +1499,11 @@ static int digi_read_oob_callback(struct urb *urb)
 			if (val & DIGI_READ_INPUT_SIGNALS_CTS) {
 				priv->dp_modem_signals |= TIOCM_CTS;
 				/* port must be open to use tty struct */
-				if (rts) {
-					tty->hw_stopped = 0;
+				if (rts)
 					tty_port_tty_wakeup(&port->port);
-				}
 			} else {
 				priv->dp_modem_signals &= ~TIOCM_CTS;
 				/* port must be open to use tty struct */
-				if (rts)
-					tty->hw_stopped = 1;
 			}
 			if (val & DIGI_READ_INPUT_SIGNALS_DSR)
 				priv->dp_modem_signals |= TIOCM_DSR;
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index c0a42e9..ddbb8fe 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1456,12 +1456,8 @@ static void handle_new_msr(struct edgeport_port *edge_port, __u8 msr)
 	tty = tty_port_tty_get(&edge_port->port->port);
 	/* handle CTS flow control */
 	if (tty && C_CRTSCTS(tty)) {
-		if (msr & EDGEPORT_MSR_CTS) {
-			tty->hw_stopped = 0;
+		if (msr & EDGEPORT_MSR_CTS)
 			tty_wakeup(tty);
-		} else {
-			tty->hw_stopped = 1;
-		}
 	}
 	tty_kref_put(tty);
 }
@@ -2177,7 +2173,6 @@ static void change_port_settings(struct tty_struct *tty,
 		dev_dbg(dev, "%s - RTS/CTS is enabled\n", __func__);
 	} else {
 		dev_dbg(dev, "%s - RTS/CTS is disabled\n", __func__);
-		tty->hw_stopped = 0;
 		restart_read(edge_port);
 	}
 
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 3dd3ff8..e9da41d 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -773,7 +773,6 @@ static void ti_set_termios(struct tty_struct *tty,
 			config->wFlags |= TI_UART_ENABLE_RTS_IN;
 		config->wFlags |= TI_UART_ENABLE_CTS_OUT;
 	} else {
-		tty->hw_stopped = 0;
 		ti_restart_read(tport, tty);
 	}
 
@@ -1291,12 +1290,8 @@ static void ti_handle_new_msr(struct ti_port *tport, __u8 msr)
 	/* handle CTS flow control */
 	tty = tty_port_tty_get(&tport->tp_port->port);
 	if (tty && C_CRTSCTS(tty)) {
-		if (msr & TI_MSR_CTS) {
-			tty->hw_stopped = 0;
+		if (msr & TI_MSR_CTS)
 			tty_wakeup(tty);
-		} else {
-			tty->hw_stopped = 1;
-		}
 	}
 	tty_kref_put(tty);
 }
-- 
2.1.0

Re: [PATCH v2 05/14] usb: serial: Remove unused tty->hw_stopped

[Johan Hovold]

On Wed, Sep 10, 2014 at 03:06:27PM -0400, Peter Hurley wrote:
> The tty core does not test tty->hw_stopped; remove from drivers
> which don't test it themselves.
> 
> cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

Acked-by: Johan Hovold <johan@kernel.org>

[PATCH v2 06/14] serial: bfin-uart: Fix auto CTS

[Peter Hurley]

Commit 64851636d568ae9f167cd5d1dcdbfe17e6eef73c,
serial: bfin-uart: Remove ASYNC_CTS_FLOW flag for hardware automatic CTS,
open-codes uart_handle_cts_change() when CONFIG_SERIAL_BFIN_HARD_CTSRTS
to skip start and stop tx.

But the CTS interrupt handler _still_ calls uart_handle_cts_change();
only call uart_handle_cts_change() if !CONFIG_SERIAL_BFIN_HARD_CTSRTS.

cc: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/bfin_uart.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index fc9fbf3..7da9911 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -122,8 +122,9 @@ static irqreturn_t bfin_serial_mctrl_cts_int(int irq, void *dev_id)
 		if (!status)
 			uport->hw_stopped = 1;
 	}
-#endif
+#else
 	uart_handle_cts_change(uport, status & TIOCM_CTS);
+#endif
 
 	return IRQ_HANDLED;
 }
-- 
2.1.0

[PATCH v2 07/14] serial: core: Use spin_lock_irq() in uart_set_termios()

[Peter Hurley]

uart_set_termios() is called with interrupts enabled; no need to
save and restore the interrupt state when taking the uart port lock.

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1f52a7d..66e2de6 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1244,7 +1244,6 @@ static void uart_set_termios(struct tty_struct *tty,
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *uport = state->uart_port;
-	unsigned long flags;
 	unsigned int cflag = tty->termios.c_cflag;
 	unsigned int iflag_mask = IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK;
 	bool sw_changed = false;
@@ -1298,19 +1297,19 @@ static void uart_set_termios(struct tty_struct *tty,
 
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
-		spin_lock_irqsave(&uport->lock, flags);
+		spin_lock_irq(&uport->lock);
 		uport->hw_stopped = 0;
 		__uart_start(tty);
-		spin_unlock_irqrestore(&uport->lock, flags);
+		spin_unlock_irq(&uport->lock);
 	}
 	/* Handle turning on CRTSCTS */
 	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
-		spin_lock_irqsave(&uport->lock, flags);
+		spin_lock_irq(&uport->lock);
 		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
 			uport->hw_stopped = 1;
 			uport->ops->stop_tx(uport);
 		}
-		spin_unlock_irqrestore(&uport->lock, flags);
+		spin_unlock_irq(&uport->lock);
 	}
 }
 
-- 
2.1.0

[PATCH v2 08/14] tty: Convert tty_struct bitfield to ints

[Peter Hurley]

The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe
and interrupt-unsafe. For example,

CPU 0                         | CPU 1
                              |
tty->flow_stopped = 1         | tty->hw_stopped = 0

One of these updates will be corrupted, as the bitwise operation
on the bitfield is non-atomic.

Ensure each flag has a separate memory location, so concurrent
updates do not corrupt orthogonal states. Because DEC Alpha EV4 and EV5
cpus (from 1995) perform RMW on smaller-than-machine-word storage,
"separate memory location" must be int instead of byte.

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

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..a707196 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -261,7 +261,10 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
+	int stopped;
+	int flow_stopped;
+	int hw_stopped;
+	int packet;
 	unsigned char ctrl_status;	/* ctrl_lock */
 	unsigned int receive_room;	/* Bytes free for queue */
 	int flow_change;
-- 
2.1.0

[PATCH v2 09/14] tty: Serialize tty flow control changes with flow_lock

[Peter Hurley]

Without serialization, the flow control state can become inverted
wrt. the actual hardware state. For example,

CPU 0                          | CPU 1
stop_tty()                     |
  lock ctrl_lock               |
  tty->stopped = 1             |
  unlock ctrl_lock             |
                               | start_tty()
                               |   lock ctrl_lock
                               |   tty->stopped = 0
                               |   unlock ctrl_lock
                               |   driver->start()
  driver->stop()               |

In this case, the flow control state now indicates the tty has
been started, but the actual hardware state has actually been stopped.

Introduce tty->flow_lock spinlock to serialize tty flow control changes.
Split out unlocked __start_tty()/__stop_tty() flavors for use by
ioctl(TCXONC) in follow-on patch.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c       | 39 ++++++++++++++++++++++++++++-----------
 include/linux/tty.h        |  5 ++++-
 include/linux/tty_driver.h |  4 ++++
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 714320b..b898e29 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -933,18 +933,18 @@ void no_tty(void)
  *	but not always.
  *
  *	Locking:
- *		Uses the tty control lock internally
+ *		ctrl_lock
+ *		flow_lock
  */
 
-void stop_tty(struct tty_struct *tty)
+void __stop_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->stopped) {
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (tty->stopped)
 		return;
-	}
 	tty->stopped = 1;
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link && tty->link->packet) {
 		tty->ctrl_status &= ~TIOCPKT_START;
 		tty->ctrl_status |= TIOCPKT_STOP;
@@ -955,6 +955,14 @@ void stop_tty(struct tty_struct *tty)
 		(tty->ops->stop)(tty);
 }
 
+void stop_tty(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->flow_lock, flags);
+	__stop_tty(tty);
+	spin_unlock_irqrestore(&tty->flow_lock, flags);
+}
 EXPORT_SYMBOL(stop_tty);
 
 /**
@@ -968,17 +976,17 @@ EXPORT_SYMBOL(stop_tty);
  *
  *	Locking:
  *		ctrl_lock
+ *		flow_lock
  */
 
-void start_tty(struct tty_struct *tty)
+void __start_tty(struct tty_struct *tty)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (!tty->stopped || tty->flow_stopped) {
-		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (!tty->stopped || tty->flow_stopped)
 		return;
-	}
 	tty->stopped = 0;
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (tty->link && tty->link->packet) {
 		tty->ctrl_status &= ~TIOCPKT_STOP;
 		tty->ctrl_status |= TIOCPKT_START;
@@ -991,6 +999,14 @@ void start_tty(struct tty_struct *tty)
 	tty_wakeup(tty);
 }
 
+void start_tty(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->flow_lock, flags);
+	__start_tty(tty);
+	spin_unlock_irqrestore(&tty->flow_lock, flags);
+}
 EXPORT_SYMBOL(start_tty);
 
 /* We limit tty time update visibility to every 8 seconds or so. */
@@ -3031,6 +3047,7 @@ void initialize_tty_struct(struct tty_struct *tty,
 	INIT_WORK(&tty->hangup_work, do_tty_hangup);
 	mutex_init(&tty->atomic_write_lock);
 	spin_lock_init(&tty->ctrl_lock);
+	spin_lock_init(&tty->flow_lock);
 	INIT_LIST_HEAD(&tty->tty_files);
 	INIT_WORK(&tty->SAK_work, do_SAK_work);
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a707196..af26171 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -252,6 +252,7 @@ struct tty_struct {
 	struct rw_semaphore termios_rwsem;
 	struct mutex winsize_mutex;
 	spinlock_t ctrl_lock;
+	spinlock_t flow_lock;
 	/* Termios values are protected by the termios rwsem */
 	struct ktermios termios, termios_locked;
 	struct termiox *termiox;	/* May be NULL for unsupported */
@@ -261,7 +262,7 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	int stopped;
+	int stopped;			/* flow_lock */
 	int flow_stopped;
 	int hw_stopped;
 	int packet;
@@ -400,7 +401,9 @@ extern int tty_paranoia_check(struct tty_struct *tty, struct inode *inode,
 extern char *tty_name(struct tty_struct *tty, char *buf);
 extern void tty_wait_until_sent(struct tty_struct *tty, long timeout);
 extern int tty_check_change(struct tty_struct *tty);
+extern void __stop_tty(struct tty_struct *tty);
 extern void stop_tty(struct tty_struct *tty);
+extern void __start_tty(struct tty_struct *tty);
 extern void start_tty(struct tty_struct *tty);
 extern int tty_register_driver(struct tty_driver *driver);
 extern int tty_unregister_driver(struct tty_driver *driver);
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index e48c608..92e337c 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -152,6 +152,8 @@
  * 	This routine notifies the tty driver that it should stop
  * 	outputting characters to the tty device.  
  *
+ *	Called with ->flow_lock held. Serialized with start() method.
+ *
  *	Optional:
  *
  *	Note: Call stop_tty not this method.
@@ -161,6 +163,8 @@
  * 	This routine notifies the tty driver that it resume sending
  *	characters to the tty device.
  *
+ *	Called with ->flow_lock held. Serialized with stop() method.
+ *
  *	Optional:
  *
  *	Note: Call start_tty not this method.
-- 
2.1.0

[PATCH v2 10/14] tty: Move packet mode flow control notifications to pty driver

[Peter Hurley]

When a master pty is set to packet mode, flow control changes to
the slave pty cause notifications to the master pty via reads and
polls. However, these tests are occurring for all ttys, not
just ptys.

Implement flow control packet mode notifications in the pty driver.
Only the slave side implements the flow control handlers since
packet mode is asymmetric; the master pty receives notifications
for slave-side changes, but not vice versa.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/tty/tty_io.c | 31 ++++---------------------------
 2 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 25c9bc7..c344de0 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -24,6 +24,7 @@
 #include <linux/devpts_fs.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
+#include <linux/poll.h>
 
 
 #ifdef CONFIG_UNIX98_PTYS
@@ -313,6 +314,42 @@ done:
 }
 
 /**
+ *	pty_start - start() handler
+ *	pty_stop  - stop() handler
+ *	@tty: tty being flow-controlled
+ *
+ *	Propagates the TIOCPKT status to the master pty.
+ *
+ *	NB: only the master pty can be in packet mode so only the slave
+ *	    needs start()/stop() handlers
+ */
+static void pty_start(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	if (tty->link && tty->link->packet) {
+		tty->ctrl_status &= ~TIOCPKT_STOP;
+		tty->ctrl_status |= TIOCPKT_START;
+		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
+	}
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+}
+
+static void pty_stop(struct tty_struct *tty)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	if (tty->link && tty->link->packet) {
+		tty->ctrl_status &= ~TIOCPKT_START;
+		tty->ctrl_status |= TIOCPKT_STOP;
+		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
+	}
+	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+}
+
+/**
  *	pty_common_install		-	set up the pty pair
  *	@driver: the pty driver
  *	@tty: the tty being instantiated
@@ -472,6 +509,8 @@ static const struct tty_operations slave_pty_ops_bsd = {
 	.set_termios = pty_set_termios,
 	.cleanup = pty_cleanup,
 	.resize = pty_resize,
+	.start = pty_start,
+	.stop = pty_stop,
 	.remove = pty_remove
 };
 
@@ -647,6 +686,8 @@ static const struct tty_operations pty_unix98_ops = {
 	.chars_in_buffer = pty_chars_in_buffer,
 	.unthrottle = pty_unthrottle,
 	.set_termios = pty_set_termios,
+	.start = pty_start,
+	.stop = pty_stop,
 	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup,
 };
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b898e29..55b9103 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -922,8 +922,7 @@ void no_tty(void)
  *	stop_tty	-	propagate flow control
  *	@tty: tty to stop
  *
- *	Perform flow control to the driver. For PTY/TTY pairs we
- *	must also propagate the TIOCKPKT status. May be called
+ *	Perform flow control to the driver. May be called
  *	on an already stopped device and will not re-call the driver
  *	method.
  *
@@ -933,24 +932,14 @@ void no_tty(void)
  *	but not always.
  *
  *	Locking:
- *		ctrl_lock
  *		flow_lock
  */
 
 void __stop_tty(struct tty_struct *tty)
 {
-	unsigned long flags;
-
 	if (tty->stopped)
 		return;
 	tty->stopped = 1;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->link && tty->link->packet) {
-		tty->ctrl_status &= ~TIOCPKT_START;
-		tty->ctrl_status |= TIOCPKT_STOP;
-		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
-	}
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 	if (tty->ops->stop)
 		(tty->ops->stop)(tty);
 }
@@ -969,33 +958,21 @@ EXPORT_SYMBOL(stop_tty);
  *	start_tty	-	propagate flow control
  *	@tty: tty to start
  *
- *	Start a tty that has been stopped if at all possible. Perform
- *	any necessary wakeups and propagate the TIOCPKT status. If this
- *	is the tty was previous stopped and is being started then the
- *	driver start method is invoked and the line discipline woken.
+ *	Start a tty that has been stopped if at all possible. If this
+ *	tty was previous stopped and is now being started, the driver
+ *	start method is invoked and the line discipline woken.
  *
  *	Locking:
- *		ctrl_lock
  *		flow_lock
  */
 
 void __start_tty(struct tty_struct *tty)
 {
-	unsigned long flags;
-
 	if (!tty->stopped || tty->flow_stopped)
 		return;
 	tty->stopped = 0;
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
-	if (tty->link && tty->link->packet) {
-		tty->ctrl_status &= ~TIOCPKT_STOP;
-		tty->ctrl_status |= TIOCPKT_START;
-		wake_up_interruptible_poll(&tty->link->read_wait, POLLIN);
-	}
-	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 	if (tty->ops->start)
 		(tty->ops->start)(tty);
-	/* If we have a running line discipline it may need kicking */
 	tty_wakeup(tty);
 }
 
-- 
2.1.0

[PATCH v2 11/14] tty: Serialize tcflow() with other tty flow control changes

[Peter Hurley]

Use newly-introduced tty->flow_lock to serialize updates to
tty->flow_stopped (via tcflow()) and with concurrent tty flow
control changes from other sources.

Merge the storage for ->stopped and ->flow_stopped, now that both
flags are serialized by ->flow_lock.

The padding bits are necessary to force the compiler to allocate the
type specified; otherwise, gcc will ignore the type specifier and
allocate the minimum number of bytes necessary to store the bitfield.
In turn, this would allow Alpha EV4 and EV5 cpus to corrupt adjacent
byte storage because those cpus use RMW to store byte and short data.

gcc versions < 4.7.2 will also corrupt storage adjacent to bitfields
smaller than unsigned long on ia64, ppc64, hppa64 and sparc64, thus
requiring more than unsigned int storage (which would otherwise be
sufficient to workaround the Alpha non-atomic byte/short storage problem).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ioctl.c | 8 ++++++--
 include/linux/tty.h     | 5 +++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 6fd60fe..ffed466 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1177,16 +1177,20 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			return retval;
 		switch (arg) {
 		case TCOOFF:
+			spin_lock_irq(&tty->flow_lock);
 			if (!tty->flow_stopped) {
 				tty->flow_stopped = 1;
-				stop_tty(tty);
+				__stop_tty(tty);
 			}
+			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCOON:
+			spin_lock_irq(&tty->flow_lock);
 			if (tty->flow_stopped) {
 				tty->flow_stopped = 0;
-				start_tty(tty);
+				__start_tty(tty);
 			}
+			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCIOFF:
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index af26171..377896d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -262,8 +262,9 @@ struct tty_struct {
 	unsigned long flags;
 	int count;
 	struct winsize winsize;		/* winsize_mutex */
-	int stopped;			/* flow_lock */
-	int flow_stopped;
+	unsigned long stopped:1,	/* flow_lock */
+		      flow_stopped:1,
+		      unused:62;
 	int hw_stopped;
 	int packet;
 	unsigned char ctrl_status;	/* ctrl_lock */
-- 
2.1.0

[PATCH v2 12/14] tty: Move and rename send_prio_char() as tty_send_xchar()

[Peter Hurley]

Relocate the file-scope function, send_prio_char(), as a global
helper tty_send_xchar(). Remove the global declarations for
tty_write_lock()/tty_write_unlock(), as these are file-scope only now.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 33 +++++++++++++++++++++++++++++++--
 drivers/tty/tty_ioctl.c | 33 ++-------------------------------
 include/linux/tty.h     |  3 +--
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 55b9103..54e359b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1037,14 +1037,14 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	return i;
 }
 
-void tty_write_unlock(struct tty_struct *tty)
+static void tty_write_unlock(struct tty_struct *tty)
 	__releases(&tty->atomic_write_lock)
 {
 	mutex_unlock(&tty->atomic_write_lock);
 	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
 }
 
-int tty_write_lock(struct tty_struct *tty, int ndelay)
+static int tty_write_lock(struct tty_struct *tty, int ndelay)
 	__acquires(&tty->atomic_write_lock)
 {
 	if (!mutex_trylock(&tty->atomic_write_lock)) {
@@ -1231,6 +1231,35 @@ ssize_t redirected_tty_write(struct file *file, const char __user *buf,
 	return tty_write(file, buf, count, ppos);
 }
 
+/**
+ *	tty_send_xchar	-	send priority character
+ *
+ *	Send a high priority character to the tty even if stopped
+ *
+ *	Locking: none for xchar method, write ordering for write method.
+ */
+
+int tty_send_xchar(struct tty_struct *tty, char ch)
+{
+	int	was_stopped = tty->stopped;
+
+	if (tty->ops->send_xchar) {
+		tty->ops->send_xchar(tty, ch);
+		return 0;
+	}
+
+	if (tty_write_lock(tty, 0) < 0)
+		return -ERESTARTSYS;
+
+	if (was_stopped)
+		start_tty(tty);
+	tty->ops->write(tty, &ch, 1);
+	if (was_stopped)
+		stop_tty(tty);
+	tty_write_unlock(tty);
+	return 0;
+}
+
 static char ptychar[] = "pqrstuvwxyzabcde";
 
 /**
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index ffed466..14d9002 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -912,35 +912,6 @@ static int set_ltchars(struct tty_struct *tty, struct ltchars __user *ltchars)
 #endif
 
 /**
- *	send_prio_char		-	send priority character
- *
- *	Send a high priority character to the tty even if stopped
- *
- *	Locking: none for xchar method, write ordering for write method.
- */
-
-static int send_prio_char(struct tty_struct *tty, char ch)
-{
-	int	was_stopped = tty->stopped;
-
-	if (tty->ops->send_xchar) {
-		tty->ops->send_xchar(tty, ch);
-		return 0;
-	}
-
-	if (tty_write_lock(tty, 0) < 0)
-		return -ERESTARTSYS;
-
-	if (was_stopped)
-		start_tty(tty);
-	tty->ops->write(tty, &ch, 1);
-	if (was_stopped)
-		stop_tty(tty);
-	tty_write_unlock(tty);
-	return 0;
-}
-
-/**
  *	tty_change_softcar	-	carrier change ioctl helper
  *	@tty: tty to update
  *	@arg: enable/disable CLOCAL
@@ -1194,11 +1165,11 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			break;
 		case TCIOFF:
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
-				return send_prio_char(tty, STOP_CHAR(tty));
+				return tty_send_xchar(tty, STOP_CHAR(tty));
 			break;
 		case TCION:
 			if (START_CHAR(tty) != __DISABLED_CHAR)
-				return send_prio_char(tty, START_CHAR(tty));
+				return tty_send_xchar(tty, START_CHAR(tty));
 			break;
 		default:
 			return -EINVAL;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 377896d..3c1276b 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -418,6 +418,7 @@ extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
 extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
 			     int buflen);
 extern void tty_write_message(struct tty_struct *tty, char *msg);
+extern int tty_send_xchar(struct tty_struct *tty, char ch);
 extern int tty_put_char(struct tty_struct *tty, unsigned char c);
 extern int tty_chars_in_buffer(struct tty_struct *tty);
 extern int tty_write_room(struct tty_struct *tty);
@@ -504,8 +505,6 @@ extern struct tty_struct *tty_pair_get_pty(struct tty_struct *tty);
 extern struct mutex tty_mutex;
 extern spinlock_t tty_files_lock;
 
-extern void tty_write_unlock(struct tty_struct *tty);
-extern int tty_write_lock(struct tty_struct *tty, int ndelay);
 #define tty_is_writelocked(tty)  (mutex_is_locked(&tty->atomic_write_lock))
 
 extern void tty_port_init(struct tty_port *port);
-- 
2.1.0

[PATCH v2 13/14] tty: Hold termios_rwsem for tcflow(TCIxxx)

[Peter Hurley]

While transmitting a START/STOP char for tcflow(TCION/TCIOFF), prevent
a termios change. Otherwise, a garbage in-band flow control char
may be sent, if the termios change overlaps the transmission setup.

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

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 14d9002..32cee97 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1164,17 +1164,21 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCIOFF:
+			down_read(&tty->termios_rwsem);
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
-				return tty_send_xchar(tty, STOP_CHAR(tty));
+				retval = tty_send_xchar(tty, STOP_CHAR(tty));
+			up_read(&tty->termios_rwsem);
 			break;
 		case TCION:
+			down_read(&tty->termios_rwsem);
 			if (START_CHAR(tty) != __DISABLED_CHAR)
-				return tty_send_xchar(tty, START_CHAR(tty));
+				retval = tty_send_xchar(tty, START_CHAR(tty));
+			up_read(&tty->termios_rwsem);
 			break;
 		default:
 			return -EINVAL;
 		}
-		return 0;
+		return retval;
 	case TCFLSH:
 		retval = tty_check_change(tty);
 		if (retval)
-- 
2.1.0

[PATCH v2 14/14] tty: Workaround Alpha non-atomic byte storage in tty_struct

[Peter Hurley]

The Alpha EV4/EV5 cpus can corrupt adjacent byte and short data because
those cpus use RMW to store byte and short data. Thus, concurrent adjacent
byte stores could become corrupted, if serialized by a different lock.
tty_struct uses different locks to protect certain fields within the
structure, and thus is vulnerable to byte stores which are not atomic.

Merge the ->ctrl_status byte and packet mode bit, both protected by the
->ctrl_lock, into an unsigned long.

The padding bits are necessary to force the compiler to allocate the
type specified; otherwise, gcc will ignore the type specifier and
allocate the minimum number of bytes required to store the bitfield.
In turn, this would allow Alpha EV4/EV5 cpus to corrupt adjacent byte
or short storage (because those cpus use RMW to store byte and short data).

gcc versions < 4.7.2 will also corrupt storage adjacent to bitfields
smaller than unsigned long on ia64, ppc64, hppa64, and sparc64, thus
requiring more than unsigned int storage (which would otherwise be
sufficient to fix the Alpha non-atomic storage problem).

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

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 3c1276b..7a0a796 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -266,8 +266,9 @@ struct tty_struct {
 		      flow_stopped:1,
 		      unused:62;
 	int hw_stopped;
-	int packet;
-	unsigned char ctrl_status;	/* ctrl_lock */
+	unsigned long ctrl_status:8,	/* ctrl_lock */
+		      packet:1,
+		      unused_ctrl:55;
 	unsigned int receive_room;	/* Bytes free for queue */
 	int flow_change;
 
-- 
2.1.0