[PATCH 1/1] 8250_core.c

[PATCH 1/1] 8250_core.c

[Dick Hollenbeck]

[-- Attachment #1: Type: text/plain, Size: 114 bytes --]

This was generated from 3.14 kernel, but since its so small it will likely apply to newer
kernels without issue.


[-- Attachment #2: 8250_core.patch --]
[-- Type: text/x-patch, Size: 1863 bytes --]

A lesser travelled code path specifically crafted for "tx interrupt buggy"
UARTs seems to be testing the wrong bit for whether or not to prime the
transmit pump.  The result is that the tx interrupt never occurs on one chip
I am using that falls into the UART_BUG_TXEN category as I understand it.

The 16550 data sheet even names the interrupt "THRE" and not transmit
shift register empty.  The interrupt is fired when the holding register, or its
logical equivalent, the fifo, goes empty;  not when the shift register goes
empty.  This is a code bug in code hoping to fix a bug in hardware.

The reason this is so late coming to the surface is that lately the 8250
interrupt handler polls for all kinds of special status beyond the intention
of the hardware's purpose specific interrupt. Therefore so long as you
were in the interrupt handler (even if for purposes of servicing a recv
interrupt, you could recover from the broken transmit chain.  But if you are
only transmitting, not receiving, then this bug can manifest itself.

This is the fix.  It is harmless because so long as the fifo is empty, it
should be legal to re-fill it fully anyways.

Signed-off-by: Dick Hollenbeck <dick@softplc.com>

=== modified file 'drivers/tty/serial/8250/8250_core.c'
--- old/drivers/tty/serial/8250/8250_core.c	2014-07-14 18:39:13 +0000
+++ new/drivers/tty/serial/8250/8250_core.c	2015-01-22 05:28:33 +0000
@@ -1302,11 +1302,15 @@ static void serial8250_start_tx(struct u
		up->ier |= UART_IER_THRI;
		serial_port_out(port, UART_IER, up->ier);

+		/*
+		 * If setting UART_IER_THRI does not cause an
+		 * immediate tx interrupt
+		 */
		if (up->bugs & UART_BUG_TXEN) {
			unsigned char lsr;
			lsr = serial_in(up, UART_LSR);
			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-			if (lsr & UART_LSR_TEMT)
+			if (lsr & UART_LSR_THRE)
				serial8250_tx_chars(up);
		}
	}

Re: [PATCH 1/1] 8250_core.c

[Frans Klaver]

Hi,

On Thu, Jan 22, 2015 at 6:56 AM, Dick Hollenbeck <dick@softplc.com> wrote:
> This was generated from 3.14 kernel, but since its so small it will likely apply to newer
> kernels without issue.

Thanks for contributing.

It is the custom that you send your patches in the body of the e-mail
instead of as an attachment. You might want to try using git
send-email, or at least have a look at other patch submissions here.

You may also want to make sure that you have a proper subject line to
your patch, like

  8250_core: fix buggy interrupts bit test

  A lesser travelled codepath...

I'm sure you can come up with a better one though.

Before you resubmit, you might want to run scripts/checkpatch.pl on your patch.
If you resubmit, you should probably also put
linux-serial@vger.kernel.org and Greg Kroah-Hartman
<gregkh@linuxfoundation.org> in CC. For a full list of suggestions,
check scripts/get_maintainer.pl.

Good luck,
Frans

Re: [PATCH 1/1] 8250_core.c

[Peter Hurley]

[ +cc linux-serial, Greg, Frans ]

Hi Dick,

On 01/22/2015 12:56 AM, Dick Hollenbeck wrote:
> This was generated from 3.14 kernel, but since its so small it will likely apply to newer
> kernels without issue.

Thanks for finding this.

In addition to Frans' comments, I've expanded the patch inline for further
comment (which is why attached patches are of limited value).

> A lesser travelled code path specifically crafted for "tx interrupt buggy"
> UARTs seems to be testing the wrong bit for whether or not to prime the
> transmit pump.  The result is that the tx interrupt never occurs on one chip
> I am using that falls into the UART_BUG_TXEN category as I understand it.

What UART is this?

> The 16550 data sheet even names the interrupt "THRE" and not transmit
> shift register empty.  The interrupt is fired when the holding register, or its
> logical equivalent, the fifo, goes empty;  not when the shift register goes
> empty.

Please include a description of why this test is broken. For example,

"So, if the fifo is already empty but the shifter is still transmitting,
no further THRE interrupt will occur, and tx will stall."

The remainder of the commit message can be dropped (except Signed-off-by).

>  This is a code bug in code hoping to fix a bug in hardware.
> 
> The reason this is so late coming to the surface is that lately the 8250
> interrupt handler polls for all kinds of special status beyond the intention
> of the hardware's purpose specific interrupt. Therefore so long as you
> were in the interrupt handler (even if for purposes of servicing a recv
> interrupt, you could recover from the broken transmit chain.  But if you are
> only transmitting, not receiving, then this bug can manifest itself.
> 
> This is the fix.  It is harmless because so long as the fifo is empty, it
> should be legal to re-fill it fully anyways.
> 
> Signed-off-by: Dick Hollenbeck <dick@softplc.com>
> 
> === modified file 'drivers/tty/serial/8250/8250_core.c'
> --- old/drivers/tty/serial/8250/8250_core.c	2014-07-14 18:39:13 +0000
> +++ new/drivers/tty/serial/8250/8250_core.c	2015-01-22 05:28:33 +0000
> @@ -1302,11 +1302,15 @@ static void serial8250_start_tx(struct u
> 		up->ier |= UART_IER_THRI;
> 		serial_port_out(port, UART_IER, up->ier);
> 
> +		/*
> +		 * If setting UART_IER_THRI does not cause an
> +		 * immediate tx interrupt
> +		 */

Please make this comment one line? Eg.,

		/* Prime tx pump for buggy UARTs */

> 		if (up->bugs & UART_BUG_TXEN) {
> 			unsigned char lsr;
> 			lsr = serial_in(up, UART_LSR);
> 			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> -			if (lsr & UART_LSR_TEMT)
> +			if (lsr & UART_LSR_THRE)
> 				serial8250_tx_chars(up);
> 		}
> 	}

Regards,
Peter Hurley