Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

Alan Cox
2004-11-09T15:53:42Z

On Maw, 2004-11-09 at 14:47, Russell King wrote:
> On Tue, Nov 09, 2004 at 01:17:22PM +0000, Alan Cox wrote:
> > So its broken, totally and utterly. Its the kind of undefined,
> > unserialized crap that I'm trying to get _OUT_ of the serial layer.
>
> I think you mean the tty layer here - the tty layer is where most of

Out of the serial layer too. While the serial layer does stuff like call
line discipline functions from the wrong context and sick hacks like
calling functions out of tty layer workqueues its also involved.

Right now calls into the ldisc side are all sane (there are two corner
cases left that need the other side fixing). The calls into serial
driver side is still pending repair and also needs the flip buffers
fixing first.

> the serialisation problems remain, and the locking in the serial
> layer is mostly there to work around the tty layers deficiencies.
>
> I would have liked to have rewritten the tty layer along the lines
> that Ted was suggesting, but that would've been far too much work
> for me to do alone.

I'm working on it. It would be helpful if the drivers/serial code would
use helpers and not dig in places it shouldnt so that the transition can
be cleaner.

Something like this for 8250.c - it fixes the sick hacks calling into
tty
workqueues, the deadlock on overrun and cleans up flip buffer whacking
to use the helper. The helpers handle the overrun case internally so
that cleans up some code, they also happen to be API's I think I can
preserve when updating while direct buffer whacking is probably a nono.

In general tty driver hacking folks should expect

- direct flip buffer poking to break
- flip buffer length checking to break (DMA cards and virtualised
machines want
variable sized buffers)

so if they can move towards tty_insert_flip_char() they'll make life
easier down the line. I'll try and push a few more "fake" helpers as I
try and get the dynamic flip buffers into shape.

--- ../linux.vanilla-2.6.10rc1/drivers/serial/8250.c 2004-11-05 15:42:15.000000000 +0000
+++ drivers/serial/8250.c 2004-11-09 15:42:49.102511656 +0000
@@ -32,7 +32,7 @@
#include <linux/serialP.h>
#include <linux/delay.h>
#include <linux/device.h>
-
+#include <linux/tty_flip.h>
#include <asm/io.h>
#include <asm/irq.h>

@@ -978,16 +978,19 @@
struct tty_struct *tty = up->port.info->tty;
unsigned char ch, lsr = *status;
int max_count = 256;
+ char flag;

do {
+ /* The following is not allowed by the tty layer and
+ unsafe. It should be fixed ASAP */
if (unlikely(tty->flip.count >= TTY_FLIPBUF_SIZE)) {
- tty->flip.work.func((void *)tty);
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- return; // if TTY_DONT_FLIP is set
+ if(tty->low_latency)
+ tty_flip_buffer_push(tty);
+ /* If this failed then we will throw away the
+ bytes but must do so to clear interrupts */
}
ch = serial_inp(up, UART_RX);
- *tty->flip.char_buf_ptr = ch;
- *tty->flip.flag_buf_ptr = TTY_NORMAL;
+ flag = TTY_NORMAL;
up->port.icount.rx++;

#ifdef CONFIG_SERIAL_8250_CONSOLE
@@ -1030,18 +1033,16 @@

if (lsr & UART_LSR_BI) {
DEBUG_INTR("handling break....");
- *tty->flip.flag_buf_ptr = TTY_BREAK;
+ flag = TTY_BREAK;
} else if (lsr & UART_LSR_PE)
- *tty->flip.flag_buf_ptr = TTY_PARITY;
+ flag = TTY_PARITY;
else if (lsr & UART_LSR_FE)
- *tty->flip.flag_buf_ptr = TTY_FRAME;
+ flag = TTY_FRAME;
}
if (uart_handle_sysrq_char(&up->port, ch, regs))
goto ignore_char;
if ((lsr & up->port.ignore_status_mask) == 0) {
- tty->flip.flag_buf_ptr++;
- tty->flip.char_buf_ptr++;
- tty->flip.count++;
+ tty_insert_flip_char(tty, ch, flag);
}
if ((lsr & UART_LSR_OE) &&
tty->flip.count < TTY_FLIPBUF_SIZE) {
@@ -1050,10 +1051,7 @@
* immediately, and doesn't affect the current
* character.
*/
- *tty->flip.flag_buf_ptr = TTY_OVERRUN;
- tty->flip.flag_buf_ptr++;
- tty->flip.char_buf_ptr++;
- tty->flip.count++;
+ tty_insert_flip_char(tty, 0, TTY_OVERRUN);
}
ignore_char:
lsr = serial_inp(up, UART_LSR);