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

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

[Alex Williamson]

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 <alex.williamson@hp.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;

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

[Will Newton]

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: serial-thre.patch --]
[-- Type: text/x-patch; name=serial-thre.patch, Size: 1483 bytes --]

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)

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

[Alex Williamson]

Hi Will,

On Tue, 2008-08-05 at 12:44 +0100, Will Newton wrote:
> 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?

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 <alex.williamson@hp.com>

Thanks, Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

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

[Will Newton]

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

On Tue, Aug 5, 2008 at 9:06 PM, Alex Williamson <alex.williamson@hp.com> wrote:
> Hi Will,
>
> On Tue, 2008-08-05 at 12:44 +0100, Will Newton wrote:
>> 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?
>
> 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 <alex.williamson@hp.com>

I've added a comment and your ack, thanks.

Andrew: Are you the appropriate person to send patches for the serial
subsystem to?

>From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@gmail.com>
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 <will.newton@gmail.com>
Acked-by: Alex Williamson <alex.williamson@hp.com>

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

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..9ccc563 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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.
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)
-- 
1.5.5.2

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-8250-Improve-workaround-for-UARTs-that-don-t-re-ass.patch --]
[-- Type: text/x-patch; name=0001-8250-Improve-workaround-for-UARTs-that-don-t-re-ass.patch, Size: 2570 bytes --]

From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@gmail.com>
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 <will.newton@gmail.com>
Acked-by: Alex Williamson <alex.williamson@hp.com>

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

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..9ccc563 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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.
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)
-- 
1.5.5.2

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

[Andrew Morton]

On Wed, 6 Aug 2008 11:53:13 +0100
"Will Newton" <will.newton@gmail.com> wrote:

> >From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
> From: Will Newton <will.newton@gmail.com>
> 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 <will.newton@gmail.com>
> Acked-by: Alex Williamson <alex.williamson@hp.com>

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


> ---
>  drivers/serial/8250.c |   16 ++++++++++++----
>  drivers/serial/8250.h |    1 +
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 342e12f..9ccc563 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -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.
> 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)

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:

--- a/drivers/serial/8250.c~serial-8250-tighten-test-for-using-backup-timer-fix
+++ a/drivers/serial/8250.c
@@ -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?

Thanks.

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

[Will Newton]

On Mon, Aug 11, 2008 at 10:32 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 6 Aug 2008 11:53:13 +0100
> "Will Newton" <will.newton@gmail.com> wrote:
>
>> >From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
>> From: Will Newton <will.newton@gmail.com>
>> 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 <will.newton@gmail.com>
>> Acked-by: Alex Williamson <alex.williamson@hp.com>
>
> What are the "recent changes" to which you refer?  I had a quick look
> and didn't spot any THRE-related changes this year?

Sorry, I should have been more specific. Commit
01c194d9278efc15d4785ff205643e9c0bdcef53.

>> ---
>>  drivers/serial/8250.c |   16 ++++++++++++----
>>  drivers/serial/8250.h |    1 +
>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
>> index 342e12f..9ccc563 100644
>> --- a/drivers/serial/8250.c
>> +++ b/drivers/serial/8250.c
>> @@ -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.
>> 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)
>
> 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.

For users of this version of this particular UART IP it is fatal. From
looking at the git history it looks like the original patch went into
2.6.26 so it might also affect that kernel.

> Also2, we should officially use setup_timer(), like this:
>
> --- a/drivers/serial/8250.c~serial-8250-tighten-test-for-using-backup-timer-fix
> +++ a/drivers/serial/8250.c
> @@ -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?

I'll look into it. Some of the weirdness comes from the way the timer
is used for this bug workaround but also for irq-less ports.

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

[David Brownell]

> > 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.
>
> For users of this version of this particular UART IP it is fatal. From
> looking at the git history it looks like the original patch went into
> 2.6.26 so it might also affect that kernel.

Second that:  serial-8250-tighten-test-for-using-backup-timer.patch
(from MMOTM) in mainline sooner-not-later seems right.

My own exposure to this is that the UART on DaVinci hardware, which
TI allegedly derived from its original 16550 logic, has periodically
gone from working to unusable with the mainline 8250.c ... and back
and forth a bunch.  Currently it's "unusable", a regression from some
previous versions.  With this patch from Alex, it's usable.

Of course there are a bunch of arch/arm/mach-davinci patches needed
to make that platform more functional in mainline.  I expect they'll
probably merge in the 2.6.28-rc0 window.  Meanwhile, running out of
ramdisk with a serial console should at least work right!  :)

- Dave

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

[Andrew Morton]

On Tue, 26 Aug 2008 10:45:34 -0700
David Brownell <david-b@pacbell.net> wrote:

> > > 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.
> >
> > For users of this version of this particular UART IP it is fatal. From
> > looking at the git history it looks like the original patch went into
> > 2.6.26 so it might also affect that kernel.
> 
> Second that:  serial-8250-tighten-test-for-using-backup-timer.patch
> (from MMOTM) in mainline sooner-not-later seems right.

ok, if you think so...

I still have an unanswered question out there about the init_timer()
handling in that area of the driver but afaict this patch didn't make any
of it worse than it already is.

> My own exposure to this is that the UART on DaVinci hardware, which
> TI allegedly derived from its original 16550 logic, has periodically
> gone from working to unusable with the mainline 8250.c ... and back
> and forth a bunch.  Currently it's "unusable", a regression from some
> previous versions.  With this patch from Alex, it's usable.
> 
> Of course there are a bunch of arch/arm/mach-davinci patches needed
> to make that platform more functional in mainline.  I expect they'll
> probably merge in the 2.6.28-rc0 window.  Meanwhile, running out of
> ramdisk with a serial console should at least work right!  :)
> 

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

[Will Newton]

On 8/26/08, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 26 Aug 2008 10:45:34 -0700
>  David Brownell <david-b@pacbell.net> wrote:
>
>  > > > 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.
>  > >
>  > > For users of this version of this particular UART IP it is fatal. From
>  > > looking at the git history it looks like the original patch went into
>  > > 2.6.26 so it might also affect that kernel.
>  >
>  > Second that:  serial-8250-tighten-test-for-using-backup-timer.patch
>  > (from MMOTM) in mainline sooner-not-later seems right.
>
>
> ok, if you think so...
>
>  I still have an unanswered question out there about the init_timer()
>  handling in that area of the driver but afaict this patch didn't make any
>  of it worse than it already is.

Sorry, I forgot to follow up on that. I had a look at the init_timer()
usage and it looks correct although it is rather strange. Reworking it
would affect several classes of hardware, most of which I don't have
available so I think it's safest to leave it as is for now.

>  > My own exposure to this is that the UART on DaVinci hardware, which
>  > TI allegedly derived from its original 16550 logic, has periodically
>  > gone from working to unusable with the mainline 8250.c ... and back
>  > and forth a bunch.  Currently it's "unusable", a regression from some
>  > previous versions.  With this patch from Alex, it's usable.
>  >
>  > Of course there are a bunch of arch/arm/mach-davinci patches needed
>  > to make that platform more functional in mainline.  I expect they'll
>  > probably merge in the 2.6.28-rc0 window.  Meanwhile, running out of
>  > ramdisk with a serial console should at least work right!  :)
>  >
>
>

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

[Will Newton]

On Tue, Aug 26, 2008 at 6:58 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 26 Aug 2008 10:45:34 -0700
> David Brownell <david-b@pacbell.net> wrote:
>
>> > > 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.
>> >
>> > For users of this version of this particular UART IP it is fatal. From
>> > looking at the git history it looks like the original patch went into
>> > 2.6.26 so it might also affect that kernel.
>>
>> Second that:  serial-8250-tighten-test-for-using-backup-timer.patch
>> (from MMOTM) in mainline sooner-not-later seems right.
>
> ok, if you think so...
>
> I still have an unanswered question out there about the init_timer()
> handling in that area of the driver but afaict this patch didn't make any
> of it worse than it already is.

Any update on this situation? I'd really like this patch to get into 2.6.27.

Let me know if there's any further information you need.

Thanks,

>> My own exposure to this is that the UART on DaVinci hardware, which
>> TI allegedly derived from its original 16550 logic, has periodically
>> gone from working to unusable with the mainline 8250.c ... and back
>> and forth a bunch.  Currently it's "unusable", a regression from some
>> previous versions.  With this patch from Alex, it's usable.
>>
>> Of course there are a bunch of arch/arm/mach-davinci patches needed
>> to make that platform more functional in mainline.  I expect they'll
>> probably merge in the 2.6.28-rc0 window.  Meanwhile, running out of
>> ramdisk with a serial console should at least work right!  :)
>>
>
>

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

[Andrew Morton]

On Mon, 1 Sep 2008 14:36:56 +0100 "Will Newton" <will.newton@gmail.com> wrote:

> On Tue, Aug 26, 2008 at 6:58 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 26 Aug 2008 10:45:34 -0700
> > David Brownell <david-b@pacbell.net> wrote:
> >
> >> > > 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.
> >> >
> >> > For users of this version of this particular UART IP it is fatal. From
> >> > looking at the git history it looks like the original patch went into
> >> > 2.6.26 so it might also affect that kernel.
> >>
> >> Second that:  serial-8250-tighten-test-for-using-backup-timer.patch
> >> (from MMOTM) in mainline sooner-not-later seems right.
> >
> > ok, if you think so...
> >
> > I still have an unanswered question out there about the init_timer()
> > handling in that area of the driver but afaict this patch didn't make any
> > of it worse than it already is.
> 
> Any update on this situation? I'd really like this patch to get into 2.6.27.

yup, I plan to include the patch in my next batch for 2.6.27.