Re: [PATCH] serial 8250: tighten test for using backup timer

On Wed, Mar 19, 2008 at 5:50 PM, Alex Williamson <alex.williamson@xxxxxx> wrote:
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,

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?

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..e2eacdf 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1908,14 +1908,18 @@ 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);
}
}

+ 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
diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h
index 78c0016..5202603 100644
--- a/drivers/serial/8250.h
+++ b/drivers/serial/8250.h
@@ -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 */

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


Alex

Signed-off-by: Alex Williamson <alex.williamson@xxxxxx>
---

diff -r 2202c155b8c3 drivers/serial/8250.c
--- a/drivers/serial/8250.c Tue Mar 18 21:34:48 2008 -0700
+++ b/drivers/serial/8250.c Wed Mar 19 10:32:40 2008 -0600
@@ -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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..e2eacdf 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1908,14 +1908,18 @@ 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);
}
}

+ 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
diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h
index 78c0016..5202603 100644
--- a/drivers/serial/8250.h
+++ b/drivers/serial/8250.h
@@ -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 */

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

Relevant Pages

  • Re: [PATCH] 8250 UART backup timer
    ... The problem is that the UART does not reassert the THRE ... RX interrupt kicks it into working again (ie. an unattended reboot could ... and removes races in the bug detection code. ...
    (Linux-Kernel)
  • [PATCH] serial 8250: tighten test for using backup timer
    ... to enable the backup timer workaround. ... to catch UARTs that don't re-assert the THRE interrupt. ...
    (Linux-Kernel)
  • Re: [PATCH] 8250 UART backup timer
    ... The problem is that the UART does not reassert the THRE ... RX interrupt kicks it into working again (ie. an unattended reboot could ...
    (Linux-Kernel)
  • Re: [PATCH] backup timer for UARTs that lose interrupts (take 2)
    ... this causes the patch to be greyed out and, more importantly, NOT included in a reply. ... The problem is that the UART does not reassert the THRE ... > RX interrupt kicks it into working again (ie. an unattended reboot could ... > backup timer can easily be extended to include other UARTs with similar ...
    (Linux-Kernel)
  • Re: DoModal isnt reentrant but failure mode could be improved
    ... The parent receives a Windows Message that causes it to inform the user that a serial overrun occured. ... The reason such a Windows Message reaches the parent even while the first modal dialog is still on the screen is that it's not a keyboard or mouse input message, and CDialog's message loop retrieves it and dispatches it. ... Temporarily I seemed to understand the idea of trying to make the UART send an additional interrupt when the buffer's 8th byte gets filled but we still really want the UART to continue buffering while it's still necessary. ...
    (microsoft.public.vc.mfc)