serial 8250: move push calls out of lock

From Alan Cox:

This time to lkml...

Subject: serial 8250: move push calls out of lock

Due to previous changes in the 8250 driver, the call to tty_flip_buffer_push is now done with interrupts disabled. Not really a huge deal, but sub-optimal.

This patch moves all of the "wake up, you have data" operations out of the lock. This will run a little faster by avoiding dropping and retaking the lock in the receive handler and will make the polled serial code (which I am working on) easier to do.

Quoted 1 line

From Alan Cox:

This is actually a very good idea as it means you don't have to deal with the problems with low_latency causing re-entry into the driver under the lock so its conceptually much easier to follow the locking

Signed-off-by Corey Minyard <[email protected]>

Acked-by Alan Cox <[email protected]>

 drivers/serial/8250.c |   53 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

Index: linux-2.6.21/drivers/serial/8250.c
===================================================================

drivers/serial/8250.c +22 -18

@@ -1146,7 +1146,7 @@ static void serial8250_stop_tx(struct ua
 	}
 }
 
-static void transmit_chars(struct uart_8250_port *up);
+static int transmit_chars(struct uart_8250_port *up);
 
 static void serial8250_start_tx(struct uart_port *port)
 {

@@ -1195,10 +1195,8 @@ static void serial8250_enable_ms(struct 
 	serial_out(up, UART_IER, up->ier);
 }
 
-static void
-receive_chars(struct uart_8250_port *up, unsigned int *status)
+static int receive_chars(struct uart_8250_port *up, unsigned int *status)
 {
-	struct tty_struct *tty = up->port.info->tty;
 	unsigned char ch, lsr = *status;
 	int max_count = 256;
 	char flag;

@@ -1262,30 +1260,30 @@ receive_chars(struct uart_8250_port *up,
 	ignore_char:
 		lsr = serial_inp(up, UART_LSR);
 	} while ((lsr & UART_LSR_DR) && (max_count-- > 0));
-	spin_unlock(&up->port.lock);
-	tty_flip_buffer_push(tty);
-	spin_lock(&up->port.lock);
 	*status = lsr;
+
+	return 1;
 }
 
-static void transmit_chars(struct uart_8250_port *up)
+static int transmit_chars(struct uart_8250_port *up)
 {
 	struct circ_buf *xmit = &up->port.info->xmit;
 	int count;
+	int xmit_ready = 0;
 
 	if (up->port.x_char) {
 		serial_outp(up, UART_TX, up->port.x_char);
 		up->port.icount.tx++;
 		up->port.x_char = 0;
-		return;
+		return 0;
 	}
 	if (uart_tx_stopped(&up->port)) {
 		serial8250_stop_tx(&up->port);
-		return;
+		return 0;
 	}
 	if (uart_circ_empty(xmit)) {
 		__stop_tx(up);
-		return;
+		return 0;
 	}
 
 	count = up->tx_loadsz;

@@ -1298,18 +1296,22 @@ static void transmit_chars(struct uart_8
 	} while (--count > 0);
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-		uart_write_wakeup(&up->port);
+		xmit_ready = 1;
 
 	DEBUG_INTR("THRE...");
 
 	if (uart_circ_empty(xmit))
 		__stop_tx(up);
+
+	return xmit_ready;
 }
 
-static unsigned int check_modem_status(struct uart_8250_port *up)
+static int check_modem_status(struct uart_8250_port *up, unsigned int *rstatus)
 {
 	unsigned int status = serial_in(up, UART_MSR);
 
+	if (rstatus)
+		*rstatus = status;
 	if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
 	    up->port.info != NULL) {
 		if (status & UART_MSR_TERI)

@@ -1320,11 +1322,10 @@ static unsigned int check_modem_status(s
 			uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
 		if (status & UART_MSR_DCTS)
 			uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
-
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		return 1;
 	}
 
-	return status;
+	return 0;
 }
 
 /*

@@ -1334,6 +1335,9 @@ static inline void
 serial8250_handle_port(struct uart_8250_port *up)
 {
 	unsigned int status;
+	int xmit_ready = 0;
+	int recv_ready = 0;
+	int msr_ready = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&up->port.lock, flags);

@@ -1343,12 +1347,21 @@ serial8250_handle_port(struct uart_8250_
 	DEBUG_INTR("status = %x...", status);
 
 	if (status & UART_LSR_DR)
-		receive_chars(up, &status);
-	check_modem_status(up);
+		recv_ready = receive_chars(up, &status);
+	msr_ready = check_modem_status(up, NULL);
 	if (status & UART_LSR_THRE)

Quoted 2 lines · Corey Minyard

This time to lkml...

NAK. Did you read your own patch, let alone try to build it? It's also utterly broken. See comments below.

Quoted 147 lines

Subject: serial 8250: move push calls out of lock

Due to previous changes in the 8250 driver, the call to tty_flip_buffer_push is now done with interrupts disabled. Not really a huge deal, but sub-optimal.

This patch moves all of the "wake up, you have data" operations out of the lock. This will run a little faster by avoiding dropping and retaking the lock in the receive handler and will make the polled serial code (which I am working on) easier to do.

Quoted 1 line

From Alan Cox:

This is actually a very good idea as it means you don't have to deal with the problems with low_latency causing re-entry into the driver under the lock so its conceptually much easier to follow the locking

Signed-off-by Corey Minyard <[email protected]>

Acked-by Alan Cox <[email protected]>

 drivers/serial/8250.c |   53 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

Index: linux-2.6.21/drivers/serial/8250.c
===================================================================

drivers/serial/8250.c +22 -19

@@ -1146,7 +1146,7 @@ static void serial8250_stop_tx(struct ua
 	}
 }
 
-static void transmit_chars(struct uart_8250_port *up);
+static int transmit_chars(struct uart_8250_port *up);
 
 static void serial8250_start_tx(struct uart_port *port)
 {

@@ -1195,10 +1195,8 @@ static void serial8250_enable_ms(struct 
 	serial_out(up, UART_IER, up->ier);
 }
 
-static void
-receive_chars(struct uart_8250_port *up, unsigned int *status)
+static int receive_chars(struct uart_8250_port *up, unsigned int *status)
 {
-	struct tty_struct *tty = up->port.info->tty;
 	unsigned char ch, lsr = *status;
 	int max_count = 256;
 	char flag;

@@ -1262,30 +1260,30 @@ receive_chars(struct uart_8250_port *up,
 	ignore_char:
 		lsr = serial_inp(up, UART_LSR);
 	} while ((lsr & UART_LSR_DR) && (max_count-- > 0));
-	spin_unlock(&up->port.lock);
-	tty_flip_buffer_push(tty);
-	spin_lock(&up->port.lock);
 	*status = lsr;
+
+	return 1;
 }
 
-static void transmit_chars(struct uart_8250_port *up)
+static int transmit_chars(struct uart_8250_port *up)
 {
 	struct circ_buf *xmit = &up->port.info->xmit;
 	int count;
+	int xmit_ready = 0;
 
 	if (up->port.x_char) {
 		serial_outp(up, UART_TX, up->port.x_char);
 		up->port.icount.tx++;
 		up->port.x_char = 0;
-		return;
+		return 0;
 	}
 	if (uart_tx_stopped(&up->port)) {
 		serial8250_stop_tx(&up->port);
-		return;
+		return 0;
 	}
 	if (uart_circ_empty(xmit)) {
 		__stop_tx(up);
-		return;
+		return 0;
 	}
 
 	count = up->tx_loadsz;

@@ -1298,18 +1296,22 @@ static void transmit_chars(struct uart_8
 	} while (--count > 0);
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-		uart_write_wakeup(&up->port);
+		xmit_ready = 1;
 
 	DEBUG_INTR("THRE...");
 
 	if (uart_circ_empty(xmit))
 		__stop_tx(up);
+
+	return xmit_ready;
 }
 
-static unsigned int check_modem_status(struct uart_8250_port *up)
+static int check_modem_status(struct uart_8250_port *up, unsigned int *rstatus)
 {
 	unsigned int status = serial_in(up, UART_MSR);
 
+	if (rstatus)
+		*rstatus = status;
 	if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
 	    up->port.info != NULL) {
 		if (status & UART_MSR_TERI)

@@ -1320,11 +1322,10 @@ static unsigned int check_modem_status(s
 			uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
 		if (status & UART_MSR_DCTS)
 			uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
-
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		return 1;
 	}
 
-	return status;
+	return 0;
 }
 
 /*

@@ -1334,6 +1335,9 @@ static inline void
 serial8250_handle_port(struct uart_8250_port *up)
 {
 	unsigned int status;
+	int xmit_ready = 0;
+	int recv_ready = 0;
+	int msr_ready = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&up->port.lock, flags);

@@ -1343,12 +1347,21 @@ serial8250_handle_port(struct uart_8250_
 	DEBUG_INTR("status = %x...", status);
 
 	if (status & UART_LSR_DR)
-		receive_chars(up, &status);
-	check_modem_status(up);
+		recv_ready = receive_chars(up, &status);
+	msr_ready = check_modem_status(up, NULL);
 	if (status & UART_LSR_THRE)
-		transmit_chars(up);

drivers/serial/8250.c serial8250_handle_port(struct uart_8250_ · line 1362

Quoted 10 lines

drivers/serial/8250.c +7 -0

+		xmit_ready = transmit_chars(up);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
+
+	if (recv_ready)
+		tty_flip_buffer_push(up->port.info->tty);
+
+	if (msr_ready)
+		wake_up_interruptible(&up->port.info->delta_msr_wait);
 

wake_up_interruptible doesn't take any locks.

drivers/serial/8250.c serial8250_handle_port(struct uart_8250_ · line 1366

Quoted 4 lines

drivers/serial/8250.c +3 -0

+
+	if (xmit_ready)
+		uart_write_wakeup(&up->port);
 

NAK. uart_write_wakeup() is specifically designed not to take any locks. It's also called from the depths of uart_handle_cts_change(). So this buys us just additional unnecessary complexity.

Quoted 11 lines

drivers/serial/8250.c +0 -0

 }

/*

drivers/serial/8250.c +1 -1

@@ -1551,7 +1564,7 @@ static unsigned int serial8250_get_mctrl
 	unsigned int status;
 	unsigned int ret;
 
-	status = check_modem_status(up);
+	check_modem_status(up, &status);
 

drivers/serial/8250.c static unsigned int serial8250_get_mctrl · line 1570

Quoted 3 lines

drivers/serial/8250.c +0 -0

 	ret = 0;
 	if (status & UART_MSR_DCD)

Use of an uninitialised variable. The reason we use check_modem_status to return the current status here is because reading the MSR _clears_ interrupts. So, repeatedly calling get_mctrl via an ioctl is a great way to introduce a hardware race condition if you just read the MSR and never act on it.

Quoted 7 lines · Russell King

Use of an uninitialised variable. The reason we use check_modem_status to return the current status here is because reading the MSR _clears_ interrupts. So, repeatedly calling get_mctrl via an ioctl is a great way to introduce a hardware race condition if you just read the MSR and never act on it.

"status" is used as a return variable; it is set by the call. I modified check_modem_status() to return if it detected a change and set the status if it was passed in.