Re: [PATCH] serial 8250: move push calls out of lock

[Russell King]

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.

> 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.

>  }
>  
>  /*
> @@ -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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
-
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/