Serial: UART_BUG_TXEN race conditions

Serial: UART_BUG_TXEN race conditions

[Ingo van Lil]

Hello everybody,

I'm currently working with Linux on a custom ASIC system with two 
crappy UART ports that show a behaviour quite similar to the problem 
that the UART_BUG_TXEN workaround is supposed to deal with. The 
interesting lines are in serial8250_start_tx 
(drivers/serial/8250.c:1127)

	if (up->bugs & UART_BUG_TXEN) {
		unsigned char lsr, iir;
		lsr = serial_in(up, UART_LSR);
		iir = serial_in(up, UART_IIR);
		if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
			transmit_chars(up);
	}

Correct me if I'm mistaken, but I see two possible race conditions with 
that piece of code:

1. What if the IIR actually equals UART_IIR_THRI at that point? The
    read access will clear the interrupt condition and the workaround
    will effect the actual opposite of its intention: Neither
    serial8250_start_tx() nor the interrupt handler will start
    transmitting characters for the ring buffer.

2. What if an interrupt is triggered shortly after reading the IIR?
    Both serial8250_start_tx() and the interrupt handler will start
    a transmission simultaneously.

Problem #1 should be quite easy to deal with: If the IIR read comes out 
as UART_IIR_THRI then the interrupt handler won't initiate a 
transmission, so we should. I'm not entirely sure how to deal with #2, 
though. Maybe it's enough to clear the THRE bit while transmitting 
characters, maybe some kind of locking is required.

Cheers,
Ingo

Re: Serial: UART_BUG_TXEN race conditions

[Russell King]

On Mon, Jun 26, 2006 at 10:07:47PM +0200, Ingo van Lil wrote:
> Correct me if I'm mistaken, but I see two possible race conditions with 
> that piece of code:
> 
> 1. What if the IIR actually equals UART_IIR_THRI at that point? The
>    read access will clear the interrupt condition and the workaround
>    will effect the actual opposite of its intention: Neither
>    serial8250_start_tx() nor the interrupt handler will start
>    transmitting characters for the ring buffer.

Gah, looks like you're right - reading the IIR will clear the transmit
pending interrupt, so we should probably just load the transmitter up
with characters anyway if the TEMT bit is set.

> 2. What if an interrupt is triggered shortly after reading the IIR?
>    Both serial8250_start_tx() and the interrupt handler will start
>    a transmission simultaneously.

This function is run under the port spinlock, so the interrupt handler
will be held off until it completes.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial: UART_BUG_TXEN race conditions

[Ingo van Lil]

Russell King <rmk+lkml@arm.linux.org.uk> schrieb:

>> 1. What if the IIR actually equals UART_IIR_THRI at that point? The
>>    read access will clear the interrupt condition and the workaround
>>    will effect the actual opposite of its intention: Neither
>>    serial8250_start_tx() nor the interrupt handler will start
>>    transmitting characters for the ring buffer.
>
> Gah, looks like you're right - reading the IIR will clear the transmit
> pending interrupt, so we should probably just load the transmitter up
> with characters anyway if the TEMT bit is set.

How about doing that on any controller, not just the buggy ones? It 
shouldn't cause any problems even on well-behaved UARTs, or could it?

> This function is run under the port spinlock, so the interrupt handler
> will be held off until it completes.

OK, great. Forget about my second concern, then.

Cheers,
Ingo

Re: Serial: UART_BUG_TXEN race conditions

[linux-os (Dick Johnson)]

On Mon, 26 Jun 2006, Ingo van Lil wrote:

> Hello everybody,
>
> I'm currently working with Linux on a custom ASIC system with two
> crappy UART ports that show a behaviour quite similar to the problem
> that the UART_BUG_TXEN workaround is supposed to deal with. The
> interesting lines are in serial8250_start_tx
> (drivers/serial/8250.c:1127)
>
> 	if (up->bugs & UART_BUG_TXEN) {
> 		unsigned char lsr, iir;
> 		lsr = serial_in(up, UART_LSR);
> 		iir = serial_in(up, UART_IIR);
> 		if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
> 			transmit_chars(up);
> 	}
>
> Correct me if I'm mistaken, but I see two possible race conditions with
> that piece of code:
>
> 1. What if the IIR actually equals UART_IIR_THRI at that point? The
>    read access will clear the interrupt condition and the workaround
>    will effect the actual opposite of its intention: Neither
>    serial8250_start_tx() nor the interrupt handler will start
>    transmitting characters for the ring buffer.
>
> 2. What if an interrupt is triggered shortly after reading the IIR?
>    Both serial8250_start_tx() and the interrupt handler will start
>    a transmission simultaneously.
>
> Problem #1 should be quite easy to deal with: If the IIR read comes out
> as UART_IIR_THRI then the interrupt handler won't initiate a
> transmission, so we should. I'm not entirely sure how to deal with #2,
> though. Maybe it's enough to clear the THRE bit while transmitting
> characters, maybe some kind of locking is required.
>
> Cheers,
> Ingo
>

With 8250 UARTs and clones, an interrupt occurs when the
transmitter holding register __becomes__ empty. That means
it must have had something in it.

Normally, one starts a transmit sequence by polling for
THRE (transmitter holding-register empty) from non-interrupt
code with interrupts enabled. It will usually not appear empty
when polled in this manner, until the last character(s)
have been sent out of the user buffer by interrupt code.
Once it appears empty (*), code sends the first byte from the
buffer. The interrupt code (for transmit) just puts each
new byte from the output buffer into the transmitter.
Ping/pong transmit buffers help synchronize operations.

(*) Even if it is not really empty, it will be by the time
the first byte is put into the TX holding register because
interrupts are NOT disabled and the interrupt that would
have cleared it was able to complete.

A common problem with 8250 UART code I've seen is that
writers often do this:

 	clear interrupts (spinlock)
 	read/check status
 	do something
 	enable interrupts (spin unlock)

This 'seems' correct, but allows sneaky things to happen between
when you read/check status and you actually handle it. To prevent
this, some writers set up a loop with the interrupts disabled.
Now, under heavy activity, the machine ends up being permanently
polled with the interrupts off. This is NotGood(tm).

To implement the above algorithm, the following has been successful.

user_write(....user_len)
{
     buf_len = check_output_buffer_space();	// Head - tail ? will
 						// always get larger or
 						// remain the same.
     len = MIN(buf_len, user_len);

     copy_from_user(transfer_buffer, user_buffer, len);	// Get user data

     spin_lock_irqsave(&uart_lock, flags);
     memcpy(&output_buffer[tail], transfer_buffer, len);
     tail += len;
     spin_unlock_irqrestore(&uart_lock, flags);
     do_transmit();	// With interrupts enabled
     return len;
}

//
// Used by both ISR and user write code. When called from
// the user write code, data will always be available unless
// it was just sent out by interrupt, at which case you
// accomplished the required task anyway.
//

do_transmit()
{
    if(output_buffer_has_data && uart_status(THRE))
        put_byte_into_UART();
}


isr()
{
    (interrupts are off by default)
    spin_lock(&uart_lock);
    do_transmit();
    do_receive();
    do_modem();
    spin_unlock(&uart_lock);
}




Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.4 on an i686 machine (5592.88 BogoMips).
New book: http://www.AbominableFirebug.com/
_
\x1a\x04

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: Serial: UART_BUG_TXEN race conditions

[Ingo van Lil]

"linux-os (Dick Johnson)" <linux-os@analogic.com> schrieb:

> A common problem with 8250 UART code I've seen is that
> writers often do this:
>
> 	clear interrupts (spinlock)
> 	read/check status
> 	do something
> 	enable interrupts (spin unlock)
>
> This 'seems' correct, but allows sneaky things to happen between
> when you read/check status and you actually handle it.

What "sneaky thinks" could possibly happen between there? Either the 
IIR read tells you that the TX FIFO is empty (in that case you can 
safely write data into it) or it will tell you something different. In 
the latter case another interrupt will be generated as soon as the FIFO 
becomes empty.

As far as I can see the main reason for all the UART confusion is the 
requirement that the interrupt condition is to be automatically cleared 
  by an IIR read, if (and only if) the indicated state ist THRE. This 
sounds like a particularly bad idea to me, but I'm not an expert.
Anyway, as the IIR read might have been performed by some other piece 
of code (e.g. serial8250_startup) the 8250 driver depends on the UART 
to trigger another interrupt as soon the THRE bit in the IER is set (by 
start_tx). The opencores.org UART behaves that way, and I guess that's 
what most controllers do, but some more exotic chips appear not to.

> To implement the above algorithm, the following has been successful.
[...]

This seems to be pretty much what I suggested yesterday: After adding 
new characters to the ring buffer, check whether the transmitter is 
idle and, if it is, write a first load of characters into the FIFO. 
This should be a safe for solution for all kinds of UARTs, and it would 
allow us to entirely drop the UART_BUG_TXEN detection.

I'm gonna have a look at the kernel patch submission guidelines and, 
unless somebody is quicker than me, propose a patch.

Cheers,
Ingo

Re: Serial: UART_BUG_TXEN race conditions

[linux-os (Dick Johnson)]

On Tue, 27 Jun 2006, Ingo van Lil wrote:

> "linux-os (Dick Johnson)" <linux-os@analogic.com> schrieb:
>
>> A common problem with 8250 UART code I've seen is that
>> writers often do this:
>>
>> 	clear interrupts (spinlock)
>> 	read/check status
>> 	do something
>> 	enable interrupts (spin unlock)
>>
>> This 'seems' correct, but allows sneaky things to happen between
>> when you read/check status and you actually handle it.
>
> What "sneaky thinks" could possibly happen between there? Either the
> IIR read tells you that the TX FIFO is empty (in that case you can
> safely write data into it) or it will tell you something different. In
> the latter case another interrupt will be generated as soon as the FIFO
> becomes empty.

The sneaky things are other bits being set after you read them.
Look under 'Interrupt Reset Control': Reading IIR register or
writing to the transmitter holding register. You read status,
then do something else because the THRE bit wasn't set yet. It
is now set just before you do something else, but you don't get
another edge interrupt because the IRQ line never moved. It's
always high, no more interrupts ... ever. This problem wouldn't
exist with level interrupts but the 8250/16450 doesn't use level
interrupts.

These problems are why you sometimes see the UART ISR code
being called from a timer-tick interrupt.

Whatever you do, you must completely satisfy the interrupt
condition in the ISR, or your code will never get the CPU back!
All the bits that could cause interrupts must be reset before
you return from your routine.

>
> As far as I can see the main reason for all the UART confusion is the
> requirement that the interrupt condition is to be automatically cleared
>  by an IIR read, if (and only if) the indicated state ist THRE. This
> sounds like a particularly bad idea to me, but I'm not an expert.
> Anyway, as the IIR read might have been performed by some other piece
> of code (e.g. serial8250_startup) the 8250 driver depends on the UART
> to trigger another interrupt as soon the THRE bit in the IER is set (by
> start_tx). The opencores.org UART behaves that way, and I guess that's
> what most controllers do, but some more exotic chips appear not to.
>
>> To implement the above algorithm, the following has been successful.
> [...]
>
> This seems to be pretty much what I suggested yesterday: After adding
> new characters to the ring buffer, check whether the transmitter is
> idle and, if it is, write a first load of characters into the FIFO.

Yep.

> This should be a safe for solution for all kinds of UARTs, and it would
> allow us to entirely drop the UART_BUG_TXEN detection.
>
> I'm gonna have a look at the kernel patch submission guidelines and,
> unless somebody is quicker than me, propose a patch.
>
> Cheers,
> Ingo
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.4 on an i686 machine (5592.71 BogoMips).
New book: http://www.AbominableFirebug.com/
_
\x1a\x04

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: Serial: UART_BUG_TXEN race conditions

[H. Peter Anvin]

linux-os (Dick Johnson) wrote:
> 
> With 8250 UARTs and clones, an interrupt occurs when the
> transmitter holding register __becomes__ empty. That means
> it must have had something in it.
> 

Dear Wrongbot,

That's what some people think, and that's how they end up shipping buggy 
hardware.

With 16x50 UARTs, an interrupt occurs *when an interrupt condition is 
asserted*, which can happen either though a state change or on a change 
in IER.

	-hpa

[PATCH] Re: Serial: UART_BUG_TXEN race conditions

[Ingo van Lil]

On Mon, Jun 26, 2006 at 09:25:36PM +0100, Russell King wrote:

> > 1. What if the IIR actually equals UART_IIR_THRI at that point? The
> >    read access will clear the interrupt condition and the workaround
> >    will effect the actual opposite of its intention: Neither
> >    serial8250_start_tx() nor the interrupt handler will start
> >    transmitting characters for the ring buffer.
> 
> Gah, looks like you're right - reading the IIR will clear the transmit
> pending interrupt, so we should probably just load the transmitter up
> with characters anyway if the TEMT bit is set.

Proposed patch. It's my first submission, I hope it meets this list's
standards.
The patch drops the buggy UART detection and always writes a first load
of characters into the transmitter FIFO if it's currently idle. This
should be fine for both well-behaved and broken UARTs.

Cheers,
Ingo


Signed-off-by: Ingo van Lil <inguin@gmx.de>
---

diff -u linux-2.6.17.1/drivers/serial/8250.c.orig linux-2.6.17.1/drivers/serial/8250.c
--- linux-2.6.17.1/drivers/serial/8250.c.orig	2006-06-27 21:37:41.000000000 +0200
+++ linux-2.6.17.1/drivers/serial/8250.c	2006-06-27 21:56:20.000000000 +0200
@@ -1119,18 +1119,13 @@
 static void serial8250_start_tx(struct uart_port *port)
 {
 	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	unsigned char lsr;
 
 	if (!(up->ier & UART_IER_THRI)) {
 		up->ier |= UART_IER_THRI;
 		serial_out(up, UART_IER, up->ier);
-
-		if (up->bugs & UART_BUG_TXEN) {
-			unsigned char lsr, iir;
-			lsr = serial_in(up, UART_LSR);
-			iir = serial_in(up, UART_IIR);
-			if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
-				transmit_chars(up);
-		}
+		lsr = serial_in(up, UART_LSR);
+		if (lsr & UART_LSR_TEMT) transmit_chars(up);
 	}
 
 	/*
@@ -1529,7 +1524,6 @@
 {
 	struct uart_8250_port *up = (struct uart_8250_port *)port;
 	unsigned long flags;
-	unsigned char lsr, iir;
 	int retval;
 
 	up->capabilities = uart_config[up->port.type].flags;
@@ -1634,25 +1628,6 @@
 
 	serial8250_set_mctrl(&up->port, up->port.mctrl);
 
-	/*
-	 * Do a quick test to see if we receive an
-	 * interrupt when we enable the TX irq.
-	 */
-	serial_outp(up, UART_IER, UART_IER_THRI);
-	lsr = serial_in(up, UART_LSR);
-	iir = serial_in(up, UART_IIR);
-	serial_outp(up, UART_IER, 0);
-
-	if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
-		if (!(up->bugs & UART_BUG_TXEN)) {
-			up->bugs |= UART_BUG_TXEN;
-			pr_debug("ttyS%d - enabling bad tx status workarounds\n",
-				 port->line);
-		}
-	} else {
-		up->bugs &= ~UART_BUG_TXEN;
-	}
-
 	spin_unlock_irqrestore(&up->port.lock, flags);
 
 	/*
diff -u linux-2.6.17.1/drivers/serial/8250.h.orig linux-2.6.17.1/drivers/serial/8250.h
--- linux-2.6.17.1/drivers/serial/8250.h.orig	2006-06-27 21:46:13.000000000 +0200
+++ linux-2.6.17.1/drivers/serial/8250.h	2006-06-27 21:46:35.000000000 +0200
@@ -48,8 +48,7 @@
 #define UART_CAP_UUE	(1 << 12)	/* UART needs IER bit 6 set (Xscale) */
 
 #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
-#define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
-#define UART_BUG_NOMSR	(1 << 2)	/* UART has buggy MSR status bits (Au1x00) */
+#define UART_BUG_NOMSR	(1 << 1)	/* UART has buggy MSR status bits (Au1x00) */
 
 #define PROBE_RSA	(1 << 0)
 #define PROBE_ANY	(~0)