serial 8250: tighten test for using backup timer

Hi,

Thomas Koeller had reported an issue where a device that had been
making use of the UART_BUG_TXEN code in the 8250 driver was mistakenly being caught by the backup timer test, causing the device to work improperly. To fix this, the patch below tightens the test requirements to enable the backup timer workaround. The backup timer is really meant to catch UARTs that don't re-assert the THRE interrupt. The expectation is that they do initially assert THRE. This patch clarifies the test. Thanks,

Alex

Signed-off-by Alex Williamson <[email protected]>

---

diff -r 2202c155b8c3 drivers/serial/8250.c

drivers/serial/8250.c Wed Mar 19 10:32:40 2008 -0600 renamed from drivers/serial/8250.c Tue Mar 18 21:34:48 2008 -0700 +3 -3

@@ -1814,6 +1814,7 @@ static int serial8250_startup(struct uar
       }
 
       if (is_real_interrupt(up->port.irq)) {
+               unsigned char iir1;
               /*
                * Test for UARTs that do not reassert THRE when the
                * transmitter is idle and the interrupt has already

@@ -1827,7 +1828,7 @@ static int serial8250_startup(struct uar
               wait_for_xmitr(up, UART_LSR_THRE);
               serial_out_sync(up, UART_IER, UART_IER_THRI);
               udelay(1); /* allow THRE to set */
-               serial_in(up, UART_IIR);
+               iir1 = serial_in(up, UART_IIR);
               serial_out(up, UART_IER, 0);
               serial_out_sync(up, UART_IER, UART_IER_THRI);
               udelay(1); /* allow a working UART time to re-assert THRE */

@@ -1840,7 +1841,7 @@ static int serial8250_startup(struct uar
                * If the interrupt is not reasserted, setup a timer to
                * kick the UART on a regular basis.
                */
-               if (iir & UART_IIR_NO_INT) {
+               if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
                       pr_debug("ttyS%d - using backup timer\n", port->line);
                       up->timer.function = serial8250_backup_timeout;
                       up->timer.data = (unsigned long)up;
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

Sorry to not have picked up on this earlier, but this change seems to break an old DesignWare UART I have in an SoC here. It has a number of known issues, one of which is it does not appropriately reassert THRE.

What seems to happen is that the first time the port is opened the code tests for incorrect reassertion of THRE and correctly sets up the backup timer. The port is closed and subsequently reopened and this time around the new logic prevents the backup timer from being enabled. I'm not 100% sure of the details of the bug that is being worked around here, but it appears that the second time the port is opened it is not possible to detect the bug because the previous THRE condition has already been acknowledged.

The attached patch fixes the problem for me and attempts to preserve the new behaviour at the same time. Comments?

Hi Will,

Quoted 15 lines · Will Newton

Hi,

Sorry to not have picked up on this earlier, but this change seems to break an old DesignWare UART I have in an SoC here. It has a number of known issues, one of which is it does not appropriately reassert THRE.

What seems to happen is that the first time the port is opened the code tests for incorrect reassertion of THRE and correctly sets up the backup timer. The port is closed and subsequently reopened and this time around the new logic prevents the backup timer from being enabled. I'm not 100% sure of the details of the bug that is being worked around here, but it appears that the second time the port is opened it is not possible to detect the bug because the previous THRE condition has already been acknowledged.

Quoted 2 lines

The attached patch fixes the problem for me and attempts to preserve the new behaviour at the same time. Comments?

It would be nice if there was a simple procedure we could do on the UART to reset it to a state so the test works repeatedly. However, with all the buggy UARTs out there, that could end up disturbing someone else.

This change works for me, though it is using up a bit in the bugs field; not that we seem to be allocating them at any great rate. I think it would be worthy of a comment in the code to understand why this new block exists outside the test so we don't need to refer back to the commit changeset.

Acked-by Alex Williamson <[email protected]>

From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001

Quoted 1 line

From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001

From: Will Newton <[email protected]>
Date: Wed, 6 Aug 2008 11:48:29 +0100
Subject: [PATCH] 8250: Improve workaround for UARTs that don't re-assert THRE correctly.

Recent changes to tighten the check for UARTs that don't correctly re-assert THRE caused problems when such a UART was opened for the second time - the bug could only successfully be detected at first initialization. This patch stores the information about the bug in the bugs field of the port structure when the port is first started up so subsequent opens can check this bit even if the test for the bug fails.

Signed-off-by Will Newton <[email protected]>

Acked-by Alex Williamson <[email protected]>

---
 drivers/serial/8250.c |   16 ++++++++++++----
 drivers/serial/8250.h |    1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/serial/8250.c +12 -4

index 342e12f..9ccc563 100644
@@ -1908,15 +1908,23 @@ static int serial8250_startup(struct uart_port *port)
 		 * kick the UART on a regular basis.
 		 */
 		if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
+			up->bugs |= UART_BUG_THRE;
 			pr_debug("ttyS%d - using backup timer\n", port->line);
-			up->timer.function = serial8250_backup_timeout;
-			up->timer.data = (unsigned long)up;
-			mod_timer(&up->timer, jiffies +
-				poll_timeout(up->port.timeout) + HZ / 5);
 		}
 	}
 
 	/*
+	 * The above check will only give an accurate result the first time
+	 * the port is opened so this value needs to be preserved.
+	 */
+	if (up->bugs & UART_BUG_THRE) {
+		up->timer.function = serial8250_backup_timeout;
+		up->timer.data = (unsigned long)up;
+		mod_timer(&up->timer, jiffies +
+			  poll_timeout(up->port.timeout) + HZ / 5);
+	}
+
+	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
 	 * driver used to do this with IRQ0.

drivers/serial/8250.h +0 -0

index 78c0016..5202603 100644

Quoted 13 lines · On Wed, 6 Aug 2008 11:53:13 +0100 "Will Newton" <[email protected]> wrote:

Quoted 1 line

From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001

From: Will Newton <[email protected]>
Date: Wed, 6 Aug 2008 11:48:29 +0100
Subject: [PATCH] 8250: Improve workaround for UARTs that don't re-assert THRE correctly.

Recent changes to tighten the check for UARTs that don't correctly re-assert THRE caused problems when such a UART was opened for the second time - the bug could only successfully be detected at first initialization. This patch stores the information about the bug in the bugs field of the port structure when the port is first started up so subsequent opens can check this bit even if the test for the bug fails.

Quoted 2 lines

Signed-off-by Will Newton <[email protected]>

Acked-by Alex Williamson <[email protected]>

What are the "recent changes" to which you refer? I had a quick look and didn't spot any THRE-related changes this year?

Quoted 47 lines

---
 drivers/serial/8250.c |   16 ++++++++++++----
 drivers/serial/8250.h |    1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/serial/8250.c +12 -4

index 342e12f..9ccc563 100644
@@ -1908,15 +1908,23 @@ static int serial8250_startup(struct uart_port *port)
               * kick the UART on a regular basis.
               */
              if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
+                     up->bugs |= UART_BUG_THRE;
                      pr_debug("ttyS%d - using backup timer\n", port->line);
-                     up->timer.function = serial8250_backup_timeout;
-                     up->timer.data = (unsigned long)up;
-                     mod_timer(&up->timer, jiffies +
-                             poll_timeout(up->port.timeout) + HZ / 5);
              }
      }
 
      /*
+      * The above check will only give an accurate result the first time
+      * the port is opened so this value needs to be preserved.
+      */
+     if (up->bugs & UART_BUG_THRE) {
+             up->timer.function = serial8250_backup_timeout;
+             up->timer.data = (unsigned long)up;
+             mod_timer(&up->timer, jiffies +
+                       poll_timeout(up->port.timeout) + HZ / 5);
+     }
+
+     /*
       * If the "interrupt" for this port doesn't correspond with any
       * hardware interrupt, we use a timer-based system.  The original
       * driver used to do this with IRQ0.

drivers/serial/8250.h +1 -0

index 78c0016..5202603 100644
@@ -47,6 +47,7 @@ struct serial8250_config {
 #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_THRE        (1 << 3)        /* UART has buggy THRE reassertion */
 

drivers/serial/8250.h struct serial8250_config { · line 53

Quoted 2 lines

drivers/serial/8250.h +0 -0

 #define PROBE_RSA    (1 << 0)
 #define PROBE_ANY    (~0)

Also, how serious is the problem which is being fixed here? It _sounds_ like it's of the "fatal for people who have that hardware" variety, in which case we should get this into 2.6.27 and probably 2.6.26.x. Not sure about 2.5.26.x though - the patch doesn't apply there, but I didn't check whether this is due to functional changes.

Also2, we should officially use setup_timer(), like this:

drivers/serial/8250.c renamed from drivers/serial/8250.c~serial-8250-tighten-test-for-using-backup-timer-fix +2 -2

@@ -1964,8 +1964,8 @@ static int serial8250_startup(struct uar
        * the port is opened so this value needs to be preserved.
        */
       if (up->bugs & UART_BUG_THRE) {
-               up->timer.function = serial8250_backup_timeout;
-               up->timer.data = (unsigned long)up;
+               setup_timer(&up->timer, serial8250_backup_timeout,
+                               (unsigned long)up);
               mod_timer(&up->timer, jiffies +
                         poll_timeout(up->port.timeout) + HZ / 5);
       }

but that is a functional change - setup_timer() runs init_timer(), whereas the code you have there does not.

We _do_ have an init_timer(&up->timer) all the way over in serial8250_isa_init_ports(). It's all a bit weird. Could you please double-check that we're being sensible about the initialisation of this timer?