Re: [PATCH] serial 8250: move push calls out of lock
From: Corey Minyard
Date: Thu Apr 26 2007 - 17:49:24 EST
Russell King wrote:
On Thu, Apr 26, 2007 at 03:51:03PM -0500, Corey Minyard wrote:
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.
The patch builds without error or warning and seems to work just fine.
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.
>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 <minyard@xxxxxxx>
Acked-by: Alan Cox <alan@xxxxxxxxxx>
drivers/serial/8250.c | 53 +++++++++++++++++++++++++++++++-------------------
1 file changed, 33 insertions(+), 20 deletions(-)
Index: linux-2.6.21/drivers/serial/8250.c
===================================================================
--- linux-2.6.21.orig/drivers/serial/8250.c
+++ linux-2.6.21/drivers/serial/8250.c
@@ -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);
+ 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.
+
+ 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.
True, these things don't take any locks. However, I'm working on
a polled capability for the serial driver and these operations may
have more to them than what they currently do. This is certainly
better for the receive case, and it seems that the more things you
can move out of a lock the better.
}
/*
@@ -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);
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.
"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.
-corey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/