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@xxxxxxxxx> wrote:

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

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.
--
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/