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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87a5b0800808050444t49480549t758f954c40139c4a@mail.gmail.com>
Date:	Tue, 5 Aug 2008 12:44:58 +0100
From:	"Will Newton" <will.newton@...il.com>
To:	"Alex Williamson" <alex.williamson@...com>
Cc:	linux-serial <linux-serial@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	"Thomas Koeller" <thomas@...ller.dyndns.org>
Subject: Re: [PATCH] serial 8250: tighten test for using backup timer

On Wed, Mar 19, 2008 at 5:50 PM, Alex Williamson <alex.williamson@...com> 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@...com>
> ---
>
> 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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

View attachment "serial-thre.patch" of type "text/x-patch" (1483 bytes)